Zeev,
On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski <[email protected]> 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
>
>
