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 API: Add hooked_blocks filter to allow hooked block extensibility #6228

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Draft

Block Hooks API: Add hooked_blocks filter to allow hooked block extensibility #6228

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
94 changes: 76 additions & 18 deletions src/wp-includes/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,14 +885,27 @@
*/
$hooked_block_types = apply_filters( 'hooked_block_types', $hooked_block_types, $relative_position, $anchor_block_type, $context );

/**
* Filters the list of hooked blocks for a given anchor and relative position.
*
* @since 6.6.0
*
* @param array[] $hooked_blocks The list of hooked blocks.
* @param string $relative_position The relative position of the hooked blocks.
* Can be one of 'before', 'after', 'first_child', or 'last_child'.
* @param array $parsed_anchor_block The parsed anchor block.
* @param WP_Block_Template|WP_Post|array $context The block template, template part, `wp_navigation` post type,
* or pattern that the anchor block belongs to.
*/
$hooked_blocks = apply_filters( 'hooked_blocks', $hooked_blocks, $relative_position, $parsed_anchor_block, $context );

// Merge hooked blocks and hooked block types.
$hooked_blocks = array_merge( $hooked_blocks, $hooked_block_types );

$markup = '';
foreach ( $hooked_block_types as $hooked_block_type ) {
$parsed_hooked_block = array(
'blockName' => $hooked_block_type,
'attrs' => array(),
'innerBlocks' => array(),
'innerContent' => array(),
);
foreach ( $hooked_blocks as $hooked_block ) {
$parsed_hooked_block = get_parsed_block( $hooked_block );
$hooked_block_type = $parsed_hooked_block['blockName'];

/**
* Filters the parsed block array for a given hooked block.
Expand Down Expand Up @@ -928,11 +941,13 @@
continue;
}

$hooked_block_signature = get_hooked_block_signature( $parsed_hooked_block );

// It's possible that the filter returned a block of a different type, so we explicitly
// look for the original `$hooked_block_type` in the `ignoredHookedBlocks` metadata.
if (
! isset( $parsed_anchor_block['attrs']['metadata']['ignoredHookedBlocks'] ) ||
! in_array( $hooked_block_type, $parsed_anchor_block['attrs']['metadata']['ignoredHookedBlocks'], true )
! in_array( $hooked_block_signature, $parsed_anchor_block['attrs']['metadata']['ignoredHookedBlocks'], true )
) {
$markup .= serialize_block( $parsed_hooked_block );
}
Expand Down Expand Up @@ -964,26 +979,28 @@

/** 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 );
if ( empty( $hooked_block_types ) ) {
/** This filter is documented in wp-includes/blocks.php */
$hooked_blocks = apply_filters( 'hooked_blocks', $hooked_blocks, $relative_position, $parsed_anchor_block, $context );
$hooked_blocks = array_merge( $hooked_blocks, $hooked_block_types );
Comment on lines +982 to +984
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to remove the filter from here (and only keep it in insert_hooked_blocks). set_ignored_hooked_blocks_metadata is the mechanism we use to tell Block Hooks not to (re-)insert a hooked block if the containing template has already been edited by the user in the Site Editor, and thus, the hooked block has become part of the template (or was removed from it; either way, we want Block Hooks to respect that).

As mentioned elsewhere, if our use case is static templates, and if we don't have any concrete plans to make them editable by the user, we shouldn't include blocks that are added by the hooked_blocks filter in ignoredHookedBlocks. (We might then want to rename the filter to injected_blocks, to distinguish it better from hooked blocks.) Specifically, this will allow us to avoid using block signatures as an identifier.

(I think that the latter would introduce significant complexity; I'll elaborate in a follow-up comment.)

Copy link
Contributor

@ockham ockham Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll try to give an example for the complexities that block signatures would introduce:

Let's assume a plugin wants to insert the Mini Cart block after the Page List block, but it wants it to display the "bag" style rather than the "cart". This would look about like this (modulo escaping and whatnot):

<!-- wp:page-list {"metadata":{"ignoredHookedBlocks":["woocommerce/mini-cart-{"miniCartIcon":"bag"}"]}} /-->
<!-- wp:woocommerce/mini-cart {"miniCartIcon":"bag"} /-->

image

Now let's assume the user opens the Site Editor to change the icon to bag-alt:

<!-- wp:page-list {"metadata":{"ignoredHookedBlocks":["woocommerce/mini-cart-{"miniCartIcon":"bag"}"]}} /-->
<!-- wp:woocommerce/mini-cart {"miniCartIcon":"bag-alt"} /-->

Block Hooks will continue to compare the block and its attribute to the signature in ignoredHookedBlocks, which is now out of sync, so it'll wrongly conclude that it needs to inject the Mini Cart block, resulting in two instances:

image

We could probably circumvent the above by taking the changed block into account when updating the ignoredHookedBlocks metadata while saving (which we don't do now; it's not needed for ignoredHookedBlocks that contain only block types). It's less straight-forward than it might seem (and might involve some level of heuristics, since we cannot know for sure that a the block was inserted by Block Hooks in the first place, or added later by the user). This extra complexity is avoidable if we only apply the filter within insert_hooked_blocks.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not opposed to keeping this in insert_hooked_blocks but there's a good chance that if we keep the product editor in blocks, it will be editable at some point. I think it might be good to solve this issue of generic blocks for core blocks insertion as well.

Do you have any ideas for how might solve that? An ID stands out to me as an easy way of doing this, but I know that blocks has been averse to adding IDs to blocks on the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any ideas for how might solve that? An ID stands out to me as an easy way of doing this, but I know that blocks has been averse to adding IDs to blocks on the server.

Yeah, it's tricky. I've tried to think about other alternatives, but so far, I keep coming back to named block variations. I'll try to elaborate why.

If we use attributes to distinguish blocks from each other -- as done in this PR by means of a block signature -- we most likely wouldn't want the entirety of attributes to enter into the picture. This is both for "information parsimony" and UX reasons: We'd likely want to avoid including every attribute of a given block in its signature (including style various block-supports related attributes), as that could yield a fairly massive string of serialized attributes.

For the user, it would also mean that if they change any of those attributes, Block Hooks will no longer recognize it as a hooked block, and re-insert the hooked block itself (see the shopping cart icon example above, but think even "smaller" changes, such as changing a font size or so).

To solve this problem, I think we'd need to select an attribute -- or a set of attributes -- that we actually want to distinguish one block from another, if they are of the same type. E.g. for the Social Icon block, we might want to distinguish block instances based on their service attribute (which can be facebook, x, tumblr, etc).

But this means that we need to introduce a way for blocks to declare which attributes carry that kind of meaning. And AFAICT, this is pretty exactly what named block variations are about.


if ( empty( $hooked_blocks ) ) {
return '';
}

$hooked_block_signatures = [];

Check failure on line 990 in src/wp-includes/blocks.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

Short array syntax is not allowed

foreach ( $hooked_block_types as $index => $hooked_block_type ) {
$parsed_hooked_block = array(
'blockName' => $hooked_block_type,
'attrs' => array(),
'innerBlocks' => array(),
'innerContent' => array(),
);
$parsed_hooked_block = get_parsed_block( $hooked_block );
$hooked_block_type = $parsed_hooked_block['blockName'];

/** This filter is documented in wp-includes/blocks.php */
$parsed_hooked_block = apply_filters( 'hooked_block', $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context );

/** This filter is documented in wp-includes/blocks.php */
$parsed_hooked_block = apply_filters( "hooked_block_{$hooked_block_type}", $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context );

if ( null === $parsed_hooked_block ) {
unset( $hooked_block_types[ $index ] );
if ( null !== $parsed_hooked_block ) {
$hooked_block_signatures[] = get_hooked_block_signature( $parsed_hooked_block );
}
}

Expand All @@ -994,7 +1011,7 @@
$parsed_anchor_block['attrs']['metadata']['ignoredHookedBlocks'] = array_unique(
array_merge(
$previously_ignored_hooked_blocks,
$hooked_block_types
$hooked_block_signatures
)
);

Expand Down Expand Up @@ -2267,3 +2284,44 @@
}
return $arg;
}

/**
* Get hooked block signature to help identify hooked blocks with differing attributes.
*
* @param array $parsed_block The block to generate a signature from.
* @return string Block signature.

Check failure on line 2292 in src/wp-includes/blocks.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

Whitespace found at end of line
*/
function get_hooked_block_signature( $parsed_block ) {
if ( empty( $parsed_block['attrs'] ) ) {
return $parsed_block['blockName'];
}

return $parsed_block['blockName'] . '-' . json_encode( $parsed_block['attrs'] );
}

/**
* Get the fully parsed block from a partial block array or block type string.
*
* @param array|string $block The block as an array or a block type string.
* @return array Parsed block.
*/
function get_parsed_block( $block ) {
if ( is_string( $block ) ) {
return array(
'blockName' => $block,
'attrs' => array(),
'innerBlocks' => array(),
'innerContent' => array(),
);
}

Check failure on line 2317 in src/wp-includes/blocks.php

View workflow job for this annotation

GitHub Actions / PHP coding standards 

Whitespace found at end of line
return array_merge(
array(
'blockName' => '',
'attrs' => array(),
'innerBlocks' => array(),
'innerContent' => array(),
),
$block
);
}
Loading