Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block Hooks: Extract insert_hooked_blocks() function #5609

Closed
wants to merge 5 commits into from
Closed

Block Hooks: Extract insert_hooked_blocks() function #5609

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b68aeac
Block Hooks: Extract insert_hooked_blocks() function
ockham Nov 1, 2023
a258a01
Remove obsolete block argument
ockham Nov 1, 2023
09bff63
Change argument order
ockham Nov 14, 2023
19ff5d5
Add PHPDoc
ockham Nov 28, 2023
0b0f7cb
Add basic test coverage for insert_hooked_blocks
ockham Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 31 additions & 56 deletions src/wp-includes/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,33 @@ function get_hooked_blocks() {
return $hooked_blocks;
}

function insert_hooked_blocks( &$anchor_block, $relative_position, $hooked_blocks, $context ) {
$anchor_block_type = $anchor_block['blockName'];
$hooked_block_types = isset( $hooked_blocks[ $anchor_block_type ][ $relative_position ] )
? $hooked_blocks[ $anchor_block_type ][ $relative_position ]
: array();

/**
* Filters the list of hooked block types for a given anchor block type and relative position.
*
* @since 6.4.0
*
* @param string[] $hooked_block_types The list of hooked block types.
* @param string $relative_position The relative position of the hooked blocks.
* Can be one of 'before', 'after', 'first_child', or 'last_child'.
* @param string $anchor_block_type The anchor block type.
* @param WP_Block_Template|array $context The block template, template part, or pattern that the anchor block belongs to.
*/
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might consider applying the filter outside of insert_hooked_blocks, to keep it ignorant of $context 🤔

Copy link
Member

Choose a reason for hiding this comment

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

You still need to document the filter in one place so it works with the documentation tool. What would be the benefit of having it repeated in all places where it has to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the benefit of having it repeated in all places where it has to be used?

Oh, that aspect is definitely a tradeoff. The benefit would be separation of concerns; we would no longer need to pass $context to insert_hooked_blocks; I believe there's even a way to get rid of $relative_position and $anchor_block.

However, I'm now also leaning to keep the filter inside insert_hooked_blocks, even though I don't love the mix of concerns.

Another idea I had was to additionally extract an even finer-grained insert_hooked_block (singular), to have the best of both worlds. I might explore that separately.

Note that I've started considering not merging this PR separately, but rather do it all as part of #5523. I've noticed that the ideal shape of helper functions is strongly determined by the eventual desired use case, which in this case includes insertion into modified layouts. I've noticed that "massaging" the function signatures and implementation into the right shape might lead to somewhat different results when done with those changes present, versus when done without them 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I see, so you were concerned about the helper function signature. The thing is that you need to run the filter before you can serialize the block(s) into markup, so the helper makes less sense if you further break it down. Anyway, it might be fine to land the same changes with the original PR if that makes things simpler for you. This PR is still useful as we can discuss the problem in isolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While adding some test coverage for this (0b0f7cb), I became even more convinced that I didn't like insert_block_hooks() 😬

It's just weird: It takes a nested $hooked_blocks array, i.e. something like

$hooked_blocks = array(
	'tests/anchor1' => array(
		'after' => array( 'tests/hooked-before' ),
	),
	'tests/anchor2' => array(
		'last_child' => array( 'tests/hooked-last-child' ),
	)
);

plus an $anchor_block and a $relative_position arg (among others), but only really looks up the hooked blocks that are relevant for that anchor block and relative position; any information about blocks hooked to other anchor blocks and/or relative positions is ignored. OTOH, that's also the only thing that the $relative_position is used for.

So I'd actually rather continue to duplicate the filter calls 😬
Instead, I filed a PR using the insert_hooked_block approach discussed here; although I ended up calling the function differently: #5712.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like your bad feelings about the excessive function's signature where spot on 👍🏻


$markup = '';
foreach ( $hooked_block_types as $hooked_block_type ) {
$markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
}

return $markup;
}

/**
* Returns a function that injects the theme attribute into, and hooked blocks before, a given block.
*
Expand Down Expand Up @@ -794,40 +821,10 @@ function make_before_block_visitor( $hooked_blocks, $context ) {

if ( $parent_block && ! $prev ) {
// Candidate for first-child insertion.
$relative_position = 'first_child';
$anchor_block_type = $parent_block['blockName'];
$hooked_block_types = isset( $hooked_blocks[ $anchor_block_type ][ $relative_position ] )
? $hooked_blocks[ $anchor_block_type ][ $relative_position ]
: array();

/**
* Filters the list of hooked block types for a given anchor block type and relative position.
*
* @since 6.4.0
*
* @param string[] $hooked_block_types The list of hooked block types.
* @param string $relative_position The relative position of the hooked blocks.
* Can be one of 'before', 'after', 'first_child', or 'last_child'.
* @param string $anchor_block_type The anchor block type.
* @param WP_Block_Template|array $context The block template, template part, or pattern that the anchor block belongs to.
*/
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
foreach ( $hooked_block_types as $hooked_block_type ) {
$markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
}
$markup .= insert_hooked_blocks( $parent_block, 'first_child', $hooked_blocks, $context );
}

$relative_position = 'before';
$anchor_block_type = $block['blockName'];
$hooked_block_types = isset( $hooked_blocks[ $anchor_block_type ][ $relative_position ] )
? $hooked_blocks[ $anchor_block_type ][ $relative_position ]
: array();

/** This filter is documented in wp-includes/blocks.php */
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
foreach ( $hooked_block_types as $hooked_block_type ) {
$markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
}
$markup .= insert_hooked_blocks( $block, 'before', $hooked_blocks, $context );

return $markup;
};
Expand Down Expand Up @@ -863,33 +860,11 @@ function make_after_block_visitor( $hooked_blocks, $context ) {
* @return string The serialized markup for the given block, with the markup for any hooked blocks appended to it.
*/
return function ( &$block, &$parent_block = null, $next = null ) use ( $hooked_blocks, $context ) {
$markup = '';

$relative_position = 'after';
$anchor_block_type = $block['blockName'];
$hooked_block_types = isset( $hooked_blocks[ $anchor_block_type ][ $relative_position ] )
? $hooked_blocks[ $anchor_block_type ][ $relative_position ]
: array();

/** This filter is documented in wp-includes/blocks.php */
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
foreach ( $hooked_block_types as $hooked_block_type ) {
$markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
}
$markup = insert_hooked_blocks( $block, 'after', $hooked_blocks, $context );

if ( $parent_block && ! $next ) {
// Candidate for last-child insertion.
$relative_position = 'last_child';
$anchor_block_type = $parent_block['blockName'];
$hooked_block_types = isset( $hooked_blocks[ $anchor_block_type ][ $relative_position ] )
? $hooked_blocks[ $anchor_block_type ][ $relative_position ]
: array();

/** This filter is documented in wp-includes/blocks.php */
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );
foreach ( $hooked_block_types as $hooked_block_type ) {
$markup .= get_comment_delimited_block_content( $hooked_block_type, array(), '' );
}
$markup .= insert_hooked_blocks( $parent_block, 'last_child', $hooked_blocks, $context );
}

return $markup;
Expand Down
Loading