Block Hooks: Make work with modified templates/parts/patterns

[ockham]

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:

• Block: Give traverse_and_serialize_block(s)' post-callback access to block instance #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:

• Template Part block: Use _build_block_template_result_from_post gutenberg#55811
• Block Types: Add metadata global attribute #5608
• Block Hooks: Extract insert_hooked_blocks() function #5609

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

---

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]

If I apply the following patch to this branch, I can see that the Mini Cart block is added to the Navigation block's blockHooks attribute in the preloaded routes:

diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
index 77c7716434..29d30017c2 100644
--- a/src/wp-includes/blocks.php
+++ b/src/wp-includes/blocks.php
@@ -772,12 +772,12 @@ function insert_hooked_blocks( &$block, $relative_position, $anchor_block_type,
                if ( ! isset( $block['attrs']['blockHooks'] ) || ! in_array( $hooked_block_type, $block['attrs']['blockHooks'] ) ) {
                        $markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
                }
-               if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
+               //if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
                        if ( ! isset( $block['attrs']['blockHooks'] ) ) {
                                $block['attrs']['blockHooks'] = array();
                        }
                        $block['attrs']['blockHooks'][] = $hooked_block_type;
-               }
+               //}
        }
        return $markup;
 }

The attribute still seems to be filtered out on the client side (in the editor) though -- I guess we need to make the global blockHooks attribute known to the editor.

I also can't see the attribute injected into the Comment Template block (for the Like Button) yet.

Needs more tweaking, I guess 😅

[gziolo]

The attribute still seems to be filtered out on the client side (in the editor) though -- I guess we need to make the global blockHooks attribute known to the editor.

Right, that would be necessary here. Unless we reuse metadata attribute that is already being injected in the editor here:

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/hooks/metadata.js

In that case, we would need to make metadata the globally registered attribute on the server (which is probably needed anyway). The information would get stored as attributes.metadata.blockHooks.

So whatever works for you best 😀

[ockham]

I've added some more comments to the Trac issue; the tl;dr is:

We'll go with ignoredHookedBlocks for a start.

Per discussion with @gziolo and @mtias, we'll indeed wrap the ignoredHookedBlocks property in the metadata attribute, i.e.

<!-- wp:navigation {"metadata":{"ignoredHookedBlocks":["mycommerce/mini-cart"]}} -->

[ockham]

On the note of the global metadata attribute: It seems that per WordPress/gutenberg#54426, it's no longer experimental 👍

Edit: Also related: WordPress/gutenberg#55194

[ockham]

Update: With the attribute renamed to ignoredHookedBlocks, and moved into metadata, the following patch

diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
index 93fad3ee95..d28274ab3a 100644
--- a/src/wp-includes/blocks.php
+++ b/src/wp-includes/blocks.php
@@ -781,12 +781,12 @@ function insert_hooked_blocks( &$block, $relative_position, $anchor_block_type,
                ) {
                        $markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
                }
-               if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
+               //if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
                        if ( ! isset( $block['attrs']['metadata']['ignoredHookedBlocks'] ) ) {
                                $block['attrs']['metadata']['ignoredHookedBlocks'] = array();
                        }
                        $block['attrs']['metadata']['ignoredHookedBlocks'][] = $hooked_block_type;
-               }
+               //}
        }
        return $markup;
 }

now successfully adds the information to the block in the editor 🎉

<!-- wp:navigation {"ref":4,"metadata":{"ignoredHookedBlocks":["woocommerce/mini-cart"]},"layout":{"type":"flex","setCascadingProperties":true,"justifyContent":"right"}} /-->

[ockham]

Also working for lastChild insertion, although it seems like it's attaching the info to the wrong block 🤔

<!-- wp:comment-template -->
<!-- wp:columns {"metadata":{"ignoredHookedBlocks":["ockham/like-button"]},"style":{"spacing":{"margin":{"bottom":"var:preset|spacing|40"}}}} -->
<div class="wp-block-columns" style="margin-bottom:var(--wp--preset--spacing--40)"><!-- wp:column {"width":"40px"} -->
<div class="wp-block-column" style="flex-basis:40px"><!-- wp:avatar {"size":40,"style":{"border":{"radius":"20px"}}} /--></div>
<!-- /wp:column -->

[ockham]

Better now:

<!-- wp:comment-template {"metadata":{"ignoredHookedBlocks":["ockham/like-button"]}} -->

(Also, no more patching needed -- the PR now Just Works™️)

[gziolo]

This is shaping up nicely. I'm surprised how little changes are necessary to open the logic for templates and template parts modified by the site editor's user.

[gziolo]

Minor: &$anchor_block might go as a first param as it's the only one that can get modified. It's probably something we can discuss in #5609.

[ockham]

Good point, thank you!

[gziolo]

That would be quite convenient for all templates stored in the database 👍🏻

[gziolo]

Is there any issue with running it always? It's a simple operation, so I don't expect it would impact the front end. I would be in favor of keeping it simple unless I miss something.

[ockham]

No issue, other than it adds a few bytes per block (and it might be somewhat confusing for people). I just thought if we revisit #5514 (which requires different treatment of frontend and REST API), we might also look into this. Definitely okay to leave it in though.

[ockham]

Some notes:

• Template Part block: Use _build_block_template_result_from_post gutenberg#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.)

[ockham]

Closing in favor or #5712.