Skip to content
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

Closed
wants to merge 32 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jan 3, 2024

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).

/**
 * Filters the parsed block array for a given hooked block.
 *
 * @since 6.5.0
 *
 * @param array                   $hooked_block      The parsed block array for the given hooked block type.
 * @param string                  $relative_position The relative position of the hooked block.
 * @param array                   $anchor_block      The anchor block.
 * @param WP_Block_Template|array $context           The block template, template part, or pattern that the anchor block belongs to.
 */
$hooked_block = apply_filters( "hooked_block_{$hooked_block_type}", $hooked_block, $relative_position, $anchor_block, $context );

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 the hooked_block_types and the newly introduced hooked_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.
diff --git a/like-button.php b/like-button.php
index 65acbc3..d416218 100644
--- a/like-button.php
+++ b/like-button.php
@@ -24,3 +24,19 @@ function create_block_like_button_block_init() {
        register_block_type( __DIR__ . '/build' );
 }
 add_action( 'init', 'create_block_like_button_block_init' );
+
+function set_like_button_block_layout_attribute_based_on_adjacent_block( $hooked_block, $relative_position, $anchor_block ) {
+       // Is the hooked block adjacent to the anchor block?
+       if ( 'before' !== $relative_position && 'after' !== $relative_position ) {
+               return $hooked_block;
+       }
+
+       // Does the anchor block have a layout attribute?
+       if ( isset( $anchor_block['attrs']['layout'] ) ) {
+               // Copy the anchor block's layout attribute to the hooked block.
+               $hooked_block['attrs']['layout'] = $anchor_block['attrs']['layout'];
+       }
+
+       return $hooked_block;
+}
+add_filter( 'hooked_block_ockham/like-button', 'set_like_button_block_layout_attribute_based_on_adjacent_block', 10, 3 );
Replace Like Button block with a pattern (that wraps the block 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

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:

diff --git a/like-button.php b/like-button.php
index 65acbc3..4122d3c 100644
--- a/like-button.php
+++ b/like-button.php
@@ -24,3 +24,34 @@ 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( $hooked_block ),
+		'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

Not exactly pretty 😬


Before After
image image

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.

@ockham ockham self-assigned this Jan 3, 2024
@ockham ockham force-pushed the try/hooked-block-filter branch 2 times, most recently from 01d1e6a to f2f104f Compare January 3, 2024 15:21
Copy link

github-actions bot commented Jan 3, 2024

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@@ -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'], '' );
Copy link
Contributor Author

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.)

Copy link
Contributor Author

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

Suggested change
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 😬)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b34e2cf.

@ockham
Copy link
Contributor Author

ockham commented Jan 4, 2024

Advantages of this approach

  • Neatly separate from hooked_block_types, reasonable separation of concerns: The former allows modifying which block types should be hooked, the latter is for more granular control -- e.g. to set attributes (and has access to anchor block’s).
  • Using the hooked_block_{$hooked_block_type} nomenclature would be somewhat familiar from e.g. render_block_{$type}.

Downsides of this approach

