WP_HTML_Tag_Processor: Add get_attribute_names_with_prefix() method

[ockham]

What?

Add a get_attribute_names_with_prefix() method to WP_HTML_Tag_Processor, which returns the set of attribute name/value pairs of all attributes whose name starts with a given prefix.

$p = new WP_HTML_Tag_Processor( '<div data-enabled class="test" data-test-id="14">Test</div>' );
$p->next_tag();
$p->get_attributes_by_prefix( 'data-' ) === array( 'data-enabled', 'data-test-id' );

Why?

Getting all attributes with a given prefix is handy for data- attributes, and custom namesepaces (e.g. ng- or x-).

Furthermore, it helps with syntax like <img wp-bind:src="myblock.imageSource" />, which is a requirement for the Block Interactivity API (see).

OTOH, it can be costly to compute the value of an attribute, which is why we're only returning the matching attribute names here. Users can then call get_attribute( $name ) for the attributes they're interested in.

This PR is the lovechild of #46672 and #46685 -- combining get_attribute_names and get_attributes_by_prefix.

More background: WordPress/block-interactivity-experiments#118 (comment).

How?

It filters the current tag's $attributes for all attribute names that start with $prefix.

Testing Instructions

Testing Instructions for Keyboard

See included unit tests.

[ockham]

In discussions in #46685, we referred to this new method as get_attribute_names_prefixed_by(). I've now opted to call it get_attribute_names_with_prefix() instead.

LMK if you'd prefer a different name!

[dmsnell]

I think I like this, but I want to make sure we aren't overlooking the possibility of wanting to return array() when not on a tag. It feels like the kind of thing where I might have suggested the opposite: return null if we're not on a tag 🙃 but oh well - is there a reason to differentiate between "there are no attributes matching this prefix" and "you shouldn't be calling this here"?

[ockham]

Yeah, good point. I don't feel strongly about this -- it used to return array() prior to this commit.

I think my chief reason to change it to null was consistency with both the "not-on-a-tag" case, and get_attribute()'s behavior when on a closing tag (for which I even added a test case, 8c51859) -- thinking that null made sense as the general indicator for "no attributes here".

But an empty array might make just as much sense.

is there a reason to differentiate between "there are no attributes matching this prefix" and "you shouldn't be calling this here"

To push this further, would it then make sense to return array() also if null === $this->tag_name_starts_at?

[dmsnell]

it doesn't seem like it'd matter to me to differentiate why you didn't call it in the right place, only that you did call it from the wrong place.

if we return null we know people will end up writing code that crashes because they won't check the results. if we return an empty array we know that people will introduce data corruption by assuming it worked.

while it may fluctuate on a weekly basis, I think given this thought I lean towards null which at least gives a way to know more clearly if something is missing attributes or that we're looking at the wrong spot in the document.

[ockham]

Happy to stick with null 😊 👍

[github-actions]

Flaky tests detected in a4b1071.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3848495111
📝 Reported issues:

• [Flaky Test] should write over selection #39938 in specs/editor/various/multi-block-selection.test.js

[dmsnell]

Left a note about removing (lowercase) in favor of lowercase but I am happy with this either way.

Thanks again for all your work on this, specifically your insight into the needs for the API.

[ockham]

Left a note about removing (lowercase) in favor of lowercase but I am happy with this either way.

Thanks again for all your work on this, specifically your insight into the needs for the API.

Thank you for your patience and feedback as I navigate these waters! 😄

[adamziel]

Good work @dmsnell and @ockham !

[gziolo]

Hello, I'm catching up after being away for a few months. I noticed several impressive additions to WP_HTML_Tag_Processor, including this one. My only observation is that this method is very much tied to the use case it solves. I see you discussed it extensively in #46685. On the surface, get_attribute_names would cover more use cases, and it would still allow finding these attribute names that are prefixed with whatever folks need.

[dmsnell]

@gziolo it's still possible to call this function with an empty string and get all the attribute names, but given the more severe performance implications of doing that we tried to come up with a name that would encourage first a responsible use of the API while not preventing necessary but costly uses.

what would get_attribute_names cover that this doesn't?

[gziolo]

what would get_attribute_names cover that this doesn't?

It didn't occur to me that it could be used with an empty string to get all attribute names. I don't mind there is this specialized method when it's going to be used extensively inside WordPress Core. I mostly wanted to point out that it stands out when you compare it with all other general-purpose public methods on the object.

[dmsnell]

I mostly wanted to point out that it stands out when you compare it with all other general-purpose public methods on the object.

yeah. we deliberated quite a lot on this. my rationale for making it seem distinct is to acknowledge that it is distinct - it carries very different performance implications than most of the rest of the code and has the potentially to really trash the speed of the processor if used as I think the most intuitive use would suggest:

// This could lead to surprisingly bad runtime speeds.
while ( $p->next_tag() ) {
	$names = $p->get_attribute_names();
	…
}

Of course, get_attributes() is much worse, and compared to a browser which already parses the attributes and values, we're not doing that here until we need to, so these functions force memory allocation and copying where none needs to be.

My hope is that _with_prefix() will be a subtle nudge that's strong enough to at least lead people to make the smarter choice first before they even realize the more dangerous option is possible. Leaving out get_attributes() and requiring that they be called by name addresses the more risky behaviors by simply not making them possible

[gziolo]

Thank you for the detailed explanation. It's always great to have the decision process documented 👍🏻