HTML API: Add a serialization regression test for enqueued attribute updates - #12947
HTML API: Add a serialization regression test for enqueued attribute updates#12947wppoland wants to merge 1 commit into
Conversation
…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. See #64567.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
irozum
left a comment
There was a problem hiding this comment.
Good, well-targeted follow-up — it closes a real coverage gap left by [62960]: that fix covered get_attribute_names_with_prefix() on WP_HTML_Tag_Processor (208 lines of tests) but never exercised the same stale-name-list bug on WP_HTML_Processor::serialize_token(), which reads through get_attribute() off that same name list.
I verified the claims in the PR description by hand: reverted class-wp-html-tag-processor.php to its pre-[62960] state and confirmed the new test fails exactly as described (<div onclick class="x"> instead of <div class="x">), then restored it and confirmed it passes. Ran the full Tests_HtmlApi_WpHtmlProcessor_Serialize suite (133 tests, all green) plus composer lint:errors and typecheck:php — both clean aside from pre-existing violations in unrelated files.
No blocking concerns — test-only change, correct @ticket/@covers tags, follows the surrounding file's conventions. One minor observation: the second assertion (set_attribute() case) uses assertStringContainsString rather than pinning the full serialized output the way the first assertion does — reasonable if attribute ordering isn't guaranteed, but worth confirming that's the reason rather than an oversight.
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 inwpHtmlTagProcessor.php.The bug had a second surface.
WP_HTML_Processor::serialize_token()iterates the names fromget_attribute_names_with_prefix( '' )but reads each value throughget_attribute(), so it consumed exactly the stale list the fix corrected. Before [62960] that meant:remove_attribute( 'onclick' )thenserialize_token()re-emitted the attribute as a value-less one:<div onclick class="x">set_attribute( 'id', 'new' )thenserialize_token()dropped the addition entirelyBoth are fixed by [62960], with no change to
serialize_token()itself. Neither is covered.This adds one test to
wpHtmlProcessor-serialize.phppinning 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:63095):OK (1 test, 2 assertions)class-wp-html-tag-processor.phpreverted to[62960]^, source otherwise untouched: fails withSo 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.