On Fri, Feb 27, 2015 at 9:44 AM, Dmitry Stogov <dmi...@zend.com> wrote:
> I've added the link to the patch > > https://github.com/php/php-src/pull/1125/files > Thanks! First, the necessary PHPUnit changes (dev-master) to avoid errors: https://gist.github.com/beberlei/8a33ae940829f1186da2 - Doctrine DBAL testsuite: 8 failures - Doctrine ORM: Crashes unrelated after half the tests, but has about 30-50% failures like Symfony2 - Symfony2 Testsuite: 6215 failures - Twig: Tests: 1108, Assertions: 1616, Errors: 125. Now probably many of the failures are related to few code paths that need fixing, however I have to find out that and will be a lot of work. But this is the good PHP code! For untested or older PHP code (and yes there is alot out there) i now have to collect the errors in production to find all the occurances. This is nothing i can just do whenever I want, I need a process to collect and fix this over time. Now every company needs this process for every project they have out there. And the typical agency has hundrets/thousands of drupal, typo3, wordpress installations out there. > > > Thanks. Dmitry. > > On Fri, Feb 27, 2015 at 11:03 AM, Benjamin Eberlei <kont...@beberlei.de> > wrote: > >> Zeev, >> >> On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski <z...@zend.com> wrote: >> >> > All, >> > >> > We've been working in the last few days to test and tune the Coercive >> STH >> > patch. I think the results are quite nice, and surprisingly better than >> > one might have expected. >> > >> Can we try the patch ourselves? I would love to run it against some >> libraries as well. >> >> > >> > Before diving into the results, we did update the RFC >> > (wiki.php.net/rfc/coercive_sth) - with the most notable difference >> being >> > allowing NULL->scalar conversions, for two reasons - it's not uncommon >> for >> > code to use 'null' as a way to denote an empty optional parameter to for >> > internal functions, and many internal functions seem to rely on that >> > behavior themselves. It should be possible to alter this behavior in >> the >> > future by changing the internal functions to explicitly handle NULL >> inputs >> > as 'please use the default value' - but it's outside the scope of the >> RFC. >> > >> >> This RFC trying to simpliy and cleanup the coercison rules, having two >> different conversion rules for NULL->scalar >> depending on userland or internal is counter-productive and bad. The >> behavior you describe as null being >> empty value is wide-spread in PHP userland code as well. >> >> >> > In addition, coercion from scalar to boolean is now limited only to >> > integer - which seems to be the most popular use case; Beforehand, >> > coercion was also allowed from float and string - but based on feedback >> > from Mike, we're reconsidering accepting strings again. >> > >> > Another change being considered and not yet in the RFC is re-allowing >> > leading and trailing spaces for numeric strings (sorry Paddy.) >> > >> >> I agree with Pierre here, it would be super helpful if we had a log in the >> RFC of the actual changes that will be happening. >> As in Francois original patch this seems to be a game of having 20 changes >> and then picking which ones to do and which not. >> >> >> > >> > Now to the tests we ran. The goal was to see what kind of effect the >> > changes to the internal function APIs had on real world apps with large >> > codebase. The test methodology was done by placing a debugger >> breakpoint >> > on zend_error, to ensure no error gets lost in the ether of settings or >> > callbacks. Here are the results: >> > >> > >> > Drupal homepage: One new E_DEPRECATED warning, which seems to catch a >> > real bug, or at least faulty looking code: >> > $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean >> > false. >> > return $path; >> > >> > Drupal admin interface (across the all pages): One new E_DEPRECATED >> > warning, which again seems to catch a real bug - stripslsahes() >> operating >> > on a boolean. >> > >> > Magento homepage (w/ Magento's huge sample DB): One new E_DEPRECATED >> > warning, again, seems to be catching a real bug of 'false' being fed as >> > argument 1 of in json_decode() - which is expecting a string full of >> json >> > there. >> > >> > WordPress homepage: One new E_DEPRECATED warning, again, seems to be >> > catching a real bug of 'false' being fed as argument 1 of substr(). >> > >> > Zend Framework 2 skeleton app: Zero new E_DEPRECATED warnings. >> > >> > Symfony ACME app: Zero new E_DEPRECATED warnings (across the app). >> > >> >> I was expecting this, because the rule changes are mostly in regard to not >> accepting >> invalid input, so what you need to test is all the edge cases. >> >> Say I rely on a validation in count() somewhere in the code to implicitly >> validate its an array: >> >> if (count($_GET['filters'])) { >> echo "Filtering my query"; >> } >> >> This would work in the "happy path" case, because i have a filter set. But >> maybe >> there is some invalid state i can get into and only then the E_DEPRECATED >> is produced. >> >> The homepages of popular systems being the essential "happy path" for a >> project, I wouldnt expect many errors to occur. >> >> >> > >> > As I'm sure you know, the above tests invoke a huge number of lines of >> > code in total, handling filesystem ops, database ops and all sorts of >> > other things. This is much of the mini test suite that we use to test >> > PHP's performance and compatibility (e.g. phpng, now PHP 7). So while >> > this isn't proof that code in the wild isn't going to have more issues - >> > it's a pretty good initial indication regarding the level of 'breakage' >> we >> > can expect. I'm putting breakage in quotes, as people would have >> several >> > years to fix these few issues, before the E_DEPRECATED becomes an error >> > (or an exception, if the engine exceptions RFC passes). >> > >> > In terms of the test suite (.phpts), the changes cause approximately 700 >> > extra tests to fail out of 13,700, in comparison to w/o the patch. >> > However, even though I didn't have a chance to go over all of them, it >> > seems that the vast majority of the failed tests are tests that were >> > intentionally designed to cover the exact parameter passing behavior, >> > rather than real likely real world code pieces. A huge number of the >> > internal functions have this in their test suites: >> > >> > $variation = array( >> > 'float 10.5' => 10.5, >> > 'float -10.5' => -10.5, >> > 'float 12.3456789000e10' => 12.3456789000e10, >> > 'float -12.3456789000e10' => -12.3456789000e10, >> > 'float .5' => .5, >> > ); >> > >> > foreach ( $variation as $var ) { >> > var_dump(readgzfile( $filename, $var ) ); >> > } >> > >> > Which clearly isn't a very likely input to the size argument of >> > readgzfile(). All of these now trigger E_DEPRECATED warnings since we >> no >> > longer allow float->int conversions if there's no data loss. For some >> > reason (perhaps a good one, not sure), we have virtually identical >> pieces >> > of code for dozens if not hundreds of internal functions that expect >> > integers - and these types of failures account for the (looks like vast) >> > majority of phpt failures. Quoting Andrea from a couple of days ago on >> > this very topic, "to be fair, most of PHP's test suite basically just >> > tests zpp's behavior", which appears to be absolutely true. The >> > real-world app tests are probably a much better indicator of the kind of >> > behaviors our users are going to see than our test suite is. >> > >> > The takeaway from this seems to be that the approach of tightening the >> > existing rule-set and applying it to both internal functions and new >> > user-land code is viable, and does not cause the sky to come down >> falling. >> > Preliminary tests suggest it actually finds real potential problems. >> > >> >> Nobody I think will argue that tightening the rules will not detect more >> problems >> and may lead to better code written by developers. >> >> The question is if this a BC break that is acceptable or not, one that our >> users can stand behind and say "Yes I am fine with being forced to invest >> time updating my application so that it still runs on PHP 8". >> >> >> > >> > More tomorrow. >> > >> > Zeev >> > >> > -- >> > PHP Internals - PHP Runtime Development Mailing List >> > To unsubscribe, visit: http://www.php.net/unsub.php >> > >> > >> > >
