Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:83935
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.223.174 as permitted sender)
MIME-Version: 1.0
Thread-Index: AdBSG/WkqUw7Hn+aStqz6TPEiLsDrQ==
Date: Fri, 27 Feb 2015 01:57:42 +0200
Message-ID: <d238d736f8fcde9e649cb38c9b05fcc1@mail.gmail.com>
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Coercive STH - some real world tests and updated RFC
From: zeev@zend.com (Zeev Suraski)

 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.

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.

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.)

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).

 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.

More tomorrow.

Zeev