Skip to content

Promote warnings to exceptions in ext/tidy #6051

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Promote warnings to exceptions in ext/tidy #6051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7cac62
Promote warnings to exceptions in ext/tidy
kocsismate Aug 28, 2020
278578a
Add config option name to error message
kocsismate Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions ext/tidy/tests/007.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,30 @@ Verbose tidy_getopt()
tidy.default_config=
--FILE--
<?php
$a = new tidy(__DIR__."/007.html");
echo "Current Value of 'tidy-mark': ";
var_dump($a->getopt("tidy-mark"));
echo "Current Value of 'error-file': ";
var_dump($a->getopt("error-file"));
echo "Current Value of 'tab-size': ";
var_dump($a->getopt("tab-size"));
$a = new tidy(__DIR__."/007.html");
echo "Current Value of 'tidy-mark': ";
var_dump($a->getopt("tidy-mark"));
echo "Current Value of 'error-file': ";
var_dump($a->getopt("error-file"));
echo "Current Value of 'tab-size': ";
var_dump($a->getopt("tab-size"));

try {
$a->getopt('bogus-opt');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

try {
tidy_getopt($a, 'non-ASCII string ���');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

var_dump($a->getopt('bogus-opt'));
var_dump(tidy_getopt($a, 'non-ASCII string ���'));
?>
--EXPECTF--
Current Value of 'tidy-mark': bool(false)
Current Value of 'error-file': string(0) ""
Current Value of 'tab-size': int(8)

Warning: tidy::getOpt(): Unknown Tidy Configuration Option 'bogus-opt' in %s007.php on line 10
bool(false)

Warning: tidy_getopt(): Unknown Tidy Configuration Option 'non-ASCII string ���' in %s007.php on line 11
bool(false)
tidy::getOpt(): Argument #1 ($option) is an invalid configuration option, "bogus-opt" given
tidy_getopt(): Argument #2 ($option) is an invalid configuration option, "non-ASCII string ���" given
10 changes: 5 additions & 5 deletions ext/tidy/tests/019.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ tidy_repair_file($l, $l, $l ,$l); // This doesn't emit any warning, TODO look in
echo "Done\n";
?>
--EXPECTF--
Warning: tidy_repair_string(): Could not load configuration file '1' in %s on line %d
Warning: tidy_repair_string(): Could not load configuration file "1" in %s on line %d

Warning: tidy_repair_string(): Could not set encoding '1' in %s on line %d
Warning: tidy_repair_string(): Could not set encoding "1" in %s on line %d

Warning: tidy_repair_string(): Could not load configuration file '' in %s on line %d
Warning: tidy_repair_string(): Could not load configuration file "" in %s on line %d

Warning: tidy_repair_string(): Could not load configuration file '1' in %s on line %d
Warning: tidy_repair_string(): Could not load configuration file "1" in %s on line %d

Warning: tidy_repair_string(): Could not set encoding '1' in %s on line %d
Warning: tidy_repair_string(): Could not set encoding "1" in %s on line %d
Path cannot be empty
Path cannot be empty
Done
11 changes: 7 additions & 4 deletions ext/tidy/tests/021.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ tidy_get_opt_doc()
--FILE--
<?php

var_dump(tidy_get_opt_doc(new tidy, 'some_bogus_cfg'));
try {
tidy_get_opt_doc(new tidy, 'some_bogus_cfg');
} catch (ValueError $exception) {
echo $exception->getMessage() . "\n";
}

$t = new tidy;
var_dump($t->getOptDoc('ncr'));
var_dump(strlen(tidy_get_opt_doc($t, 'wrap')) > 99);
?>
--EXPECTF--
Warning: tidy_get_opt_doc(): Unknown Tidy Configuration Option 'some_bogus_cfg' in %s021.php on line 3
bool(false)
--EXPECT--
tidy_get_opt_doc(): Argument #2 ($optname) is an invalid configuration option, "some_bogus_cfg" given
string(73) "This option specifies if Tidy should allow numeric character references. "
bool(true)
2 changes: 1 addition & 1 deletion ext/tidy/tests/bug54682.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ $nx->diagnose();

?>
--EXPECTF--
Warning: tidy::__construct(): Cannot Load '*' into memory%win %s on line %d
Warning: tidy::__construct(): Cannot load "*" into memory%win %s on line %d
3 changes: 2 additions & 1 deletion ext/tidy/tests/tidy_error1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ $config = array('bogus' => 'willnotwork');

$tidy = new tidy();
var_dump($tidy->parseString($buffer, $config));

?>
--EXPECTF--
Notice: tidy::parseString(): Unknown Tidy Configuration Option 'bogus' in %s on line %d
Warning: tidy::parseString(): Unknown Tidy configuration option "bogus" in %s on line %d
bool(true)
52 changes: 26 additions & 26 deletions ext/tidy/tidy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@
TIDY_OPEN_BASE_DIR_CHECK(Z_STRVAL_P(_val)); \
switch (tidyLoadConfig(_doc, Z_STRVAL_P(_val))) { \
case -1: \
php_error_docref(NULL, E_WARNING, "Could not load configuration file '%s'", Z_STRVAL_P(_val)); \
php_error_docref(NULL, E_WARNING, "Could not load configuration file \"%s\"", Z_STRVAL_P(_val)); \
break; \
case 1: \
php_error_docref(NULL, E_NOTICE, "There were errors while parsing the configuration file '%s'", Z_STRVAL_P(_val)); \
php_error_docref(NULL, E_NOTICE, "There were errors while parsing the configuration file \"%s\"", Z_STRVAL_P(_val)); \
break; \
} \
} \
Expand Down Expand Up @@ -158,7 +158,7 @@ if (php_check_open_basedir(filename)) { \
#define TIDY_SET_DEFAULT_CONFIG(_doc) \
if (TG(default_config) && TG(default_config)[0]) { \
if (tidyLoadConfig(_doc, TG(default_config)) < 0) { \
php_error_docref(NULL, E_WARNING, "Unable to load Tidy configuration file at '%s'.", TG(default_config)); \
php_error_docref(NULL, E_WARNING, "Unable to load Tidy configuration file at \"%s\"", TG(default_config)); \
} \
}
/* }}} */
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this can be bumped to a ValueError in 8.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that very similar errors below are now ValueError, I think we should indeed do that for the sake of consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, that makes sense.

return FAILURE;
}

if (tidyOptIsReadOnly(opt)) {
php_error_docref(NULL, E_NOTICE, "Attempting to set read-only option '%s'", optname);
php_error_docref(NULL, E_WARNING, "Attempting to set read-only option \"%s\"", optname);
return FAILURE;
}

Expand Down Expand Up @@ -356,8 +356,8 @@ static void php_tidy_quick_repair(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_fil
}

if (ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(data))) {
php_error_docref(NULL, E_WARNING, "Input string is too long");
RETURN_FALSE;
zend_argument_value_error(1, "is too long");
RETURN_THROWS();
}

