Tidy config strenghtening #18751

devnexen · 2025-06-03T19:58:46Z

No description provided.

Girgias

Overall liking the changes, some nits and comments.

Girgias · 2025-06-04T13:32:43Z

ext/tidy/tidy.c

@@ -222,19 +222,23 @@ static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string *str_str
 	return SUCCESS;
 }

-static int _php_tidy_set_tidy_opt(TidyDoc doc, const char *optname, zval *value)
+static int _php_tidy_set_tidy_opt(TidyDoc doc, const char *optname, zval *value, int arg)


Suggested change

static int _php_tidy_set_tidy_opt(TidyDoc doc, const char *optname, zval *value, int arg)

static zend_result _php_tidy_set_tidy_opt(TidyDoc doc, const char *optname, zval *value, uint32_t arg)

Girgias · 2025-06-04T13:33:00Z

ext/tidy/tidy.c

@@ -209,10 +209,10 @@ static void php_tidy_load_config(TidyDoc doc, const char *path)
 	}
 }

-static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string *str_string, const HashTable *ht_options)
+static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string *str_string, const HashTable *ht_options, int arg)


Suggested change

static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string *str_string, const HashTable *ht_options, int arg)

static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string *str_string, const HashTable *ht_options, uint32_t arg)

Girgias · 2025-06-04T13:33:33Z

ext/tidy/tests/tidy_error1.phpt

+tidy::parseString(): Argument #2 ($config) Unknown Tidy configuration option "bogus"
+tidy::parseString(): Argument #2 ($config) must be of type array with keys as string
+tidy::parseString(): Argument #2 ($config) Attempting to set read-only option "doctype-mode"


Test for the exception name too?

Girgias · 2025-06-04T13:34:44Z

ext/tidy/tidy.c

@@ -779,7 +783,7 @@ static void php_tidy_create_node(INTERNAL_FUNCTION_PARAMETERS, tidy_base_nodetyp
 	tidy_create_node_object(return_value, obj->ptdoc, node);
 }

-static int _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options)
+static int _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options, int arg)


Suggested change

static int _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options, int arg)

static zend_result _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options, uint32_t arg)

Girgias · 2025-06-04T13:36:34Z

ext/tidy/tidy.c

 			if (opt_name == NULL) {
 				continue;
 			}
-			_php_tidy_set_tidy_opt(doc, ZSTR_VAL(opt_name), opt_val);
+			_php_tidy_set_tidy_opt(doc, ZSTR_VAL(opt_name), opt_val, arg);
 		} ZEND_HASH_FOREACH_END();
+		return SUCCESS;
+	} else {
+		zend_argument_type_error(arg, "must be of type array with keys as string");
+		return FAILURE;
 	}


An array which has the following structure:

$array = ['key1' => 'val1', 5 => 'val2', 'key3' => 'val3'];

would not emit an exception.

nielsdos · 2025-06-04T17:56:37Z

The tidyOptIsReadOnly change may be worth backporting BTW. The reason is that deprecations eventually become build failures.

devnexen · 2025-06-04T18:02:26Z

Sure if @Girgias does not mind I just might even though no idea who is actually the maintainer :)

nielsdos · 2025-06-04T18:05:01Z

who is actually the maintainer :)

We have a maintainer for this? 🤔

devnexen · 2025-06-04T18:13:49Z

At first I thought it was either you (as you seem to like document oriented extensions) or Gina but yes indeed ... no one.

using tidyOptGetCategory when possible. related phpGH-18751

Girgias · 2025-06-04T18:32:14Z

I don't mind backporting this.

And yes we don't have a designated maintainer for this, I have done a few fixes and refactoring here and there but nothing major. :)

using tidyOptGetCategory when possible. related GH-18751 close GH-18763

Girgias

Final minor nits but LGTM :)

Girgias · 2025-06-05T11:33:54Z

ext/tidy/tests/tidy_error1.phpt

