Block Hooks: Consolidate approach to get the list of hooked blocks.

[tomjcafferkey]

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

1. We use get_hooked_blocks() to get all of the hooked blocks which are hooked from the block.json file.
2. 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.

I feel there's room for improvement to provide a better API for getting all of the hooked blocks by consolidating these two approaches into a single function somehow.

How this to work we would also need to know the current context so we can pass that data to the filter callbacks.

[Bernhard Reiter]

Agree, this is an architectural flaw/oversight on my part.

How this to work we would also need to know the current context so we can pass that data to the filter callbacks.

I think we'll have to add not only $context but also $anchor_block_type and $relative_position as arguments to get_hooked_blocks(), since we need all of them to apply the filters (see ​`hooked_block_types` and ​`hooked_block`.

We'd likely want the arguments in a different order though, probably get_hooked_blocks( $anchor_block_type, $relative_position, $context ). We'd then apply the filters inside of the function if those arguments are non-null.

This means that the return value should also look a bit different, since we no longer need to return a ​nested array (by anchor block and relative position) like we do now.

Makes me wonder if we should introduce a separate helper function for this (since it seems like it would be different enough from get_hooked_blocks()). get_hooked_blocks_for_block_hook() or so maybe. (If we assume that a block hook is defined by an anchor block and a relative position.)

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

[tomjcafferkey]

Makes me wonder if we should introduce a separate helper function for this (since it seems like it would be different enough from get_hooked_blocks()). get_hooked_blocks_for_block_hook() or so maybe. (If we assume that a block hook is defined by an anchor block and a relative position.)

This is the approach I was thinking of taking. If we were to modify get_hooked_blocks() we'd need to change both its signature and return value.

We may also need to revisit the visitor functions and the callbacks they return since they all pass down $hooked_blocks which won't be necessary any longer if a new helper function will handle getting this value inside insert_hooked_blocks and set_ignored_hooked_blocks_metadata internally.

I think modifying the visitor functions to accommodate a new helper function might be a better option since they're marked as @private and get_hooked_blocks() isn't.

[Bernhard Reiter]

Replying to tomjcafferkey:

We may also need to revisit the visitor functions and the callbacks they return since they all pass down $hooked_blocks which won't be necessary any longer if a new helper function will handle getting this value inside insert_hooked_blocks and set_ignored_hooked_blocks_metadata internally.

Ah, good point! I think we can do that.

I think modifying the visitor functions to accommodate a new helper function might be a better option since they're marked as @private and get_hooked_blocks() isn't.

Agree.

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

@ockham I believe this is ready for rereview 🙏🏻

I'll rebase.

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 ​https://github.com/WordPress/wordpress-develop/pull/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 😕

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.