# [VOTE] Undefined Variable Error Promotion - Thread: https://externals.io/message/117336 - Original archive: https://news-web.php.net/php.internals/117336 - Started by come@chilliet.eu on 2022-03-15T16:54:11+00:00 - Messages: 13 - 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 --- ## #117336 — come@chilliet.eu — 2022-03-15T16:54:11+00:00 URL: https://externals.io/message/117336#117336 Le lundi 14 mars 2022, 18:18:55 CET Mark Randall a écrit : > I have started the vote for promoting undefined variable access to throw > an Error exception. > > The vote will run for 2 weeks until March 28th 2022. > > https://wiki.php.net/rfc/undefined_variable_error_promotion This does not mention properties, are they concerned by the change? What about array keys? Côme --- ## #117337 — Christoph M. Becker — 2022-03-15T18:17:12+00:00 URL: https://externals.io/message/117336#117337 · In reply to: #117336 > Le lundi 14 mars 2022, 18:18:55 CET Mark Randall a écrit : > > > […] > > This does not mention properties, are they concerned by the change? What about array keys? The RFC states: | If the code does not currently emit a “Warning: Undefined variable | $varname” then it is out of scope for this RFC. This RFC does NOT | apply to array indexes. That explicitly excludes undefined array indexes, and apparently also excludes undefined properties (which raise "Warning: Undefined property:") -- Christoph M. Becker --- ## #117338 — Miguel Rosales — 2022-03-15T19:07:52+00:00 URL: https://externals.io/message/117336#117338 · In reply to: #117337 Christoph M. Becker wrote on 15/3/22 19:17: > > > > […] > The RFC states: > > | If the code does not currently emit a “Warning: Undefined variable > | $varname” then it is out of scope for this RFC. This RFC does NOT > | apply to array indexes. > > That explicitly excludes undefined array indexes, and apparently also > excludes undefined properties (which raise "Warning: Undefined property:") > AFAIU error promotion for undefined properties has already been covered by the Deprecate Dynamic Properties RFC , which will start throwing errors in 9.0. --- ## #117339 — Sara Golemon — 2022-03-15T19:32:49+00:00 URL: https://externals.io/message/117336#117339 · In reply to: #117338 On Tue, Mar 15, 2022 at 2:08 PM Miguel Rosales wrote: > > […] property:") > > […] > AFAIU error promotion for undefined properties has already been covered > by the Deprecate Dynamic Properties RFC > , which will > start throwing errors in 9.0. > What I'm hearing is that we also need an RFC for promoting 'Undefined index...' warnings to Errors as well. -Sara --- ## #117340 — Mark Randall — 2022-03-15T20:35:54+00:00 URL: https://externals.io/message/117336#117340 · In reply to: #117339 > What I'm hearing is that we also need an RFC for promoting 'Undefined > index...' warnings to Errors as well. We do. If it would pass or not is another matter, it would need someone braver than I to take it up. I think there's some other engine stragglers, e.g. foreach must be type array|object comes to mind, that should really be an exception, maybe array to string conversion too. Who knows, maybe PHP 9 can be the version that removes remove *all* warnings from the zend engine. --- ## #117341 — Patrick ALLAERT — 2022-03-15T23:02:28+00:00 URL: https://externals.io/message/117336#117341 Hello Mark and the list, Le mar. 15 mars 2022 à 21:36, Mark Randall a écrit : > Who knows, maybe PHP 9 can be the version that removes remove *all* > warnings from the zend engine. > This is not far from the reason why I voted "no". I am not against the fact this warning becomes an error per se. I am just not very fan of cherry-picking an individual kind of problem (read: notice/warning/error/...) and changing it without a more global frame. While it may not be a good practice. It's not that we cannot recover from that, it is more that we don't want to. This doesn't match really the notion of "error" which is not very much documented, apart from https://www.php.net/manual/en/errorfunc.constants.php: "E_ERROR: Fatal run-time errors. These indicate errors that can not be recovered from, such as a memory allocation problem. Execution of the script is halted." Currently `E_WARNING` is documented as: "Run-time warnings (non-fatal errors). Execution of the script is not halted." This describes how the core reacts, not when it is supposed to happen. This could potentially be extended to: "Run-time warnings (non-fatal errors) that are likely to denote bad programming practice (like the use of something undefined) that are usually solved with code changes. Execution of the script is not halted." But then this wouldn't be true anymore if this RFC passes as it would result in: Undefined variables: Error Undefined index: Warning Undefined property: Warning IMHO, this leads to something more inconsistent than what we have now. I know other RFCs could target the other aspects. But there is no way to vote "yes, provided that all other RFCs under the same umbrella would pass for the same version". This is probably a limitation of our current RFC process, but in order to maintain as much consistency as possible, I see no other option than voting "no" even though I am +1 on the principle. Kind regards, Patrick --- ## #117343 — Christian Schneider — 2022-03-16T08:42:58+00:00 URL: https://externals.io/message/117336#117343 · In reply to: #117338 Am 15.03.2022 um 20:07 schrieb Miguel Rosales : > Christoph M. Becker wrote on 15/3/22 19:17: > > […] > AFAIU error promotion for undefined properties has already been covered by the Deprecate Dynamic Properties RFC >, which will start throwing errors in 9.0. That RFC covers the *creation* of undefined properties, not the *reading* of undefined properties. - Chris --- ## #117345 — Mark Randall — 2022-03-16T09:00:44+00:00 URL: https://externals.io/message/117336#117345 · In reply to: #117341 > I am not against the fact this warning becomes an error per se. I am just > not very fan of cherry-picking an individual kind of problem (read: > notice/warning/error/...) and changing it without a more global frame. I think we should try to hit all of them, ideally in time for PHP 9. That was the premise behind: https://externals.io/message/116918 It will likely take several RFCs until we've got it in the state we want it, some of those are more contentious than others (such as undefined array indexes). I think undefined property access should throw too, and will likely have supermajority support, but let's do it in a separate RFC. What I don't think we want is the entire package being blocked because a particular item (i.e. array keys) is contentious. --- ## #117346 — Christian Schneider — 2022-03-16T09:17:48+00:00 URL: https://externals.io/message/117336#117346 Am 16.03.2022 um 10:00 schrieb Mark Randall : > > > […] > > I think we should try to hit all of them, ideally in time for PHP 9. > > That was the premise behind: https://externals.io/message/116918 > > It will likely take several RFCs until we've got it in the state we want it, some of those are more contentious than others (such as undefined array indexes). > > I think undefined property access should throw too, and will likely have supermajority support, but let's do it in a separate RFC. > > What I don't think we want is the entire package being blocked because a particular item (i.e. array keys) is contentious. This is actually exactly what Patrick is complaining about: No global plan. I know I'm on the "losing" side of this discussion but sometimes it feels like one of the main arguments is "we already changed this and that to exceptions, now we have to do it in this case too". Which, to me, fits the first paragraph of https://en.wikipedia.org/wiki/Salami_slicing_tactics Maybe we should ask ourselves the question: Why would the entire package be blocked? Just because it is too big or maybe there *are* subtleties which have not been properly resolved? - Chris --- ## #117347 — Mark Randall — 2022-03-16T10:39:03+00:00 URL: https://externals.io/message/117336#117347 · In reply to: #117346 > Maybe we should ask ourselves the question: Why would the entire package be blocked? Just because it is too big or maybe there *are* subtleties which have not been properly resolved? It's politics and the practicalities of getting things done. An all-or-nothing approach will inevitably end up with nothing, because those who are opposed on principle need only rally around attacking the weakest element, without the need to address any of the other parts which might individually be much stronger. That's not to say that the weakest elements shouldn't be debated, they should. But we shouldn't sabotage ourselves, and bring about a state of permanent project paralysis, by mandating that semi-related weaker changes are bundled with stronger ones and must be voted on as a whole. It would just open the door to the constant use of a poison pill argument. --- ## #117348 — Christian Schneider — 2022-03-16T11:00:32+00:00 URL: https://externals.io/message/117336#117348 Am 16.03.2022 um 11:39 schrieb Mark Randall : > > > […] > > It's politics and the practicalities of getting things done. A nice description of salami slicing tactics ;-) > An all-or-nothing approach will inevitably end up with nothing, because those who are opposed on principle need only rally around attacking the weakest element, without the need to address any of the other parts which might individually be much stronger. > > That's not to say that the weakest elements shouldn't be debated, they should. > > But we shouldn't sabotage ourselves, and bring about a state of permanent project paralysis, by mandating that semi-related weaker changes are bundled with stronger ones and must be voted on as a whole. > > It would just open the door to the constant use of a poison pill argument. The problem is you are saying "the weakest should be debated but not right now, let's move forward anyway" and you are also stipulating that the changes are only semi-related. By doing that you're dismissing Patrick's notion that they are more closely related than you acknowledge because he's saying having inconsistent handling of undefined stuff is also a WTF. And at the same time I really hope once we start discussing undefined array indices you won't bring up consistency with undefined variables (and possibly object properties) as the reason to change it. That would feel weird given that you want to treat them as separate issues right now ;-) - Chris --- ## #117349 — Rowan Tommins — 2022-03-16T13:27:14+00:00 URL: https://externals.io/message/117336#117349 · In reply to: #117341 > This is not far from the reason why I voted "no". > > I am not against the fact this warning becomes an error per se. I am just > not very fan of cherry-picking an individual kind of problem (read: > notice/warning/error/...) and changing it without a more global frame. I have also voted no for similar reasons: the scope of this RFC appears to be "all the places that are convenient", rather than "all the places that are covered by the claimed rationale". I think it is sensible to raise an error for the kind the kind of "undefined foo" that is almost certainly a typo. I think we should be more cautious about the kind that could be lazy but intentional dynamic access. I would have voted Yes to an RFC which defined its scope in such terms. The language does not currently make that distinction consistently, and I don't think we should be planning an error on a basis as vague as "wherever we currently have a warning". For instance, in-place increment is covered, but array append is not: $undefined++; // Warning, planned Error $undefined[] = 1; // completely silent Similarly, variable-variables are covered, even though they are more similar to array keys, which are not: $foo = 'undefined'; $$foo++; // covered by RFC $array = []; $key = 'undefined'; $array[$key]++; // not covered by RFC None of this is even mentioned in the RFC, let alone justified. Regards, -- Rowan Tommins [IMSoP] --- ## #117351 — Peter Bowyer — 2022-03-17T07:51:56+00:00 URL: https://externals.io/message/117336#117351 · In reply to: #117349 > > > […] > > > I have also voted no for similar reasons: the scope of this RFC appears > to be "all the places that are convenient", rather than "all the places > that are covered by the claimed rationale". > > [snip] > > None of this is even mentioned in the RFC, let alone justified. > I have voted no for the same reasons. Peter