[RFC] Consistent type errors for internal functions

5 years ago by Nikita Popov — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will make
(zpp) parameter parsing failures always result in a TypeError (rather than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regards,
Nikita

5 years ago by Girgias — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will make
(zpp) parameter parsing failures always result in a TypeError (rather than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regards,
Nikita

I'm all for it but what is the scope of the RFC?
Is it all core functions, bundled extension functions, or all extension
functions?
Also does this means that there will be argument type hinting in core
functions that
could be found out via reflection?

Best regards

George P. Banyard

5 years ago by Nikita Popov — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will
make
(zpp) parameter parsing failures always result in a TypeError (rather than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regards,
Nikita

I'm all for it but what is the scope of the RFC?
Is it all core functions, bundled extension functions, or all extension
functions?

It affects all internal functions using the zpp APIs, which covers pretty
much all core functions, bundled extension functions and third-party
extension functions.

Also does this means that there will be argument type hinting in core
functions that
could be found out via reflection?

Not as a direct result of the proposal, but the RFC does remove the big
blocker for it. Once it lands we need one more change (don't actually
verify arginfo types for internal functions to avoid double type checking),
and then we can start adding the necessary type information. That will also
take some work, as we have many functions, but it's something everyone can
help with.

Nikita

5 years ago by Girgias — view source

unread

[. . .]

I'm all for it but what is the scope of the RFC?
Is it all core functions, bundled extension functions, or all extension
functions?

It affects all internal functions using the zpp APIs, which covers pretty
much all core functions, bundled extension functions and third-party
extension functions.

Thanks for the clarification, that's great

Also does this means that there will be argument type hinting in core

functions that
could be found out via reflection?

Not as a direct result of the proposal, but the RFC does remove the big
blocker for it. Once it lands we need one more change (don't actually
verify arginfo types for internal functions to avoid double type checking),
and then we can start adding the necessary type information. That will also
take some work, as we have many functions, but it's something everyone can
help with.

Nikita

I suppose if this RFC gets voted (and implemented) there should be enough
(maybe I'm wrong) time to also implement this.

Really like this RFC even with the potential BC break as there are Static
analysis tools (such as PHPStan, Psalm and forgot the third major one)
which can help point out potential type errors.

Best regards

George P. Banyard

5 years ago by Andrea Faulds — view source

unread

Hi Nikita,

Nikita Popov wrote:

I'd like to bring forward the following proposal for PHP 8, which will make
(zpp) parameter parsing failures always result in a TypeError (rather than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

I like this proposal. IMO PHP's E_WARNING + NULL is the worst of its
“Keep Calm and Carry On” (sorry) behaviours, it would be nice to get rid
of it for good, rather than just in the comfy world of strict_types=1.

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regrettably, as I pointed out to you via another channel, that idea also
faces the problem of the other deliberate inconsistency I am responsible
for in userland scalar type declarations, namely that the non-nullable
variety of those reject null as a valid value, unlike internal functions
which will happily coerce it. It's funny to mention this here, as the
E_WARNING + NULL behaviour your RFC would drop was a primary
justification of mine for making NULL special here. Unfortunately it's
not the only case, I'm sure uncountably much PHP code relies on things
like strlen($_GET['nonexistent']) working… but I digress.

Thanks,
Andrea

5 years ago by Nicolas Grekas — view source

unread

Hi Nikita

https://wiki.php.net/rfc/consistent_type_errors

Would it make sense and be possible to trigger a deprecation notice in PHP
7.4?

That might help the ecosystem move forward in a smooth way instead of
experimenting the failure when actually moving to 8.

Nicolas

5 years ago by Nikita Popov — view source

unread

On Wed, Feb 6, 2019 at 7:30 AM Nicolas Grekas nicolas.grekas@gmail.com
wrote:

Hi Nikita

https://wiki.php.net/rfc/consistent_type_errors

Would it make sense and be possible to trigger a deprecation notice in PHP
7.4?

That might help the ecosystem move forward in a smooth way instead of
experimenting the failure when actually moving to 8.

Nicolas

I don't think so. We generally consider a warning a "harder" error than a
deprecation (unlike deprecations, they are part of the default
error_reporting level), so I don't think replacing or adding a deprecation
would do much here. The only way you can be unaware of problems resulting
from this change is if you are deliberately suppressing warnings, in which
case chances are very good that you are also suppressing deprecations.

Nikita

5 years ago by Nicolas Grekas — view source

unread

https://wiki.php.net/rfc/consistent_type_errors

Would it make sense and be possible to trigger a deprecation notice in
PHP 7.4?

That might help the ecosystem move forward in a smooth way instead of
experimenting the failure when actually moving to 8.

We generally consider a warning a "harder" error than a deprecation

Do all situations that will throw after the RFC trigger a warning right
now? Eg var_dump(substr(null, 1)); doesn't trigger anything but returns
false - does that mean it will not throw a TypeError in your proposal?

if you are deliberately suppressing warnings, in which case chances are
very good that you are also suppressing deprecations.

This assumption is wrong in the Symfony ecoystem!
We built the deprecation framework around silenced (and unsilenced)
deprecations, which are all caught and logged.
So it would make a huge difference actually, at least for situations where
no warning is triggered (see substr example above).

Nicolas

5 years ago by Nikita Popov — view source

unread

On Wed, Feb 6, 2019 at 9:43 AM Nicolas Grekas nicolas.grekas@gmail.com
wrote:

https://wiki.php.net/rfc/consistent_type_errors

Would it make sense and be possible to trigger a deprecation notice in
PHP 7.4?

That might help the ecosystem move forward in a smooth way instead of
experimenting the failure when actually moving to 8.

We generally consider a warning a "harder" error than a deprecation

Do all situations that will throw after the RFC trigger a warning right
now? Eg var_dump(substr(null, 1)); doesn't trigger anything but returns
false - does that mean it will not throw a TypeError in your proposal?

That's correct, it will not throw a TypeError. If the call currently
succeeds, then it will continue to succeed. substr(null, 1) is considered a
perfectly valid substr() call right now.

if you are deliberately suppressing warnings, in which case chances are

very good that you are also suppressing deprecations.

This assumption is wrong in the Symfony ecoystem!
We built the deprecation framework around silenced (and unsilenced)
deprecations, which are all caught and logged.
So it would make a huge difference actually, at least for situations where
no warning is triggered (see substr example above).

Right, but a warning is always triggered here. If there were no warning,
then yes, I'd of course agree that this should throw a deprecation first.

Nikita

5 years ago by Nikita Popov — view source

unread

Hi Nikita,

Nikita Popov wrote:

I'd like to bring forward the following proposal for PHP 8, which will
make
(zpp) parameter parsing failures always result in a TypeError (rather
than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

I like this proposal. IMO PHP's E_WARNING + NULL is the worst of its
“Keep Calm and Carry On” (sorry) behaviours, it would be nice to get rid
of it for good, rather than just in the comfy world of strict_types=1.

The goal here is to remove one of the inconsistencies between
user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regrettably, as I pointed out to you via another channel, that idea also
faces the problem of the other deliberate inconsistency I am responsible
for in userland scalar type declarations, namely that the non-nullable
variety of those reject null as a valid value, unlike internal functions
which will happily coerce it. It's funny to mention this here, as the
E_WARNING + NULL behaviour your RFC would drop was a primary
justification of mine for making NULL special here. Unfortunately it's
not the only case, I'm sure uncountably much PHP code relies on things
like strlen($_GET['nonexistent']) working… but I digress.

Right, thanks for pointing that one out. We could either resolve this by
making the behavior of internal functions consistent with user-defined ones
here (which would be another BC break for another RFC), or alternatively we
could make the arginfo handling for internal functions consistent with zpp.
I already mentioned previously that we'll have to drop arginfo type checks
for internal functions if we want to get serious about adding those type
annotations, otherwise we'd be paying for duplicate type checks. That would
leave us with just the zpp behavior, which allows null for scalars. It's
not great, but probably much better than functions either accepting null or
not depending on whether they use arginfo.

Nikita

5 years ago by Nikita Popov — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will
make (zpp) parameter parsing failures always result in a TypeError (rather
than generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

Regards,
Nikita

Heads up: This is headed for voting on Tuesday, assuming no significant
issues are raised in the meantime.

Nikita

5 years ago by Ben Ramsey — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will make
(zpp) parameter parsing failures always result in a TypeError (rather than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

I like this RFC, and from a user perspective, this consistency is much-needed.

While warnings don’t affect continued processing, uncaught TypeErrors do. What is the recommended upgrade path for users? How will this affect the silence @-operator, since I’m sure many users use that to squelch these warnings, and they accept the null or false return value as acceptable for processing the rest of the script.

Cheers,
Ben

5 years ago by Benjamin Eberlei — view source

unread

Ben Ramsey ben@benramsey.com schrieb am Sa. 16. Feb. 2019 um 18:35:

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will
make
(zpp) parameter parsing failures always result in a TypeError (rather
than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between
user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

I like this RFC, and from a user perspective, this consistency is
much-needed.

While warnings don’t affect continued processing, uncaught TypeErrors do.
What is the recommended upgrade path for users? How will this affect the
silence @-operator, since I’m sure many users use that to squelch these
warnings, and they accept the null or false return value as acceptable for
processing the rest of the script.

Cheers,
Ben

This exception is only for argument errors. Fopen would continue to warn +
false when file doesnt exist, which is what @ is nostly used for (using
fopen as example)

5 years ago by Ben Ramsey — view source

unread

This exception is only for argument errors. Fopen would continue to warn + false when file doesnt exist, which is what @ is nostly used for (using fopen as example)

I’ve seen many cases where it’s understood that the function might receive the wrong type, and the developer decides this is okay, so they use the @-operator to ignore the warning and allow the script to continue processing.

5 years ago by Nikita Popov — view source

unread

Hi internals,

I'd like to bring forward the following proposal for PHP 8, which will
make
(zpp) parameter parsing failures always result in a TypeError (rather
than
generating a warning+null, depending on circumstances):

https://wiki.php.net/rfc/consistent_type_errors

The goal here is to remove one of the inconsistencies between
user-defined
and internal functions, and to put us in a position where we can actually
start specifying type information in arginfo without fear of breaking
things.

I like this RFC, and from a user perspective, this consistency is
much-needed.

While warnings don’t affect continued processing, uncaught TypeErrors do.
What is the recommended upgrade path for users? How will this affect the
silence @-operator, since I’m sure many users use that to squelch these
warnings, and they accept the null or false return value as acceptable for
processing the rest of the script.

Cheers,
Ben

The recommended upgrade path here is basically "deal with it". If a
codebase does this (intentionally calling functions with invalid params and
suppressing errors) a lot, then it might make sense to register an error
handler that collects all "expected parameter" style errors in a production
environment, to easily identify all code that needs to be fixed. The error
messages have a consistent format, so it should be simple to identify and
eliminate all warnings of this type.

Nikita

5 years ago by Christian Schneider — view source

unread

Am 17.02.2019 um 17:24 schrieb Nikita Popov nikita.ppv@gmail.com:

The recommended upgrade path here is basically "deal with it". If a
codebase does this (intentionally calling functions with invalid params and
suppressing errors) a lot, then it might make sense to register an error
handler that collects all "expected parameter" style errors in a production
environment, to easily identify all code that needs to be fixed. The error
messages have a consistent format, so it should be simple to identify and
eliminate all warnings of this type.

I have an example where this might be harder than necessary:
I'm importing data from an external source. Now in the real-world the import data files can once in a blue moon contain bogus data, e.g. there could be an array instead of a string in one single entry and I'm doing a strlen() on it.

Previously this triggered a Warning but the import job completed for all the other entries while still informing me that something was amiss.

The new way leaves me with three options:

My import jobs stops at the single broken entry
I can add type-checks for every single field
I can start wrapping code with try/catch

Neither of the three options are very appealing to me.
Option 1) might be appropriate when you are importing data which life and death depends on but for lots of stuff you're more interested in having the other 99% of your data updated instead of failing completely.
Option 2) sounds like the most defensive way of programming but it adds lots of boiler-plate code and doesn't feel phpish to me.
Option 3) is Java's RuntimeExceptions madness all over again which is surprisingly had to get right (where to catch what).

I'm a bit surprised the PHP community had such a change of mind concerning Exception in the core and BC breaks over the last years but as the vote seems very much to be in favour of this change I guess I'll have to bite the bullet once PHP 8 arrives.

Regards,

Chris

5 years ago by Rowan Collins — view source

unread

I have an example where this might be harder than necessary:
I'm importing data from an external source. Now in the real-world the
import data files can once in a blue moon contain bogus data, e.g.
there could be an array instead of a string in one single entry and I'm
doing a strlen() on it.

Previously this triggered a Warning but the import job completed for
all the other entries while still informing me that something was
amiss.

The problem with Warnings is that processing doesn't just continue for all the other entries, it continues with this entry that turned out to be bogus. So now rather than a missing record, you have a record containing a 0, or the word "array", etc; and the Warning probably won't tell you which record it was that failed.

The new way leaves me with three options:

My import jobs stops at the single broken entry

I can add type-checks for every single field

I can start wrapping code with try/catch

Option 2 can be stated differently as "add validation for each input record": this isn't the same kind of type check as a library function picking up coding mistakes, it's handling of untrusted input.

Option 3 can be thought of as the transactional approach: if an error happens within a logical record, you can log that record to a reject file and continue with the next record.

There is definitely more effort to doing one of these compared to not doing it, but both seem preferable to letting bad data through, whether the language forces you to do something or not.

Regards,

--
Rowan Collins
[IMSoP]