# Proposal: Raise severity of undefined constants - Thread: https://externals.io/message/81264 - Original archive: https://news-web.php.net/php.internals/81264 - Started by Rowan Collins on 2015-01-27T22:21:03+00:00 - Messages: 15 - 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 --- ## #81264 — Rowan Collins — 2015-01-27T22:21:03+00:00 URL: https://externals.io/message/81264#81264 Hi All, I would like to propose that the error given for an undefined constant should be raised from E_NOTICE, probably to E_RECOVERABLE_ERROR, in PHP 7. I'm happy to expand this into a proper RFC, and work on patches for the engine, tests, and spec, but want to see if initial reception to the idea is favourable first. Current behaviour: any bare word which is not a defined constant is treated as though it were a quoted string, with an `E_NOTICE` issued. Proposed behaviour: an `E_RECOVERABLE_ERROR` is issued; if the error is caught, either the existing string behaviour could be retained, or the value could be treated as NULL. (I'm open to other alternatives.) PROS 1. Consistency. An undefined class constant (e.g. Foo::BAR) gives a fatal error, but a normal global constant gives only a notice. Class constants being "more modern" made this seem reasonable, but with namespaces, it's even worse - a qualified namespace constant (e.g. Foo\BAR) gives a fatal error, as does a global constant in namespace notation (e.g. \BAR); but a namespace constant referenced relative to the current namespace is a bare word, and gives only a notice (e.g. namespace Foo; const BAR=42; echo BAAR;). Functions and classes also give fatal errors if used without declaring; variables do not, but this is more useful, because they can meaningfully be created as null, and there isn't a syntax to declare a local variable, only to initialise it. 2. Inherent severity. It's probably relatively common to turn off `E_NOTICE` in error_reporting, or to miss one notice among many others. But an undefined constant magically turning into a string could cause relatively major problems. Firstly, a constant is likely to have some value which needs to be used or matched somewhere else. A READ_ONLY flag mis-typed as REED_ONLY could be interpreted as int(0) (i.e. intval('REED_ONLY')) and cause a catastrophic error. Secondly, keywords are also bare words, and any expression can be a statement. Thus the following is an infinite loop, no matter what blahblah() returns: while(true) { if ( blahblah() ) { brek; } } Obviously, there are other mistakes which could cause such errors, but this is one that the engine could easily pick up on and protect the user. CONS Like most changes, the downside is a break in compatibility. As far as I can make out, the current behaviour is actually for compatibility back as far as PHP 3 (correct me if I'm wrong, I didn't dig far). I'm not aware of any notice officially deprecating barewords-as-strings, but nor can I find many references to it in the manual at all. Its use for array keys has been explicitly discouraged in the manual since some time in 2001. [1] Outside code golf, I'd be very surprised if any code written or updated in the last 10 years deliberately uses this "feature", and I think changing it in PHP 7 is justifiable given the benefits to maintainers of even the simplest non-OO, non-namespaced code. What do people think? [1] http://web.archive.org/web/20010614144157/http://www.php.net/manual/en/language.types.array.php#language.types.array.donts Regards, -- Rowan Collins [IMSoP] --- ## #81267 — Andrea Faulds — 2015-01-28T00:19:02+00:00 URL: https://externals.io/message/81264#81267 · In reply to: #81264 Hey Rowan, I actually was thinking about doing this myself, if function/constant autoloading passed. That would mean that if you really needed this behaviour for some reason, you could write a constant autoloader that reimplements it. One concern I’d have is the use of constants in constant scalar expressions. In implementation, we’d need to make sure those produce an error, not just normal constant usage. Anyway, I’m very much in favour of this. Please go ahead! Thanks. -- Andrea Faulds http://ajf.me/ --- ## #81281 — francois@tekwire.net — 2015-01-28T05:04:15+00:00 URL: https://externals.io/message/81264#81281 · In reply to: #81264 > De : Rowan Collins [mailto:rowan.collins@gmail.com] > I would like to propose that the error given for an undefined constant > should be raised from E_NOTICE, probably to E_RECOVERABLE_ERROR, in > PHP 7. +1 --- ## #81288 — Ferenc Kovacs — 2015-01-28T07:20:50+00:00 URL: https://externals.io/message/81264#81288 · In reply to: #81281 2015.01.28. 6:04 ezt írta ("François Laupretre" ): > > > […] > > +1 > I don't like that. Depending if we want to keep the behavior on the long term I would either go with `E_WARNING` or `E_DEPRECATED` Turning it into `E_RECOVERABLE_ERROR` would be for most users just as bad as removing it while we still have to support the behavior so no real gain for us. --- ## #81289 — Yasuo Ohgaki — 2015-01-28T07:24:41+00:00 URL: https://externals.io/message/81264#81289 · In reply to: #81264 Hi Rowan, On Wed, Jan 28, 2015 at 7:21 AM, Rowan Collins wrote: > What do people think? I would like to vote +1 for this indeed. My concern is that there are too many users ignore E_NOTICE. Use of undefined constant is fatal error IMHO, though. I will not vote and let people decide. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81290 — Benjamin Eberlei — 2015-01-28T08:43:47+00:00 URL: https://externals.io/message/81264#81290 · In reply to: #81264 On Tue, Jan 27, 2015 at 11:21 PM, Rowan Collins wrote: > Hi All, > > I would like to propose that the error given for an undefined constant > should be raised from E_NOTICE, probably to E_RECOVERABLE_ERROR, in PHP 7. > > I'm happy to expand this into a proper RFC, and work on patches for the > engine, tests, and spec, but want to see if initial reception to the idea > is favourable first. > > Current behaviour: any bare word which is not a defined constant is > treated as though it were a quoted string, with an `E_NOTICE` issued. > > Proposed behaviour: an `E_RECOVERABLE_ERROR` is issued; if the error is > caught, either the existing string behaviour could be retained, or the > value could be treated as NULL. (I'm open to other alternatives.) > > PROS > > 1. Consistency. > > An undefined class constant (e.g. Foo::BAR) gives a fatal error, but a > normal global constant gives only a notice. Class constants being "more > modern" made this seem reasonable, but with namespaces, it's even worse - a > qualified namespace constant (e.g. Foo\BAR) gives a fatal error, as does a > global constant in namespace notation (e.g. \BAR); but a namespace constant > referenced relative to the current namespace is a bare word, and gives only > a notice (e.g. namespace Foo; const BAR=42; echo BAAR;). > > Functions and classes also give fatal errors if used without declaring; > variables do not, but this is more useful, because they can meaningfully be > created as null, and there isn't a syntax to declare a local variable, only > to initialise it. > > 2. Inherent severity. > > It's probably relatively common to turn off `E_NOTICE` in error_reporting, > or to miss one notice among many others. But an undefined constant > magically turning into a string could cause relatively major problems. > > Firstly, a constant is likely to have some value which needs to be used or > matched somewhere else. A READ_ONLY flag mis-typed as REED_ONLY could be > interpreted as int(0) (i.e. intval('REED_ONLY')) and cause a catastrophic > error. > > Secondly, keywords are also bare words, and any expression can be a > statement. Thus the following is an infinite loop, no matter what > blahblah() returns: > while(true) { > if ( blahblah() ) { > brek; > } > } > > Obviously, there are other mistakes which could cause such errors, but > this is one that the engine could easily pick up on and protect the user. > > > CONS > > Like most changes, the downside is a break in compatibility. > > As far as I can make out, the current behaviour is actually for > compatibility back as far as PHP 3 (correct me if I'm wrong, I didn't dig > far). > > I'm not aware of any notice officially deprecating barewords-as-strings, > but nor can I find many references to it in the manual at all. Its use for > array keys has been explicitly discouraged in the manual since some time in > 2001. [1] > > Outside code golf, I'd be very surprised if any code written or updated in > the last 10 years deliberately uses this "feature", and I think changing it > in PHP 7 is justifiable given the benefits to maintainers of even the > simplest non-OO, non-namespaced code. > > > What do people think? > I think this is too big a BC break. The usage of $array[key] = 0 instead of "key" is widespread. > > [1] http://web.archive.org/web/20010614144157/http://www.php. > net/manual/en/language.types.array.php#language.types.array.donts > > Regards, > > -- > Rowan Collins > [IMSoP] --- ## #81298 — Rowan Collins — 2015-01-28T10:34:17+00:00 URL: https://externals.io/message/81264#81298 · In reply to: #81290 Benjamin Eberlei wrote on 28/01/2015 08:43: > I think this is too big a BC break. > > The usage of $array[key] = 0 instead of "key" is widespread. As I mentioned, the advice *not* to write that has been in the manual since 2001, so I can't think why any tutorial or example code would teach it as the right way, but it is possible, I suppose. I realise the majority of code is not available publicly, but it would be good to make some attempt to judge whether it really is widespread, so if you have any examples or way of measuring it, that would be useful. Regards, -- Rowan Collins [IMSoP] --- ## #81299 — Rowan Collins — 2015-01-28T10:48:57+00:00 URL: https://externals.io/message/81264#81299 · In reply to: #81288 Ferenc Kovacs wrote on 28/01/2015 07:20: > > > 2015.01.28. 6:04 ezt írta ("François Laupretre" >): > > […] > ] > > […] > > I don't like that. > Depending if we want to keep the behavior on the long term I would > either go with `E_WARNING` or `E_DEPRECATED` > Turning it into `E_RECOVERABLE_ERROR` would be for most users just as > bad as removing it while we still have to support the behavior so no > real gain for us. > `E_DEPRECATED` is likely to be even more ignored than E_NOTICE, so would be a step backwards; my whole assumption is that most people getting the message are not using a deprecated feature, but accidentally mis-typing a constant or keyword. I'd be happy with raising it to `E_WARNING` to make it more noticeable, though, if we don't think removing the functionality is acceptable. On the other hand, if the concern is having to support it, then it could just be E_ERROR. The reason I chose `E_RECOVERABLE_ERROR` is because that had recently been adopted for other things, such as invalid method calls. As I mentioned in my original e-mail, the recovery behaviour needn't be the existing string interpretation, but could just be to treat the value as NULL, which would probably be simpler to implement. Personally, I've never actually recovered an E_RECOVERABLE_ERROR, so am not sure what use cases we should be supporting. Regards, -- Rowan Collins [IMSoP] --- ## #81302 — Yasuo Ohgaki — 2015-01-28T11:21:00+00:00 URL: https://externals.io/message/81264#81302 · In reply to: #81299 Hi Rowan, On Wed, Jan 28, 2015 at 7:48 PM, Rowan Collins wrote: > On the other hand, if the concern is having to support it, then it could > just be E_ERROR. The reason I chose `E_RECOVERABLE_ERROR` is because that had > recently been adopted for other things, such as invalid method calls. As I > mentioned in my original e-mail, the recovery behaviour needn't be the > existing string interpretation, but could just be to treat the value as > NULL, which would probably be simpler to implement. Personally, I've never > actually recovered an E_RECOVERABLE_ERROR, so am not sure what use cases we > should be supporting. Please don't use E_ERROR. It cannot be catched by user error handler now. If user error handler can catch it, then I don't mind it. E_RECOVERBLE_ERROR is better than E_ERROR. I use user defined error handler always for immediate notification. I use log analyzer also, but it has time lag. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #81303 — Andrey Andreev — 2015-01-28T11:43:53+00:00 URL: https://externals.io/message/81264#81303 · In reply to: #81298 HI, > Benjamin Eberlei wrote on 28/01/2015 08:43: > > […] > > > As I mentioned, the advice *not* to write that has been in the manual since > 2001, so I can't think why any tutorial or example code would teach it as > the right way, but it is possible, I suppose. > > I realise the majority of code is not available publicly, but it would be > good to make some attempt to judge whether it really is widespread, so if > you have any examples or way of measuring it, that would be useful. From what I've seen, most developers seem to put everything in quotes - strings (obviously), integers, floats, even numeric array keys ... boolean and null being the only exceptions (but I've seen that too). No opinion on the subject, just my observations. Cheers, Andrey. --- ## #81305 — Ferenc Kovacs — 2015-01-28T12:13:40+00:00 URL: https://externals.io/message/81264#81305 · In reply to: #81299 On Wed, Jan 28, 2015 at 11:48 AM, Rowan Collins wrote: > Ferenc Kovacs wrote on 28/01/2015 07:20: > > > […] > `E_DEPRECATED` is likely to be even more ignored than E_NOTICE, so would be > a step backwards; my whole assumption is that most people getting the > message are not using a deprecated feature, but accidentally mis-typing a > constant or keyword. I'd be happy with raising it to `E_WARNING` to make it > more noticeable, though, if we don't think removing the functionality is > acceptable. > > turn it into `E_WARNING` if we are planning to keep the feature, turn it into `E_DEPRECATED` if we want to remove the feature later on. > On the other hand, if the concern is having to support it, then it could > just be E_ERROR. The reason I chose `E_RECOVERABLE_ERROR` is because that had > recently been adopted for other things, such as invalid method calls. As I > mentioned in my original e-mail, the recovery behaviour needn't be the > existing string interpretation, but could just be to treat the value as > NULL, which would probably be simpler to implement. Personally, I've never > actually recovered an E_RECOVERABLE_ERROR, so am not sure what use cases we > should be supporting. ok, I was assuming that by turning it into `E_RECOVERABLE_ERROR` you would still keep the assuming string behavior if there is a custom error handler which returns true for this error. I think if we want to remove the assuming string feature, we should handle it the same way as we handle referencing undefined function call or class reference(and for consistency the same should be considered for "Undefined variable", which is also an `E_NOTICE` atm). at this point I think I would -1 on removing the assuming this, +0.5 on turning it into `E_DEPRECATED` and +1 on turning the "Use of undefined constant"/"Undefined variable" errors into E_WARNING. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #81308 — Rowan Collins — 2015-01-28T15:42:31+00:00 URL: https://externals.io/message/81264#81308 · In reply to: #81305 Ferenc Kovacs wrote on 28/01/2015 12:13: > > `E_DEPRECATED` is likely to be even more ignored than E_NOTICE, so > would be a step backwards; my whole assumption is that most people > getting the message are not using a deprecated feature, but > accidentally mis-typing a constant or keyword. I'd be happy with > raising it to `E_WARNING` to make it more noticeable, though, if we > don't think removing the functionality is acceptable. > > > turn it into `E_WARNING` if we are planning to keep the feature, turn it > into `E_DEPRECATED` if we want to remove the feature later on. I see absolutely no advantage in changing this into E_DEPRECATED, as it's even more likely to be hidden or ignored than E_NOTICE. There are actually two parts to the message: that you used an undefined constant, and that PHP interpreted this as you wanting a string. If you really intended a string, then "deprecated" is relevant; but if you intended a keyword or constant, then there is nothing to deprecate, it's just an error. If we want to keep the string fallback for now, how about we raise the severity to E_WARNING, and change the message to mention deprecation? e.g. Warning: Use of undefined constant foo - assumed 'foo', but this behaviour will change in a future version Regards, -- Rowan Collins [IMSoP] --- ## #81309 — Rowan Collins — 2015-01-28T15:49:20+00:00 URL: https://externals.io/message/81264#81309 · In reply to: #81305 Ferenc Kovacs wrote on 28/01/2015 12:13: > > ok, I was assuming that by turning it into `E_RECOVERABLE_ERROR` you > would still keep the assuming string behavior if there is a custom > error handler which returns true for this error. I'm undecided on whether the string value or null would make more sense, and listed both possibilities in my original post: > Proposed behaviour: an `E_RECOVERABLE_ERROR` is issued; if the error is caught, either the existing string behaviour could be retained, or the value could be treated as NULL. (I'm open to other alternatives.) Returning null would probably allow the engine code to be simplified, but returning the string would allow people to write compatibility layers using a custom error handler. Regards, -- Rowan Collins [IMSoP] --- ## #81310 — Ferenc Kovacs — 2015-01-28T16:42:32+00:00 URL: https://externals.io/message/81264#81310 · In reply to: #81308 On Wed, Jan 28, 2015 at 4:42 PM, Rowan Collins wrote: > Ferenc Kovacs wrote on 28/01/2015 12:13: > > > […] > > I see absolutely no advantage in changing this into E_DEPRECATED, as it's > even more likely to be hidden or ignored than E_NOTICE. > removing a feature (which is present in the language since 4.0, probably even previously) without marking it deprecated first would be a rude move imo. so if we want to remove it, I can't see any other way. > > There are actually two parts to the message: that you used an undefined > constant, and that PHP interpreted this as you wanting a string. If you > really intended a string, then "deprecated" is relevant; but if you > intended a keyword or constant, then there is nothing to deprecate, it's > just an error. > yeah, the problem is that we can't tell, what was the intention from the author, so while I can see how this behavior can cause huge WTF moments for our users (http://3v4l.org/QW8qV), but there are still a bunch of code out there relying on it. having a php version where it is `E_DEPRECATED` allows people to upgrade to that version, log the deprecated messages and fix their code. (ofc. you can also try to detect this specific usage with static analysis, but that would still miss dynamic usages like eval). > > If we want to keep the string fallback for now, how about we raise the > severity to E_WARNING, and change the message to mention deprecation? > e.g. > Warning: Use of undefined constant foo - assumed 'foo', but this behaviour > will change in a future version I don't like mixing the deprecation notice in the string but maybe it's just me. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #81312 — Rowan Collins — 2015-01-28T17:28:08+00:00 URL: https://externals.io/message/81264#81312 · In reply to: #81310 Ferenc Kovacs wrote on 28/01/2015 16:42: > > > On Wed, Jan 28, 2015 at 4:42 PM, Rowan Collins > > wrote: > > Ferenc Kovacs wrote on 28/01/2015 12:13: > > > `E_DEPRECATED` is likely to be even more ignored than > E_NOTICE, so > would be a step backwards; my whole assumption is that > most people > getting the message are not using a deprecated feature, but > accidentally mis-typing a constant or keyword. I'd be > happy with > raising it to `E_WARNING` to make it more noticeable, > though, if we > don't think removing the functionality is acceptable. > > > turn it into `E_WARNING` if we are planning to keep the feature, > turn it into `E_DEPRECATED` if we want to remove the feature > later on. > > > I see absolutely no advantage in changing this into E_DEPRECATED, > as it's even more likely to be hidden or ignored than E_NOTICE. > > > removing a feature (which is present in the language since 4.0, > probably even previously) without marking it deprecated first would be > a rude move imo. > so if we want to remove it, I can't see any other way. The only documentation I can find mentioning it explicitly discourages is it, so I don't think it's a huge stretch to say that it's already deprecated, but if we feel that's not enough, we may not want to jump straight to fatal. However, I don't think changing to `E_DEPRECATED` is the only, or best, way of warning people of future removal. > (ofc. you can also try to detect this specific usage with static > analysis, but that would still miss dynamic usages like eval). Actually, I was going to mention that it's quite hard to spot with static analysis, because like classes and functions, you have to chase includes to know what's defined. Unless you look for bare words which aren't all caps, I suppose. > If we want to keep the string fallback for now, how about we raise > the severity to E_WARNING, and change the message to mention > deprecation? > e.g. > Warning: Use of undefined constant foo - assumed 'foo', but this > behaviour will change in a future version > > > I don't like mixing the deprecation notice in the string but maybe > it's just me. I know it's unusual, but I think really there are two messages we're expressing: "Warning: Undefined constant" and "Deprecated: Using unquoted string". Unless we issue both at once (which seems unnecessarily confusing), I think the higher of the two severities should be used. But maybe that's just me, I'll see what others think. Regards, -- Rowan Collins [IMSoP]