I've added the link to the patch
https://github.com/php/php-src/pull/1125/files
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
> >
> >
>
