Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:112919
Message-ID: <81C8F593-454B-491B-BFF5-0E6D8D39DEE9@cschneid.com>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_6C4FE349-1D79-4CFE-9661-886451B13A0B"
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.40.0.2.32\))
Date: Mon, 18 Jan 2021 13:52:06 +0100
In-Reply-To: <CAF+90c_fPVj6t+73-w0upCaR855a7TQzO4YrtPVTOHBtR+27Cw@mail.gmail.com>
Cc: PHP internals <internals@lists.php.net>
To: Nikita Popov <nikita.ppv@gmail.com>
References: <70E24D44-D791-4AEB-92C3-3B8C9579F5CC@cschneid.com>
 <CAF+90c_fPVj6t+73-w0upCaR855a7TQzO4YrtPVTOHBtR+27Cw@mail.gmail.com>
Subject: Re: [PHP-DEV] Consistent type errors for internal functions and BC
 breaks
From: cschneid@cschneid.com (Christian Schneider)

--Apple-Mail=_6C4FE349-1D79-4CFE-9661-886451B13A0B
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

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>
>=20
> 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?
>=20
> 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


--Apple-Mail=_6C4FE349-1D79-4CFE-9661-886451B13A0B--