Improve error messages of various extensions #5278
Conversation
ext/bz2/bz2.c
Outdated
|
|
||
| if (CHECK_ZVAL_NULL_PATH(file)) { | ||
| zend_type_error("Filename must not contain null bytes"); | ||
| zend_argument_value_error(1, "must not contain null bytes"); |
There was a problem hiding this comment.
or should I change it back to a type error?
There was a problem hiding this comment.
I think the value error is the proper way to go here, at least from a logical stand point
There was a problem hiding this comment.
My feelings as well! P.S. null byte errors are 50-50% TypeErrors and ValueErrors in the codebase, so we'll have to adapt them accordingly.
There was a problem hiding this comment.
I think it is a type error because the ZPP PATH (P modifier) which is used to check for strings without any null bytes so I would imagine that would need to change too in that case.
There was a problem hiding this comment.
Yes, these are type error because of zpp. And I think it's a type error in that case, because zpp always throws TypeError (even ArgumentCountError is a sub-class of TypeError, despite that not making any sense)
There was a problem hiding this comment.
Thank you all for the explanation! I've just changed it back to TypeError
|
@kocsismate Do you mind if I label all these PRs with a dedicated label? Something like |
|
@carusogabriel No, not at all! Feel free :) |
kocsismate
force-pushed
the
consistent-error-messages3-various
branch
2 times, most recently
from
093cf4a to
699c4fb
Compare
|
I don't have permissions on GitHub to create it 😕 If someone can create, please, I'd appreciated it. |
kocsismate
force-pushed
the
consistent-error-messages3-various
branch
from
699c4fb to
074547e
Compare
kocsismate
force-pushed
the
consistent-error-messages3-various
branch
from
074547e to
fdf1401
Compare
5 participants
No description provided.