Skip to content

Tests: Clarify valid_unicode() coverage and rejected C0 controls - #15105

Open
shail-mehta wants to merge 8 commits into
WordPress:trunkfrom
shail-mehta:65819/tests-valid-unicode-follow-up
Open

shail-mehta wants to merge 8 commits into
WordPress:trunkfrom
shail-mehta:65819/tests-valid-unicode-follow-up

Conversation

@shail-mehta

@shail-mehta shail-mehta commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

What

Trac ticket: #65819

Follow-up to #14741.

Adds rejected C0 control cases and a caveat clarifying XML Char-style semantics (not "valid Unicode"), preferring newer HTML APIs over extending call sites.

Why

Cover the remaining rejected C0 control boundaries, and clarify in the tests that valid_unicode() checks XML Char-style allowability rather than “valid Unicode,” so callers prefer newer HTML APIs instead of extending use of this helper.

How

In tests/phpunit/tests/kses/validUnicode.php:

  • Add rejected cases for 0xB, 0xC, and 0x1F
  • Add a file-level caveat clarifying XML Char-style semantics and preferring newer HTML APIs over extending call sites

Test-only; no production code changes. Deprecation left for a separate follow-up.

Testing Instructions

  1. npm run test:php -- --filter Tests_Kses_ValidUnicode
  2. npm run test:php -- --filter Tests_Kses_ValidUnicode -c tests/phpunit/multisite.xml
  3. npm run test:php -- --filter Tests_Kses_ValidUnicode --order-by=random
  4. npm run test:php -- --group kses

Use of AI Tools

  • Cursor

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues were identified for the test-only expansion of valid_unicode() coverage and documentation clarifications.

No open issues 🎉

What changed in this PR

This test-only follow-up to #14741 expands valid_unicode() coverage and clarifies that the helper checks historical XML Char-style ranges, not general Unicode validity. No production code changes are included.

Changes:

  • Add rejected C0 control cases for 0xB, 0xC, and 0x1F.
  • Warn against calling the helper from new code and clarify test parameter documentation.
File Description
tests/​phpunit/​tests/​kses/​validUnicode.php Adds three rejected control cases and clarifies the helper’s purpose and usage restrictions.

Review effort: Balanced

@shail-mehta
shail-mehta marked this pull request as ready for review October 28, 2026 07:20
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Core Committers: Use this line as a base for the props when committing in SVN:

Props shailu25, dmsnell, lancewillett.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

* @param int|string $codepoint Code point under the XML Char-style check.
* @param bool $expected Whether valid_unicode() accepts it.
*/
public function test_valid_unicode( $codepoint, $expected ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how about renaming this to prevent carrying along the mis-naming?

public function test_indicates_if_codepoint_is_valid_xml_char( $codepoint, $is_xml_char )

or better yet, if we want to add meaningful tests here, why not remove the hard-coded and sparse set of test cases with a comprehensive test based on an external XML parser?

public function test_indicates_valid_xml_chars() {
	$dom = new DOMDocument(); // should be present in all test runners
	set_error_handler( fn () => throw new Error( 'invalid char' ) );

	for ( $codepoint = 0; $codepoint <= 0x10FFFF; $codepoint++ ) {
		$hex_codepoint = strtoupper( str_pad( dechex( $codepoint ), 2, '0', STR_PAD_LEFT );

		if ( $i < 0xD800 || $i > 0xE000 ) {
				$char = mb_chr( $i );
			} else {
				/*
				 * Since the UTF-16 surrogate halves are not valid Unicode characters,
				 * these have to be manually constructed as invalid UTF-8.
				 */
				$byte1 = chr( 0xE0 | ( $i >> 12 ) );
				$byte2 = chr( 0x80 | ( ( $i >> 6 ) & 0x3F ) );
				$byte3 = chr( 0x80 | ( $i & 0x3F ) );

				$char = "{$byte1}{$byte2}{$byte3}";
			}
		}

		try {
			$dom->loadXML( "<test>{$char}</test>" );
			$this->assertTrue(
				valid_unicode( $codepoint ),
				"Failed to detect U+{$hex_codepoint} as a valid XML Char."
			);
		} catch ( Error $e ) {
			$this->assertFalse(
				valid_unicode( $codepoint ),
				"Should not have detected U+{$hex_codepoint} as a valid XML Char."
			);
		}
	}

	restore_error_handler();
}

the error handling would probably need to go in some setup and teardown method

* HT/LF/CR are rejected even though they are valid Unicode (for example
* U+0008 BACKSPACE), as are surrogates and U+FFFE/U+FFFF. Prefer newer HTML
* APIs from the wp_kses() rewrite rather than extending call sites here.
* These tests only document the helper's historical accept/reject ranges.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this would be great on the function itself where people will see it.

the preference for the HTML API though is potentially not best; it’s probably more appropriate to redirect people to the newer Unicode functionality (e.g. the UTF-8 updates in WordPress 6.9.

we can replace its use in Core (which is indirect via wp_kses_normalize_entities()). only three plugins in the directory call it, and then only in cases where they are polyfilling the functions from Core.

  • duplicator: installer/dup-installer/classes/utilities/class.u.php
  • smart-slider-3: Nextend/Framework/Sanitize.php
  • gotmls: safe-load/wp-settings.php

this means it should be relatively safe to deprecate, but only after Core no longer calls it

@lancewillett
lancewillett requested a balanced review from Copilot October 28, 2026 23:06

@lancewillett lancewillett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please address @dmsnell’s documentation feedback before landing:

  • Rename the test and expected-value parameter to describe XML-character acceptance, rather than general Unicode validity.
  • Put the caller-facing caveat on valid_unicode() itself and replace the recommendation to use newer HTML APIs with the appropriate Unicode guidance.

Keep this small: a million-code-point XML-parser loop is not required for this PR. If you pursue that alternative, agree on its scope and measure its CI cost separately. The vertical-tab and form-feed cases exercise the same rejected gap; one is enough unless there is a distinct regression to protect.

The current tests and PHPCS pass locally in single-site and multisite; the remaining concern is the guidance and unresolved review feedback.

AI review · gpt-6

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for this test-only update to control-character rejection coverage and helper documentation.

No open issues 🎉

Review effort: Balanced

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are reported for the test-coverage and documentation changes.

No open issues 🎉

Review effort: Balanced

@shail-mehta

shail-mehta commented Oct 29, 2026 •

Copy link
Copy Markdown
Member Author

Thanks @dmsnell and @lancewillett - feedback addressed.

Renamed the test and $is_xml_char param, moved the caveat onto valid_unicode() itself, and pointed callers to the newer Unicode/UTF-8 helpers instead of HTML APIs. Kept the existing sparse cases.

Happy to change further if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants