-
Notifications
You must be signed in to change notification settings - Fork 3k
Block Bindings: Allow more generic setting of block attributes #9469
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.
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
base: trunk
Are you sure you want to change the base?
Block Bindings: Allow more generic setting of block attributes #9469
Conversation
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
e390302
to
83b3ae1
Compare
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Here's a little experiment that builds on this PR: ockham#5 |
This is awesome! Can we replicate the same logic in the Gutenberg plugin or does it require any code from HTML API in I would be happy to explore how this could be integrated with WordPress/gutenberg#70975. |
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've left some early feedback and questions.
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (likeBR
), nor foreign content with a self-closing flag (likeG
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.
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 think this should rely on
WP_HTML_Text_Replacement
andlexical_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.
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.
That said, I can spin up another PR to try out the
lexical_updates
-based approach.
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 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 currentlyreplace_rich_text()
, thenseek()
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.
<?php | ||
|
||
/** | ||
* WP_Block_Bindings_Processor class. |
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.
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.
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:
Line 18 in 66f3fa0
final class WP_Interactivity_API_Directives_Processor extends WP_HTML_Tag_Processor { |
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.
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.
Yeah, happy to warn more verbosely against that 👍
Is there any way to avoid adding the file and class? The HTML API should publicly provide the necessary functionality someday.
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.
Is it possible implement this with an anonymous class in the function that processes block bindings?
Probably 🤔 I can give that a try.
I don't think it relies on any new additions to the HTML API that are only present in
👍 FWIW, non-conditional binding (i.e. replacement of an existing Patchdiff --git a/src/wp-includes/class-wp-block.php b/src/wp-includes/class-wp-block.php
index 173b0dbe80..902093fe3b 100644
--- a/src/wp-includes/class-wp-block.php
+++ b/src/wp-includes/class-wp-block.php
@@ -112,6 +112,7 @@ class WP_Block {
'core/image' => array( 'id', 'url', 'title', 'alt' ),
'core/button' => array( 'url', 'text', 'linkTarget', 'rel' ),
'core/post-date' => array( 'datetime' ),
+ 'core/pullquote' => array( 'citation' ),
);
/**
diff --git a/tests/phpunit/tests/block-bindings/render.php b/tests/phpunit/tests/block-bindings/render.php
index 60b970cf79..d2c38b7951 100644
--- a/tests/phpunit/tests/block-bindings/render.php
+++ b/tests/phpunit/tests/block-bindings/render.php
@@ -83,6 +83,17 @@ HTML
,
'<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">test source value</a></div>',
),
+ 'pullquote block' => array(
+ 'citation',
+ <<<HTML
+<!-- wp:pullquote -->
+<figure class="wp-block-pullquote"><blockquote><p>This should not appear</p><cite>Quote McQuoteface</cite></blockquote></figure>
+<!-- /wp:pullquote -->
+HTML
+ ,
+ '<figure class="wp-block-pullquote"><blockquote><p>This should not appear</p><cite>test source value</cite></blockquote></figure>',
+ )
+
);
}
|
Currently, Block Bindings support for block attributes such as
core/paragraph
'scontent
orcore/button
'stext
depends on hard-coded logic to locate and replace the respective attributes. Since that logic is not filterable, it means that extending support for additional Core or third-party blocks requires hand-writing similar code in the block's PHP. This is limiting the scalability of Block Bindings.Thus, the existing block-specific custom logic should be replaced by more generic code that is able to locate and replace an attribute based on the
selector
definition in itsblock.json
.Ultimately, this will require a
set_inner_html()
method from the HTML API (which doesn't exist yet); but we can already provide a decent solution based on what's currently available from the HTML API.This is a preparatory step for making it easier to have block bindings support more blocks and block attributes, see WordPress/gutenberg#70642 (comment).
For example, try applying the following patch, which enables Block Bindings support for the Image block's
caption
attribute, and test coverage to verify the latter. Check that tests pass (npm run test:php -- --group=block-bindings
):Patch
Trac ticket: https://core.trac.wordpress.org/ticket/63840
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.