I think this should rely on WP_HTML_Text_Replacement and lexical_updates, is there a reason it does not?

The algorithm should be something like:

• Ensure the processor is stopped on an open tag that is not atomic (like SCRIPT), void (like BR), nor foreign content with a self-closing flag (like G in (<svg><g /></svg>).
• It may be necessary to flush updates with get_updated_html, I'm not sure off the top of my head.
• Find the open tag, use a bookmark to get its closing position.
• Find the matching close tag, use a bookmark to find the byte length of the replacement.
• Push a lexical update replacement like this:

$this->lexical_updates[] = new WP_HTML_Text_Replacement(
	$start,
	$length,
	$replacement_html
);

I'd like if the function mentioned that the replacement is not checked for proper HTML nesting, it's unsafe by nature. The method provides raw HTML replacement between tags. There's no guarantee that the HTML will nest as expected after the replacement is applied.

I think this should rely on WP_HTML_Text_Replacement and lexical_updates, is there a reason it does not?

The algorithm should be something like: [...]

I think that would amount to a basic version of set_inner_html(), no? The idea for WP_Block_Bindings_Processor was that it would give weaker guarantees -- e.g. you can't currently replace_rich_text(), then seek() to an earlier position, and make another change. This simpler and less safe implementation should however be sufficient for the problem domain -- i.e. block bindings -- where we can make a few assumptions on what kind of operations are needed.

The current approach is based on a recommendation and an earlier experiment by @dmsnell, which is similar in spirit (with a string buffer that's totally separate from the one that's kept by WP_HTML_Processor).

That said, I can spin up another PR to try out the lexical_updates-based approach.

I'd like if the function mentioned that the replacement is not checked for proper HTML nesting, it's unsafe by nature. The method provides raw HTML replacement between tags. There's no guarantee that the HTML will nest as expected after the replacement is applied.

Good point. Something like that is mentioned in the PHPDoc of the class, but it's a good idea to also include it in the method's.

That said, I can spin up another PR to try out the lexical_updates-based approach.

ockham#6

I think that would amount to a basic version of set_inner_html(), no?

Yes, but an unsafe version of set_inner_html(). Similar to what's implemented here.

The idea for WP_Block_Bindings_Processor was that it would give weaker guarantees -- e.g. you can't currently replace_rich_text(), then seek() to an earlier position, and make another change. This simpler and less safe implementation should however be sufficient for the problem domain -- i.e. block bindings -- where we can make a few assumptions on what kind of operations are needed.

The current approach is based on a recommendation and an earlier experiment by @dmsnell, which is similar in spirit (with a string buffer that's totally separate from the one that's kept by WP_HTML_Processor).

It's unclear to me why this would be safer. The unsafe operation is intending to put HTML inside a tag that potentially "leaks" to change external structure. Once we do that here, it feels like the unsafe part has happened. Taking the result of this and putting it back into another processor easily circumvents the prevention on seeks and such.

for context I was using substr() because we controlled the entire execution environment and only moved forward after making any changes.

while I have no problem moving to using lexical updates, it may be valuable to wait until the end to do things with them given that the HTML Processor is already relatively slow.

what I’ve done in the past is keep an external array of updates, then at the end dump them into a new Tag Processor for batch replacement. but still, I think that moving forward only is important here.

$this->replacements[] = new WP_HTML_Text_Replacment(…);

…

$replacer = new class( $html ) extends WP_HTML_Tag_Processor {
	public function swap_replacements( $replacements ) {
		$this->lexical_updates = $replacements;
	}
}

$replacer->swap_replacements( $this->replacements );
return $replacer->get_updated_html();

@ockham and I did discuss safety and I was suggesting that this serialization builder is probably good enough for now, providing comparable security to what Dom\HtmlDocument provides. everything above that is even better.

• Ensure the processor is stopped on an open tag that is not atomic (like SCRIPT), void (like BR), nor foreign content with a self-closing flag (like G in (<svg><g /></svg>).

Looks like expects_closer() (negated) should cover those cases: 3410f40

FWIW, the way that the attribute replacing in block bindings currently works is as follows:

• For each bound block attribute, we create a new processor for the block markup. The type of processor we create depends on the attribute source:
  • If the attribute source is rich-text, we create a block bindings processor; if the attribute source is an HTML attribute, we create an HTML Tag Processor.
• We use the processor to locate the sourced attribute's content in the block markup, and replace it with its new value.
• We then call get_updated_html() to get the resulting HTML.

This means that there's currently only ever one attribute replaced per processor created. We also only move forward within that processor, with one exception: If we have a comma-separated selector like e.g. the Button block's text attribute has, we iterate over those individual tag selectors, and reset the processor's cursor to the first tag (by seek()ing a bookmark that we set initially).

In practice, a comma-separated selector such as a,button is there because the user can choose whether they want their button block to use an a tag or a button. These choices are of course mutually exclusive -- one button block will either contain an a or a button, but never both. As a result, the seek will only happen before the block markup is modified, which AFAICT should be fine ✅

It is however arguable that instantiating a new processor for each attribute isn't great in terms of performance. If we were ever going to change that and instead use one processor to update all bound attributes, we'd have to deal with cases like the following -- an image block whose url and caption attributes need to be bound:

<!-- wp:image {"metadata":{"bindings":{"url":{"source":"test/source"}}}} -->
<figure class="wp-block-image">
<img src="fallback.jpg" alt=""/>
<figcaption class="wp-element-caption">Fallback caption</figcaption></figure>
<!-- /wp:image -->

I guess we'd change the algorithm to advance through the markup tag-by-tag, check if the current tag matches any of the sourced attribute selectors, and replace it if yes. So we'd probably still be able to ensure forward movement.

So what's the bottom line? I'd say that we can guarantee forward-only movement right now, meaning that the serialization builder would probably be good enough indeed. I'm also fine with the WP_HTML_Text_Replacment-based one.

@dmsnell As for the extra optimization of keeping a separate stack of WP_HTML_Text_Replacments, and feeding them to a tag processor at the end for better performance, I'm wondering if that makes a difference for our use case? We stop traversing the document and apply the (only) change via get_updated_html() once we've located the portion that needs replacing anyway.

I've applied Dennis' suggestion locally:

diff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index e2b75f6643..54f175bb38 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -435,7 +435,13 @@ class WP_Block {
 						// TODO: Use `WP_HTML_Processor::set_inner_html` method once it's available.
 						$block_reader->release_bookmark( 'iterate-selectors' );
 						$block_reader->replace_rich_text( wp_kses_post( $source_value ) );
-						return $block_reader->get_updated_html();
+						$replacer = new class( $block_content ) extends WP_HTML_Tag_Processor {
+							public function swap_replacements( $replacements ) {
+								$this->lexical_updates = $replacements;
+							}
+						};
+						$replacer->swap_replacements( $block_reader->my_lexical_updates );
+						return $replacer->get_updated_html();
 					} else {
 						$block_reader->seek( 'iterate-selectors' );
 					}
@@ -463,6 +469,8 @@ class WP_Block {
 
 	private static function get_block_bindings_processor( string $block_content ) {
 		$internal_processor_class = new class('', WP_HTML_Processor::CONSTRUCTOR_UNLOCK_CODE) extends WP_HTML_Processor {
+			public $my_lexical_updates = array();
+
 			/**
 			 * Replace the rich text content between a tag opener and matching closer.
 			 *
@@ -494,7 +502,7 @@ class WP_Block {
 				$end         = $tag_closer->start;
 				$this->release_bookmark( '_wp_block_bindings_tag_closer' );
 
-				$this->lexical_updates[] = new WP_HTML_Text_Replacement(
+				$this->my_lexical_updates[] = new WP_HTML_Text_Replacement(
 					$start,
 					$end - $start,
 					$rich_text

One thing I've noted based on the unit tests I wrote for this PR is that it becomes harder to mix attribute changes (that rely on methods that are native to the HTML (Tag) Processor) with rich text replacements that rely on the block bindings processor -- essentially requiring an extra get_updated_html() to apply the former before the latter:

diff --git a/tests/phpunit/tests/block-bindings/wp-block-get-block-bindings-processor.php b/tests/phpunit/tests/block-bindings/wp-block-get-block-bindings-processor.php
index b199e10b0e..1714734d5a 100644
--- a/tests/phpunit/tests/block-bindings/wp-block-get-block-bindings-processor.php
+++ b/tests/phpunit/tests/block-bindings/wp-block-get-block-bindings-processor.php
@@ -45,26 +52,35 @@ class Tests_Blocks_GetBlockBindingsProcessor extends WP_UnitTestCase {
 		$figure_opener = '<figure class="wp-block-image">';
 		$img           = '<img src="breakfast.jpg" alt="" class="wp-image-1"/>';
 		$figure_closer = '</figure>';
-		$processor     = self::$get_block_bindings_processor_method->invoke(
-			null,
-			$figure_opener .
+
+		$html = $figure_opener .
 			$img .
 			'<figcaption class="wp-element-caption">Breakfast at a <em>café</em> in Berlin</figcaption>' .
-			$figure_closer
-		);
+			$figure_closer;
+
+		$processor = self::$get_block_bindings_processor_method->invoke( null, $html );
 
 		$processor->next_tag( array( 'tag_name' => 'figure' ) );
 		$processor->add_class( 'size-large' );
+		$html = $processor->get_updated_html(); // This right here.
 
 		$processor->next_tag( array( 'tag_name' => 'figcaption' ) );
 
 		$this->assertTrue( $processor->replace_rich_text( '<strong>New</strong> image caption' ) );
+
+		$replacer = new class( $html ) extends WP_HTML_Tag_Processor {
+			public function swap_replacements( $replacements ) {
+				$this->lexical_updates = $replacements;
+			}
+		};
+		$replacer->swap_replacements( $processor->my_lexical_updates );
+
 		$this->assertEquals(
 			'<figure class="wp-block-image size-large">' .
 			$img .
 			'<figcaption class="wp-element-caption"><strong>New</strong> image caption</figcaption>' .
 			$figure_closer,
-			$processor->get_updated_html()
+			$replacer->get_updated_html()
 		);
 	}
 

It gets of course even messier if the attribute change is made after the rich text change, and at an earlier position than the rich text change.

reasonable follow-up idea: verify that the processor completed and didn’t pause at an incomplete token or reach unsupported markup.

Finally addressing these notes over at #10029.

This look about good? 8d7590e
(I was at first thinking to check if state was STATE_MATCHED_TAG, but then I thought, the higher-level check should give me that anyway.)

looks good; I left a comment on the new PR.

this is certainly welcome to do, to release the bookmarks, but performance-wise it’s not setting a better example than leaving them dangling.

given that this function contains all of the scanning, it’s even fine to trap the bookmark bounds and re-use one, such as set_bookmark( 'here' ). I’ve considered exposing something like ->current_token() but haven’t yet.

part of this is the fact that we expect isolation of bookmarks here and we will dismiss the processor when we’re done.

applying lexical updates that span token boundaries is not something we have particularly tested well. it’s probably working well enough and hopefully the tests here will catch them, but this is sort of uncharted territory within the HTML Processor. we should make sure that we don’t seek anywhere here to verify that we don’t cross indices.

this single issue is one of the main reasons I proposed the single linear pass of the serialization builder, because it structurally prevents the ability of jumping around or referring to indices after changing positions.

I’m not particularly confident I would know how this interacts with virtual tokens, for example, whose boundaries are coincident with bookmarks (vs. realized tokens which all have non-overlapping spans)