# Coercive STH - some real world tests and updated RFC

- Thread: https://externals.io/message/83935
- Original archive: https://news-web.php.net/php.internals/83935
- Started by Zeev Suraski on 2015-02-26T23:57:42+00:00
- Messages: 43
- Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL)
- Next page: https://externals.io/message/83935.md?page=2
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #83935 — Zeev Suraski — 2015-02-26T23:57:42+00:00

URL: https://externals.io/message/83935#83935

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

---

## #83936 — Nikita Popov — 2015-02-27T00:06:22+00:00

URL: https://externals.io/message/83935#83936 · In reply to: #83935

>  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
>
> --
> 
> 
>
>
How many deprecations do you get running the ZF2 and Symfony testsuites?

Nikita

---

## #83938 — Zeev Suraski — 2015-02-27T00:08:14+00:00

URL: https://externals.io/message/83935#83938 · In reply to: #83936

> How many deprecations do you get running the ZF2 and Symfony testsuites?

None, but it may have to do with the fact I haven't run them yet :)

Zeev

---

## #83939 — padraic.brady@gmail.com — 2015-02-27T00:19:53+00:00

URL: https://externals.io/message/83935#83939 · In reply to: #83935

Hi Zeev,


> Another change being considered and not yet in the RFC is re-allowing
> leading and trailing spaces for numeric strings (sorry Paddy.)

Not to worry. I know where Github keeps the servers ;).

There are far worse things than spaces.

Paddy

--
Pádraic Brady

http://blog.astrumfutura.com
http://www.survivethedeepend.com

---

## #83941 — Pierre Joye — 2015-02-27T00:32:02+00:00

URL: https://externals.io/message/83935#83941 · In reply to: #83935

> 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;

So far nobody answered this question but Francois (tried). You keep
using this `E_DEPRECATED` message as a safety net to catch possible bad
things. What's about other changes that may not match the current
casting rules (or lack of) and will be different with this RFC? Those
considered as "Yes". This is what I worry about, even if minimal these
cases are pains to track down and fix.

Also I would really like a clear table describing in details what will
be changed, to compare how it works now and how it work later. This is
something that is crystal clear with the dual mode RFC, nothing
changes by defaullt, and strict rules if strict mode is enabled for a
file.

As you would expect, I will vote no as it is proposed now.

---

## #83954 — francois@php.net — 2015-02-27T02:39:04+00:00

URL: https://externals.io/message/83935#83954 · In reply to: #83941

> De : Pierre Joye [mailto:pierre.php@gmail.com]
> 
> So far nobody answered this question but Francois (tried). You keep
> using this `E_DEPRECATED` message as a safety net to catch possible bad
> things.

Yes, even if we don't really 'catch' things, because we just raise `E_DEPRECATED` and, then, continue execution exactly as before. Actually, it is more an 'alert' net than a safety net, as its role is to alert developers that they have a potential issue to solve during the next 5 or 10 years. In most cases, it is a real bug they'd better fix before 5 years ;)

> What's about other changes that may not match the current
> casting rules (or lack of) and will be different with this RFC? Those
> considered as "Yes".

I now understand your previous post. There's absolutely no change in the way things are converted. Every conversion works *exactly* the same as in PHP 5. The only additional behavior we introduce is raising some `E_DEPRECATED` in some cases. But, after raising E_DEPRECATED, we still do the work ! So, if you turn `E_DEPRECATED` off, the behavior is exactly the same as PHP 5.

Even when `E_DEPRECATED` will be turned to fatal error in the future, the ruleset will remain a pure subset of the cases supported in PHP 5, failing for more cases, but with no change in the way conversion are performed. I think it is not clear enough in the RFC, explaining your misconception.

Actually, I must admit this is not *perfectly* true. There is one conversion I would like to modify. If we reintroduce getting bool from string, the current case for false is "0" or empty string only. For consistency reasons, I would like to extend "0" to any string that would convert to a null numeric value (also considering as false "0.0", "   0",...) but I hope you will consider that's a marginal change and the risk is low. But this is just a suggestion. Even for this, if some prefer to stick with the current behavior, it will be preserved and kept for a future RFC.

> Also I would really like a clear table describing in details what will
> be changed, to compare how it works now and how it work later.

