Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#62427 closed defect (bug) (fixed)

HTML API: Next tag tag name query is case sensitive

Reported by: jonsurrell's profile jonsurrell Owned by: desrosj's profile desrosj
Milestone: 6.7.2 Priority: normal
Severity: normal Version: 6.6
Component: HTML API Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

The HTML API ::next_tag method accepts a tag_name query. The tag name matching should be case-insensitive. Tag names are generally UPPERCASE in the processors, a lower or mixed case name doesn't make much sense.

<?php
// This is OK
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( 'div' ) );

// This is OK
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( array( 'breadcrumbs' => array( 'div' ) ) ) );

// This does not work but should
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( array( 'tag_name' => 'div' ) ) ); // AssertionError!

This is also supported in the tag processor case-insensitively:

<?php
$processor = new WP_HTML_Tag_Processor( '<section><DIV>' );
assert( $processor->next_tag( array( 'tag_name' => 'div' ) ) );
$processor->get_tag(); // "DIV"

Change History (13)

This ticket was mentioned in PR #7754 on WordPress/wordpress-develop by @jonsurrell.


3 months ago
#1

  • Keywords has-patch has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/62427

The HTML API ::next_tag method accepts a tag_name query. The tag name matching should be case-insensitive. Tag names are generally UPPERCASE in the processors, a lower or mixed case name doesn't make much sense.

// This is OK
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( 'div' ) );

// This is OK
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( array( 'breadcrumbs' => array( 'div' ) ) ) );

// This does not work but should
$processor = WP_HTML_Processor::create_fragment( '<DIV>' );
assert( $processor->next_tag( array( 'tag_name' => 'div' ) ) ); // AssertionError!

This is also supported in the tag processor case-insensitively:

$processor = new WP_HTML_Tag_Processor( '<section><DIV>' );
assert( $processor->next_tag( array( 'tag_name' => 'div' ) ) );
$processor->get_tag(); // "DIV"

@dmsnell commented on PR #7754:


3 months ago
#2

Did we originally do this intentionally ASCII case-sensitively, and if so, why?

@jonsurrell commented on PR #7754:


3 months ago
#3

As far as I can tell this was an oversight. I can't think of a reason _to_ match this case-sensitively.

I suspect this was not noticed because the shorthand exists for the next tag with a tag name in the HTML Processor: next_tag( 'div' ).

Under the hood that uses next_tag( array( 'breadcrumbs' => array( 'div' ) ) ):

https://github.com/WordPress/wordpress-develop/blob/e9dfa8c34c74cb51da93ae4b4e61c1df0378b3d7/src/wp-includes/html-api/class-wp-html-processor.php#L547-L549

And _that_ is handled correctly by the matches_breadcrumbs function:

https://github.com/WordPress/wordpress-develop/blob/e9dfa8c34c74cb51da93ae4b4e61c1df0378b3d7/src/wp-includes/html-api/class-wp-html-processor.php#L776

(I had included a change in this patch to change the shorthand implementation to use tag_name instead of breadcrumbs, but removed that as out-of-scope for a bugfix.)

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 months ago

#5 @desrosj
3 months ago

  • Milestone changed from 6.7.1 to 6.7.2

Thanks @jonsurrell! Does this cause any unintended side effects as is? It's not clear if this fixes unexpected behavior or is meant for consistency/clarity.

That said, because of a few bug reports opened since 6.7 was released, the Core team is evaluating the need for a short 6.7.1 cycle (possibly next week).

To help prepare for this scenario in case it's decided to move forward, I'm going to punt this to the 6.7.2 milestone. This issue was not introduced in 6.7, so it now falls outside of the focus for 6.7.1 as currently defined.

#6 @jonsurrell
3 months ago

6.7.2 should be fine for this change. Thanks for the heads up.

#7 @czapla
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 59422:

HTML API: Use case insensitive tag_name comparison in ::next_tag.

The HTML API ::next_tag method now performs case-insensitive matching when searching for tags by name. For example, searching for 'DIV' will match both '<div>' and '<DIV>' tags.

Props jonsurrell, dmsnell.
Fixes #62427.

#9 @jonsurrell
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for consideration for 6.7.2.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


2 months ago

#11 @jorbin
2 months ago

  • Keywords dev-feedback added

#12 @desrosj
2 months ago

  • Owner changed from jonsurrell to desrosj
  • Status changed from reopened to reviewing

#13 @desrosj
2 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59535:

HTML API: Use case insensitive tag_name comparison in ::next_tag.

The HTML API ::next_tag method now performs case-insensitive matching when searching for tags by name. For example, searching for 'DIV' will match both '<div>' and '<DIV>' tags.

Reviewed by desrosj.
Merges [59422] to the 6.7 branch.

Props jonsurrell, dmsnell, czapla.
Fixes #62427.

Note: See TracTickets for help on using tickets.