Promote warnings to exceptions in ext/tidy #6051
Conversation
kocsismate
force-pushed
the
tidy-warning
branch
from
74188b0 to
3292fd8
Compare
| RETURN_FALSE; | ||
| } | ||
|
|
||
| if (ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(contents))) { | ||
| php_error_docref(NULL, E_WARNING, "Input string is too long"); | ||
| RETURN_FALSE; | ||
| zend_value_error("Input string is too long"); |
There was a problem hiding this comment.
Can't this use zend_arguent_value_error() as you are in a PHP_FUNCTION
There was a problem hiding this comment.
zend_arguent_value_error() doesn't really support the wording which is needed here. It should say something like: Argument ... is a file whose contents is too long. I can't make it make sense. :)
| @@ -289,12 +289,12 @@ static int _php_tidy_set_tidy_opt(TidyDoc doc, char *optname, zval *value) | |||
| zend_long lval; | |||
|
|
|||
| if (!opt) { | |||
| php_error_docref(NULL, E_NOTICE, "Unknown Tidy Configuration Option '%s'", optname); | |||
| php_error_docref(NULL, E_WARNING, "Unknown Tidy configuration option \"%s\"", optname); | |||
There was a problem hiding this comment.
I wonder if this can be bumped to a ValueError in 8.0.
There was a problem hiding this comment.
Given that very similar errors below are now ValueError, I think we should indeed do that for the sake of consistency.
There was a problem hiding this comment.
The only notable difference from the rest is that when this warning is emitted, processing continues, and the option is ignored (it comes from an array). That's the main reason I didn't want to take 2 steps ahead and make it an exception.
| php_error_docref(NULL, E_WARNING, "Unknown Tidy Configuration Option '%s'", optname); | ||
| RETURN_FALSE; | ||
| zend_argument_value_error(getThis() ? 1 : 2, "is an invalid configuration option"); | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
This feels like one of the cases where showing the passed string in the error message might make sense.
There was a problem hiding this comment.
As I don't have any hard feelings against this, let's do it. But in the future (for PHP 8.1+) we should consolidate when to show these values, because this message will be an outlier from the rest now. :)
kocsismate
force-pushed
the
tidy-warning
branch
from
3292fd8 to
278578a
Compare
No description provided.