HTML API: Respect enqueued updates in get_attribute_names_with_prefix(). - #12757
Conversation
|
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. |
|
Verified this branch against every case raised on #64567, including the ones from the earlier PRs. All seven pass, where
Method: a standalone harness that loads the HTML API files directly against a checkout, so no test DB is involved. It reproduces trunk's failures exactly as reported in comment:19, then passes 7/7 here. Two things worth noting. The The It belongs in /**
* Ensures that serialize_token() reflects enqueued attribute updates instead
* of emitting removed attributes as value-less attributes or omitting added ones.
*
* serialize_token() iterates the names from get_attribute_names_with_prefix( '' )
* but reads each value through get_attribute(). Before #64567 the name list did not
* reflect enqueued updates while the values did, so a removed attribute survived in
* the output as a boolean attribute and an added attribute was dropped entirely.
*
* @ticket 64567
*
* @covers WP_HTML_Processor::serialize_token
*/
public function test_serialize_token_reflects_enqueued_attribute_updates() {
$processor = WP_HTML_Processor::create_fragment( '<div onclick="alert(1)" class="x">Text</div>' );
$processor->next_tag();
$processor->remove_attribute( 'onclick' );
$this->assertSame(
'<div class="x">',
$processor->serialize_token(),
'An attribute enqueued for removal was serialized as a value-less attribute.'
);
$processor = WP_HTML_Processor::create_fragment( '<div class="x">Text</div>' );
$processor->next_tag();
$processor->set_attribute( 'id', 'new' );
$this->assertStringContainsString(
'id="new"',
$processor->serialize_token(),
'An attribute enqueued via set_attribute() was not serialized.'
);
}Take it straight into this branch if it's useful — no attribution needed. Also attached to the ticket as To be precise about what I checked: the seven behaviours above were verified through the standalone harness, not by running the PHPUnit suite, so the test above is verified as behaviour rather than as a green CI run. |
There was a problem hiding this comment.
Pull request overview
This PR updates WP_HTML_Tag_Processor::get_attribute_names_with_prefix() so it reflects pending (enqueued) attribute/class mutations—without requiring a prior get_updated_html()—to avoid returning stale attribute name lists.
Changes:
- Incorporates enqueued attribute add/remove updates into
get_attribute_names_with_prefix(). - Flushes pending class-name updates so
classattribute presence is reflected in name lookups. - Adds PHPUnit coverage for immediate (pre-flush) visibility of attribute/class changes and for ignoring modifiable-text updates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/wp-includes/html-api/class-wp-html-tag-processor.php |
Updates prefix-based attribute-name lookup to consider enqueued lexical updates (including class-derived updates). |
tests/phpunit/tests/html-api/wpHtmlTagProcessor.php |
Adds tests covering immediate reflection of enqueued attribute removals/additions, class updates, and ignoring modifiable-text updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/wp-includes/html-api/class-wp-html-tag-processor.php:2976
- The new logic explicitly handles
add_class()/remove_class()affecting whether theclassattribute exists, but the added tests only cover the “class attribute added” case. Please add a test whereremove_class()removes the last remaining class and verifyget_attribute_names_with_prefix( '' )(and/or'cla') no longer reportsclassbeforeget_updated_html()is called.
/*
* For the `class` attribute, ensure that enqueued class changes from
* `add_class` and `remove_class` are flushed into attribute updates.
*/
$has_class = isset( $this->attributes['class'] );
if ( '' === $comparable || str_starts_with( 'class', $comparable ) ) {
my.phpunit.xml:8
my.phpunit.xmlappears to be a developer-specific PHPUnit configuration (custom bootstrap, limited suites). Keeping this in the PR can confuse contributors and tooling; core typically usesphpunit.xml.distand expects local overrides to remain untracked.
<?xml version="1.0" encoding="utf-8" ?>
<phpunit
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/9.2/phpunit.xsd"
bootstrap="my.bootstrap.php"
testdox="true"
colors="true"
>
my.bootstrap.php:12
my.bootstrap.phplooks like a local test bootstrap and is not wired into the project’s standard PHPUnit runner. Committing this file (including the commented#require_once) is likely accidental and adds maintenance surface without being used by CI.
// Core stuff.
#require_once __DIR__ . '/wp-test-config.php';
require_once __DIR__ . '/src/wp-load.php';
wp_cache_init();
require_wp_db();
require_once __DIR__ . '/tests/phpunit/includes/phpunit-adapter-testcase.php';
require_once __DIR__ . '/tests/phpunit/includes/abstract-testcase.php';
require_once __DIR__ . '/tests/phpunit/includes/testcase.php';
require_once __DIR__ . '/tests/phpunit/includes/functions.php';
55203cc to
5d1151c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/wp-includes/html-api/class-wp-html-tag-processor.php:2986
$has_classis derived only from the originally-parsed attributes ($this->attributes['class']) and ignores any already-enqueued lexical update forclass(e.g.remove_attribute( 'class' )). In that situation, a subsequentadd_class()should re-introduceclass, but this guard won’t flush class name updates and the method will incorrectly treatclassas removed (because$removals['class']is set). Consider deriving$has_classfromget_enqueued_attribute_value( 'class' )first so pending removals/additions are respected when deciding whether class updates could add/remove the attribute.
$has_class = isset( $this->attributes['class'] );
if ( '' === $comparable || str_starts_with( 'class', $comparable ) ) {
foreach ( $this->classname_updates as $class_name => $update ) {
if (
( $has_class && self::REMOVE_CLASS === $update ) ||
…x()`. Trac ticket: Core-64567. Previously, `get_attribute_names_with_prefix()` was overlooking enqueued attribute and class name updates which occurred before `get_updated_html()` had been called. This resulted in reporting stale data which might overlook attributes which were added, and might report attributes which were removed. In this patch, the method now examines the enqueued updates to determine if any of them introduce or remove attributes. It also examines enqueued class updates to ensure that if the `class` attribute would be added or removed because of them that it will also be properly reported. Co-Authored-By: Anup Kankale <[email protected]> Co-Authored-By: Igor Rozum <[email protected]> Co-Authored-By: Jeffrey Carandang <[email protected]> Co-Authored-By: Jerome B. <[email protected]> Co-Authored-By: Khokan Sardar <[email protected]> Co-Authored-By: Luis Herranz <[email protected]> Co-Authored-By: Mariusz Szatkowski <[email protected]> Co-Authored-By: SACHINRAJ CP <[email protected]>
5d1151c to
92667cf
Compare
|
Rebased and squashed for merge prep. |
|
thanks @wppoland
I’m going to leave that out for now and let it be part of improving the serialization tests. it’s valuable, but in part because it’s the beta part of the release I want to focus on the core fix, which is |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/wp-includes/html-api/class-wp-html-tag-processor.php:2986
- The conditional flush of
classname_updatesis based only on whetherclassexists in the parsed attributes and whether updates are ADD/REMOVE. This misses cases where the enqueuedclassstate changes the attribute’s existence, e.g.remove_attribute( 'class' )followed byadd_class( 'x' )(should re-addclass), orset_attribute( 'class', 'a' )followed byremove_class( 'a' )(should removeclass). In these cases,get_attribute_names_with_prefix()can incorrectly omit or includeclassbecauseclass_name_updates_to_attributes_updates()is never called.
$has_class = isset( $this->attributes['class'] );
if ( '' === $comparable || str_starts_with( 'class', $comparable ) ) {
foreach ( $this->classname_updates as $class_name => $update ) {
if (
( $has_class && self::REMOVE_CLASS === $update ) ||
( ! $has_class && self::ADD_CLASS === $update )
) {
$this->class_name_updates_to_attributes_updates();
break;
}
}
}
tests/phpunit/tests/html-api/wpHtmlTagProcessor.php:588
- The added tests cover attribute additions/removals and a basic
add_class()case, but they don’t exercise edge cases whereclassexistence flips due to mixingset_attribute()/remove_attribute( 'class' )withadd_class()/remove_class(). These combinations are whereget_attribute_names_with_prefix()is most likely to disagree withget_attribute()and would help prevent regressions.
public function test_get_attribute_names_with_prefix_immediately_reflects_class_after_adding_classes() {
$processor = new WP_HTML_Tag_Processor( '<div existing>' );
$processor->next_tag();
$this->assertSame(
array( 'existing' ),
$processor->get_attribute_names_with_prefix( '' ),
'Expected to only report the existing attribute: check test setup.'
);
$processor->add_class( 'added' );
$this->assertSame(
array( 'class' ),
$processor->get_attribute_names_with_prefix( 'class' ),
'Failed to report the newly-added `class` attribute.'
);
}
| */ | ||
| $has_class = isset( $this->attributes['class'] ); | ||
| if ( '' === $comparable || str_starts_with( 'class', $comparable ) ) { | ||
| foreach ( $this->classname_updates as $class_name => $update ) { |
There was a problem hiding this comment.
Minor: the $class_name variable is not used.
| foreach ( $this->classname_updates as $class_name => $update ) { | |
| foreach ( $this->classname_updates as $update ) { |
There was a problem hiding this comment.
Aside: It would be nice if WP_HTML_Tag_Processor::$classname_updates were typed as array<non-empty-string, self::ADD_CLASS|self::REMOVE_CLASS>.
There was a problem hiding this comment.
I did not realize this would iterate the values of the associative array, which makes me want to leave the unused var in place as it seems so prone to misunderstanding.
can add the type annotations, though this is currently a bug-fix and not an enhancement. is that fine for a beta merge?
There was a problem hiding this comment.
Yes, docs changes are free to do any time. And I see static analysis issues as being defects which should likewise be done at any time (similar to unit tests).
There was a problem hiding this comment.
static analysis issues
this is more of an issue with the static analyzer than an issue the static analyzer uncovered though, right? the code is right and there are no static defects? I just want to make sure I understand properly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/phpunit/tests/html-api/wpHtmlTagProcessor.php:590
- There’s no regression test covering cases where
add_class()/remove_class()must override an enqueuedclassattribute removal/addition (e.g.remove_attribute( 'class' )followed byadd_class()). This scenario currently breaksget_attribute_names_with_prefix()because it may skip flushing class updates, so adding a focused test here would prevent future regressions.
/**
* Ensures get_attribute_names_with_prefix() agrees with get_attribute()
* after pending updates, returning each name once with no stale entries.
*
* @ticket 64567
src/wp-includes/html-api/class-wp-html-tag-processor.php:2986
- The conditional flush of
$this->classname_updatesis based only on whether the parsed tag currently has aclassattribute (isset( $this->attributes['class'] )). This misses cases where the enqueued state ofclassdiffers from the parsed state (e.g.<div class="a">, thenremove_attribute( 'class' ), thenadd_class( 'b' ); orset_attribute( 'class', 'a' )thenremove_class( 'a' )). In these cases,get_attribute()will callclass_name_updates_to_attributes_updates()and correctly reflect the final presence/absence ofclass, butget_attribute_names_with_prefix()may skip flushing and return stale results (e.g. omitclasseven though it will be reintroduced, or reportclasseven though it will be removed).
$has_class = isset( $this->attributes['class'] );
if ( '' === $comparable || str_starts_with( 'class', $comparable ) ) {
foreach ( $this->classname_updates as $update ) {
if (
( $has_class && self::REMOVE_CLASS === $update ) ||
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/wp-includes/html-api/class-wp-html-tag-processor.php:757
- The docblock claims
$classname_updatesis keyed bynon-empty-string, butadd_class()/remove_class()don’t validate$class_nameand can enqueue an empty-string key. This makes the type annotation inaccurate for static analysis and documentation.
* @since 6.2.0
* @var array<non-empty-string, self::ADD_CLASS|self::REMOVE_CLASS>
*/
private $classname_updates = array();
| 'Failed to find expected existing attributes: check test setup.' | ||
| ); | ||
|
|
||
| $this->assertEquals( |
There was a problem hiding this comment.
Per https://core.trac.wordpress.org/ticket/38266
| $this->assertEquals( | |
| $this->assertSame( |
| 'Failed to find expected existing `data-keep` attribute value: check test setup.' | ||
| ); | ||
|
|
||
| $this->assertEquals( |
There was a problem hiding this comment.
| $this->assertEquals( | |
| $this->assertSame( |
| $additions = array(); | ||
| $removals = array(); | ||
| foreach ( $this->lexical_updates as $update_name => $update ) { | ||
| if ( is_int( $update_name ) || 'modifiable text' === $update_name ) { |
There was a problem hiding this comment.
This line was somewhat surprising to me, specifically is_int( $update_name ), given that when looking at $this->lexical_updates it is typed as WP_HTML_Text_Replacement[]. The examples don't really show non-int keys either. Seems like this would be an opportunity to flesh out the docs for what non-int keys means, and do:
--- a/src/wp-includes/html-api/class-wp-html-tag-processor.php
+++ b/src/wp-includes/html-api/class-wp-html-tag-processor.php
@@ -810,7 +810,7 @@ class WP_HTML_Tag_Processor {
* );
*
* @since 6.2.0
- * @var WP_HTML_Text_Replacement[]
+ * @var array<int|string, WP_HTML_Text_Replacement>
*/
protected $lexical_updates = array();
Trac ticket: Core-64567.
Replaces #10828, #12484, #12619.
Previously,
get_attribute_names_with_prefix()was overlooking enqueued attribute and class name updates which occurred beforeget_updated_html()had been called. This resulted in reporting stale data which might overlook attributes which were added, and might report attributes which were removed.In this patch, the method now examines the enqueued updates to determine if any of them introduce or remove attributes. It also examines enqueued class updates to ensure that if the
classattribute would be added or removed because of them that it will also be properly reported.