+try {
+	$tidy->parseString($buffer, $config);
+} catch (\ValueError $e) {
+	echo get_class($e) . ": " . $e->getMessage(), PHP_EOL;


Either consistent use of . or ,

Suggested change

echo get_class($e) . ": " . $e->getMessage(), PHP_EOL;

echo $e::class, ": ", $e->getMessage(), PHP_EOL;

Girgias · 2025-06-05T11:33:59Z

ext/tidy/tests/tidy_error1.phpt

+try {
+	$tidy->parseString($buffer, $config);
+} catch (\TypeError $e) {
+	echo $e->getMessage(), PHP_EOL;


Suggested change

echo $e->getMessage(), PHP_EOL;

echo $e::class, ": ", $e->getMessage(), PHP_EOL;

Girgias · 2025-06-05T11:34:06Z

ext/tidy/tests/tidy_error1.phpt

+try {
+	var_dump($tidy->parseString($buffer, $config));
+} catch (\ValueError $e) {
+	echo $e->getMessage(), PHP_EOL;


Suggested change

echo $e->getMessage(), PHP_EOL;

echo $e::class, ": ", $e->getMessage(), PHP_EOL;

Girgias · 2025-06-05T11:34:12Z

ext/tidy/tests/tidy_error1.phpt

+try {
+	var_dump($tidy->parseString($buffer, $config));
+} catch (\TypeError $e) {
+	echo $e->getMessage(), PHP_EOL;


Suggested change

echo $e->getMessage(), PHP_EOL;

echo $e::class, ": ", $e->getMessage(), PHP_EOL;

github-actions bot added Category: Build System Extension: tidy labels Jun 3, 2025

devnexen marked this pull request as ready for review June 4, 2025 04:26

Girgias requested changes Jun 4, 2025

View reviewed changes

devnexen force-pushed the tidy_config_strenghtening branch 2 times, most recently from 1886b26 to 4175508 Compare June 4, 2025 17:36

devnexen added a commit to devnexen/php-src that referenced this pull request Jun 4, 2025

ext/tidy: anticipate tidyOptIsReadOnly retirement.

c71ab2c

using tidyOptGetCategory when possible. related phpGH-18751

devnexen mentioned this pull request Jun 4, 2025

ext/tidy: anticipate tidyOptIsReadOnly retirement. #18763

Closed

devnexen added a commit that referenced this pull request Jun 4, 2025

ext/tidy: anticipate tidyOptIsReadOnly retirement.

e13ba36

using tidyOptGetCategory when possible. related GH-18751 close GH-18763

devnexen added 3 commits June 4, 2025 20:40

ext/tidy: config options checks strengthening.

12af82b

add forbidden option test

b0d087c

changes from feedback

62181f1

devnexen force-pushed the tidy_config_strenghtening branch from 4175508 to 62181f1 Compare June 4, 2025 19:40

Girgias approved these changes Jun 5, 2025

View reviewed changes

update test

0e4d4fe

devnexen closed this in 91becb3 Jun 5, 2025

	static int _php_tidy_set_tidy_opt(TidyDoc doc, const char optname, zval value, int arg)
	static zend_result _php_tidy_set_tidy_opt(TidyDoc doc, const char optname, zval value, uint32_t arg)

	static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string str_string, const HashTable ht_options, int arg)
	static zend_result php_tidy_apply_config(TidyDoc doc, const zend_string str_string, const HashTable ht_options, uint32_t arg)

	static int _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options, int arg)
	static zend_result _php_tidy_apply_config_array(TidyDoc doc, const HashTable *ht_options, uint32_t arg)

	echo get_class($e) . ": " . $e->getMessage(), PHP_EOL;
	echo $e::class, ": ", $e->getMessage(), PHP_EOL;

	echo $e->getMessage(), PHP_EOL;
	echo $e::class, ": ", $e->getMessage(), PHP_EOL;

Tidy config strenghtening #18751

Tidy config strenghtening #18751

Uh oh!

Conversation

devnexen commented Jun 3, 2025

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Jun 4, 2025

Uh oh!

devnexen commented Jun 4, 2025

Uh oh!

nielsdos commented Jun 4, 2025

Uh oh!

devnexen commented Jun 4, 2025

Uh oh!

Girgias commented Jun 4, 2025

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!