The 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 core/pattern block, and specify the pattern's slug via the slug attribute. However, it is still required to add the core/pattern block as a hooked block in the first place. Since third-party blocks cannot modify a Core block's block.json (and shouldn't resort to doing so via the [block_type_metadata or block_type_metadata_settings filters](https://developer.wordpress.org/block-editor/reference-guides/filters/block-filters/#registration)), this would need to be done via the hooked_block_types filter.

For example: A Like button block uses the hooked_block_types filter to add a core/pattern block after the Post Content block, and then uses the hooked_block filter to set its slug attribute to my/like-button-wrapper.

But what if a Subscribe button block does the exact same, except setting the pattern slug to third-party/subscribe-button-wrapper?

Both blocks might assume at hooked_block filter that the core/pattern block that was added after core/post-content is "theirs"; consequently, they will both try to set the Pattern block's slug attribute. As a result, we'll likely end up with only one hooked Pattern block -- and it's determined by filter order which block pattern prevails. Not ideal 😕

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.

@ockham
Copy link
Contributor Author

ockham commented Jan 4, 2024

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 hooked_block filter -- when called for the Like button block -- would return a Group block (which has the Like button block as its inner block). Hopefully not a dealbreaker though 🤔

@ockham
Copy link
Contributor Author

ockham commented Jan 9, 2024

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 layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

@ockham
Copy link
Contributor Author

ockham commented Jan 9, 2024

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

@ockham
Copy link
Contributor Author

ockham commented Jan 10, 2024

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 😬

@ockham ockham force-pushed the try/hooked-block-filter branch 2 times, most recently from 8669a32 to 389cbb0 Compare January 16, 2024 13:21
@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2024

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.

@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2024

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

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.

@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2024

@gziolo suggested to allow returning null from the filter, to indicate that the hooked block shouldn't be rendered after all.

I decided against this. The reason is that we already have the hooked_block_types filter to add and remove a block from the list of hooked blocks; and while returning null from the hooked_block filter would allow us to remove it, the opposite -- adding a block via that filter -- is not possible (lack of symmetry).

Furthermore, by not allowing removal of a block via the hooked_block filter, we retain the invariant that that filter will not change the list of hooked blocks, which should make code overall more predictable and resilient. (It avoids e.g. problems if one hooked_block filter returns null, and the next one tries to access and/or modify that block.)

@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2024

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 🙌

@ockham ockham marked this pull request as ready for review January 16, 2024 13:55
@ockham ockham requested a review from gziolo January 16, 2024 13:56
@ockham ockham changed the title Block Hooks: Experiment with a new hooked_block filter Block Hooks: Introduce a new hooked_block_{$block_type} filter Jan 16, 2024
@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2024

FYI @tjcafferkey @nerrad @yansern @TimBroddin 🙂 Feel free to review!


$markup = '';
foreach ( $hooked_block_types as $hooked_block_type ) {
$hooked_block = array(
Copy link
Member

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?

Copy link
Contributor Author

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:

  1. 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. Allowing innerBlocks/innerContent to be set provides at least a low-level escape hatch for these use cases.
  2. 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)?
  3. 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!

Copy link
Member

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 and innerContent 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.

Copy link
Contributor Author

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 and innerContent 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 🙂

Copy link
Member

@gziolo gziolo Jan 24, 2024

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.

@tjcafferkey
Copy link

tjcafferkey commented Jan 17, 2024

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:

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

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.

@WordPress WordPress deleted a comment from aijaz227 Jan 17, 2024
@ockham
Copy link
Contributor Author

ockham commented Jan 17, 2024

Thanks a lot for your feedback @tjcafferkey, it's much appreciated!

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:

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

Yeah, that's a fair point; I agree now that we shouldn't wholesale ban Core blocks 👍

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.

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 core/template-part; it might be a bit trickier with core/post-content (where it would end up injecting hooked blocks into the actual post, which would mean we'd actually start injecting into content, and those hooked blocks would appear in the post editor; this would be kind of a major decision to make).

The thing is that by default, firstChild/lastChild insertion only works with container blocks (i.e. blocks that support having inner blocks), which the Navigation, Template Part, and Post Content block aren't, which is why we need to manually add support. I still think there could be some benefit in showing a warning when people attempt to insert into something like that, as I've seen a number of folks try that, and it left them confused when it didn't work. I'd love for them to have a shorter feedback loop to understand why that's happening (and to not give up on using Block Hooks 😅), and it seems easy enough to implement a warning like that.

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 😅

*
* @covers ::insert_hooked_blocks
*/
public function test_insert_hooked_blocks_filter_can_wrap_block() {
Copy link
Member

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.

@ockham
Copy link
Contributor Author

ockham commented Jan 25, 2024

Committed to Core in https://core.trac.wordpress.org/changeset/57354.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants