Skip to content

HTML API: Add a serialization regression test for enqueued attribute updates - #12947

Open
wppoland wants to merge 2 commits into
WordPress:trunkfrom
wppoland:tests/64567-serialize-token-regression
Open

wppoland wants to merge 2 commits into
WordPress:trunkfrom
wppoland:tests/64567-serialize-token-regression

Conversation

@wppoland

@wppoland wppoland commented Aug 9, 2026 •

Copy link
Copy Markdown

Trac ticket: https://core.trac.wordpress.org/ticket/64567

Test-only follow-up to [62960].

[62960] made get_attribute_names_with_prefix() respect enqueued updates, and it landed with 208 lines of tests. All of them are in wpHtmlTagProcessor.php.

The bug had a second surface. WP_HTML_Processor::serialize_token() iterates the names from get_attribute_names_with_prefix( '' ) but reads each value through get_attribute(), so it consumed exactly the stale list the fix corrected. Before [62960] that meant:

  • remove_attribute( 'onclick' ) then serialize_token() re-emitted the attribute as a value-less one: <div onclick class="x">
  • set_attribute( 'id', 'new' ) then serialize_token() dropped the addition entirely

Both are fixed by [62960], with no change to serialize_token() itself. Neither is covered.

This adds one test to wpHtmlProcessor-serialize.php pinning both directions, so the serialization path cannot silently regress if the name-list logic is touched again.

Verification

Run against tests/phpunit/tests/html-api/wpHtmlProcessor-serialize.php:

  • On trunk (63095): OK (1 test, 2 assertions)
  • With only class-wp-html-tag-processor.php reverted to [62960]^, source otherwise untouched: fails with
    An attribute enqueued for removal was serialized as a value-less attribute.
    -'<div class="x">'
    +'<div onclick class="x">'
    

So it genuinely exercises what [62960] changed rather than passing vacuously.

Timing

Tests only, no source changes, so this is safe during RC, but nothing breaks if it slips to 7.2 instead. Entirely the committers' call.

Background: I raised this serialization symptom on the ticket at comment:15 while the fix was still being chosen between four competing PRs, and offered the test to whichever branch was picked. #12757 landed without it, so I am submitting it here rather than letting it drop.

Use of AI Tools

AI assistance: Yes
Tool(s): Antigravity IDE
Model(s): Gemini 2.5 Pro
Used for: Code analysis, conflict resolution, test suite updates, and patch verification. All changes were manually reviewed and tested locally.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@github-actions

github-actions Bot commented Aug 9, 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. You can also manually ask me to refresh this list by adding the props-bot label.

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

Props motylanogha, irozum.

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

@github-actions

github-actions Bot commented Aug 9, 2026

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.

irozum

This comment was marked as low quality.

@wppoland
wppoland force-pushed the tests/64567-serialize-token-regression branch from 751b1b2 to 891c513 Compare August 28, 2026 14:22
@wppoland

Copy link
Copy Markdown
Author

Thanks @irozum, and thanks in particular for reverting the tag processor and running it rather than reading it.

On the assertStringContainsString

Not a deliberate hedge against ordering, and you were right to ask. I measured it: the output is deterministic, and a newly added attribute is emitted before the existing ones.

set id on class-only        <div id="new" class="x">
set id on two attrs         <div id="new" class="x" data-a="1">
overwrite existing class    <div class="y">
set id then remove class    <div id="new" data-a="1">

So the assertion can pin the whole string the way the removal assertion above it does, and it now does, in 891c513:

$this->assertSame(
	'<div id="new" class="x">',
	$processor->serialize_token(),
	'An attribute enqueued via set_attribute() was not serialized. A newly added attribute is emitted before the existing ones.'
);

The failure message names the ordering on purpose. Either assertion catches the original defect, since that one dropped the attribute outright, so this is about strength rather than correctness: the tighter form also pins where an added attribute lands, which nothing else in the file covers.

Also rebased

The branch was 119 commits behind and class-wp-html-tag-processor.php has moved since, so I rebased onto current trunk rather than leave the result inferred from an old base. Tests_HtmlApi_WpHtmlProcessor_Serialize on trunk 4a8a86b: OK (133 tests, 269 assertions). phpcs clean on the file.

@wppoland

Copy link
Copy Markdown
Author

CI note so the one red mark is not read as this test failing.

891c513 is 108 success, 16 skipped, 1 cancelled. The cancelled one is PHP 7.4 / MySQL 8.0, and it is a job timeout rather than a test result:

........................ 30739 / 31011 ( 99%)
##[error]The operation was canceled.

Not one F or E in 30,739 tests before the runner was killed. It ran 14:23:01 to 14:43:21, exactly 20 minutes, which is the ceiling set in reusable-phpunit-tests-v3.yml:133:

timeout-minutes: ${{ inputs.coverage-report && 120 || inputs.php == '8.4' && 30 || 20 }}

That cell normally finishes with only a couple of minutes to spare. On trunk today the same job ran eight times at 13:02, 13:12, 13:20, 13:33, 13:34, 13:37, 13:44 and 18:20 minutes. A slow runner tips it over the line.

It is not specific to PHP 7.4 either: trunk run 33160385316 lost PHP 8.5 / MySQL 8.0 multisite at 20:17 the same way, with no patch of mine anywhere near it.

I have no write access here, so I cannot re-run the job. Happy to push a no-op commit to retrigger if a committer would rather see the matrix fully green before looking at this, but the change under review is one assertion in one HTML API test file.

@wppoland
wppoland force-pushed the tests/64567-serialize-token-regression branch from 891c513 to fec52b2 Compare September 13, 2026 07:40
…updates.

[62960] made get_attribute_names_with_prefix() respect enqueued updates, but
its tests only cover WP_HTML_Tag_Processor. WP_HTML_Processor::serialize_token()
consumed the same stale name list while reading values through get_attribute(),
so a removed attribute was re-emitted as a value-less attribute and an added one
was dropped. That path gained no coverage.

Fails at [62960]^ with '<div onclick class="x">', passes on trunk.

Both assertions pin the full serialized output. A newly added attribute is
emitted before the existing ones, so the set_attribute() case is deterministic
and does not need a substring match (per @irozum's review).

Rebased onto trunk.

See #64567.
@wppoland
wppoland force-pushed the tests/64567-serialize-token-regression branch from 19c48db to b08dd68 Compare September 25, 2026 06:24
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.

2 participants