-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add the Block Bindings API #5888
Add the Block Bindings API #5888
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. |
Actually, this changed recently, there are now no backports for pattern overrides other than the registration of the binding source. |
// Use the postId attribute if available | ||
if ( isset( $source_attrs['postId'] ) ) { | ||
$post_id = $source_attrs['postId']; | ||
} else { | ||
// $block_instance->context['postId'] is not available in the Image block. | ||
$post_id = get_the_ID(); | ||
} | ||
|
||
return get_post_meta( $post_id, $source_attrs['value'], true ); |
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.
Looking at this here I would love it if the function in core could already take care of looking up the post_id in the current context.
I cannot think of any instance where I wouldn't want to get the ID of the current queried post within a query loop for example. So this code that is shown here will be needed in 100% of the cases where this API is used and feels error prone. If we could make it so that the callback is already getting passed the current post id :)
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.
Could we also pass the result through a filter here, passing through the post ID, and source_attrs['value']
.
This could be useful for ACF to hydrate a raw post meta value into it's formatted value which users would expect.
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 don't think we should pass this through filters, this is its own abstractions if a third-party plugin wants its own kind of values, they can implement their own custom source.
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.
@fabiankaegy I'm not sure I understand the comment, can you clarify a bit?
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.
@youknowriad What I mean is that I would love it if the wp_block_bindings_register_source
function already passed in the current post ID as a parameter of the callback function.
Having to manually do the check to see if $block_instance->context['postId']
is available and if so using it vs. accessing the id via the get_the_ID
function is something that every single block binding source will need to do if it should work correctly in all context such as inside query loops.
I would like it if the callback could be simplified to:
/**
* Add the post_meta source to the Block Bindings API.
*
* @since 6.5.0
* @package WordPress
*/
function post_meta_source_callback( $source_attrs, $post_id ) {
// Override the current post id when an attribute called `postId` exists
if ( isset( $source_attrs['postId'] ) ) {
$post_id = $source_attrs['postId'];
}
return get_post_meta( $post_id, $source_attrs['value'], true );
}
If I'm not mistaken there even is a bug in this implementation here. Because it doesn't use the context value at all. It should instead check for the availability of the context and if so use that over get_the_ID
. And because of that very reason I think ideally core would already handle that check for context and pass the correct post id as a parameter to the callback.
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.
Personally it doesn't make sense to me. The sources can be anything, from posts to a random database table, to a call to some remote API... I don't think the signature should accommodate a specific source over any other.
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.
But don't sources always need to know which context they are rendered within?
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 still not sold on the name source, because it implies that the value is sourced from something external. In practice, the value used for replacement could be for example :
- current date
- translation
- hardcoded value
Overall, we are opening an API to dynamically compute the value of the attribute on the server so people don't need to default to custom and dynamic blocks.
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 context actually is helpful! In my mind, I was really mostly concerned with dynamic attributes on the post object / external API connections that need to be aware of the current context which is why I was so focussed on the Post ID.
In that case an unrelated side note, I would love it if simply all blocks anywhere had the usesContext: [ 'postId' ]
specified because the whole accessing the correct ID is an error-prone thing that often gets overlooked and the a feature doesn't work correctly in query loops etc
src/wp-includes/blocks.php
Outdated
function _process_block_bindings( $block_content, $block, $block_instance ) { | ||
|
||
// Allowed blocks that support block bindings. | ||
// TODO: Look for a mechanism to opt-in for this. Maybe adding a property to block attributes? |
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 totally fine with this static list right now, especially since it's a bit early for this API but isn't the presence of the "metadata.bindings" attribute a sufficient indicator here. in other words, maybe we can just remove this check in the future.
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 hope is that with HTML API we will be able to replace all attributes that are serialized in the saved HTML of the block.
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 is my main feedback https://github.com/WordPress/wordpress-develop/pull/5888/files#r1462908142
Other than that, it's looking good overall.
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.
Summarizing my comments, we could further simplify what gets exposed as a public API for v1. We should consider moving two functions to WP_Block
class as private methods:
wp_block_bindings_replace_html
_process_block_bindings
That leaves us with three public functions:
wp_block_bindings
wp_block_bindings_register_source
wp_block_bindings_get_sources
One change that @youknowriad and I agree on is that we should make the bindings registration open for extensions. Therefore, the signature should take that name of the bindings and the array of settings. Similar use cases existing in the codebase that can be used for reference:
wordpress-develop/src/wp-includes/blocks.php
Line 607 in e42e65d
function register_block_type( $block_type, $args = array() ) { |
wordpress-develop/src/wp-includes/blocks.php
Line 1619 in e42e65d
function register_block_style( $block_name, $style_properties ) { |
wordpress-develop/src/wp-includes/class-wp-block-pattern-categories-registry.php
Line 177 in 1381ad9
function register_block_pattern_category( $category_name, $category_properties ) { |
function register_block_pattern( $pattern_name, $pattern_properties ) { |
Which leads me to the next observation, that we should follow the established pattern and rename the method to align:
function register_block_binding( $name, $properties )
If we follow other registration helpers, the class name would better fit as:
WP_Block_Bindings_Registry
Examples:
final class WP_Block_Pattern_Categories_Registry { |
final class WP_Block_Patterns_Registry { |
regarding methods, we could have register
public function register( $pattern_name, $pattern_properties ) { |
and get_all_registered
public function get_all_registered() { |
// $block_instance->context['postId'] is not available in the Image block. | ||
$post_id = get_the_ID(); |
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 there might be a bug here.
Instead of completely omitting the usage of the $block_instance->context['postId'] here, we should check if the value isset and if so use it over the ID we get from get_the_ID
.
Something like this:
$post_id = isset( $block_instance->context['postId'] ) ? $block_instance->context['postId'] : get_the_ID();
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 also wonder if we should always use the context here. as if I'm not wrong the default context should be set as get_the_ID
if there's no parent query block.
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.
From what I tested, $block_instance->context
is an empty array for the heading, button, and image block, so that's why we couldn't get the postId
from there. Is that not expected?
We could definitely use the conditional suggested. However, I'd like to understand if just using $block_instance->context
should work.
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.
Maybe we have an issue somewhere but in the client the block context has some "default context values" like the post ID if I'm not wrong, and in the server you're saying that these are not present. IMO, the block context should be the same for the client and the server.
I think it's fine if we defer the potential fix to a dedicated issue but we might want to track it somewhere.
The unit tests are failing because Because of this, the If understand correctly, I'm not sure what the right behavior here should be. I see these options:
What do you think @youknowriad @gziolo ? |
The following diff should resolve the issue that exists only in the test env: diff --git a/tests/phpunit/includes/functions.php b/tests/phpunit/includes/functions.php
index 81d4339db1..0fdff9c71a 100644
--- a/tests/phpunit/includes/functions.php
+++ b/tests/phpunit/includes/functions.php
@@ -339,10 +339,15 @@ tests_add_filter( 'send_auth_cookies', '__return_false' );
* @since 5.0.0
*/
function _unhook_block_registration() {
+ // Block types.
require __DIR__ . '/unregister-blocks-hooks.php';
remove_action( 'init', 'register_core_block_types_from_metadata' );
remove_action( 'init', 'register_block_core_legacy_widget' );
remove_action( 'init', 'register_block_core_widget_group' );
remove_action( 'init', 'register_core_block_types_from_metadata' );
+
+ // Block binding sources.
+ remove_action( 'init', '_register_block_bindings_pattern_overrides_source' );
+ remove_action( 'init', '_register_block_bindings_post_meta_source' );
}
tests_add_filter( 'init', '_unhook_block_registration', 1000 ); It will never be the case that |
…n `process_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.
It should be good to land the next iteration. We still need more unit tests to cover registered block binding sources, but it can be done separately.
Thanks all, commit https://core.trac.wordpress.org/changeset/57514 |
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.
@youknowriad Left some nit-pick
|
||
foreach ( $selectors as $selector ) { | ||
// If the parent tag, or any of its children, matches the selector, replace the HTML. | ||
if ( strcasecmp( $block_reader->get_tag( $selector ), $selector ) === 0 || $block_reader->next_tag( |
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.
get_tag()
doesn't take any argument, and it could return NULL
, which would throw an error when passed as the first argument of strcasecmp
. I think we should guard against it first.
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.
There is currently an error being triggered in trunk
:
Deprecated: strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /var/www/html/wp-includes/class-wp-block.php on line 330
To reproduce:
- Create a new post
- Add a heading block and add the text 'default' to it
- Using the block settings menu, click 'Create pattern'.
- In the resulting modal, give the pattern a name and click 'Create'
- Click the 'Edit original' button that appears on the resulting pattern block
- Select the heading block and check the 'Allow instance overrides' option in the advanced inspector tools
- Save and click the 'Back' button from the topbar
- Now you're back in the post editor, change the heading text 'default' to 'override'
Preview the post
Expected: The preview should show the heading with the text 'override'
Actual: It still shows 'default' and the above error is shown
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 believe having something like this should solve the issue:
$parent_tag = $block_reader->get_tag();
if ( ! is_null( $parent_tag ) || strcasecmp( $parent_tag, $selector ) === 0 || $block_reader->next_tag(
array(
'tag_name' => $selector,
)
) )
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.
when using next_tag()
we will always get a string for get_tag()
unless no tag was found.
it would be better to check the result of next_tag()
so we don't descend into this function as if a tag were matched.
if ( ! $block_reader->next_tag() ) {
continue; // or whatever control flow is appropriate
}
return $amended_button->get_updated_html(); | ||
} | ||
} else { | ||
$block_reader->seek( 'iterate-selectors' ); |
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.
seek()
could return false
which means we fail to seek the bookmark. This could happen when we reach the end the of input.
I think there's currently a bug in WP_HTML_Tag_Processor
API that seek()
after next_tag()
won't correctly put the cursor at the right place.
$p = new WP_HTML_Tag_Processor( '<h2></h2>' );
$p->next_tag();
$p->set_bookmark( 'bookmark' );
$p->next_tag();
var_dump( $p->seek( 'bookmark' ) );
var_dump( $p->get_tag() );
For instance, I expect the above snippet to output true
and H2
, but instead it outputs false
and NULL
. @dmsnell might know better if it's a bug or not 🙇.
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.
@kevin940726, what was the user interaction, and what HTML was saved for the block that triggered the issue? It's a great opportunity to add a unit test that will help fix the issue.
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 left some further details on the comment above - #5888 (comment)
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.
thanks for the find @kevin940726 - pushed out #6021 to fix it.
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.
@kevin940726 fixed in trunk
now 👍
…ag processor Fix for the Block Bindings processing. See #5888 (comment). Props: czapla, dmsnell, gziolo. git-svn-id: https://develop.svn.wordpress.org/trunk@57561 602fd350-edb4-49c9-b593-d223f7449a82
…ag processor Fix for the Block Bindings processing. See WordPress/wordpress-develop#5888 (comment). Props: czapla, dmsnell, gziolo. Built from https://develop.svn.wordpress.org/trunk@57561 git-svn-id: http://core.svn.wordpress.org/trunk@57062 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…ag processor Fix for the Block Bindings processing. See WordPress/wordpress-develop#5888 (comment). Props: czapla, dmsnell, gziolo. Built from https://develop.svn.wordpress.org/trunk@57561 git-svn-id: https://core.svn.wordpress.org/trunk@57062 1a063a9b-81f0-0310-95a4-ce76da25c4cd
More information about the Block Bindings API: WordPress/gutenberg#54536
Trac ticket: https://core.trac.wordpress.org/ticket/60282
This PR introduces the Block Bindings API for WordPress.
The API allows developers to connects block attributes to different sources. In this PR, two such sources are included: "post meta" and "pattern". Attributes connected to sources can have their HTML replaced by values coming from the source in a way defined by the binding.
How does it work?
On a technical level, the way the API works is a 3-step process:
1. Create a binding
The bindings have the following structure:
An example binding for the
content
attribute of Paragraph block and theurl
attribute of the Image block.Both using the
post_meta
source.This PR does not include any UI for adding those
metatada
attributes to blocks at the moment. This is intentional as the API is meant as a building block and such UI will be developed later.2. Get the value
This PR includes a mechanism for registering "sources" for the block bindings. A "source" defines where to get a value for the binding from. Two sources are included in this PR:
Each source is responsible any logic required to obtain the value from a source.
3. Update the HTML
At runtime, the HTML API is used to update the HTML of the block with values from the block binding sources. At the moment, a limited and hardcoded number of blocks and attributes is supported in this PR:
Testing Instructions
Register a new custom field however you prefer. You can use a snippet similar to this:
In a page/post
Test paragraph
Test heading
Repeat the paragraph test but using a heading.
Test button
Test image