Block Hooks API: Consolidate approach to get all hooked blocks

[tjcafferkey]

Our current approach to getting hooked blocks has two parts to it:

We use get_hooked_blocks() to get all of the hooked blocks which are hooked from the block.json file.
In function insert_hooked_blocks we apply the filter callbacks (passing a derived value from step 1 as the default value) to get hooked blocks that are applied by the hooked_block_types filter.

Currently it's a bit confusing and without knowledge you'd assume get_hooked_blocks() would retrieve all of the hooked blocks. Additionally it feels like you have to run a few different pieces of code separately and combine their resulting values to get a complete picture which could result in some bugs or unnecessary complexities.

This PR aims to fix that by creating a single function to get hooked blocks based on anchor block and position.

Note: As a result of this refactor it has meant executing get_hooked_blocks() multiple times in the lifecycle of a request, to prevent any performance degradation I've memoized the result of the function but would appreciate a second opinion on this.

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

Related WordPress/gutenberg#62658

---

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.

[github-actions]

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.

[ockham]

I think this approach (i.e. the newly introduced function) makes sense 👍

[tjcafferkey]

get_hooked_blocks() is now run here and inside get_hooked_blocks_by_anchor_block which is executed on every block traversal. I think we need to figure out a way to either avoid this, or a caching strategy around it.

[tjcafferkey]

In this commit (6bdec1e) I have memoized the result of get_hooked_blocks(). The cache will persist per-request which means we won't be degrading performance when comparing it to how it currently works but would like a second opinion on this for further opportunities for improvements.

[tjcafferkey]

Open to a better function name but intentionally used maybe_ to signal that even though the filter could be used it still may result in no hooked blocks.

[tjcafferkey]

_build_block_template_result_from_post has these lines inside the conditional above it. Is there a reason we aren't doing the same here?

[ockham]

If memory serves, it's because we need to inject the theme attribute into template parts (and set it to the currently active theme) when first loading a template part from a file. For better or worse, that logic ended up in make_before_block_visitor.

Now the need for that attribute is of course unrelated to the presence of hooked blocks -- it needs to be injected unconditionally 😬

(I'd be curious if we have test coverage at that level, i.e. if a test would start failing if we moved these lines into the conditional.)

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props bernhard-reiter, tomjcafferkey.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ockham]

I'm afraid we cannot change the function signature like that 😕 (When it comes to back-compat in WordPress, think of the @access private as mostly ornamental 😬)

We might be able to deprecate the argument (like we did here). (We'll need to check if that's possible in a case like this, since it's not the last argument of the function.)

The same also applies to some other functions touched by this PR, e.g. make_before_block_visitor and make_after_block_visitor, insert_hooked_blocks_and_set_ignored_hooked_blocks_metadata, etc.

[tjcafferkey]

😢 I suspected this might be the case, but hoped it wasn't. As we've discussed privately in DM there is precedent for deprecating non-last args

wordpress-develop/src/wp-admin/includes/upgrade.php

Line 47 in df256e4

function wp_install( $blog_title, $user_name, $user_email, $is_public, $deprecated = '', $user_password = '', $language = '' ) {

[tjcafferkey]

Updated in:

• a0807c6
• 937c667
• dda337c
• 98ced4b

[tjcafferkey]

@ockham I believe this is ready for rereview 🙏🏻

[ockham]

I'll rebase.

[ockham]

The major problem to solve here is performance. FWIW, make_before_block_visitor and make_before_after_visitor didn't always accept $hooked_blocks as an argument; we added it later (in #5399) as an optimization, since we were concerned that get_hooked_blocks() would be called too often (from those visitors).

At the time, we decided against caching the result of get_hooked_blocks(), since it was deemed too error-prone. I seem to remember one specific scenario where caching would actually cause issues: get_hooked_blocks() iterates over all known (registered) blocks to look for blockHooks fields. The problem was that get_hooked_blocks() ended up getting called before all blocks were registered (I think via the Template Part block's PHP code somehow), which meant that any blocks that were registered afterwards wouldn't be taken into account.

(Generally, people were skeptical about caching things that are depending on registered blocks, as that had turned out tricky in the past.)

For the above reasons, I'm a bit skeptical about caching inside of get_hooked_blocks, as that would be affected by the "called before all blocks are registered"-problem. Maybe we can cache at a different level? 🤔 It's a tricky problem, since the whole point of this PR is to bundle functionality in get_hooked_blocks_by_anchor_block() (which by definition is called for each anchor block), which is at odds with having something called once (at most) per tree traversal 😕

[tjcafferkey]

At the time, we decided against caching the result of get_hooked_blocks(), since it was deemed too error-prone. I seem to remember one specific scenario where caching would actually cause issues: get_hooked_blocks() iterates over all known (registered) blocks to look for blockHooks fields. The problem was that get_hooked_blocks() ended up getting called before all blocks were registered (I think via the Template Part block's PHP code somehow), which meant that any blocks that were registered afterwards wouldn't be taken into account.

Interesting, I wasn't aware of the background here so appreciate you providing that additional context for me. I didn't experience this in my testing but that's not to say it's not an issue at all, just an issue that didn't surface itself to me yet.

For the above reasons, I'm a bit skeptical about caching inside of get_hooked_blocks, as that would be affected by the "called before all blocks are registered"-problem. Maybe we can cache at a different level? 🤔 It's a tricky problem, since the whole point of this PR is to bundle functionality in get_hooked_blocks_by_anchor_block() (which by definition is called for each anchor block), which is at odds with having something called once (at most) per tree traversal 😕

Hmm, interesting. I'll have a further think and potentially revisit this improvement but I can see how you're thinking about this solution.