# [RFC] Consistent type errors for internal functions - Thread: https://externals.io/message/104175 - Original archive: https://news-web.php.net/php.internals/104175 - Started by Nikita Popov on 2019-02-05T11:22:24+00:00 - Messages: 17 - RFC: Consistent type errors for internal functions (Implemented (PHP 8.0)) — https://externals.io/rfc/consistent_type_errors.md - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #104175 — Nikita Popov — 2019-02-05T11:22:24+00:00 URL: https://externals.io/message/104175#104175 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 --- ## #104177 — Girgias — 2019-02-05T12:23:20+00:00 URL: https://externals.io/message/104175#104177 · In reply to: #104175 > 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 --- ## #104178 — Nikita Popov — 2019-02-05T13:29:17+00:00 URL: https://externals.io/message/104175#104178 · In reply to: #104177 > > > > […] > > 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 --- ## #104179 — Girgias — 2019-02-05T13:37:27+00:00 URL: https://externals.io/message/104175#104179 · In reply to: #104178 > > > > […] > > 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 > > […] > > 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 --- ## #104216 — Andrea Faulds — 2019-02-05T23:40:30+00:00 URL: https://externals.io/message/104175#104216 · In reply to: #104175 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 --- ## #104222 — Nicolas Grekas — 2019-02-06T06:30:07+00:00 URL: https://externals.io/message/104175#104222 · In reply to: #104175 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 --- ## #104227 — Nikita Popov — 2019-02-06T08:18:25+00:00 URL: https://externals.io/message/104175#104227 · In reply to: #104222 On Wed, Feb 6, 2019 at 7:30 AM Nicolas Grekas 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 --- ## #104228 — Nikita Popov — 2019-02-06T08:22:47+00:00 URL: https://externals.io/message/104175#104228 > Hi Nikita, > > Nikita Popov wrote: > > […] > make > > […] > than > > […] > > 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. > > > […] > user-defined > > […] > > 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 --- ## #104230 — Nicolas Grekas — 2019-02-06T08:43:02+00:00 URL: https://externals.io/message/104175#104230 · In reply to: #104227 > https://wiki.php.net/rfc/consistent_type_errors > > […] > > 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 --- ## #104232 — Nikita Popov — 2019-02-06T08:56:02+00:00 URL: https://externals.io/message/104175#104232 · In reply to: #104230 On Wed, Feb 6, 2019 at 9:43 AM Nicolas Grekas wrote: > > https://wiki.php.net/rfc/consistent_type_errors > > […] > > 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 --- ## #104446 — Nikita Popov — 2019-02-16T11:14:45+00:00 URL: https://externals.io/message/104175#104446 > 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 --- ## #104448 — Ben Ramsey — 2019-02-16T17:34:41+00:00 URL: https://externals.io/message/104175#104448 > > > 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 --- ## #104449 — Benjamin Eberlei — 2019-02-16T18:28:48+00:00 URL: https://externals.io/message/104175#104449 Ben Ramsey schrieb am Sa. 16. Feb. 2019 um 18:35: > > […] > make > > […] > than > > […] > user-defined > > […] > > > 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) --- ## #104450 — Ben Ramsey — 2019-02-16T18:37:19+00:00 URL: https://externals.io/message/104175#104450 > > > 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. --- ## #104453 — Nikita Popov — 2019-02-17T16:24:15+00:00 URL: https://externals.io/message/104175#104453 > > […] > make > > […] > than > > […] > user-defined > > […] > > > 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 --- ## #104536 — Christian Schneider — 2019-03-01T12:02:29+00:00 URL: https://externals.io/message/104175#104536 Am 17.02.2019 um 17:24 schrieb Nikita Popov : > 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: 1) My import jobs stops at the single broken entry 2) I can add type-checks for every single field 3) 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 --- ## #104544 — Rowan Collins — 2019-03-02T09:45:22+00:00 URL: https://externals.io/message/104175#104544 >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: >1) My import jobs stops at the single broken entry >2) I can add type-checks for every single field >3) 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]