Blocks: Change get_hooked_blocks() to returned hooked blocks grouped by relative position

[ockham]

See the unit test diff to see how the return format has changed.

The main reason for doing is is that all existing calls of get_hooked_blocks in production code used to need an extra array_keys call. This seemed beside the point.

Furthermore, this allows us to remove the extra $relative_position argument from the function again, as the same data can now be simply fetched via array access.

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

---

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]

These extra checks are probably the biggest downside of the suggested change (needed for each position; see below).

The alternative would be to make get_hooked_blocks create empty arrays for all positions, but I didn't like that option so much either; one drawback would be that we'd need to give get_hooked_blocks a list of possible relative positions (which we'd also need to update if we introduce new ones); whereas with the current approach, we can keep get_hooked_blocks ignorant of what relative positions exist, and let it create arrays for whatever it finds.

[ockham]

Rebased.

[gziolo]

Maybe, we could avoid all the further conditional checks by initializing all possible relative positions?

$hooked_blocks = array(
	'before'       => array(),
	'after'          => array(),
	'first_child' => array(),
	'last_child'  => array(),
);

What do you think?

[ockham]

See my other comment (which was probably hidden after rebasing):

The alternative would be to make get_hooked_blocks create empty arrays for all positions, but I didn't like that option so much either; one drawback would be that we'd need to give get_hooked_blocks a list of possible relative positions (which we'd also need to update if we introduce new ones); whereas with the current approach, we can keep get_hooked_blocks ignorant of what relative positions exist, and let it create arrays for whatever it finds.

[gziolo]

I missed that comment somehow.

It's also possible to put allowed values in a constant on WP_Block_Type:

const BLOCK_HOOKS_POSITIONS = array( 'after', 'before', 'first_child', 'last_child' );

This way, we don't need to worry about the maintenance if we use that constant to initialize the $hooked_block array.

[ockham]

Yeah, I was also considering a const, but still not totally loving the idea 😬

I kinda prefer a return value of e.g.

array(
	'first_child' => array(
		'tests/injected-two',
	),
)

to all the empty arrays in

array(
	'before'      => array(),
	'after'       => array(),
	'first_child' => array(
		'tests/injected-two',
	),
	'last_child'  => array(),
)

which is kinda noisy. But again, that might be a matter of taste 😊

[gziolo]

I like have you framed it: "matter of taste" 😄

Both work the same 👍🏻

[gziolo]

I like the proposed refactoring 👍🏻

[ockham]

Thank you for approving @gziolo! I won't change the return value structure for now (see), but we can revisit if it turns out to e.g. have an impact on performance.

[ockham]

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