Blocks: Change get_hooked_blocks() to returned hooked blocks grouped by relative position#5297
Blocks: Change get_hooked_blocks() to returned hooked blocks grouped by relative position#5297ockham wants to merge 5 commits into
Conversation
| if ( array_key_exists( $relative_position, $hooked_block_types ) ) { | ||
| $hooked_block_types = $hooked_block_types[ $relative_position ]; | ||
| } else { | ||
| $hooked_block_types = array(); | ||
| } |
There was a problem hiding this comment.
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.
dc2626d to
4131951
Compare
|
Rebased. |
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
| function get_hooked_blocks( $name, $relative_position = '' ) { | ||
| function get_hooked_blocks( $name ) { | ||
| $block_types = WP_Block_Type_Registry::get_instance()->get_all_registered(); | ||
| $hooked_blocks = array(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
See my other comment (which was probably hidden after rebasing):
The alternative would be to make
get_hooked_blockscreate empty arrays for all positions, but I didn't like that option so much either; one drawback would be that we'd need to giveget_hooked_blocksa list of possible relative positions (which we'd also need to update if we introduce new ones); whereas with the current approach, we can keepget_hooked_blocksignorant of what relative positions exist, and let it create arrays for whatever it finds.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😊
There was a problem hiding this comment.
I like have you framed it: "matter of taste" 😄
Both work the same 👍🏻
|
I like the proposed refactoring 👍🏻 |
|
Committed to Core in https://core.trac.wordpress.org/changeset/56704. |
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_blocksin production code used to need an extraarray_keyscall. This seemed beside the point.Furthermore, this allows us to remove the extra
$relative_positionargument 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.