Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:83991
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain beberlei.de from 74.125.82.177 cause and error)
MIME-Version: 1.0
In-Reply-To: <CA+9eiLsjwe_MXm3k6ZdApVmr5rBcxgoF7-hrsApc5i7biKqpUQ@mail.gmail.com>
References: <d238d736f8fcde9e649cb38c9b05fcc1@mail.gmail.com>
	<CAEPJduntsR-Z9EtrjuN49Po0ctOrdnpga2hC18qQy2k6q_c8rw@mail.gmail.com>
	<CA+9eiLsjwe_MXm3k6ZdApVmr5rBcxgoF7-hrsApc5i7biKqpUQ@mail.gmail.com>
Date: Fri, 27 Feb 2015 12:18:40 +0100
Message-ID: <CAEPJdukTSM0xMkyks2sfzL1JPYfnWVwJS-bHejLpEfE05tqN4A@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: Zeev Suraski <zeev@zend.com>, PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c381ceda76280510100721
Subject: Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
From: kontakt@beberlei.de (Benjamin Eberlei)

--001a11c381ceda76280510100721
Content-Type: text/plain; charset=UTF-8

On Fri, Feb 27, 2015 at 9:44 AM, Dmitry Stogov <dmitry@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 <kontakt@beberlei.de>
> wrote:
>
>> Zeev,
>>
>> On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski <zeev@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
>> >
>> >
>>
>
>

--001a11c381ceda76280510100721--