Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WP_HTML_Tag_Processor: Add get_attributes_by_prefix() method #46672

Closed
wants to merge 3 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Dec 20, 2022

What?

Add a get_attributes_by_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' => true, 'data-test-id' => '14' );

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).

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

How?

It filters the current tag's $attributes for all attribute names that start with $prefix, and then calls get_attribute for each of them.

Testing Instructions

See included unit test.

@adamziel
Copy link
Contributor

This docstring makes me more hesitant about this API:

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

get_attributes_by_prefix( 'data-' ) would return:

  • Some attributes you expect
  • Some arbitrary attributes you don't expect that just randomly happened to be set there by the user or some block

In other words, even after calling get_attributes_by_prefix( 'data-' ) you'll still have to filter/sanitize the results:

$attrs = $p->get_attributes_by_prefix( 'data-' );
if( array_key_exists( $attr, 'data-show' ) ) { /* ... */ }

What is the added value over get_attributes_names()?

$attrs = $p->get_attributes_names();
if( in_array( 'data-show', $attr, true ) ) { /* ... */ }

@ockham
Copy link
Contributor Author

ockham commented Dec 20, 2022

In other words, even after calling get_attributes_by_prefix( 'data-' ) you'll still have to filter/sanitize the results:

$attrs = $p->get_attributes_by_prefix( 'data-' );
if( array_key_exists( $attr, 'data-show' ) ) { /* ... */ }

Case in point: We want to allow "binding" an arbitrary attribute to the value of an expression, e.g.

 <!-- Written by the developer -->
 <img wp-bind:src="myblock.imageSource" />
 <div wp-class:red="myblock.someValue" class="blue"></div>
 <div wp-style:color="myblock.someValue" style="background: blue;"></div>

 <!-- After processing -->
 <img wp-bind:src="myblock.imageSource" src="Value of myblock.imageSource" />
 <div wp-class:red="myblock.truthyValue" class="blue red"></div>
 <div
   wp-style:color="myblock.greenValue"
   style="background: blue; color: green;"
 ></div>

(from WordPress/block-interactivity-experiments#118 (comment); cf. Alpine's x-bind directive).

So I don't think we'll do much filtering of allowed attributes there (since we basically want to allow binding to pretty much any arbitrary attribute) 🤔 Instead, if we have wp-bind:something="some.expression", we'll just add the something attribute and set it to whatever the expression evaluates to.

What is the added value over get_attributes_names()?

$attrs = $p->get_attributes_names();
if( in_array( 'data-show', $attr, true ) ) { /* ... */ }

Mostly that it aligns nicely with the concept of prefixes for custom attributes and the like, I'd say. (I don't feel too strongly about this, though.)

@ockham
Copy link
Contributor Author

ockham commented Dec 20, 2022

Update: I've experimented a bit more over at WordPress/block-interactivity-experiments#118 and implemented a basic wp-bind directive (WordPress/block-interactivity-experiments@751027c).

It seems like get_attributes_names should work fine for that. I'll file a PR to implement that, as an alternative to this one.

@ockham
Copy link
Contributor Author

ockham commented Dec 20, 2022

Update: I've experimented a bit more over at WordPress/block-hydration-experiments#118 and implemented a basic wp-bind directive (WordPress/block-hydration-experiments@751027c).

It seems like get_attributes_names should work fine for that. I'll file a PR to implement that, as an alternative to this one.

#46685

@ockham ockham force-pushed the add/get-attributes-by-prefix-method branch from 8aa8204 to e06c326 Compare December 21, 2022 09:34
* @covers WP_HTML_Tag_Processor::get_attributes_by_prefix
*/
public function test_get_attributes_by_prefix_returns_set_of_prefixed_attributes() {
$p = new WP_HTML_Tag_Processor( '<div data-enabled class="test" data-test-id="14">Test</div>' );
Copy link
Contributor

Choose a reason for hiding this comment

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

What should happen in these cases?

  • <div DATA-enabled class="test" data-test-id="14">Test</div> and $p->get_attributes_by_prefix( 'data-' );
  • <div DATA-enabled class="test" data-test-id="14">Test</div> and $p->get_attributes_by_prefix( 'DATA-' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice one! 😄

Well, I'd say it should align with get_attribute()'s behavior. The "invariant" should be that if I do something like $attrs = $p->get_attributes_by_prefix( $prefix ); and then call $p->get_attribute() for each of $attrs' keys, it shouldn't return null for any of them.

  1. Let's assume get_attribute() is case-sensitive, i.e.
$p = new WP_HTML_Tag_Processor( '<div DATA-enabled class="test" data-test-id="14">Test</div>' );
$p->get_attribute( 'DATA-enabled' ) === true;
$p->get_attribute( 'DATA-test-id' ) === null;

$p->get_attribute( 'data-enabled' ) === null;
$p->get_attribute( 'data-test-id' ) === '14';

$p->get_attribute( 'DATA-ENABLED' ) === null;
$p->get_attribute( 'DATA-TEST-ID' ) === null;

Then, IMO,

  • get_attributes_by_prefix( 'data-' ) should return array( 'data-test-id' => '14')
  • get_attributes_by_prefix( 'DATA-' ) should return array( 'DATA-enabled' => true)
  1. Assuming get_attribute() is case-insensitive, i.e.
$p = new WP_HTML_Tag_Processor( '<div DATA-enabled class="test" data-test-id="14">Test</div>' );
$p->get_attribute( 'DATA-enabled' ) === true;
$p->get_attribute( 'DATA-test-id' ) === '14';

$p->get_attribute( 'data-enabled' ) === true;
$p->get_attribute( 'data-test-id' ) === '14';

$p->get_attribute( 'DATA-ENABLED' ) === true;
$p->get_attribute( 'DATA-TEST-ID' ) === '14';

Then, IMO,

  • get_attributes_by_prefix( 'data-' ) should return array( 'DATA-enabled' => true, 'data-test-id' => '14' )
  • get_attributes_by_prefix( 'DATA-' ) should return array( 'DATA-enabled' => true, 'data-test-id' => '14' )

Does that make sense?


Reality-checking the behavior of get_attribute() is fun 😬

$p = new WP_HTML_Tag_Processor( '<div DATA-enabled class="test" data-test-id="14">Test</div>' );
$p->next_tag();
$p->get_attribute( 'data-enabled' ) === null; // Okay, I guess we're case-sensitive.
$p->get_attribute( 'DATA-ENABLED' ) === null; // Yep, looks like.
$p->get_attribute( 'DATA-enabled' ) === null; // ???

Is this expected? 😬

Copy link
Contributor

@adamziel adamziel Dec 22, 2022

Choose a reason for hiding this comment

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

Oh golly, that's a bug. Attributes should be case-insensitive. Here's a fix: #46748

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for the fix! I just merged it 😄

@ockham ockham force-pushed the add/get-attributes-by-prefix-method branch from e06c326 to b7cac44 Compare January 2, 2023 11:02
@ockham
Copy link
Contributor Author

ockham commented Jan 2, 2023

Rebased (to include #46748).

@ockham
Copy link
Contributor Author

ockham commented Jan 10, 2023

Closing if favor of #46840.

@ockham ockham closed this Jan 10, 2023
@ockham ockham deleted the add/get-attributes-by-prefix-method branch January 10, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants