HTML API: Fix tag processor token length

[sirreal]

It seems like the open tags have incorrect length in the tag processor. I'd expect that substr( $html, $token_start, $token_length ); to produce the entire tag. This patch fixes that, before it would exclude the final > on tags.

This is mostly internal and it seems to have adjustments in several places to fix it, but I'd like to have a consistent internal representation. Other types of tokens behave as I'd expect (with substr( $html, $start, $length )).

@dmsnell thoughts?

I'd expect something like this to hold:

<?php

$t = '<div class="target">text</div>trailing text';
$start = 0;
$token_length = 20;
$text_start = $token_length - $start;
$text_length = 4;

$end_start = $text_start + $text_length;
$end_length = 6;

var_dump(
    substr( $t, $start, $token_length ),
    substr( $t, $text_start, $text_length ),
    substr( $t, $end_start, $end_length ),
);

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The 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

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[dmsnell]

I'd rather us avoid reflection in tests, giving tests privileges that normal code shouldn't have.

not sure where, but I had a test set once which verified not the lengths, but the bookmark spans.

$processor = new class ( $html ) extends WP_HTML_Tag_Processor {
	public function get_raw_token() {
		$this->set_bookmark( 'here' );
		$mark = $this->bookmarks['here'];

		return substr( $this->html, $mark->start, $mark->length );
	}
}

for ( $i = 0; $i < $nth_token_in_html; $i++ ) {
	$processor->next_token();
}

$this->assertSame(
	$raw_token,
	$processor->get_raw_token(),
	'Failed to identify full extent of token.'
);

[dmsnell]

This has been a longstanding annoyance to me, as it's clearly wrong, but I've had trouble fixing it without breaking other tokens. Let's see if we can test it from the bookmarks, and then add a comprehensive test dataset with all kinds of tokens and various HTML constructs (e.g. the HTML contains more than just the token by itself so we can see edge cases).

[github-actions]

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, dmsnell, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[dmsnell]

The test failure in #6651 makes me nervous.

[dmsnell]

@sirreal can you look into the failure in the linked PR? I think it's related to seeking with updates and getting the wrong $this->bytes_already_processed

[sirreal]

The test failure in #6651 makes me nervous.
Can you look into the failure in the linked PR? I think it's related to seeking with updates and getting the wrong $this->bytes_already_processed

68a3251 was missing from that branch. There was a test that used the broken offset. The offset adjustment was removed in this branch, I pushed it to #6651.

[sirreal]

@westonruter This is a bugfix for incorrect token lengths in tag bookmarks in the Tag Processor. Subclasses might see and be affected by the bug (and the fix) if they ever work directly with tag bookmarks lengths. I believe you have some plugins that may be doing advanced things with the Tag Processor so wanted to make sure you were aware.

Below is an example fix from this PR, plugins would be impacted in the same way and would need to remove the off-by-one correction on the bookmark lengths:

diff --git a/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php b/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
index 3b2dcb123797..b12dcb4b3b15 100644
--- a/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
+++ b/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
@@ -107,7 +107,7 @@ public function append_content_after_template_tag_closer( string $new_content ):
 
 		$bookmark = 'append_content_after_template_tag_closer';
 		$this->set_bookmark( $bookmark );
-		$after_closing_tag = $this->bookmarks[ $bookmark ]->start + $this->bookmarks[ $bookmark ]->length + 1;
+		$after_closing_tag = $this->bookmarks[ $bookmark ]->start + $this->bookmarks[ $bookmark ]->length;
 		$this->release_bookmark( $bookmark );
 
 		// Appends the new content.
@@ -140,7 +140,7 @@ private function get_after_opener_tag_and_before_closer_tag_positions( bool $rew
 		}
 		list( $opener_tag, $closer_tag ) = $bookmarks;
 
-		$after_opener_tag  = $this->bookmarks[ $opener_tag ]->start + $this->bookmarks[ $opener_tag ]->length + 1;
+		$after_opener_tag  = $this->bookmarks[ $opener_tag ]->start + $this->bookmarks[ $opener_tag ]->length;
 		$before_closer_tag = $this->bookmarks[ $closer_tag ]->start;
 
 		if ( $rewind ) {

[westonruter]

@sirreal See the following method in this subclass of WP_HTML_Tag_Processor:

	/**
	 * Appends HTML to the provided bookmark.
	 *
	 * @param string $bookmark Bookmark.
	 * @param string $html     HTML to inject.
	 * @return bool Whether the HTML was appended.
	 */
	public function append_html( string $bookmark, string $html ): bool {
		if ( ! $this->has_bookmark( $bookmark ) ) {
			return false;
		}

		$start = $this->bookmarks[ $bookmark ]->start;

		$this->lexical_updates[] = new WP_HTML_Text_Replacement(
			$start,
			$this->old_text_replacement_signature_needed ? $start : 0,
			$html
		);
		return true;
	}

So since it's not dealing with length then is a patch needed?

[dmsnell]

So since it's not dealing with length then is a patch needed?

this should be correct, @westonruter

[dmsnell]

Great fix ahead of exposing it in any public interface!

[dmsnell]

Merged in [58233]
6ca5bdc