You're right. I had planned to write two tables. One with the changes and one with the final situation. But I didn't have enough time for both. I will add the 'changes' table in the next hours.

Regards

François

---

## #83961 — francois@php.net — 2015-02-27T06:43:42+00:00

URL: https://externals.io/message/83935#83961 · In reply to: #83941

Hi Pierre,

> De : Pierre Joye [mailto:pierre.php@gmail.com]
>
> Also I would really like a clear table describing in details what will
> be changed, to compare how it works now and how it work later.

The RFC now contains a table listing the changes compared to current PHP 5 rules. Enjoy it.

Regards

François

---

## #83971 — Benjamin Eberlei — 2015-02-27T08:03:32+00:00

URL: https://externals.io/message/83935#83971 · In reply to: #83935

Zeev,



>  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

---

## #83981 — Dmitry Stogov — 2015-02-27T08:44:14+00:00

URL: https://externals.io/message/83935#83981 · In reply to: #83971

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 <kontakt@beberlei.de>
wrote:

> Zeev,
>
> 
>
> > […]
> Can we try the patch ourselves? I would love to run it against some
> libraries as well.
>
> > […]
> for
> > […]
> inputs
> > […]
> 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.
>
>
> > […]
>
> 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.
>
>
> > […]
>
> 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.
>
>
> > […]
> we
> > […]
> falling.
> > […]
>
> 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".
>
>
> > […]

---

## #83991 — Benjamin Eberlei — 2015-02-27T11:18:40+00:00

URL: https://externals.io/message/83935#83991 · In reply to: #83981

> 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:
>
> > […]

---

## #83993 — Benjamin Eberlei — 2015-02-27T11:30:15+00:00

URL: https://externals.io/message/83935#83993 · In reply to: #83991

On Fri, Feb 27, 2015 at 12:18 PM, Benjamin Eberlei <kontakt@beberlei.de>
wrote:

>
>
> 
>
> > […]
>
> 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.
>

I used the following configure line and then /usr/local/php7/lib/php.ini

./configure --prefix=/usr/local/php7 --with-mysql=mysqlnd
--with-mysqli=mysqlnd --with-tidy=/usr --with-curl=/usr --with-curlwrappers
--with-zlib-dir=/usr --enable-mbstring --with-xpm-dir=/usr
--enable-pdo=shared --with-pdo-mysql=shared,mysqlnd --without-sqlite
--with-pdo-sqlite=shared,/usr --with-xsl=/usr --with-xmlrpc
--with-iconv-dir=/usr --with-snmp=/usr --enable-exif --enable-calendar
--with-bz2=/usr --with-mcrypt=/usr --with-gd --with-jpeg-dir=/usr
--with-png-dir=/usr --with-zlib-dir=/usr --with-freetype-dir=/usr
--enable-mbstring --enable-zip --with-pear --enable-soap --with-gd
--enable-intl --with-bz2=/usr --enable-sockets --with-sqlite3=shared
--enable-pcntl

max_execution_time=600
memory_limit=128M
error_reporting=-1
display_errors=1
log_errors=0
user_ini.filename=
realpath_cache_size=2M
cgi.check_shebang_line=0

zend_extension=opcache.so
opcache.enable_cli=1
opcache.save_comments=1
opcache.fast_shutdown=1
opcache.validate_timestamps=1
opcache.revalidate_freq=60
opcache.use_cwd=1
opcache.max_accelerated_files=100000
opcache.max_wasted_percentage=5
opcache.memory_consumption=128
opcache.consistency_checks=0
date.timezone=Europe/Berlin

extension=pdo.so
extension=pdo_sqlite.so
extension=pdo_mysql.so
extension=sqlite3.so

---

## #83997 — Zeev Suraski — 2015-02-27T12:53:46+00:00

URL: https://externals.io/message/83935#83997 · In reply to: #83991

> -----Original Message-----
> From: Benjamin Eberlei [mailto:kontakt@beberlei.de]
> Sent: Friday, February 27, 2015 1:19 PM
> To: Dmitry Stogov
> Cc: Zeev Suraski; PHP internals
> Subject: Re: [PHP-DEV] Coercive STH - some real world tests and updated
> RFC
>
>
>
> 
>
>
> 	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.

Thanks for the tests!  We'll look into those.

> 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!

Judging by the fact WordPress, Magento and Drupal appear a lot cleaner - you
have to wonder..? :)

But on a more serious note, I don't think we should assume just about
anything from the test suite as reflecting on real world app behavior.  I'm
not familiar with the Symfony test suite (yet), but I do know that the
Symfony skeleton app ran cleanly without a single warning.  Is it possible
it's testing a lot of things with garbage input, as you would in unit tests,
as the our own PHP test suite does?  Those account for most of the failures.
Also take into account that a project with that many tests (and
specifically, failures) is probably not going to have just one or two people
responsible for it, but more.  It's not that you personally would have to
fix Symfony, there are at least a dozen if not dozen people that would help
out.

> 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.

I wish that were true, but in reality, migration is painfully slow and
judging by the numbers, doesn't happen - more often than happening.  The
'good' companies have ~5yrs after PHP 7 comes out and before PHP 7.x will no
longer be supported (and that's according to my hope for a fast PHP 8
timeline;  it may be longer as you suggested).  Would all projects migrate
by that timeline?  Let's make a simpler question - would all projects
migrate to PHP 7 by 2017, the year 5.6 theoretically stops being supported?
We all know the answer to that.

The good companies, the security conscious ones that do keep up with new
versions, will likely be willing to invest the effort of fixing the code (or
hiring someone to do it for them), especially as it'll help them make their
apps more robust/secure.  And especially as - at least so far - it seems
that real world apps have not even a handful of issues per page.

All that said, I think we must understand the huge gap between the very high
degree of test failures you're seeing in test suites, and the almost none
we're seeing in real world apps.  If it's similar to the PHP test suite,
it's a non-issue, and even if it would take some work to fix the unit tests,
it's well worth it.  If it's legit, but as you're guessing, may just be
several common paths of code that will do away with most of the failures -
it's still worth it.  Investing several days over the course of several
years is great ROI for the value it brings.  If, however, the test suite
does find a multitude of many different issues - it may bring me to
reconsider the RFC, as I told Anthony a few days ago - especially if the
signal to noise ratio would be bad.  After the results we've seen with real
world apps I find it hard to believe, but it's certainly a possible
scenario.

At the end of the day, it boils down to how much real work do we think will
be required to update real world apps out there.  Let's also not set
unreasonable bars for this change compared to other compatibility breakages
we've done (and are stil doing) over the years - again, most of which didn't
bring any tangible value to developers (which this change does, a lot).

> And the typical agency has hundrets/thousands of drupal, typo3,
> wordpress installations out there

Correct, but would you have to patch each one separately?   You'd have to
install updated core for Drupal, Typo3, WP - which you commonly have to do
anyway if you care about security.  Then there'd be some work to get your
common custom modules that you developed update - between the two of those,
that should account for most of the code.  It's not as if you're going to
start with each deployment from scratch, is it?

Anyway, thanks again for running the tests - we'll definitely need to
understand that better.

Thanks,

Zeev

---

## #84000 — Damien Tournoud — 2015-02-27T13:37:50+00:00

URL: https://externals.io/message/83935#84000 · In reply to: #83935

Hi Zeev,



> 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.
>

All those are due to a bug in `substr()`, that we see now only thanks to
proper type identification. There is no reason for `substr()` to ever return
a boolean. It really needs to be fix to always return a string.

Damien

---

## #84002 — Benjamin Eberlei — 2015-02-27T13:45:23+00:00

URL: https://externals.io/message/83935#84002 · In reply to: #84000

> Hi Zeev,
>
> 
>
> > […]
>
> All those are due to a bug in `substr()`, that we see now only thanks to
> proper type identification. There is no reason for `substr()` to ever return
> a boolean. It really needs to be fix to always return a string.
>

Yes, weird behavior that substr("", 2, 2); for example returns false. But
changing that is just another evil BC break.


>
> Damien

---

## #84003 — Lester Caine — 2015-02-27T13:57:06+00:00

URL: https://externals.io/message/83935#84003 · In reply to: #84002

> > […]
> Yes, weird behavior that substr("", 2, 2); for example returns false. But
> changing thatis just another evil BC break.

