Improve a few compile-time error messages #6385
Conversation
| @@ -11,4 +11,4 @@ try { | |||
|
|
|||
| ?> | |||
| --EXPECT-- | |||
| Illegal offset type | |||
| Illegal array key type: Closure | |||
There was a problem hiding this comment.
There is a similar compile-time error message, so I decided to also change the run-time ones as well.
There was a problem hiding this comment.
Also, leaving the : felt a little bit weird for me, but I'm ok to do so for consistency with other messages.
There was a problem hiding this comment.
Is that consistent with other messages though? I would expect this to be something like Illegal array key of type Closure.
There was a problem hiding this comment.
No, not exactly. And the Illegal array key of type Closure format is the best; unfortunately, it didn't come to my mind :)
Speaking about which branch to target: I was also unsure which branch to use. I first thought that the changes won't be very offensive, but it turned out that they are, so I agree, let's instead go for PHP 8.1.
| @@ -7,4 +7,4 @@ function foo () { | |||
| } | |||
| ?> | |||
| --EXPECTF-- | |||
| Fatal error: 'break' operator accepts only positive integers in %sbreak_error_001.php on line 3 | |||
| Fatal error: The break statement accepts only an integer argument greater than or equal to 0 in %s on line %d | |||
There was a problem hiding this comment.
The second half of the change is for consistency with the value error messages, but I don't insist on it too much in this case.
| @@ -9,4 +9,4 @@ use Blah\Exception; | |||
| use Blah\Ex; | |||
| ?> | |||
| --EXPECTF-- | |||
| Fatal error: Cannot use Blah\Ex as Ex because the name is already in use in %sbug42859.php on line 6 | |||
| Fatal error: Cannot import class Blah\Ex as Ex, Ex has already been declared in %s on line %d | |||
There was a problem hiding this comment.
So I normalized the different error messages related to the use statement to use import. i think this change doesn't make them significantly less clear, but I felt that they are a bit more "smooth". So all in all, I don't mind whichever expression we end up with.
| @@ -7,4 +7,4 @@ interface stdClass { } | |||
|
|
|||
| ?> | |||
| --EXPECTF-- | |||
| Fatal error: Cannot declare interface stdClass, because the name is already in use in %s on line %d | |||
| Fatal error: interface stdClass has already been declared in %s on line %d | |||
There was a problem hiding this comment.
I wasn't sure if the the name is already in use part adds any useful context to the messages, so I got rid of it :) With the new format, it is slightly annoying that the symbol type doesn't start with a capitalized letter.
| @@ -5,4 +5,4 @@ Error message for isset(func()) | |||
| isset(1 + 1); | |||
| ?> | |||
| --EXPECTF-- | |||
| Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in %s on line %d | |||
| Fatal error: Cannot use isset() on the result of an expression (you can use "expression !== null" instead) in %s on line %d | |||
There was a problem hiding this comment.
I got rid of the Yoda-style condition, because is not the primary style, as far as I noticed so far (thankfully :D)
There was a problem hiding this comment.
Not super fan of the usage of you from the original message. Maybe something like:
| Fatal error: Cannot use isset() on the result of an expression (you can use "expression !== null" instead) in %s on line %d | |
| Fatal error: Cannot use isset() on the result of an expression ("expression !== null" can be used instead) in %s on line %d |
There was a problem hiding this comment.
Thanks! This change makes sense for me!
| @@ -8,4 +8,4 @@ var_dump($foo?->bar = 'bar'); | |||
|
|
|||
| ?> | |||
| --EXPECTF-- | |||
| Fatal error: Can't use nullsafe operator in write context in %s.php on line 4 | |||
| Fatal error: Cannot use the null-safe operator in write context in %s on line %d | |||
There was a problem hiding this comment.
I believe nullsafe is not the most correct form. At least, my built-in type checkers are always complaining. I'm not sure if null safe or null-safe is the best choice, though.
There was a problem hiding this comment.
null-safe would be the more grammatically correct version from my understanding as it is compound modifying the noun operator and therefore requires a hyphen according to various sources (https://www.grammarly.com/blog/hyphen/ https://www.gsbe.co.uk/grammar-the-hyphen.html https://dictionary.cambridge.org/grammar/british-grammar/hyphens)
Edit: might just be British English tho
There was a problem hiding this comment.
Thanks for the background info and the resources! These are always welcome :)
kocsismate
force-pushed
the
compile-error-messages
branch
from
d300136 to
12c5166
Compare
| @@ -11,4 +11,4 @@ try { | |||
|
|
|||
| ?> | |||
| --EXPECT-- | |||
| Illegal offset type | |||
| Illegal array key type: Closure | |||
There was a problem hiding this comment.
Is that consistent with other messages though? I would expect this to be something like Illegal array key of type Closure.
No description provided.