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.

Sign up for GitHub

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

Jump to bottom

WP_HTML_Tag_Processor: Add get_attribute_names_with_prefix() method #46840

Merged
merged 15 commits into from
Jan 10, 2023
Merged

WP_HTML_Tag_Processor: Add get_attribute_names_with_prefix() method #46840

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4e4b957
WP_HTML_Tag_Processor: Implement get_attribute_names()
ockham Dec 20, 2022
1573010
Remove superfluous parameter comment
ockham Dec 22, 2022
8be4ac9
Move test down
ockham Jan 2, 2023
8c51859
Add test for get_attribute() when in closing tag
ockham Jan 2, 2023
ca06c64
Add more get_attribute_names tests()
ockham Jan 2, 2023
2dbf838
Fix implementation
ockham Jan 2, 2023
5b248c0
Verify that get_attribute_names finds attribute added by set_attribute
ockham Jan 2, 2023
5f3e9ff
Replace get_attribute_names with get_attribute_names_with_prefix
ockham Jan 2, 2023
0c61498
Check for case-insensitive matching
ockham Jan 2, 2023
a9d8ec3
Document prefix param
ockham Jan 3, 2023
8a8c1e2
Forgot to include arg in examples
ockham Jan 5, 2023
263cfa1
Polish PHPDoc
ockham Jan 5, 2023
a4b1071
Optimize
ockham Jan 5, 2023
37c94c3
Use foreach
ockham Jan 9, 2023
ca46027
Remove parens around '(lowercase)'
ockham Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions lib/experimental/html/class-wp-html-tag-processor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,40 @@ public function get_attribute( $name ) {
return html_entity_decode( $raw_value );
}

/**
* Returns the names of all attributes matching a given prefix in the currently-opened tag.
ockham marked this conversation as resolved.
Show resolved Hide resolved
*
* Example:
* <code>
* $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_attribute_names_with_prefix() === array( 'data-enabled', 'data-test-id' );
*
* $p->next_tag( [] ) === false;
* $p->get_attribute_names_with_prefix() === null;
* </code>
*
* @since 6.2.0
*
* @param string $prefix Prefix of requested attribute names.
* @return array|null List of attribute names, or `null` if not at a tag.
*/
function get_attribute_names_with_prefix( $prefix ) {
if ( $this->is_closing_tag || null === $this->tag_name_starts_at ) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to stick with null 😊 👍

}

$comparable = strtolower( $prefix );

$matches = array_filter(
array_keys( $this->attributes ),
function( $attr ) use ( $comparable ) {
return str_starts_with( $attr, $comparable );
}
);
return array_values( $matches );
ockham marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Returns the lowercase name of the currently-opened tag.
*
Expand Down
94 changes: 94 additions & 0 deletions phpunit/html/wp-html-tag-processor-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,19 @@ public function test_get_attribute_returns_null_when_not_in_open_tag() {
$this->assertNull( $p->get_attribute( 'class' ), 'Accessing an attribute of a non-existing tag did not return null' );
}

/**
* @ticket 56299
*
* @covers next_tag
* @covers get_attribute
*/
public function test_get_attribute_returns_null_when_in_closing_tag() {
$p = new WP_HTML_Tag_Processor( '<div class="test">Test</div>' );
$this->assertTrue( $p->next_tag( 'div' ), 'Querying an existing tag did not return true' );
$this->assertTrue( $p->next_tag( array( 'tag_closers' => 'visit' ) ), 'Querying an existing closing tag did not return true' );
$this->assertNull( $p->get_attribute( 'class' ), 'Accessing an attribute of a closing tag did not return null' );
}

/**
* @ticket 56299
*
Expand Down Expand Up @@ -195,6 +208,87 @@ public function test_set_attribute_is_case_insensitive() {
$this->assertEquals( '<div data-enabled="abc">Test</div>', $p->get_updated_html(), 'A case-insensitive set_attribute call did not update the existing attribute.' );
}

/**
* @ticket 56299
*
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_null_before_finding_tags() {
$p = new WP_HTML_Tag_Processor( '<div data-foo="bar">Test</div>' );
$this->assertNull( $p->get_attribute_names_with_prefix( 'data-' ) );
}

/**
* @ticket 56299
*
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_null_when_not_in_open_tag() {
$p = new WP_HTML_Tag_Processor( '<div data-foo="bar">Test</div>' );
$p->next_tag( 'p' );
$this->assertNull( $p->get_attribute_names_with_prefix( 'data-' ), 'Accessing attributes of a non-existing tag did not return null' );
}

/**
* @ticket 56299
*
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_null_when_in_closing_tag() {
$p = new WP_HTML_Tag_Processor( '<div data-foo="bar">Test</div>' );
$p->next_tag( 'div' );
$p->next_tag( array( 'tag_closers' => 'visit' ) );
$this->assertNull( $p->get_attribute_names_with_prefix( 'data-' ), 'Accessing attributes of a closing tag did not return null' );
}

/**
* @ticket 56299
*
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_empty_array_when_no_attributes_present() {
$p = new WP_HTML_Tag_Processor( '<div>Test</div>' );
$p->next_tag( 'div' );
$this->assertSame( array(), $p->get_attribute_names_with_prefix( 'data-' ), 'Accessing the attributes on a tag without any did not return an empty array' );
}

/**
* @ticket 56299
*
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_matching_attribute_names_in_lowercase() {
$p = new WP_HTML_Tag_Processor( '<div DATA-enabled class="test" data-test-ID="14">Test</div>' );
$p->next_tag();
$this->assertSame(
array( 'data-enabled', 'data-test-id' ),
$p->get_attribute_names_with_prefix( 'data-' )
);
}

/**
* @ticket 56299
*
* @covers set_attribute
* @covers get_updated_html
* @covers get_attribute_names_with_prefix
*/
public function test_get_attribute_names_with_prefix_returns_attribute_added_by_set_attribute() {
$p = new WP_HTML_Tag_Processor( '<div data-foo="bar">Test</div>' );
$p->next_tag();
$p->set_attribute( 'data-test-id', '14' );
$this->assertSame(
'<div data-test-id="14" data-foo="bar">Test</div>',
$p->get_updated_html(),
"Updated HTML doesn't include attribute added via set_attribute"
);
$this->assertSame(
array( 'data-test-id', 'data-foo' ),
$p->get_attribute_names_with_prefix( 'data-' ),
"Accessing attribute names doesn't find attribute added via set_attribute"
);
}

/**
* @ticket 56299
*
Expand Down