Now I don' think that 'weird' ... Although the correct return should
perhaps be 'null', but it's long been practice that s there is no result
we get 'false' so how any places will have a sanity check based on the
'false' return?

-- 
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

---

## #84004 — Benjamin Eberlei — 2015-02-27T13:58:35+00:00

URL: https://externals.io/message/83935#84004 · In reply to: #84000

> Hi Zeev,
>
> 
>
> > […]
>
> All those are due to a bug in `substr()`, that we see now only thanks to
> proper type identification. There is no reason for `substr()` to ever return
> a boolean. It really needs to be fix to always return a string.
>
> Damien
>

This kind of code here exhibits the failure:
https://github.com/pdepend/pdepend/blob/master/src/main/php/PDepend/Source/Language/PHP/PHPTokenizerInternal.php#L584

This is classic PHP style code where you rely on the implicit casting to
make the algorithm work for you.

if `substr()` here returns false, then the error is:

1767)
PDepend\Source\Parser\UnstructuredCodeTest::testParserHandlesNonPhpCodeInFileProlog
 `strlen()` expects parameter 1 to be string, boolean given

/home/benny/code/php/workspace/pdepend/src/main/php/PDepend/Source/Language/PHP/PHPTokenizerInternal.php:586

The funny thing is that the fix for this is:

- substr($image, strrpos($image, "\n") + 1)
+ (string)substr($image, strrpos($image, "\n") + 1)

Which is that sort of casting that is put forward as argument against the
dual/strict mode.

---

## #84005 — Benjamin Eberlei — 2015-02-27T14:15:41+00:00

URL: https://externals.io/message/83935#84005 · In reply to: #84003

> 
> > […]
> operating
> > […]
> return
> > […]
>
> Now I don' think that 'weird' ... Although the correct return should
> perhaps be 'null', but it's long been practice that s there is no result
> we get 'false' so how any places will have a sanity check based on the
> 'false' return?
>

As an example i ported PDepend to work with coercive typehints, this is the
quick and dirty patch:

https://gist.github.com/beberlei/6a5a6b65839d35bb27f0

In longer chains of string handling, i just cast everything to (string)
that could potentially return false here.

Interestingly functions like `is_infinite()`, `is_dir()`, `is_file()` all expect
correct types like float or string here although from the naming convention
"is_something" i would expect it says just false on *anything* else (which
it almost does right now).

The patch also shows that this will lead to weird behavior when the PHP API
is actually wrong like DOMNode::cloneNode() expecting an integer and not a
boolean as argument, even though the PHP Documentation says it should be a
boolean.

I can literally forsee this is the way this kind of code will be fixed.

Imho the problem is that the return values of php internal functions being
string|false will lead to massive consecutive errors when passing this on
to other internal unctions.


>
> --
> Lester Caine - G8HFL
> -----------------------------
> Contact - http://lsces.co.uk/wiki/?page=contact
> L.S.Caine Electronic Services - http://lsces.co.uk
> EnquirySolve - http://enquirysolve.com/
> Model Engineers Digital Workshop - http://medw.co.uk
> Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

---

## #84007 — Christian Stoller — 2015-02-27T14:38:49+00:00

URL: https://externals.io/message/83935#84007 · In reply to: #84000

From: Damien Tournoud [mailto:damz@damz.org], Sent: Friday, February 27, 2015 2:38 PM

> Hi Zeev,
>
> 
>
> > […]
>
> All those are due to a bug in `substr()`, that we see now only thanks to
> proper type identification. There is no reason for `substr()` to ever return
> a boolean. It really needs to be fix to always return a string.
>
> Damien

It is not a bug. `FALSE` as a return value of `substr()` is the identificator
for an error (e.g. invalid arguments), as it is stated in the documentation:

"Returns the extracted part of string; or `FALSE` on failure, or an 
empty string." [1]

This is an example which shows, that Zeevs RFC helps to find bugs in
applications. Because here are given invalid arguments to the method. In
other languages for example Java, you'll get an IndexOutOfBoundsException [2]
if you are trying the same ;-)


  1: http://php.net/manual/en/function.substr.php
  2: http://docs.oracle.com/javase/7/docs/api/java/lang/String.html#substring%28int,%20int%29

