Use metadata.name only as the hint for pattern overrides
#59956
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
kevin940726
added
[Type] Enhancement
|
Size Change: -439 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
talldan
requested review from
youknowriad,
gziolo,
artemiomorales,
michalczaplinski and
SantosGuillamot
talldan
reviewed
Mar 19, 2024
There was a problem hiding this comment.
Looks good to me so far! The amount of code it allows us to remove is impressive, and it'll be much more reliable than the effect that was in place.
I guess we'll need to consider the back compat story as part of this. Suddenly any named block in a pattern is overridable. Maybe that's ok while the feature is plugin only.
Alternatively the opt-out needs to be shipped first before this so that users have an option (Pattern Overrides: No opt-out mechanism exists for named overrides).
I'd be interested to hear the thoughts of others.
youknowriad
reviewed
Mar 19, 2024
lib/compat/wordpress-6.6/blocks.php
Outdated
| return $modified_block_content; | ||
| } | ||
|
|
||
| add_filter( 'render_block', 'gutenberg_process_pattern_overrides_bindings', 20, 3 ); |
There was a problem hiding this comment.
This hook feels like it recreates part of the block binding framework. Maybe another way to achieve the same is to actually "inject" the block bindings attributes in the render callback of the core/block block before calling do_blocks. That way the logic of the block bindings continue to apply just today, we'll just be "not persisting" the binding.
There was a problem hiding this comment.
This hook is actually mutually exclusive to the original gutenberg_process_block_bindings hook because that hook only checks when there's a metadata.bindings attribute. I think we can merge them together in Core if it makes sense too.
There was a problem hiding this comment.
I'm intrigued by @youknowriad's suggestion of generating the binding attributes at runtime though, as it avoids changing the block bindings code at all. That makes sense to me given block bindings is the broader API, while pattern overrides are a single use case of that API.
I guess a downside is it might be more fragile, as it relies on a specific order of operations, but then there are tests for that.
Could be something to explore.
There was a problem hiding this comment.
There are no hooks used in WP core for block bindings integration, so I don’t think there would be any issue with updating the attributes in a proper place in WP_Block class.
There was a problem hiding this comment.
In 7fba5e9, I updated the code to inject the bindings instead. Not sure if it's what you mean here?
There was a problem hiding this comment.
yep that matches what I had in mind basically.
kevin940726
force-pushed
the
update/pattern-overrides-name-only
branch
from
76bbdd1
to
7fba5e9
Compare
15 tasks
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?
One possible solution for #58601.
Use only
metadata.nameas the "hint" for pattern overrides. Instead of requiring thebindingsattribute for each bound attribute, we instead rely on only the existence of themetadata.nameattribute to enable pattern overrides. This also means we hardcode and maintain a list of supported attributes for each supported block.Note
One thing to consider is that block renaming API will land in 6.5 but pattern overrides will not. Using names as the only requirement for enabling pattern overrides might surprise the users in some way. We have to figure out the UX about this.
Why?
See #58601.
How?
Remove the need for
bindingsfor pattern overrides.Testing Instructions
CI should pass.
Screenshots or screencast
TBD