Block Bindings: Add block context needed for bindings in PHP #58554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SantosGuillamot
added
[Type] Code Quality
SantosGuillamot
requested review from
youknowriad,
gziolo and
michalczaplinski
SantosGuillamot
requested review from
spacedmonkey and
ellatrix
as code owners
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/block-bindings/block-bindings.php ❔ lib/compat/wordpress-6.5/block-bindings/class-wp-block-bindings-registry.php ❔ lib/compat/wordpress-6.5/block-bindings/class-wp-block-bindings-source.php ❔ lib/compat/wordpress-6.5/block-bindings/pattern-overrides.php ❔ lib/compat/wordpress-6.5/block-bindings/post-meta.php ❔ lib/compat/wordpress-6.5/blocks.php |
youknowriad
reviewed
Feb 1, 2024
| @@ -13,8 +13,7 @@ function gutenberg_block_bindings_post_meta_callback( $source_attrs ) { | |||
| if ( isset( $source_attrs['postId'] ) ) { | |||
| $post_id = $source_attrs['postId']; | |||
| } else { | |||
| // I tried using $block_instance->context['postId'] but it wasn't available in the image block. | |||
| $post_id = get_the_ID(); | |||
| $post_id = isset( $block_instance->context['postId'] ) ? $block_instance->context['postId'] : get_the_ID(); | |||
There was a problem hiding this comment.
In which case the fallback is needed?
There was a problem hiding this comment.
Probably it should never be needed 😄 I can remove that
There was a problem hiding this comment.
I'm still not sure how $source_attrs['postId'] would be set here. Most of the blocks inherit the post id from the context so get_the_ID() would also be set to the same value as the parent block. Maybe, in the case where there is no parent block that sets the context, get_the_ID() is the way to go.
Example:
gutenberg/packages/block-library/src/post-title/index.php
Lines 20 to 28 in 0e75767
There was a problem hiding this comment.
Maybe, in the case where there is no parent block that sets the context, get_the_ID() is the way to go.
In my mind there's always a parent context, because if I'm not wrong we probably set get_the_ID() as the default context top level no?
There was a problem hiding this comment.
For $source_attrs['postId'] I agree, it seems useless to me and we should just remove that condition.
There was a problem hiding this comment.
We need some tests to cover all those scenarios. Post meta is a very difficult one to start with.
youknowriad
reviewed
Feb 1, 2024
lib/compat/wordpress-6.5/blocks.php
Outdated
| * @param string $block_name The name of the block to process. | ||
| * @return array The modified arguments for registering a block type. | ||
| */ | ||
| function gutenberg_add_context_for_block_bindings( $args, $block_name ) { |
There was a problem hiding this comment.
This should not be an adhoc hook like that, it should be part of the "block binding API", in other words it's the "source" that should define this logic somehow. Maybe a new property of the source object.
There was a problem hiding this comment.
And how is this done in the client, IMO, the source should also define it in the same way in both cases.
There was a problem hiding this comment.
You mean adding that context only if the source used needs it, like the case of "post meta", right? That makes sense. I'll explore that. Thanks for the feedback 🙂
There was a problem hiding this comment.
yeah, I mean the definition of the source could have something like
export default {
name: 'core/post-meta',
label: __( 'Post Meta' ),
//...
usesContext: [ 'postId' ],
};
and same for the registration in the backend. And the framework should be wired to understand this property and register the hooks accordingly when needed.
There was a problem hiding this comment.
Perfect! That's what I understood. I agree it feels much better than what we have right now. I'll check if I can make it work.
There was a problem hiding this comment.
I was thinking about the use case where there is an existing block and I add the bindings attribute. In that case, according to my tests, it should work because the PHP registration handles that.
However, I just thought that you can start with a blank page, add an image, and then add the bindings. I assume the PHP code won't run there and the block won't have the context. For that reason, I think we have to keep (and adapt), registerBlockType filter used here.
There was a problem hiding this comment.
Random question: why we inject queryId there?
There was a problem hiding this comment.
It is not needed. I was hoping to remove it as part of this PR, and that's why I didn't include it here.
There was a problem hiding this comment.
I will go with option 2 then. I will:
- Create a PR in core to add the context defined by the sources in PHP and adapt the post meta source.
- Modify this PR in Gutenberg to adapt the post meta source and the editor hook.
There was a problem hiding this comment.
Okay, I have created the first version of the potential solution:
- Core pull request: It adds the possibility to add the
uses_contextargument in the sources and read it before registration. It seems that code runs in the editor even if the specific block is not in the page, so we don't need the editor hook. - I adapted the code in Gutenberg to match those changes.
I still have to review everything properly, but feel free to leave any feedback.
|
Size Change: -3.45 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
youknowriad
added
[Type] Bug
|
I think this is a bug fix as well. Without this, the wrong postId will be used if a bound block is used within query block for instance. |
42 tasks
SantosGuillamot
force-pushed
the
update/add-block-context-for-bindings-in-php
branch
from
45ec9e4
to
a26e8e3
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
gziolo
reviewed
Feb 6, 2024
SantosGuillamot
force-pushed
the
update/add-block-context-for-bindings-in-php
branch
from
3d058ff
to
1dbf2c4
Compare
SantosGuillamot
added
the
Backport to WP 6.7 Beta/RC
|
Flaky tests detected in d0c3e9e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7930900164
|
gziolo
reviewed
Feb 16, 2024
| $source = new WP_Block_Bindings_Source( | ||
| $source_name, | ||
| $source_properties | ||
| ); | ||
|
|
||
| $this->sources[ $source_name ] = $source; | ||
|
|
||
| // Add `uses_context` defined by block bindings sources. | ||
| // If `get_uses_context` exists (from WP 6.5), use its filter because it is more reliable. | ||
| if ( method_exists( 'WP_Block_Type', 'get_uses_context' ) ) { |
There was a problem hiding this comment.
This is a compat layer for WP 6.5, so this class is going to be used in WP 6.3 and 6.4. In effect, this check is not needed. If I follow the flow correctly, only register_block_type_args filter is necessary here because in older version of WP there is no get_uses_context method.
There was a problem hiding this comment.
You're right. I've removed it in this commit.
There was a problem hiding this comment.
Maybe include a comment that it differs intentionally from WP core so it doesn't get replaced by accident.
There was a problem hiding this comment.
I didn't see this one, sorry 🙏 I can do it in a follow-up PR.
gziolo
approved these changes
Feb 16, 2024
SantosGuillamot
deleted the
update/add-block-context-for-bindings-in-php
branch
getdave
pushed a commit
that referenced
this pull request
Feb 20, 2024
* Modify `uses_context` for allowed blocks in PHP * Update post meta source to use block context * Remove old hook used for the editor * Remove possibility of passing post id from source * Set correct post id * Adapt post meta source * Adapt pattern overrides source * Remove old usesContext from block.json * Remove old filter * Add backport to `uses_context` in source registration * Update source class to include `uses_context` * Use `get_block_type_uses_context` when available * Use `empty` function in post meta source * Update descriptions * Update $supported_blocks variable name * Use `in_array` properly * Update bindings registry comments * Update allowed_blocks variable name * Check allowed properties in the registry * Use only compatibility filter
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 3a6fad4 |
getdave
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Feb 20, 2024
* Modify `uses_context` for allowed blocks in PHP * Update post meta source to use block context * Remove old hook used for the editor * Remove possibility of passing post id from source * Set correct post id * Adapt post meta source * Adapt pattern overrides source * Remove old usesContext from block.json * Remove old filter * Add backport to `uses_context` in source registration * Update source class to include `uses_context` * Use `get_block_type_uses_context` when available * Use `empty` function in post meta source * Update descriptions * Update $supported_blocks variable name * Use `in_array` properly * Update bindings registry comments * Update allowed_blocks variable name * Check allowed properties in the registry * Use only compatibility filter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
EDIT: I created other pull request to handle this in wordpress-develop: link. I'm using this PR to adapt Gutenberg to those changes.
Tests are expected to fail until the core pull request is merged.
Why?
If the core PR gets merged, we need to adapt the existing sources.
How?
uses_contextargument in the sources.pattern/overridesdefinition inblock.jsonbecause now it is handled by the new approach.Testing Instructions