---

## #84019 — Anthony Ferrara — 2015-02-27T15:31:03+00:00

URL: https://externals.io/message/83935#84019 · In reply to: #84007

Cristian,

> > […]
>
> It is not a bug. `FALSE` as a return value of `substr()` is the identificator
> for an error (e.g. invalid arguments), as it is stated in the documentation:
>
> "Returns the extracted part of string; or `FALSE` on failure, or an
> empty string." [1]
>
> This is an example which shows, that Zeevs RFC helps to find bugs in
> applications. Because here are given invalid arguments to the method. In
> other languages for example Java, you'll get an IndexOutOfBoundsException [2]
> if you are trying the same ;-)

Well, it depends on your definition of bug.

If you look at PHPUnit's TestSuite::isTestMethod method, you'll see this:

$doc_comment = $method->getDocComment();
return strpos($doc_comment, '@test') !== false ||
strpos($doc_comment, '@scenario') !== false;

Now, with the coercive mode (and my strict mode), that will error if
the method does not have a docblock.

The reason is that `$method->getDocComment()` returns false if one
does not exist.

Is that a bug? Well, semantically, maybe. But from the application
level, no. Passing the boolean result to `strpos` directly ensures
that the result will be `bool(false)`. So the overall method behaves
as expected returning true in the correct context (it's a test method)
and false in incorrect contexts (it's not).

So did the coercive mode find a bug? Not really. It found something
that may have worked in a way you didn't expect it to, but it wasn't a
bug (at least to the application).

Now, if you care about types and type changes, then yes, that is a
bug. And that's why the dual-mode RFC gives you the choice. You can
choose to care about types or not. Not some weird hybrid where you
have to care even if the code is practically correct...

Anthony

---

## #84022 — Damien Tournoud — 2015-02-27T15:53:33+00:00

URL: https://externals.io/message/83935#84022 · In reply to: #84007

Hi Christian,

On Fri, Feb 27, 2015 at 3:38 PM, Christian Stoller <stoller@leonex.de>
wrote:

> It is not a bug. `FALSE` as a return value of `substr()` is the identificator
> for an error (e.g. invalid arguments), as it is stated in the
> documentation:
> [...]
>

"It is documented that way" and "it is not a bug" are two very different
things.

In that case, the semantics of `substr()` are just wrong. It makes a lot
more sense for a sub-string function to silently allow reading before the
beginning and past the end of a string.

Moreover, the *conditions* under which `substr()` returns `FALSE` looks
completely arbitrary, see http://3v4l.org/gtFjk -- in a nutshell: so it is
OK to read past the beginning, it is OK to try to get more characters than
the string has, but it is NOT OK to try to start reading past the end?

This behavior is unpredictable, and as a consequence using `substr()`
properly would require a lot of painful and unnecessary up-front checks. It
traditionally did not matter, because of the fluid scalar casts. But if we
want now to introduce stricter casts, we are going to have to tackle this
problem, we cannot just dismiss it as a programming error.

Damien

---

## #84024 — Christian Stoller — 2015-02-27T16:15:28+00:00

URL: https://externals.io/message/83935#84024 · In reply to: #84022

From: Damien Tournoud [mailto:damz@damz.org], Sent: Friday, February 27, 2015 4:54 PM

> Hi Christian,
>
> 
> It is not a bug. `FALSE` as a return value of `substr()` is the identificator
> for an error (e.g. invalid arguments), as it is stated in the documentation:
> [...]
>
> "It is documented that way" and "it is not a bug" are two very different things.
>

That’s not true. Quoting Wikipedia "A software bug is an error, flaw,
failure, or fault in a computer program or system that causes it 
to produce an incorrect or unexpected result, or to behave in 
unintended ways." [1]

In this case `FALSE` is an expected result and it is intended. And as
I said other languages are going the same way.

>
> In that case, the semantics of `substr()` are just wrong. It makes 
> a lot more sense for a sub-string function > to silently allow
> reading before the beginning and past the end > of a string.

Just because it makes more sense for you, it does not imply that
it makes more sense for everybody. In my opinion it makes sense to
return an "error code" if a function is called with invalid
arguments.

> This behavior is unpredictable, and as a consequence using `substr()` 
> properly would require a lot of painful and unnecessary up-front 
> checks. 

I agree with you, here. With the coercive patch this will lead to
More checks. But that's different from "it is a bug" ;-)

  1: http://en.wikipedia.org/wiki/Software_bug

---

## #84027 — Damien Tournoud — 2015-02-27T16:46:31+00:00

URL: https://externals.io/message/83935#84027 · In reply to: #84024

Hi Christian,

On Fri, Feb 27, 2015 at 5:15 PM, Christian Stoller <stoller@leonex.de>
wrote:

> In this case `FALSE` is an expected result and it is intended. And as
> I said other languages are going the same way.
>

Other languages are also doing something saner. It's not really an argument
either way.

Just because it makes more sense for you, it does not imply that
> it makes more sense for everybody. In my opinion it makes sense to
> return an "error code" if a function is called with invalid
> arguments.
>

You probably haven't read the examples that I pasted. If you think that it
makes sense to return an error code if the function is called with invalid
arguments, it should be done consistently. The current behavior is just an
absurdly inconsistent, it is really hard to find an argument in favor of
it, other than the argument in favor of backward compatibility.

Damien

---

## #84029 — Lester Caine — 2015-02-27T17:09:00+00:00

URL: https://externals.io/message/83935#84029 · In reply to: #84005

> Imho the problem is that the return values of php internal functions being
> string|false will lead to massive consecutive errors when passing this on
> to other internal functions.

This is perhaps the crux of my objection to both types of 'error
checking' ... OK the return should be an empty string rather than false,
but certainly one does not want an exception when nothing is returned.
So much code IS based on doing one thing if there is a value and another
when there is not, so if that is the problem people are claiming needs
fixing perhaps that is what needs addressing? string|false is a core
element of most of PHP?

-- 
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

---

## #84031 — Brian Moon — 2015-02-27T17:30:41+00:00

URL: https://externals.io/message/83935#84031 · In reply to: #83971

> 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.

I agree here with Benjamin. The thing I have wanted in user land for 
years is to be able to build a user land function that worked the way 
internal functions do in terms of type checking without having to build 
my own type checking system. Having internal functions convert null to a 
scalar and not having user land do the same creates an inconsistency. 
For example, sometimes you want to wrap/extend an internal function (you 
see it a lot with json* to do encoding checking). I can't reliably take 
the same input to my user land wrapper that I can send to the internal 
function. I would prefer to see the same rules apply to internal and 
user land.

-- 

Brian.
--------
http://brian.moonspot.net/

---

## #84034 — Lester Caine — 2015-02-27T17:51:12+00:00

URL: https://externals.io/message/83935#84034 · In reply to: #84027

> You probably haven't read the examples that I pasted. If you think that it
> makes sense to return an error code if the function is called with invalid
> arguments, it should be done consistently. The current behavior is just an
> absurdly inconsistent, it is really hard to find an argument in favor of
> it, other than the argument in favor of backward compatibility.

That some changes to the logic of substr have been made is a fact.
Negative values were added to read back from the end of the string a
function that has been played with over time,

# It is NOT OK to start reading past the end of the string, no, no. Bad
programmer.
If there are no characters left ... the string is empty.

# But it is OK to ask for a bigger slice than the length of the string.
Yes ... length is the MOST it will return, if less are available then
that is all you can get.

# It is also OK to read past the begining of the string, why not.
If the reading point does not cut characters then yes this is
consistent. If the length requested was '2', then I would expect 'a' and
false which I think was what the 'bug fix' for PHP5.2.2 was supposed to
do, but it was reverted later in 5.2.7

# On the other hand, it is NOT OK to stop reading before the end of the
string. Bad programmer.
Take 2 characters off the string ... empty string.

This may not be what YOU want substr to do and it would perhaps be
useful to ADD additional checks so that 'false' is returned when it
can't created a string because of the 'invalid arguments', but type
hints makes no difference to part of the jigsaw. What happens is exactly
what one would expect ... nothing left in the string -> an empty string.
Having then to check every time for a '0' string length got shortened to
simply being able to check 'is there a string' yes/no, and if no you can
do something else. There is nothing inconsistent ...

-- 
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

---

Next page: https://externals.io/message/83935.md?page=2