Block Bindings: Add block context needed for bindings in PHP#58554
Merged
SantosGuillamot merged 20 commits intotrunkfrom Feb 16, 2024
Merged
Block Bindings: Add block context needed for bindings in PHP#58554SantosGuillamot merged 20 commits intotrunkfrom
SantosGuillamot merged 20 commits intotrunkfrom
Conversation
SantosGuillamot
added
[Type] Code Quality
SantosGuillamot
requested review from
gziolo,
michalczaplinski and
youknowriad
SantosGuillamot
requested review from
ellatrix and
spacedmonkey
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
| } 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(); |
Contributor
There was a problem hiding this comment.
In which case the fallback is needed?
Contributor
Author
There was a problem hiding this comment.
Probably it should never be needed 😄 I can remove that
Member
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
Contributor
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?
Contributor
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.
Member
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 ) { |
Contributor
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.
Contributor
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.
Contributor
Author
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 🙂
Contributor
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.
Contributor
Author
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.
Contributor
Author
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.
Contributor
There was a problem hiding this comment.
Random question: why we inject queryId there?
Contributor
Author
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.
Contributor
Author
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.
Contributor
Author
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
Contributor
|
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
|
Flaky tests detected in d0c3e9e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7930900164
|
gziolo
reviewed
Feb 16, 2024
|
|
||
| // 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' ) ) { |
Member
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.
Contributor
Author
There was a problem hiding this comment.
You're right. I've removed it in this commit.
Member
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.
Contributor
Author
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
Member
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Nice work on this one. It all seems to work nicely in WordPress core. Let's make sure the changes are carried over to the Gutenberg plugin. The changes to block.json can now get synced to WordPress core to go in 6.5 release.
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
Contributor
|
I just cherry-picked this PR to the cherry-pick-beta-2 branch to get it included in the next release: 3a6fad4 |
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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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