Everything

php

Consistent type errors for internal functions and BC breaks

php.internals

Christian Schneider

5 years ago

While testing the migration of our site to PHP 8 we noticed the BC break in internal functions like round() for parameter type errors:
round("foo") used to (silently, without warning) cast to float and return 0 where as now it throws a TypeError: round():
https://3v4l.org/pU0LD

The RFC at https://wiki.php.net/rfc/consistent_type_errors#backward_incompatible_changes <https://wiki.php.net/rfc/consistent_type_errors> states

> Backward Incompatible Changes
>
> A TypeError will be thrown instead of a warning if incorrectly typed parameters are passed to a function, which is a backwards incompatible change.

But that is not the whole truth because it suggests that there was a warning in 7.4 which is not the case for functions like round() and possibly many more.

This makes the BC break much more impactful as there is no easy way to know if old code will suddenly abort with an exception whereas it used to run without any warning.

Is there any way of knowing what functions are affected by this?

This might be a blocker for people to migrate to PHP 8, should there be a way to have a warning-instead-of-exception stage added to PHP 8 even though there first versions were already released to help people migrate?

Or did I miss something?

- Chris

Nikita Popov

5 years ago

On Sat, Jan 16, 2021 at 6:07 PM Christian Schneider <cschneid@cschneid.com>
wrote:

> While testing the migration of our site to PHP 8 we noticed the BC break
> in internal functions like round() for parameter type errors:
> round("foo") used to (silently, without warning) cast to float and return
> 0 where as now it throws a TypeError: round():
>         https://3v4l.org/pU0LD
>
> The RFC at
> https://wiki.php.net/rfc/consistent_type_errors#backward_incompatible_changes
> <https://wiki.php.net/rfc/consistent_type_errors> states
> > Backward Incompatible Changes
> >
> > A TypeError will be thrown instead of a warning if incorrectly typed
> parameters are passed to a function, which is a backwards incompatible
> change.
>
> But that is not the whole truth because it suggests that there was a
> warning in 7.4 which is not the case for functions like round() and
> possibly many more.
>
> This makes the BC break much more impactful as there is no easy way to
> know if old code will suddenly abort with an exception whereas it used to
> run without any warning.
>
> Is there any way of knowing what functions are affected by this?
>
> This might be a blocker for people to migrate to PHP 8, should there be a
> way to have a warning-instead-of-exception stage added to PHP 8 even though
> there first versions were already released to help people migrate?
>
> Or did I miss something?
>

This is the interaction of two changes. The RFC does exactly what it says,
and promotes certain warnings to Error exceptions. At the same time, PHP 8
moved some function implementations to use standardized parameter handling.
These two changes in conjunction can, in rare cases, conspire to produce an
Error where previously no warning was thrown. round() is an example of
where this happened.

I don't think there's a comprehensive list of edge cases where something
like this has occurred. It would certainly be useful if any cases that are
found make their way into the manual. Relevant changelog entries are being
added to functions as their methodsynopses are updated for PHP 8 -- this is
ongoing work and has not reached the round() function yet. (You can find
all changelog entries for PHP 8.0.0 that are currently available on
https://www.php.net/manual/en/doc.changelog.php, though this has more
information than is probably relevant to you. Many of those changes aren't
backwards-incompatible, just document previous behavior.)

Regards,
Nikita

Christian Schneider

5 years ago

Am 16.01.2021 um 20:01 schrieb Nikita Popov <nikita.ppv@gmail.com>:

> On Sat, Jan 16, 2021 at 6:07 PM Christian Schneider <cschneid@cschneid.com <mailto:cschneid@cschneid.com>> wrote:
> While testing the migration of our site to PHP 8 we noticed the BC break in internal functions like round() for parameter type errors:
> round("foo") used to (silently, without warning) cast to float and return 0 where as now it throws a TypeError: round():
>         https://3v4l.org/pU0LD <https://3v4l.org/pU0LD>
>
> This might be a blocker for people to migrate to PHP 8, should there be a way to have a warning-instead-of-exception stage added to PHP 8 even though there first versions were already released to help people migrate?
>
> This is the interaction of two changes. The RFC does exactly what it says, and promotes certain warnings to Error exceptions. At the same time, PHP 8 moved some function implementations to use standardized parameter handling. These two changes in conjunction can, in rare cases, conspire to produce an Error where previously no warning was thrown. round() is an example of where this happened.

Thanks for the explanation, I was wondering how this slipped through. Now I know :-)

> I don't think there's a comprehensive list of edge cases where something like this has occurred. It would certainly be useful if any cases that are found make their way into the manual.

I think this should definitely be in the Migration/Upgrade guide.

But my main concern stays the same: It is an unfortunate situation that there is no way of knowing if code will abort once switching to PHP 8 for basic things like round().

Background: We have dozens of data sources where we want to ignore single invalid input values without breaking the process and/or discarding the whole data element just because one field has e.g "n/a" instead of a float. You can question this decision but the fact is that we have a lot of battle-proven code working happily with round() doing enough canonization for our needs.

I currently see the following options for someone wanting to switch to PHP 8:
1) Do a full code review of all code involving round() etc. (assuming there is a list of affected functions which there currently isn't)
2) Convert all code to use typed variables for everything which could end up in round() etc.
3) Convert all round(...) to round(floatval(...)) (and doing similar stuff for other affected functions)
4) Have a copy of your production environment feeding it live data and wait some time to see if any exceptions are triggered before exposing PHP 8 to the users.

I think
1) and 2) are a lot of work and 2) might be a drastic coding style change which could lead to even more work.
3) is an ugly hack and litters the code.
4) Might be the most realistic migration path but delays the PHP 8 switch and is not guaranteed to catch all code paths.

That's why I'd like to suggest
5) Generate warnings instead of exceptions for the cases where previously silent code now generates an exception with 8.0.1. This could be in 7.4 (to give users a way of preparing for the transition before actually doing it) or (preferably) in 8.0.2 to speed up PHP 8 adoption while giving people more peace of mind that their code won't break if they have been running in E_ALL mode for a while.

- Chris