Make error messages more consistent #5066
Conversation
kocsismate
force-pushed
the
consistent-error-messages
branch
from
c820870 to
a40a4ac
Compare
|
Not sure if I mentioned it to you before, but we have |
Ah, not yet! Thanks! To answer you all the review comments at once: first, I started with mass-capitalization, and during doing so, I noticed some problematic patterns for which I tried to come up with a solution. The first suggestion in the description is in line with your comments, but I'd like to only address them after we have decided about the possible directions and how far to go (e.g. in this case: to stick with lower-case, or try to circumvent the issue). So @nikic, @cmb69, @Girgias, if you are also interested in this topic, I'd love to hear your opinion about the proposals, and if you have anything else to add to the list. |
|
On the top of my head, the length error/warning messages should also be consistent, because we currently have mix usage of |
|
Thanks for the idea, @Girgias , I agree with you. I'll double check possible problems with capitalization + fix the tests so that this PR will be able to be merged, and I'll try to address the other suggestions in separate - and smaller - PRs afterwards. |
kocsismate
force-pushed
the
consistent-error-messages
branch
4 times, most recently
from
ce1d0e4 to
82b85b4
Compare
kocsismate
force-pushed
the
consistent-error-messages
branch
from
82b85b4 to
0a3ed92
Compare
|
AppVeyor failures look legit. |
kocsismate
force-pushed
the
consistent-error-messages
branch
from
0a3ed92 to
3dae4e6
Compare
|
Thanks for the heads up! To sum up the changes since the last review: I tried to remove all the problematic/questionable changes, and made the tests green. |
kocsismate
changed the title
Zend/tests/dynamic_call_002.phpt
Outdated
| @@ -1,5 +1,5 @@ | |||
| --TEST-- | |||
| Testing dynamic call with invalid value for method name | |||
| Testing dynamic call with Invalid value for method name | |||
ext/oci8/oci8_interface.c
Outdated
| @@ -424,7 +424,7 @@ PHP_FUNCTION(oci_lob_read) | |||
| PHP_OCI_ZVAL_TO_DESCRIPTOR(tmp, descriptor); | |||
|
|
|||
| if (length <= 0) { | |||
| php_error_docref(NULL, E_WARNING, "Length parameter must be greater than 0"); | |||
| php_error_docref(NULL, E_WARNING, "length parameter must be greater than 0"); | |||
There was a problem hiding this comment.
In such cases, I think capitalization can be retained. While "length" is also the parameter name, it also makes sense if it's just a word.
There was a problem hiding this comment.
Makes sense, I was a bit uncertain about in which direction to go.
ext/openssl/openssl.c
Outdated
| return 0; | ||
| } | ||
| if (!X509_STORE_CTX_init(csc, ctx, x, untrustedchain)) { | ||
| php_openssl_store_errors(); | ||
| php_error_docref(NULL, E_WARNING, "cert store initialization failed"); | ||
| php_error_docref(NULL, E_WARNING, "Cert store initialization failed"); |
kocsismate
changed the title
Currently, error messages aren't consistent at all across all the extensions. I'd like to improve the situation by collecting some general principles that can be followed when writing/rewriting error messages.
For a start, find below some ideas that I could come up with. Any help or addition is welcome. :)
Capitalize all the error messages
If the message starts with a function/parameter/ini setting name (or something that is not suitable for capitalization), then either use
'around them, or circumvent the issue (e.g.enclosure must be a character->The 'enclosure' parameter must be a single characterorArgument 1 must be a single character)Use words consistently
E.g.: "cannot" or "can not", "filename" or "file name", "parameter" or "argument", "null" or "NUL".
Structure sentences consistently
E.g. Currently, we structure type error messages a lot of different ways:
Expects string, null, long or boolelan value for PostgreSQL '%s' (%s)(sic!)Expected array value for this optionArray expected as options when using translate or scaleInstead of structuring messages all the possible ways, we should stick with similar structures.
For improving type error messages specifically, see suggestion #5.
Use user provided values more consistently
Some example formats I found:
"Unable to fork [%s]","Type '%s' not supported",Error opening file %s,"Invalid 'encoding' option - '%s'"etc. Additionally, there are many cases when we don't even mention what the invalid user-provided value was:Invalid OID specified. I think we should try to display user-provided values when possible by using a common format.Phrase internal errors consistent to user-land errors
In my opinion, error messages coming from internal functions should be similar to what is also reported for user-land functions/methods. E.g. instead of
Object or string expected,Argument 1 passed to foo() must be of the type object|string, array givencould be written. (Of course, in this case, the best solution would be to change the ZPP checks in question to use union types).Another example: instead of
Requires 1 or 2 arguments, we could write something similar to the user-land error message:Too few arguments to function foo(), 0 passed and at least 1 expectedThis PR only contains capitalization changes so far - without any test fixes.