Skip to content

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

Closed
ockham wants to merge 5 commits into
WordPress:trunkfrom
ockham:update/get-hooked-blocks-return-value
Closed

Blocks: Change get_hooked_blocks() to returned hooked blocks grouped by relative position#5297
ockham wants to merge 5 commits into
WordPress:trunkfrom
ockham:update/get-hooked-blocks-return-value

Conversation

@ockham

@ockham ockham commented Sep 25, 2023

Copy link
Copy Markdown
Contributor

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 ockham self-assigned this Sep 25, 2023
Comment thread src/wp-includes/blocks.php Outdated
Comment on lines +808 to +795
if ( array_key_exists( $relative_position, $hooked_block_types ) ) {
$hooked_block_types = $hooked_block_types[ $relative_position ];
} else {
$hooked_block_types = array();
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/wp-includes/blocks.php Outdated
@ockham ockham force-pushed the update/get-hooked-blocks-return-value branch from dc2626d to 4131951 Compare September 25, 2023 18:52
@ockham

ockham commented Sep 25, 2023

Copy link
Copy Markdown
Contributor Author

Rebased.

Comment thread src/wp-includes/blocks.php Outdated
ockham and others added 2 commits September 26, 2023 09:07
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();

@gziolo gziolo Sep 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Both work the same 👍🏻

@gziolo

gziolo commented Sep 26, 2023

Copy link
Copy Markdown
Member

I like the proposed refactoring 👍🏻

@ockham

ockham commented Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

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

ockham commented Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

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

@ockham ockham closed this Sep 26, 2023
@ockham ockham deleted the update/get-hooked-blocks-return-value branch September 26, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants