-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Block Hooks: Introduce a new hooked_block_{$block_type}
filter
#5835
Conversation
01d1e6a
to
f2f104f
Compare
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. |
src/wp-includes/blocks.php
Outdated
@@ -786,7 +787,62 @@ function get_hooked_block_markup( &$anchor_block, $hooked_block_type ) { | |||
// However, its presence does not affect the frontend. | |||
$anchor_block['attrs']['metadata']['ignoredHookedBlocks'][] = $hooked_block_type; | |||
|
|||
return get_comment_delimited_block_content( $hooked_block_type, array(), '' ); | |||
return get_comment_delimited_block_content( $hooked_block_type, $hooked_block['attrs'], '' ); |
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.
We're currently not passing inner blocks to get_comment_delimited_block_content
.
(We're actually not passing any information other than the block type and 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.
To do so, we'd basically need to
return get_comment_delimited_block_content( $hooked_block_type, $hooked_block['attrs'], '' ); | |
return serialize_block( $hooked_block ); |
(Strictly speaking, one might consider using traverse_and_serialize_blocks
, but not only do we not have access to the block visitors, we also might want to steer away from infinite recursions 😬)
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.
Done in b34e2cf.
Advantages of this approach
Downsides of this approachThe major downside is that while it might be tempting to use this filter to use a pattern instead of a hooked block, it's not a good idea to do so. If we allow attributes, people might start adding a For example: A Like button block uses the But what if a Subscribe button block does the exact same, except setting the pattern Both blocks might assume at To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks. (See #5810 for a somewhat related PR that seeks to warn against "incompatible" anchor blocks.) Then again, maybe this is not so clear-cut: After all, it makes sense to e.g. use Core's Login/out block as a hooked block that's the last child of the Navigation block. To offer alternatives for extenders, we might want to allow passing inner blocks to the filter; see. |
Noting that there might be a bit of an inconsistency with this when used to solve https://core.trac.wordpress.org/ticket/60126, as e.g. the |
Here's a proof-of-concept (based on the example from #5837) that works with this PR to replace the Like Button block with a pattern that wraps it in a Group block: diff --git a/like-button.php b/like-button.php
index 65acbc3..4728ce1 100644
--- a/like-button.php
+++ b/like-button.php
@@ -22,5 +22,34 @@
*/
function create_block_like_button_block_init() {
register_block_type( __DIR__ . '/build' );
+
+ register_block_pattern(
+ 'ockham/like-button-wrapper',
+ array(
+ 'title' => __( 'Like Button', 'like-button' ),
+ 'description' => _x( 'A button to like content.', 'Block pattern description', 'like-button' ),
+ 'content' => '<!-- wp:group {"layout":{"type":"constrained"}} --><div class="wp-block-group"><!-- wp:ockham/like-button /--></div><!-- /wp:group -->',
+ 'inserter' => false
+ )
+ );
}
add_action( 'init', 'create_block_like_button_block_init' );
+
+function insert_like_button_pattern_after_post_content( $hooked_block, $position, $anchor_block ) {
+ if ( 'before' !== $position && 'after' !== $position ) {
+ return $hooked_block;
+ }
+
+ if ( 'core/post-content' !== $anchor_block['blockName'] ) {
+ return $hooked_block;
+ }
+
+ // We replace the "simple" block with a pattern that contains a Group block wrapper.
+ return array(
+ 'blockName' => 'core/pattern',
+ 'attrs' => array(
+ 'slug' => 'ockham/like-button-wrapper',
+ ),
+ );
+}
+add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_pattern_after_post_content', 10, 3 ); Branch: https://github.com/ockham/like-button/tree/try/hooked-block-filter-with-pattern This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the |
I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that. |
Per b34e2cf (which I just worked on with @c4rl0sbr4v0), it is now possible to pass a block that has inner blocks: diff --git a/like-button.php b/like-button.php
index 65acbc3..2c66824 100644
--- a/like-button.php
+++ b/like-button.php
@@ -24,3 +24,41 @@ function create_block_like_button_block_init() {
register_block_type( __DIR__ . '/build' );
}
add_action( 'init', 'create_block_like_button_block_init' );
+
+function insert_like_button_after_post_content( $hooked_block, $position, $anchor_block ) {
+ if ( 'before' !== $position && 'after' !== $position ) {
+ return $hooked_block;
+ }
+
+ if ( 'core/post-content' !== $anchor_block['blockName'] ) {
+ return $hooked_block;
+ }
+
+ $attrs = isset( $anchor_block['attrs']['layout']['type'] )
+ ? array(
+ 'layout' => array(
+ 'type' => $anchor_block['attrs']['layout']['type']
+ )
+ )
+ : array();
+
+ // Wrap the Like Button block in a Group block.
+ return array(
+ 'blockName' => 'core/group',
+ 'attrs' => $attrs,
+ 'innerBlocks' => array(
+ array(
+ 'blockName' => 'ockham/like-button',
+ 'attrs' => array(),
+ 'innerBlocks' => array(),
+ 'innerContent' => array(),
+ ),
+ ),
+ 'innerContent' => array(
+ '<div class="wp-block-group">',
+ null,
+ '</div>'
+ ),
+ );
+}
+add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_after_post_content', 10, 3 ); Branch: https://github.com/ockham/like-button/tree/try/hooked-block-filter-with-group-block-wrapper It's not exactly pretty though 😬 |
8669a32
to
389cbb0
Compare
I've absorbed this comment and this one into the PR description to see at a glance how the new filter can be used with different techniques. |
I've talked to folks who are working on pattern overrides, and it seems like they only work for a block's content, not for arbitrary attributes. |
I decided against this. The reason is that we already have the Furthermore, by not allowing removal of a block via the |
389cbb0
to
49aa634
Compare
Opening for review. Reviewers, please take the time to read the PR description, which I've updated with the rationale behind this filter, plus some code snippets that demonstrate how to use it 🙌 |
hooked_block
filterhooked_block_{$block_type}
filter
FYI @tjcafferkey @nerrad @yansern @TimBroddin 🙂 Feel free to review! |
src/wp-includes/blocks.php
Outdated
|
||
$markup = ''; | ||
foreach ( $hooked_block_types as $hooked_block_type ) { | ||
$hooked_block = array( |
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's a very low-level concept as it's the shape of the parsed block. Are we sure that we want to open innerBlocks
plus innerContent
? What alternatives do we have here to represent the shape in something more abstract that doesn't impact performance?
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.
Yeah, that's a good question. I was originally only thinking to allow modifying attributes, e.g.
$hooked_block_attributes = apply_filters( "hooked_block_attributes_{$hooked_block_type}", $hooked_block_attributes, $hooked_block_type, $relative_position, $anchor_block, $context );
(Note how $hooked_block_attributes
is both the first attribute and the return value. I believe we'd need to do it like that (rather than making $hooked_block_type
the first argument and return value), since we'd expect filters to change attributes, while the block type is generally going to remain the same.)
There were a number of reasons against this kind of filter signature; I'll try to list them by relevance:
- Discussion over at Block Hooks: Set hooked block's layout attribute based on anchor block's #5811 revealed that extenders might still want to wrap their hooked block in a Group block, in order to get the
layout
attribute to work properly, as I was cautioned by folks more familiar with Layout block-support (in case it's impossible to add it directly to the hooked block). Additionally, we've heard of other use cases where people would like to inject e.g. two hooked blocks inside of a shared Row block parent. AllowinginnerBlocks
/innerContent
to be set provides at least a low-level escape hatch for these use cases. - Only allowing attributes to be set would raise some questions about the filter signature, if we also want to provide access to the anchor block (which we do). Would we provide the entire parsed anchor block (even though it's more inconsistent with having only the hooked block type and attributes)? Or would we also only provide the anchor block's type and attributes (which is more consistent, but lacks information about inner blocks)?
- We already have the parsed block format, and
WP_Block
class instances; I didn't want to add yet another way of specifying a block 😬
Overall, I agree that the parsed block format isn't exactly pretty; I just found it rather hard to come up with a better format or function signature that worked better. Allowing access to innerBlocks
and innerContent
is kind of a side effect, but not totally unwanted (see 1.).
But if you do have an idea how we could do this differently, please LMK!
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.
What if we pass the result of get_hooked_block_markup
call instead? In the majority of cases, that would never change, but if plugins would like to produce a different markup, then they can use the filter to rewrite the HTML to whatever they want:
- change the attributes
- wrap with a parent block
- rewrite it to any HTML they want, which is the only risky but the same issue exists with
innerBlocks
andinnerContent
as you can put there anything really
I don't think it's any different from what the API currently offers, but we operate on a much higher level without forcing folk to learn the parsed block structure.
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.
What if we pass the result of
get_hooked_block_markup
call instead?
Hmm, that would be serialized block markup, i.e. something like <!-- my/hooked-block /-->
.
In the majority of cases, that would never change, but if plugins would like to produce a different markup, then they can use the filter to rewrite the HTML to whatever they want:
- change the attributes
- wrap with a parent block
- rewrite it to any HTML they want, which is the only risky but the same issue exists with
innerBlocks
andinnerContent
as you can put there anything really
But even for a simple operation like changing (or adding) an attribute, they would either have to do some string/RegEx-gymnastics (which can be error-prone if they get the markup wrong), or run parse_blocks
again on something that was just parsed and re-serialized.
My thinking is that we want to make it easy enough to do a "simple" (and often-requested) change like set/change attributes (with the current state of this PR: $hooked_block['attrs']['layout'] = $anchor_block['attrs']['layout'];
), and a bit harder to wrap with a parent or include inner blocks; it seems reasonable to require that an extender "knows what they're doing" if they want to do something more complex like that. (If they'd like to avoid manually nesting arrays to create the parsed block structure, they could still run parse_blocks
on a string literal, e.g. parse_blocks( <-- wp:group /--><div class="wp-block-group"><!-- my/hooked-block /--></div><-- /wp:group -->' )
.)
I don't think it's any different from what the API currently offers, but we operate on a much higher level without forcing folk to learn the parsed block structure.
Personally, I see markup as lower-level than parsed block arrays 😅 but that might be subjective. It's true however that extenders might be familiar with (block) markup, but not necessarily with parsed block arrays.
FWIW, some of our filters in the render pipeline -- e.g. render_block
or render_block_data
-- also pass parsed block arrays as arguments; this was partly the inspiration here 🙂
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.
FWIW, some of our filters in the render pipeline -- e.g. render_block or render_block_data -- also pass parsed block arrays as arguments; this was partly the inspiration here 🙂
Interesting observation. In effect, let's make it clear then and use $parsed_hooked_block
for clarity. It isn't that obvious when looking at render_block
that exposes $block
.
From my brief time working with and understanding this API, this looks like an acceptable solution to me. I like how your solution is relatively 'simple' and readable yet solves this particular problem. I do have a few comments though: I'd be against this for the reason you've outlined (using the Also, the API feels quite flexible based on the use cases you've presented in the PR description. If we release something which can be used in a multitude of ways would it limit our ability to safely iterate on and improve it going forward if we need to consider so many use-cases? Just a thought. |
Thanks a lot for your feedback @tjcafferkey, it's much appreciated!
Yeah, that's a fair point; I agree now that we shouldn't wholesale ban Core blocks 👍
Yes, the technique we're about to explore in WordPress/gutenberg#57754 might work with similar use cases (i.e. blocks that fetch data from a separate entity in the DB) 👍 Off the top of my head, it might carry over quite well to The thing is that by default, OTOH I agree that we might continue to add support for some of these blocks in GB, and we don't want to discourage people forever from using them. Maybe we need a filterable list of unsupported blocks that the warning checks against 😅 |
9cba041
to
aef39d1
Compare
d11d318
to
7fdf956
Compare
* | ||
* @covers ::insert_hooked_blocks | ||
*/ | ||
public function test_insert_hooked_blocks_filter_can_wrap_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.
Excellent addition, these two tests cover new filters. It's exactly what I expected.
Committed to Core in https://core.trac.wordpress.org/changeset/57354. |
Add a new
hooked_block_{$block_type}
filter that allows changing a hooked block (in parsed block format), while providing read access to its anchor block (in the same format).This allows block authors to e.g. set a hooked block's attributes; the filter can peruse information about the anchor block when doing so. As such, this PR provides a solution to both
#59572
and#60126
.This seems to strike a good balance and separation of concerns with the existing
hooked_block_types
filter, which allows addition or removal of a block to the list of hooked blocks for a given hooked blocks -- all of which are identified only by their block types. This new filter, OTOH, only applies to one hooked block at a time, and allows modifying the entire (parsed) hooked block; it also gives (read) access to the parsed anchor block.This PR also introduces a new
insert_hooked_blocks
helper function (as originally explored in #5609). This is mostly to avoid code duplication; in particular, we’d otherwise be applying both thehooked_block_types
and the newly introducedhooked_block_{$hooked_block_type}
filter four times.Furthermore, the new filter requires some changes to the
get_hooked_block_markup()
function, which now also has to accept the entire hooked block (as a parsed block array); for more consistency, we also change the argument order.The following three snippets demonstrate how this new filter can be used (to solve
#60126
). To try them out, apply the respective patch to the Like Button block code.Copy `layout.type` attribute from anchor block to hooked block.
Replace Like Button block with a pattern (that wraps the block in a Group block).
Branch: https://github.com/ockham/like-button/tree/try/hooked-block-filter-with-pattern
Note that this approach doesn't allow setting the
layout
attribute dynamically (and instead hard-wires it to be"type": "constrained"
).Manually wrap the Like Button block in a Group block and copy its `layout.type` attribute from the anchor block.
Per b34e2cf (which I just worked on with @c4rl0sbr4v0), it is now possible to pass a block that has inner blocks:
Branch: https://github.com/ockham/like-button/tree/try/hooked-block-filter-with-group-block-wrapper
Not exactly pretty 😬
Trac ticket: https://core.trac.wordpress.org/ticket/60126
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.