Block Bindings: Allow more generic setting of block attributes #9469
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * WP_Block_Bindings_Processor class. | ||
| * | ||
| * This class can be used to perform the sort of structural | ||
| * changes to an HTML document that are required by | ||
| * Block Bindings. Namely, proper nesting structure of HTML is | ||
| * maintained, but HTML updates could still leak out of the | ||
| * containing parent node. For example, this allows inserting | ||
| * an A element inside an open A element, which would close | ||
| * the containing A element. | ||
|
|
||
| * Modifications may be requested for a document _once_ after | ||
| * matching a token. Due to the way the modifications are | ||
| * applied, it's not possible to replace the rich text content | ||
| * for a node more than once. Furthermore, if a `replace_rich_text()` | ||
| * operation is followed by a `seek()` to a position before the | ||
| * updated rich text content, any modification at that earlier | ||
| * position will lead to broken output. | ||
| * | ||
| * @access private | ||
| * | ||
| * @package WordPress | ||
| * @subpackage Block Bindings | ||
| * @since 6.9.0 | ||
| */ | ||
| class WP_Block_Bindings_Processor extends WP_HTML_Processor { | ||
| private $output = ''; | ||
| private $end_of_flushed = 0; | ||
|
|
||
| public function build() { | ||
| return $this->output . substr( $this->get_updated_html(), $this->end_of_flushed ); | ||
| } | ||
|
|
||
| /** | ||
| * Replace the rich text content between a tag opener and matching closer. | ||
| * | ||
| * When stopped on a tag opener, replace the content enclosed by it and its | ||
| * matching closer with the provided rich text. | ||
| * | ||
| * @param string $rich_text The rich text to replace the original content with. | ||
| * @return bool True on success. | ||
| */ | ||
| public function replace_rich_text( $rich_text ) { | ||
|
There was a problem hiding this comment. I think this should rely on The algorithm should be something like:
$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. There was a problem hiding this comment.
I think that would amount to a basic version of 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 That said, I can spin up another PR to try out the
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. There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, but an unsafe version of
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. |
||
| if ( $this->is_tag_closer() ) { | ||
| return false; | ||
| } | ||
|
|
||
| $depth = $this->get_current_depth(); | ||
|
|
||
| $this->set_bookmark( '_wp_block_bindings_tag_opener' ); | ||
| // The bookmark names are prefixed with `_` so the key below has an extra `_`. | ||
| $bm = $this->bookmarks['__wp_block_bindings_tag_opener']; | ||
| $this->output .= substr( $this->get_updated_html(), $this->end_of_flushed, $bm->start + $bm->length ); | ||
| $this->output .= $rich_text; | ||
| $this->release_bookmark( '_wp_block_bindings_tag_opener' ); | ||
|
|
||
| // Find matching tag closer. | ||
| while ( $this->next_token() && $this->get_current_depth() >= $depth ) { | ||
| } | ||
|
|
||
| $this->set_bookmark( '_wp_block_bindings_tag_closer' ); | ||
| $bm = $this->bookmarks['__wp_block_bindings_tag_closer']; | ||
| $this->end_of_flushed = $bm->start; | ||
| $this->release_bookmark( '_wp_block_bindings_tag_closer' ); | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| <?php | ||
| /** | ||
| * Tests for WP_Block_Bindings_Processor. | ||
| * | ||
| * @package WordPress | ||
| * @subpackage Blocks | ||
| * @since 6.5.0 | ||
| * | ||
| * @group blocks | ||
| * @group block-bindings | ||
| */ | ||
| class Tests_Blocks_wpBlockBindingsProcessor extends WP_UnitTestCase { | ||
| /** | ||
| * @ticket 63840 | ||
| */ | ||
| public function test_replace_rich_text() { | ||
| $button_wrapper_opener = '<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">'; | ||
| $button_wrapper_closer = '</a></div>'; | ||
| $processor = WP_Block_Bindings_Processor::create_fragment( | ||
| $button_wrapper_opener . 'This should not appear' . $button_wrapper_closer | ||
| ); | ||
| $processor->next_tag( array( 'tag_name' => 'a' ) ); | ||
|
|
||
| $this->assertTrue( $processor->replace_rich_text( 'The hardest button to button' ) ); | ||
| $this->assertEquals( | ||
| $button_wrapper_opener . 'The hardest button to button' . $button_wrapper_closer, | ||
| $processor->build() | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @ticket 63840 | ||
| */ | ||
| public function test_set_attribute_and_replace_rich_text() { | ||
| $figure_opener = '<figure class="wp-block-image">'; | ||
| $img = '<img src="breakfast.jpg" alt="" class="wp-image-1"/>'; | ||
| $figure_closer = '</figure>'; | ||
| $processor = WP_Block_Bindings_Processor::create_fragment( | ||
| $figure_opener . | ||
| $img . | ||
| '<figcaption class="wp-element-caption">Breakfast at a <em>café</em> in Berlin</figcaption>' . | ||
| $figure_closer | ||
| ); | ||
|
|
||
| $processor->next_tag( array( 'tag_name' => 'figure' ) ); | ||
| $processor->add_class( 'size-large' ); | ||
|
|
||
| $processor->next_tag( array( 'tag_name' => 'figcaption' ) ); | ||
|
|
||
| $this->assertTrue( $processor->replace_rich_text( '<strong>New</strong> image caption' ) ); | ||
| $this->assertEquals( | ||
| '<figure class="wp-block-image size-large">' . | ||
| $img . | ||
| '<figcaption class="wp-element-caption"><strong>New</strong> image caption</figcaption>' . | ||
| $figure_closer, | ||
| $processor->build() | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * @ticket 63840 | ||
| */ | ||
| public function test_replace_rich_text_and_seek() { | ||
| $figure_opener = '<figure class="wp-block-image">'; | ||
| $img = '<img src="breakfast.jpg" alt="" class="wp-image-1"/>'; | ||
| $figure_closer = '</figure>'; | ||
| $processor = WP_Block_Bindings_Processor::create_fragment( | ||
| $figure_opener . | ||
| $img . | ||
| '<figcaption class="wp-element-caption">Breakfast at a <em>café</em> in Berlin</figcaption>' . | ||
| $figure_closer | ||
| ); | ||
|
|
||
| $processor->next_tag( array( 'tag_name' => 'img' ) ); | ||
| $processor->set_bookmark( 'image' ); | ||
|
|
||
| $processor->next_tag( array( 'tag_name' => 'figcaption' ) ); | ||
|
|
||
| $this->assertTrue( $processor->replace_rich_text( '<strong>New</strong> image caption' ) ); | ||
|
|
||
| $processor->seek( 'image' ); | ||
|
|
||
| $this->assertEquals( | ||
| $figure_opener . | ||
| $img . | ||
| '<figcaption class="wp-element-caption"><strong>New</strong> image caption</figcaption>' . | ||
| $figure_closer, | ||
| $processor->build() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has
@access private, but it may be worth noting early and clearly in this description that the class is only for use by WordPress core and usage by extenders is not supported and likely to break.Is there any way to avoid adding the file and class? The HTML API should publicly provide the necessary functionality someday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with such a narrative in WordPress core. Everything is rather done in the spirit that it can be used by 3rd party devs, so public API needs to be continued indefinitely. At least it should not fatally affect the existing code if major changes are necessary.
It's perfectly fine to use a specialized class for Block Bindings even if it one day becomes only a facade. It's similar to how Interactivity API directives processing is structured:
wordpress-develop/src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php
Line 18 in 66f3fa0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only caveat of having a final class is that we won't be able to extend it from Gutenberg in future developments. Meaning we will have to always combine a WordPress 'develop' version with Gutenberg in case we need to work both with Core functions and editor features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, happy to warn more verbosely against that 👍
It's a problem of balancing the desire for the "right" implementation (that will take longer) with the need for a "good enough" one (that we can use sooner). Block Bindings currently suffer from being not very scalable, which is to a large degree due to the way that its attribute replacing logic is hard-coded on a per-block basis. We're pretty adamant that we need to replace this with a more generic way in order to see wider adoption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible implement this with an anonymous class in the function that processes block bindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably 🤔 I can give that a try.