doc = tidyCreate();
Expand All @@ -382,7 +382,7 @@ static void php_tidy_quick_repair(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_fil

if(enc_len) {
if (tidySetCharEncoding(doc, enc) < 0) {
php_error_docref(NULL, E_WARNING, "Could not set encoding '%s'", enc);
php_error_docref(NULL, E_WARNING, "Could not set encoding \"%s\"", enc);
RETVAL_FALSE;
}
}
Expand Down Expand Up @@ -798,7 +798,7 @@ static int php_tidy_parse_string(PHPTidyObj *obj, char *string, uint32_t len, ch

if(enc) {
if (tidySetCharEncoding(obj->ptdoc->doc, enc) < 0) {
php_error_docref(NULL, E_WARNING, "Could not set encoding '%s'", enc);
php_error_docref(NULL, E_WARNING, "Could not set encoding \"%s\"", enc);
return FAILURE;
}
}
Expand Down Expand Up @@ -995,8 +995,8 @@ PHP_FUNCTION(tidy_parse_string)
}

if (ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(input))) {
php_error_docref(NULL, E_WARNING, "Input string is too long");
RETURN_FALSE;
zend_argument_value_error(1, "is too long");
RETURN_THROWS();
}

tidy_instanciate(tidy_ce_doc, return_value);
Expand Down Expand Up @@ -1058,13 +1058,13 @@ PHP_FUNCTION(tidy_parse_file)
obj = Z_TIDY_P(return_value);

if (!(contents = php_tidy_file_to_mem(ZSTR_VAL(inputfile), use_include_path))) {
php_error_docref(NULL, E_WARNING, "Cannot Load '%s' into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (Using include path)" : "");
php_error_docref(NULL, E_WARNING, "Cannot load \"%s\" into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (using include path)" : "");
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this use zend_arguent_value_error() as you are in a PHP_FUNCTION

Copy link
Member Author

@kocsismate kocsismate Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

RETURN_THROWS();
}

TIDY_APPLY_CONFIG_ZVAL(obj->ptdoc->doc, options);
Expand Down Expand Up @@ -1155,8 +1155,8 @@ PHP_FUNCTION(tidy_get_opt_doc)
opt = tidyGetOptionByName(obj->ptdoc->doc, optname);

if (!opt) {
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, \"%s\" given", optname);
RETURN_THROWS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like one of the cases where showing the passed string in the error message might make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

}

if ( (optval = (char *) tidyOptGetDoc(obj->ptdoc->doc, opt)) ) {
Expand Down Expand Up @@ -1299,8 +1299,8 @@ PHP_FUNCTION(tidy_getopt)
opt = tidyGetOptionByName(obj->ptdoc->doc, optname);

if (!opt) {
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, \"%s\" given", optname);
RETURN_THROWS();
}

optval = php_tidy_get_opt_val(obj->ptdoc, opt, &optt);
Expand Down Expand Up @@ -1350,13 +1350,13 @@ PHP_METHOD(tidy, __construct)

if (inputfile) {
if (!(contents = php_tidy_file_to_mem(ZSTR_VAL(inputfile), use_include_path))) {
php_error_docref(NULL, E_WARNING, "Cannot Load '%s' into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (Using include path)" : "");
php_error_docref(NULL, E_WARNING, "Cannot load \"%s\" into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (using include path)" : "");
return;
}

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");
RETURN_THROWS();
}

TIDY_APPLY_CONFIG_ZVAL(obj->ptdoc->doc, options);
Expand Down Expand Up @@ -1386,13 +1386,13 @@ PHP_METHOD(tidy, parseFile)
}

if (!(contents = php_tidy_file_to_mem(ZSTR_VAL(inputfile), use_include_path))) {
php_error_docref(NULL, E_WARNING, "Cannot Load '%s' into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (Using include path)" : "");
php_error_docref(NULL, E_WARNING, "Cannot load \"%s\" into memory%s", ZSTR_VAL(inputfile), (use_include_path) ? " (using include path)" : "");
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");
RETURN_THROWS();
}

TIDY_APPLY_CONFIG_ZVAL(obj->ptdoc->doc, options);
Expand Down Expand Up @@ -1421,8 +1421,8 @@ PHP_METHOD(tidy, parseString)
}

if (ZEND_SIZE_T_UINT_OVFL(ZSTR_LEN(input))) {
php_error_docref(NULL, E_WARNING, "Input string is too long");
RETURN_FALSE;
zend_argument_value_error(1, "is too long");
RETURN_THROWS();
}

obj = Z_TIDY_P(object);
Expand Down