Implement Block Hooks

[Bernhard Reiter]

This ticket is about porting Gutenberg PHP code that implemented Block Hooks to Core. Skip to the bottom of this ticket for a TODO list.

Synopsis

First implemented in Gutenberg under the name ​Auto-inserting Blocks and recently ​renamed to "Block Hooks", this feature is meant to provide an extensibility mechanism for Block Themes, in analogy to WordPress' ​Hooks concept that has allowed extending Classic Themes through filters and actions.

Specifically, Block Hooks allow a third-party block to specify a position relative to a given block into which it will then be automatically inserted (e.g. a "Like" button block can ask to be inserted after the Post Content block, or an eCommerce shopping cart block can ask to be inserted after the Navigation block).

The two core tenets for block hooks are:

1. Insertion into the frontend should happen right after a plugin containing a hooked block is activated (i.e. the user isn't required to insert the block manually in the editor first); similarly, disabling the plugin should remove the hooked block from the frontend.
2. The user has the ultimate power to customize that automatic insertion: The hooked block is also visible in the editor, and the user's decision to persist, dismiss, customize, or move it will be respected (and reflected on the frontend).

To account for both tenets, we've made the tradeoff that automatic block insertion only works for unmodified templates (and template parts, respectively). The reason for this is that the simplest way of storing the information whether a block has been persisted to (or dismissed from) a given template (or part) is right in the template markup. This was first suggested ​here.

To accommodate for that tradeoff, we've ​added UI controls (toggles) to increase visibility of hooked blocks, and to allow their later insertion into templates (or parts) that already have been modified by the user.

Implementation

Since we wanted hooked blocks to appear both in the frontend and in the editor (see tenet number 2), we had to make sure they were inserted into both the frontend markup and the REST API (templates and patterns endpoints) equally. As a consequence, this means that automatic insertion couldn't only be implemented at block render stage, as for the editor, we needed to modify the serialized (but unrendered) markup.

However, thanks to the tradeoff we made (see above), we could at least limit ourselves to only inserting hooked blocks into unmodified templates (and parts), i.e. those that were coming directly from a Block Theme's template (or parts) file, rather than the database.

It turns out that there's a rather natural stage for automatic insertion of hooked blocks to happen, which is during template file retrieval.

While we had to leverage the get_block_templates and get_block_file_template hooks in Gutenberg to insert those blocks, we'll be able to directly modify the _build_block_template_result_from_file function in Core.

Furthermore, hooked blocks also have to be inserted into block patterns. Since almost no filters exist for the patterns registry, this was done in the patterns REST API controller in the Gutenberg codebase; for Core, we'll likely want to this at a lower level.

Core merge

Based on an early exploration in this ​experimental PR, I believe that we can break down the implementation of Block Hooks in Core into the following steps. Note that due to the nature of the existing code, the first steps might appear somewhat surprising and unrelated; however, they are very much relevant and will allow us to extend existing functionality through a series of self-contained steps.

• #59325: Add proper unit test coverage to verify that _build_block_template_result_from_file injects the theme attribute.
  • While we have coverage for _inject_theme_attribute_in_block_template_content, those tests only verify that that function does what is supposed to do; there's however no guarantee that _build_block_template_result_from_file uses that function (or whatever other technique) to actually inject the theme attribute (​see).
• #59327: Add a $callback argument to serialize_block() (​see) and serialize_blocks() (​see).
  • In order to insert our hooked blocks, we need to traverse the parsed block tree (before it is eventually serialized). In order to minimize the number of tree traversals (which is a potentially costly operation), it seems that adding a callback function argument to serialize_block() -- which inevitable has to traverse the entire tree anyway -- is a natural fit.
• #59338: Use the latter to replace the call to _inject_theme_attribute_in_block_template_content in _build_block_template_result_from_file with a newly written _inject_theme_attribute_in_template_part_block (​see).
  • Note that _inject_theme_attribute_in_block_template_content is still used in another spot: The Template Part block’s render.php (i.e. in GB). We might also want to replace that with _inject_theme_attribute_in_template_part_block so that we can deprecate _inject_theme_attribute_in_block_template_content.
• #59346: Add block_hooks field to WP_Block_Type, block.json loading, REST API endpoint, etc (see ​here and ​here).
• #59383: Implement get_hooked_blocks() to receive a list of blocks hooked into a given "anchor" block (​see).
  • We probably want to ​cache this.
• #59385: Implement block insertion functions.
• #59412: Add post-block callback arg to traverse_and_serialize_block(s), change signature.
• #59399: Implement insert_hooked_blocks (to insert all hooked blocks for a given anchor block).
• Call the latter alongside _inject_theme_attribute_in_template_part_block in a newly written visitor function that's passed as $callback to serialize_blocks (​see).
• Apply automatic insertion of hooked blocks to block patterns (see ​here and ​here).
• Add a filter to allow people to e.g. limit automatic insertion of hooked blocks to a certain template or template part type only (​also see).
  • This somewhat informs the architecture of the code (e.g. function signatures, passing of $block_template context) and goes beyond what is currently in Gutenberg.
  • Note that this hasn't been implemented in the code sync PR yet and is thus still a bit more tentative.

The TODO list is based on my notes from a call with @gziolo earlier today.

Port the Block Hooks feature (​formerly known as Auto-inserting Blocks) from Gutenberg to Core.

WIP. Might break this into separate PRs later.

Trac ticket: https://core.trac.wordpress.org/ticket/59313

Hey @ockham! I know this PR is still in draft. I'm just leaving some nitpick feedback early in case I miss a chance to review this one later, and to avoid additional instances of these being introduced later in the PR's development. 🙂

Feel free to ping me at any stage if I can help to review the implementation, tests, docs, etc. 🙂

Thank you very much @costdev! Good to address these early one, before I break the PR down into smaller ones 😊

Spun off from ​https://github.com/WordPress/wordpress-develop/pull/5158.

See https://core.trac.wordpress.org/ticket/59313 for details

TODO: Add unit test coverage.

Trac ticket: TBD

While we have coverage for _inject_theme_attribute_in_block_template_content, those tests only verify that that function does what is supposed to do; there's however no guarantee that _build_block_template_result_from_file uses that function (or whatever other technique) to actually inject the theme attribute (​see).

Needed for https://core.trac.wordpress.org/ticket/59313.

### Testing Instructions

• Apply the following patch first on trunk, and run unit tests (npm run test:php -- --group=block-templates). Verify that they pass (although they shouldn't) ✅ 👎
• Then, apply the patch on top of this branch, and run unit tests again. Verify that this time, they fail. :x: 👍
• Finally, remove the patch (or check CI for this branch): Unit tests should pass ✅ 👍

src/wp-includes/block-template-utils.php

@@ -565 +565 @@
         $template                 = new WP_Block_Template();
         $template->id             = $theme . '//' . $template_file['slug'];
         $template->theme          = $theme;
-        $template->content        = _inject_theme_attribute_in_block_template_content( $template_content );
+        $template->content        = ;
         $template->slug           = $template_file['slug'];
         $template->source         = 'theme';
         $template->type           = $template_type;

Trac ticket: TBD.

Ah, used the wrong repo to create the branch. Apologies.

For some operations, we need to traverse the parsed block tree (before it is eventually serialized). In order to minimize the number of tree traversals (which is a potentially costly operation), it seems that adding a callback function argument to serialize_block() -- which inevitable has to traverse the entire tree anyway -- is a natural fit.

Examples where this could come in handy:

• theme arg injection into a block template loaded from a file.
• automatic insertion of hooked blocks (see https://core.trac.wordpress.org/ticket/59313)

Trac ticket: https://core.trac.wordpress.org/ticket/59327

Opened #5187 instead.

Hi! Please forgive my dropping in with a few comments, but this PR was interesting to me.

As a regular reader of the block API, I found the $callback parameter somewhat unclear. In the context #5158, of course, it makes more sense. Otherwise, $callback seems just as likely to apply at the end of the function as to the beginning. Perhaps it acts as replacement logic for the whole function. A longer name like $tree_callback or something might help disambiguate.

Next, after thinking about the variable name, I wondered why this function would introduce a dedicated parameter for what most other functions would make accessible as a filter — pre_serialize_block or something. My instinct as a developer at an agency would be that if core has a need to modify the behavior of serialize_block(), doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.

Still, I can think of some reasons against a filter, such as being wary about adding a filter to a hot code path, a desire to keep the new behavior in #5158 "private," or disliking the pattern of adding and removing one-off filters. So that leads to my final thought, which is, rather than add a callback to serialize_block(), create a separate function for traversing the block tree, and call that before serialize_blocks(), e.g.

$pattern            = $this->registered_patterns[ $pattern_name ];
$blocks             = parse_blocks( $pattern['content'] );
$visitor            = _parsed_block_visitor( $pattern );
$blocks             = map_block_tree( $visitor, $blocks ); // Here
$pattern['content'] = serialize_blocks( $blocks );

This new function could be @access private if you thought that was important, although I can think of some cases where we could have used such a function with our clients.

Regardless, a new function would keep an added responsibility out of serialize_block(s)() and still allow you to modify the block tree without interference when Block Hooks required doing so.

Thanks for considering it!

As for the functionality, these new tests cover the part of the functionality that is going to be refactored as outlined in ​https://github.com/WordPress/wordpress-develop/pull/5158 to bring support for the new Block Hooks feature. I can confirm that the tests look as intended.

@dlh01, you shared great feedback in your comment about $callback. It's a difficult topic as serialize_blocks needs to be extremely performant.

This new function could be @access private if you thought that was important, although I can think of some cases where we could have used such a function with our clients.

Regardless, a new function would keep an added responsibility out of serialize_block(s)() and still allow you to modify the block tree without interference when Block Hooks required doing so.

This would work, but that would require traversing the tree of blocks twice. You would essentially have to recreate recursion inside map_block_tree. What's interesting, a very similar helper is already present in the code: _inject_theme_attribute_in_block_template_content.

My instinct as a developer at an agency would be that if core has a need to modify the behavior of serialize_block(), doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.

Yes, that would definitely be my favorite approach. I'm very hesitant to go this path though, as it's hard to predict the consequences. In this case, it's safer to start small and allow modifications only when loading the templates and patterns from the files on the server, as we know it will get processed only once. In fact, Block Hooks feature will most likely include a filter as part of the flow so plugin devs can cover more advanced use cases where they have specific limitations for automatically inserting blocks.

Anyway, let's keep the discussion going as we implement the basic functionality to enable the feature for WP 6.4 beta 1.

Hey @dlh01, thanks a lot for your thoughtful feedback! I think you're making a number of great points there, so it's much appreciated 😄

As a regular reader of the block API, I found the $callback parameter somewhat unclear. In the context #5158, of course, it makes more sense. Otherwise, $callback seems just as likely to apply at the end of the function as to the beginning. Perhaps it acts as replacement logic for the whole function. A longer name like $tree_callback or something might help disambiguate.

Yeah, I found the naming part hard. Eventually, I went with the fairly generic $callback, as is also used by somewhat similar PHP functions such as ​`array_map`.

I'm a bit reluctant to use $tree_callback as it seems to put too much focus on the tree aspect 🤔 $block_callback also comes to mind, but to me, that evokes more something like a render_block callback... Naming is hard! 😅 Maybe I can solve this by a better argument description in the PHPDoc?

Next, after thinking about the variable name, I wondered why this function would introduce a dedicated parameter for what most other functions would make accessible as a filter — pre_serialize_block or something. My instinct as a developer at an agency would be that if core has a need to modify the behavior of serialize_block(), doing so with a filter would be more in keeping with the extensibility of the project and preserve the ability for developers to integrate with core.

Still, I can think of some reasons against a filter, such as being wary about adding a filter to a hot code path, a desire to keep the new behavior in #5158 "private," or disliking the pattern of adding and removing one-off filters.

Yes, that was pretty much my thought process! I'll add that the main reason against a filter was that serialize_block is a fairly low-level function that's used in a variety of different contexts, so it seemed risky to require the user to remove the filter after serialization to make sure it wouldn't be accidentally applied when running serialization in a different context. Finally, if we have different parts of the codebase add different filters for serialize_blocks (to be used in different contexts), it could also get a bit hard to tell which ones apply to a certain code path, since the addition and removal of filters isn't necessarily colocated with the serialize_block() call.

So that leads to my final thought, which is, rather than add a callback to serialize_block(), create a separate function for traversing the block tree, and call that before serialize_blocks(), e.g. [...]

I also considered this option 😅 but ultimately decided against. Two reasons here:

• Efficiency: It would duplicate tree traversal, which is a potentially costly operation. As stated in the PR description, serialize_block _has_ to recursively traverse the tree; if we need to traverse it for another purpose, we might as well do it here 😄
• Established pattern: There's arguable _some_ precedent for a pattern like this -- application of a function to every element of a data structure while it is being traversed (the basic one being once again array_map). In cases like these, languages like PHP (but also JS) tend to accept the callback as a function argument.

---

FWIW, I was considering still some other alternatives/variations (e.g. even re-implementing serialize_blocks itself using a variation of your map_block_tree function, and/or accepting an array of callbacks rather than just an individual one) but eventually landed on the present approach since it seemed to strike a good balance between the requirements described above, and pretty good backwards-compatibility (via adding a new argument to an existing function). It also allowed keeping serialize_blocks perfectly generic and ignorant of applications such as hooked block insertion, as that can be completely contained in the callback.

With all that said, I could see us adding a generic filter to serialize_block like you suggested later if a strong need arises, but for the time being, I'd rather go with the callback argument, for the above reasons 😊

@costdev Thank you for your feedback! I believe I've addressed it all 😊

Would you mind giving it another look when you have a moment? I'd love to land this soon as it's only the first step for Block Hooks to land in 6.4 😊

Thank you @gziolo!

I'll make one minor change to the PHPDoc for $callback to clarify what it's for.

[Bernhard Reiter]

In 56557:

General: Add optional callback argument to serialize_block(s).

Allow passing a function callback to serialize_block(s) that is invoked on each node in the tree of parsed blocks as it is traversed for serialization.

A function argument was chosen for passing the callback function as it reflects a pattern familiar from other algorithms that apply a given callback function while traversing a data structure -- most notably PHP's own array_map().

Introducing a filter was considered as an alternative but ultimately dismissed. For a fairly low-level and general-purpose function such as serialize_block(), using a filter to pass the callback seemed risky, as it also requires removing that filter after usage in order to prevent the callback from being accidentally applied when serialize_block() is called in an entirely different context.

Introducing a separate function for applying a given operation during tree traversal (i.e. independently of serialization) was also considered but dismissed, as it would unnecessarily duplicate tree traversal.

Props dlh, gziolo.
Fixes #59327. See #59313.

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

BTW here's a basic first change that's going to leverage the new callback arg: ​https://github.com/WordPress/wordpress-develop/pull/5192 😊

Thanks @ockham and @gziolo for your comments! (And thanks @ockham for the generous props.)

Both of you are right, of course, that a separate function would require traversing the block tree again, which is not optimal. I would say in response, first, that a separate function leaves the door open to refactoring the logic in both functions to reduce the duplication with minimal effects on backwards compatibility.

Second, the performance effect of traversing the tree twice would be felt in only the locations where the new function was called, whereas the current implementation commits core to checking is_callable() before serializing every block everywhere. On a client project that we recently finished, for example, serialize_blocks() is called three times just within the scope of the_content as part of fulfilling various requirements for the site. I don't have the numbers to show it one way or the other, but I wonder whether these is_callable()s running all the time would soon add up to more than the cost of the traversals running some of the time, an effect that would reach our projects, at least.

[Bernhard Reiter]

In 56562:

Themes: Add test for theme atttibute in file-based block template.

While we already have unit test coverage for _inject_theme_attribute_in_block_template_content, those tests only verify that that function does what is supposed to do; there's however no guarantee that _build_block_template_result_from_file uses that function (or whatever other technique) to actually inject the theme attribute.

This patch adds test coverage to verify that the theme attribute is correctly injected by _build_block_template_result_from_file.

Props costdev, gziolo, mukesh27, spacedmonkey.
Fixes #59325. See #59313.

Thanks all!

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

Rather than using _inject_theme_attribute_in_block_template_content to inject the theme attribute into all Template Part blocks found in a given file-based Block Template, introduce a new function tentatively called _inject_theme_attribute_in_template_part_block, and use that as second argument to serialize_blocks() (introduced in #5187) in order to inject said attribute during tree traversal for serialization.

This allows for a more modular approach that will eventually be extended to implement automatic insertion of hooked blocks (see).

Note that we're guarding _build_block_template_result_from_file() (i.e. the callsite of _inject_theme_attribute_in_template_part_block and previously of _inject_theme_attribute_in_block_template_content) against regressions through additional unit test coverage added in #5186.

TODO:

• [ ] Add test coverage for _inject_theme_attribute_in_template_part_block (same scenarios as _inject_theme_attribute_in_block_template_content).
• [x] Rebase once ​https://github.com/WordPress/wordpress-develop/pull/5186 has been committed.

Trac ticket: https://core.trac.wordpress.org/ticket/59338

Thanks @ockham and @gziolo for your comments! (And thanks @ockham for the generous props.)

Both of you are right, of course, that a separate function would require traversing the block tree again, which is not optimal. I would say in response, first, that a separate function leaves the door open to refactoring the logic in both functions to reduce the duplication with minimal effects on backwards compatibility.

But if it needs serialization in addition to another operation, it will always traverse the tree at least twice, won't it? I'd really rather avoid that as a lower boundary for file-based template processing, when it's clear that it _is_ avoidable.
(Furthermore, I think that there's a risk that with this pattern, people will end up introducing another full tree traversal for each additional operation they would like to perform on the tree, rather than "merging" that operation into an existing traversal.)

Second, the performance effect of traversing the tree twice would be felt in only the locations where the new function was called, whereas the current implementation commits core to checking is_callable() before serializing every block everywhere. On a client project that we recently finished, for example, serialize_blocks() is called three times just within the scope of the_content as part of fulfilling various requirements for the site. I don't have the numbers to show it one way or the other, but I wonder whether these is_callable()s running all the time would soon add up to more than the cost of the traversals running some of the time, an effect that would reach our projects, at least.

That's fair enough (although my hunch is that is_callable will be a fairly quick check). I would still like to continue to stick with the API of a callback argument for serialize_blocks, and to avoid a lower boundary of two tree traversals for file-based templates.

I believe that it should be easy enough to optimize the chosen approach: If the is_callable call proves too costly, we could do something like the following:

function serialize_blocks( $blocks, $callback = null ) {
        $result = '';
        if ( is_callable( $callback ) ) {
                foreach ( $blocks as $block ) {
                        $result .= serialize_block( $block, $callback );
                };
        } else {
                $result .= _serialize_block_without_callback( $block );
        }
        return $result;
}

With _serialize_block_without_callback defined like the "old" (pre-callback) serialize_block. A similar check could be added to serialize_block (singular). AFAICS, that would limit the overhead to just one is_callable check.

For visibility, this is the performance impact from this PR included on CI ​https://github.com/WordPress/wordpress-develop/actions/runs/6173429870/job/16778816255?pr=5192:

I checked out of the curiosity whether this change impacted performance metrics reported in trunk with ​https://github.com/WordPress/wordpress-develop/actions/runs/6159812895/job/16715609415:

It gets compensated with the further refactoring as documented in ​https://github.com/WordPress/wordpress-develop/pull/5192#issuecomment-1719000338.

Thank you very much!

[Bernhard Reiter]

In 56578:

Themes: Inject theme attribute during serialization.

Rather than using _inject_theme_attribute_in_block_template_content to inject the theme attribute into all Template Part blocks found in a given file-based Block Template, introduce a new function called _inject_theme_attribute_in_template_part_block, and use that as second argument to serialize_blocks() (introduced in [56557]) in order to inject said attribute during tree traversal for serialization.

This allows for a more modular approach that will eventually be extended to implement automatic insertion of hooked blocks.

Note that we're guarding _build_block_template_result_from_file() (i.e. the callsite of _inject_theme_attribute_in_template_part_block and previously of _inject_theme_attribute_in_block_template_content) against regressions through additional unit test coverage added in [56562].

Props @gziolo.
Fixes #59338. See #59313.

Committted to Core in https://core.trac.wordpress.org/changeset/56578.

In order to implement Block Hooks (see `#59313`), we need to add a new block_hooks field to the WP_Block_Type class, as well as to block registration related functions, the block type REST API controller, etc.

Based on #5158.

Trac ticket: https://core.trac.wordpress.org/ticket/59346

One more place to include block_hooks:

​https://github.com/WordPress/wordpress-develop/blob/943e2b95511794417d545ca06fa22c0167391267/src/wp-admin/includes/post.php#L2216-L2234

One more place to include block_hooks:

​https://github.com/WordPress/wordpress-develop/blob/943e2b95511794417d545ca06fa22c0167391267/src/wp-admin/includes/post.php#L2216-L2234

Let's tackle splitting the test case in a follow-up. I volunteer for doing it.

Tests splitted with https://core.trac.wordpress.org/changeset/56584.

Thank you @gziolo!

[...] the key is the block name that can be anything that matches the pattern for the block name. If that helps we could include the pattern also for the name field:

​https://github.com/WordPress/wordpress-develop/blob/72de12010b0bd67f814e699705e709f3a125351e/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php#L478-L484

That reminded me of something else; maybe we can cross-reference the name field in the block_hooks schema definition? Would make it more semantic -- we'd basically state, "The keys in block_hooks are block names.".

I see there is custom logic for mapping blockHooks to block_hooks. I'd like to work on a follow-up to cover that part with unit tests. before case is covered indirectly by other values still could use some testing.

Ah yeah, that's a fair point; especially for the camelCase -> snake_case mappings.

One last bit of feedback is for the _doing_it_wrong logic. I like that there is additional security enforced. I'm wondering if we should move it to the WP_Block_Type class though, to cover also the case when someones registers the block with:

register_block_type( 'my-plugin/my-block', array(
    'block_hooks' => array(
        'my-plugin/my-block': 'first_child',
    ),
) );

FWIW, register_block_type actually calls register_block_type_from_metadata for block registration, so your example should already be covered: ​https://github.com/WordPress/wordpress-develop/blob/e005108fe18e6278f4e6dd0fec030e503fe0ff46/src/wp-includes/blocks.php#L584-L590

(We might want to add some unit test for this too, BTW 🤔 )

It also reminds me that in genera; there isn't that much sanitization present on the code for settings provided during block registration which I find a bit unforunate.

Yeah, good point!

[Bernhard Reiter]

In 56587:

General: Add block_hooks field to block type registration, REST API.

In order to implement Block Hooks, we need to add a new block_hooks field to the WP_Block_Type class, as well as to block registration related functions, the block types REST API controller, etc.

Props gziolo.
Fixes #59346. See #59313.

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

[gziolo]

In 56588:

Tests: Improve the assertions for REST API endpoint for block types

Follow-up to [56587], [55673]. While working on #59346, it was noted that selectors fiels is not always included in the assertions. While looking at it is was difficult to spot the issue because the random order of how REST API fields where listed.

Reorderd the REST API fields in the test cases to follow the list from the documentation: ​https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md. This way it's going to be easier to maintain the list moving forward.

Props ockham.
See #59346, #59313, #57585.

In ​https://github.com/WordPress/wordpress-develop/pull/5218 I added some refactorings discussed above:

• ​https://github.com/WordPress/wordpress-develop/pull/5203#discussion_r1326102815 - gutenberg text domain remove
• There is custom logic for mapping blockHooks to block_hooks when handling metadata. I covered that part in unit tests.
• One last bit of feedback was for the _doing_it_wrong logic. I moved the check to the WP_Block_Type class to cover also the case when someone registers the block with a regular register_block_type call without using metadata file.

But if it needs serialization in addition to another operation, it will always traverse the tree at least twice, won't it? I'd really rather avoid that as a lower boundary for file-based template processing, when it's clear that it _is_ avoidable. (Furthermore, I think that there's a risk that with this pattern, people will end up introducing another full tree traversal for each additional operation they would like to perform on the tree, rather than "merging" that operation into an existing traversal.)

I think you sort of answered your own question 😄 Traversing the tree twice is definitely avoidable. In my original comment, I suggested a separate function to apply the callback throughout the tree before serialization occurs. Another way of achieving the goal of minimizing the impact to serialize_block(s) would be to reverse your suggestion of a new function that serializes without a callback and create a _serialize_block(s)_with_callback().

Both new functions would, though, put us back to where we started, which is that the logic to recurse through the tree would be recreated.

I think at the heart of my original comment was the sense that it would be preferable to accept that duplicated logic in a new function that served the specific purposes presented by block hooks, compared to permanently encoding it in the main serialize_block(s) functions.

A new internal function can be deprecated and forgotten as the block API takes on new use cases (for example, if it becomes necessary to merge multiple operations into a traversal), but the new arguments to the existing functions will live on.

Anyway: This is fun to talk about, but I know you have other things to work on, so we can leave it here. I appreciate you indulging me in this discussion and will be happy to talk about it more if it ever comes up again!

Trac ticket: https://core.trac.wordpress.org/ticket/59313

Extracted from ​https://github.com/WordPress/wordpress-develop/pull/5158 and covered with unit tests.

Looking good overall, just one ​request regarding tests.

For the other PRs we've filed for block hooks, I've actually filed separate tickets; it'd be great if we could do the same here 😊 (I can do that if you're busy with other things.)

Finally, just noting that I agree that we should eventually cache get_hooked_blocks as we ​discussed; but let's keep that for a follow-up.

For the other PRs we've filed for block hooks, I've actually filed separate tickets; it'd be great if we could do the same here 😊 (I can do that if you're busy with other things.)

Feel free to open another ticket. Otherwise, I'm fine referencing the existing ticket that covers the block hooks feature.

I addressed the feedback, and I will open a ticket for this functionality now that I have some additional time.

The ticket is https://core.trac.wordpress.org/ticket/59383.

WIP. See https://core.trac.wordpress.org/ticket/59313 and #5158.

TODO:

• [ ] Better name for _parsed_block_visitor.
• [ ] Add test coverage for insert_hooked_block, insert_hooked_blocks.
• [ ] Implement pattern insertion?

Trac ticket: TBD

[gziolo]

In 56610:

Blocks: Introduce helper function to retrieve hooked blocks

In order to implement Block Hooks (see #59313), we added block_hooks field to the WP_Block_Type class, as well as to block registration related functions. In this follow-up, new helper function gets introduced that is going to compute the list of hooked blocks by other registered blocks for a given block type.

Extracted from ​https://github.com/WordPress/wordpress-develop/pull/5158 and covered with unit tests.

Props ockham.
Fixes #59383.

Committed with https://core.trac.wordpress.org/changeset/56610.

Trac ticket: https://core.trac.wordpress.org/ticket/59313

Related to ​https://github.com/WordPress/wordpress-develop/pull/5240.

Still work in progress...

@gziolo I've decided to only implement insert_block_hook and inser_block_hooks here (plus unit test coverage). We'll only wire them to block templates and patterns to actually insert blocks in a follow-up 😄

@gziolo There's another fix we might have to backport to GB: e9931e7.

@gziolo This needs test coverage for insert_hooked_blocks (plural) (see ​https://github.com/WordPress/wordpress-develop/pull/5241#discussion_r1328811091); otherwise, it might be about ready 🤞

See https://core.trac.wordpress.org/ticket/59313.

Trac ticket: TBD

@gziolo I think this (plus #5240) is how we can implement sibling insertion 🤞

Will need ​https://github.com/WordPress/wordpress-develop/pull/5242 to eventually pass the parent arg to insert_hooked_blocks.

I was considering doing one more thing: Split insert_hooked_block into insert_hooked_child_block and insert_hooked_sibling_block, since they're pretty different. I also might adjust the function signature of the latter a bit more to align with ​this.

@gziolo Since we're adding quite a bit of complexity to serialize_block(s), I'm starting to consider renaming it to e.g. traverse_and_serialize_block(s), and keep serialize_block(s) as it was before #5187. (This will allow us to also optimize traverse_and_serialize_block(s), as it would enable it to call serialize_block(s) for its children if no callback is given -- thus making the is_callable check only at the top level, but avoiding it for all descendants, as basically discussed ​here.)

WDYT?

traverse_and_serialize_block - that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.

traverse_and_serialize_block - that sounds like a good idea given that it's going to be customized more than we discussed 👍🏻 The only question is whether it's possible to share as much as possible code between both implementations so they don't diverge over time. Maybe, it won't be a problem. Let's see later.

I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s), which is what we'd try to avoid.)

I was considering doing one more thing: Split insert_hooked_block into insert_hooked_child_block and insert_hooked_sibling_block, since they're pretty different. I also might adjust the function signature of the latter a bit more to align with ​this.

Might actually rename to insert_child_block and insert_sibling_block, since they're arguably not even that specific to hooked blocks.

@dlh01 Just a quick update: Since we need to ​add a bit more logic, we'll be reverting the change to serialize_block(s) and will ​introduce a new `traverse_and_serialize_block(s)` instead. Thanks again for the discussion, it was insightful feedback that eventually influenced this decision! 😄

@gziolo Should be ready for review! I've changed it a lot, but I'm really happy with it now. It's much less specific to hooked blocks now, yet the functions are exactly the way we'll need them 😄

Test failures seem unrelated, so maybe there is a need to rebase the PR.

I like how the code is split now into smaller helper functions that are specialized and nicely encapsulate their task. One final note, they will be exposed in the dev docs portal, which I'm fine with.

I'm afraid we won't be able to share much code between the two -- it's the tradeoff we make for optimizing. (Otherwise, we'd likely end up again with a bunch of extra conditionals in serialize_block(s), which is what we'd try to avoid.)

I know realized that we are going to add more changes later in the process. That's fine 👍🏻

Trac ticket: https://core.trac.wordpress.org/ticket/59313.

Reverts changes from #5187 and implements changes discussed in #5242.

I opened ​https://github.com/WordPress/wordpress-develop/pull/5246, which addresses the changes mentioned by @ockham.

Opened ​https://github.com/WordPress/wordpress-develop/pull/5246 with the changes discussed in the comments.

Test failures seem unrelated, so maybe there is a need to rebase the PR.

Yeah, looks like unit tests have been failing for all recent PRs 😕

I like how the code is split now into smaller helper functions that are specialized and nicely encapsulate their task. One final note, they will be exposed in the dev docs portal, which I'm fine with.

Yeah, I think that's perfectly okay -- the functions are now generic enough that they might even come in handy for different purposes.

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/5246.

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

I don't think it's necessary anymore after https://core.trac.wordpress.org/changeset/56618 landed.

WIP. See https://core.trac.wordpress.org/ticket/59313.

Trac ticket: TBD

[gziolo]

In 56620:

Blocks: Introduce a variation of serialize blocks helper with traversing

Introduces two new functions traverse_and_serialize_blocks and traverse_and_serialize_block with the additional $callback argument. It is possible to pass parent block, block index, chunk index to the callback argument.

Reverts changes applied to serialize_blocks and serialize_block in #59327 with [56557].

Props ockham, mukesh27.
See #59313.

Committed with https://core.trac.wordpress.org/changeset/56620.

Very interesting, @ockham — thanks for the heads up!

On the subject of ​sharing code between the functions, the decorator pattern might become helpful as the block API evolves. ​Here's an example implementation that I was experimenting with after our initial discussion, based on the original changes to serialize_block(s). WordPress doesn't really go for interfaces, but in this example I think they would allow for sharing the innerContent loop without conditionals.

[Bernhard Reiter]

In 56634:

Blocks: Revert implementation of block insertion functions.

In [56618], three functions (insert_inner_block, prepend_inner_block, and append_inner_block) were introduced. They were meant to be used for insertion of hooked blocks; however, it was discovered that the original idea wouldn't work for sibling insertion. Instead, a different approach will be taken (see #59412), and these functions are no longer needed and can thus be removed.

Reverts [56618].
See #59412, #59385, #59313.

Replaces #5158.

Rebase after #5257 is merged -- the latter has now some additional polish.

Trac ticket: https://core.trac.wordpress.org/ticket/59313

@gziolo I'm hoping that this PR will become the final piece to implement Block Hooks. It should be pretty much feature-complete if I didn't miss anything 🙂

[Bernhard Reiter]

In 56644:

Blocks: Change traverse_and_serialize_block(s)'s callback signature.

During work on #59399, it was discovered that sibling block insertion wasn't likely going to work the way it was planned, which required devising an alternative solution. This new solution requires some changes to traverse_and_serialize_block(s):

• Change the signature of the existing callback such that:
  • the return value is a string that will be prepended to the result of the inner block traversal and serialization;
  • the function arguments are: a reference to the current block (so it can be modified inline, which is important e.g. for theme attribute insertion), the parent block, and the previous block (instead of the block index and chunk index).
• Add a second callback argument to traverse_and_serialize_block(s), which is called after the block is traversed and serialized.
  • Its function arguments are a reference to the current block, the parent block, and the next block.

Props gziolo.
Fixes #59412. See #59313.

Superseded by #5261.

Weird, unit tests are currently failing:

Invalid argument supplied for foreach()

/var/www/src/wp-includes/blocks.php:754

That's this line: ​https://github.com/WordPress/wordpress-develop/blob/aa033cba5cd3424a0445c6cd95e34307986880a1/src/wp-includes/blocks.php#L754

I wonder if/why the block_hooks field isn't available at all (rather than defaulting to the empty array)?

Workaround in bd1c57bb20. Let's see if that works.

Seeing the same test failure I had ​earlier:

Invalid argument supplied for foreach()

/var/www/src/wp-includes/blocks.php:757

Maybe I need to add an is_array check alongside property_exists.

Seeing the same test failure I had ​earlier:

Invalid argument supplied for foreach()

/var/www/src/wp-includes/blocks.php:757

Maybe I need to add an is_array check alongside property_exists.

​https://github.com/WordPress/wordpress-develop/pull/5261/commits/22c75d9e338b5ab021e5f481c0e389136b05eb70

Thanks a lot for the test coverage @gziolo, I quite like your approach! 😄

[Bernhard Reiter]

In 56649:

Blocks: Implement automatic block insertion into Block Hooks.

Block Hooks allow a third-party block to specify a position relative to a given block into which it will then be automatically inserted (e.g. a "Like" button block can ask to be inserted after the Post Content block, or an eCommerce shopping cart block can ask to be inserted after the Navigation block).

The underlying idea is to provide an extensibility mechanism for Block Themes, in analogy to WordPress' ​Hooks concept that has allowed extending Classic Themes through filters and actions.

The two core tenets for Block Hooks are:

1. Insertion into the frontend should happen right after a plugin containing a hooked block is activated (i.e. the user isn't required to insert the block manually in the editor first); similarly, disabling the plugin should remove the hooked block from the frontend.
2. The user has the ultimate power to customize that automatic insertion: The hooked block is also visible in the editor, and the user's decision to persist, dismiss (i.e. remove), customize, or move it will be respected (and reflected on the frontend).

To account for both tenets, the tradeoff was made to limit automatic block insertion to unmodified templates (and template parts, respectively). The reason for this is that the simplest way of storing the information whether a block has been persisted to (or dismissed from) a given template (or part) is right in the template markup.

To accommodate for that tradeoff, ​UI controls (toggles) are being added to increase visibility of hooked blocks, and to allow for their later insertion into templates (or parts) that already have been modified by the user.

For hooked blocks to appear both in the frontend and in the editor (see tenet number 2), they need to be inserted into both the frontend markup and the REST API (templates and patterns endpoints) equally. As a consequence, this means that automatic insertion couldn't (only) be implemented at block render stage, as for the editor, the serialized (but unrendered) markup needs to be modified.

Furthermore, hooked blocks also have to be inserted into block patterns. Since practically no filters exist for the patterns registry, this has to be done in the registry's get_registered and get_all_registered methods.

Props gziolo.
Fixes #59313.

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

Trac ticket: https://core.trac.wordpress.org/ticket/59313

Follow-up for: https://core.trac.wordpress.org/changeset/56649

These two new unit tests document how Block Hooks behave with firstChild and lastChild relative positions. The hooked blocks will only get inserted in the case where the parent block has at least one child block present. While it seems like a limitation, in practice, it's hard to think of a case where the template would use a parent block without its children. It's more likely to happen with patterns in general, but in the case of patterns wired with the block theme, it also seems unlikely. The reasoning here is that out of the box, the block theme should produce a fully functional and valid HTML.

Replaces unit tests that cover similar cases in the Gutenberg plugin that are going to be removed with ​https://github.com/WordPress/gutenberg/pull/54651.

I assume we no longer need it after landing an alternative version. I see now that https://core.trac.wordpress.org/ticket/59399 is closed, so I will close this one, too.

[gziolo]

In 56701:

Blocks: Add more unit test covering edge cases for Block Hooks

These two new unit tests document how Block Hooks behave with first_child and last_child relative positions. The hooked blocks will only get inserted in the case where the parent block has at least one child block present. While it seems like a limitation, in practice, it's hard to think of a case where the template would use a parent block without its children. It's more likely to happen with patterns in general, but in the case of patterns wired with the block theme, it also seems unlikely. The reasoning here is that out of the box, the block theme should produce a fully functional and valid HTML.

Props ockham.
See #59313.
Follow-up [56649].

Committed with https://core.trac.wordpress.org/changeset/56701.

Trac ticket: https://core.trac.wordpress.org/ticket/59313

For start, I created a simplified version of ​Twenty Twenty-Three theme that we used for testing Block Hooks integration that contains:

• The required index.html and the single.html template that I plan to use with tests.
• 3 template oarts where two of them reference patterns.
• 3 Patterns referenced in templates and template parts.

I want to automatically register 4 custom blocks where each of them hooks into another block using all four target relative positions: before, after, firstChild, lastChild.

Now, the goal would be to verify that the block gets hooked into the correct positions when targeting:

• template
• template part
• pattern

Bonus points would be for using a filter to prevent insertion of the block based on some made-up conditions.

Separately from that, with such a block theme in place and four hooked blocks, we could run some profiling on the block pattern registry that will contain at least 3 patterns to see if get_hooked_blocks is really the hot path we should be worried about.

Bonus points would be for using a filter to prevent insertion of the block based on some made-up conditions.

Separately from that, with such a block theme in place and four hooked blocks, we could run some profiling on the block pattern registry that will contain at least 3 patterns to see if get_hooked_blocks is really the hot path we should be worried about.

This PR is big enough, so I would prefer to split other test scenarios into a follow-up work.

I addressed all comments, but ​https://github.com/WordPress/wordpress-develop/pull/5340#discussion_r1342899705. As noted, I'm happy to iterate on the test theme and reduce it further after profiling Block Hooks usage in a real-life scenario when rendering a single post page.

[gziolo]

In 56759:

Tests: Cover Block Hooks integration with a custom block theme

Adds a simplified version of Twenty Twenty-Three theme that helps testing Block Hooks integration. The theme contains:

• The required index.html template.
• The optional single.html template used with tests.
• 3 template parts where two of them reference patterns.
• 3 patterns referenced in the templates and the template parts.

New tests automatically register 4 custom blocks with the test theme where each of them hooks into another block using all four target relative positions: before, after, firstChild, lastChild.

The tests verify that the block gets hooked into the correct positions when targeting:

• template
• template part
• pattern

Props ockham, costdev.
See #59313, #59383.
Follow-up [56610].

Committed with https://core.trac.wordpress.org/changeset/56759.

From a pair-programming session with @gziolo.

The biggest tradeoff we made in the implementation of Block Hooks was that we had to limit them to templates, template parts, and patterns that didn't have any user modifications (see `#59313` for the reason). We would like to remove this limitation, so they’ll also work with user-modified templates, parts, and patterns.

The crucial problem we need to solve is to acknowledge if a user has opted to remove or persist a hooked block, so that the auto-insertion mechanism won't run again and inject an extraneous hooked block on the frontend when none is solicited.

TODO:

• [x] ​https://github.com/WordPress/wordpress-develop/pull/5525
• [ ] Make sure hookedBlocks global attribute default is set correctly (to empty array).
• [ ] Make sure hooked block is correctly added to parent's hookedBlocks attribute for first and last child cases.
• [ ] Inject hooked blocks into DB-based templates.
• [ ] Add unit tests to make sure ignoredHookedBlocks is added to the correct block.

Consider this PR experimental; we might want to break it down into smaller pieces to land invididually:

• [ ] Register global metadata attribute.
• [ ] Extract insert_hooked_blocks function.

Trac ticket: https://core.trac.wordpress.org/ticket/59646

[hellofromTonya]

In 57066:

Blocks: Document Block Hooks functions as private.

Documents the 4 new 6.4 Block Hooks global functions as private and for Core-only internal usage:

• make_before_block_visitor()
• make_after_block_visitor()
• traverse_and_serialize_block()
• traverse_and_serialize_blocks()

This is being done as the architectural design of these new functions may change in the next cycle. Further denoting them as private / Core only can help to avoid extender churn if any of these functions are deprecated.

Follow up to [56649], [56620].

Props azaozz, hellofromTonya, bernhard-reiter, gziolo, mikeschroder.
Fixes #59783.
See #59313.

[hellofromTonya]

In 57071:

Blocks: Document Block Hooks functions as private.

Documents the 4 new 6.4 Block Hooks global functions as private and for Core-only internal usage:

• make_before_block_visitor()
• make_after_block_visitor()
• traverse_and_serialize_block()
• traverse_and_serialize_blocks()

This is being done as the architectural design of these new functions may change in the next cycle. Further denoting them as private / Core only can help to avoid extender churn if any of these functions are deprecated.

Follow up to [56649], [56620].

Reviewed by karmatosed.
Merges [57066] to the 6.4 branch.

Props azaozz, hellofromTonya, bernhard-reiter, gziolo, mikeschroder.
Fixes #59783.
See #59313.

Some notes:

• ​https://github.com/WordPress/gutenberg/pull/55811 has been merged; we might want to do a package sync to pull in those changes, as we'll need them for hooked block insertion into modified templates. (I hope that a package sync won't have any adversary side effects, e.g. if other blocks rely on PHP code that's not in Core yet? Might be less risky since we're only at the beginning of the release cycle, and there's not too much stuff yet in GB's ​`lib/compat/wordpress-6.5`. Curious to hear your thoughts @gziolo)
• In a similar vein, we need to test the Block Hooks UI with this PR. We will need to update how it works, which has to be done in GB. Depending on the logic we'll have to implement there (to possibly accommodate both for the "old" way of hooked block insertion and the "new" way), this might raise the question if we should also implement insertion into modified templates in GB first 🤔 (We will probably be able to make an informed decision based on what those changes to the UI logic will need to look like.)

[johnbillion]

In 57140:

Docs: Correct the documented type for the block_hooks argument when registering a block type.

This argument is an associative array of strings, not an array of arrays.

See #59313, 59651

WIP. Alternative approach to #5523.

The biggest tradeoff we made in the implementation of Block Hooks was that we had to limit them to templates, template parts, and patterns that _didn't have any user modifications_ (see `#59313` for the reason). We would like to remove this limitation, so they’ll also work with user-modified templates, parts, and patterns.

The crucial problem we need to solve is to acknowledge if a user has opted to remove or persist a hooked block, so that the auto-insertion mechanism won't run again and inject an extraneous hooked block on the frontend when none is solicited.

Trac ticket: https://core.trac.wordpress.org/ticket/59646

Alternative approach to ​https://github.com/WordPress/wordpress-develop/pull/5523. See ​https://github.com/WordPress/wordpress-develop/pull/5609#discussion_r1408393016 for the rationale.

The proposed alternative looks very appealing. I need to think more about it, but on the first look I would go with this approach.

It's less important, but the Ci job reports some coding style violations.

The concept introduced around ignoredHookedBlocks on the anchor block works great, and I'm unable to find any drawbacks of this approach on the server side. If you still favor the new helper proposed get_hooked_block_markup (most likely annotated as private), feel free to commit it together with related unit tests.

I'm a bit hesitant to open modified templates todat to fully integrate with Block Hooks because of a single edge discovered. Everything works like charm with the special ignoredHookedBlocks metadata value for existing and modified blocks. The only blocker is the case where a new block gets inserted in the editor that is also an anchor block. We discussed with Bernie possible solutions and it isn't that trivial. However, I'm leaning towards trying to avoid auto-inserting hooked blocks for the newly inserted anchor blocks and finding a way to annotate it with the list of hooked blocks to make it as close as possible to what happens on the server. The only limitation on the client is no ability to apply WP filter that can modify the list of hooked blocks for a certain target position.

Closing in favor or #5712.

I'm a bit hesitant to open modified templates todat to fully integrate with Block Hooks because of a single edge discovered. Everything works like charm with the special ignoredHookedBlocks metadata value for existing and modified blocks. The only blocker is the case where a new block gets inserted in the editor that is also an anchor block. We discussed with Bernie possible solutions and it isn't that trivial. However, I'm leaning towards trying to avoid auto-inserting hooked blocks for the newly inserted anchor blocks and finding a way to annotate it with the list of hooked blocks to make it as close as possible to what happens on the server. The only limitation on the client is no ability to apply WP filter that can modify the list of hooked blocks for a certain target position.

Here's a short screencast to illustrate the issue:

As stated in this comment,

I've filed `#60008` to have a dedicated ticket for introducing the ignoredBlockHooks metadata, without actually enabling the new technique for modified layouts. For the latter, we'll continue to use this ticket.

I'll link this PR to the new ticket, will update the title and description, and remove the changes to _build_block_template_result_from_post.

[Bernhard Reiter]

In 57157:

Block Hooks: Store ignored hooked blocks metadata in anchor block.

The biggest tradeoff that was made in the implementation of Block Hooks was that they were limited to layouts (i.e. templates, template parts, and patterns) that didn't have any user modifications (see #59313 for the reason). This changeset is a preparatory step to remove this limitation, so they’ll eventually also work with user-modified layouts.

The crucial problem to solve is how to acknowledge that a user has opted to remove or persist a hooked block, so that the auto-insertion mechanism won't run again and inject an extraneous hooked block on the frontend when none is solicited.

This is achieved by storing all known blocks hooked to a given anchor block in the metadata attribute on that anchor block; specifically in a field called ignoredHookedBlocks inside of the metadata. Hooked blocks are only rendered on the frontend if they're absent from that field; OTOH, they're injected into that field (via the REST API) when first loaded in the editor.

This simple logic guarantees that once a user modifies a given layout, those changes are respected on the frontend; yet if a plugin that includes a hooked block is activated after those modifications have taken place, the hooked block will be rendered on the frontend. This new technique supplants the one previously used (i.e. rendering hooked blocks on the frontend only if a layout doesn't have any modifications) in a rather direct way.

Note that this changeset only introduces the new metadata field and relevant logic; it does not yet enable hooked block insertion into modified layouts. That will be done in a subsequent step (see #59646).

Props gziolo.
Closes #60008.

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