Skip to content

Block Hooks: Introduce a new hooked_block_{$block_type} filter #5835

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

Closed
ockham wants to merge 32 commits into from
Closed

Block Hooks: Introduce a new hooked_block_{$block_type} filter #5835

Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
a36ac67
Block Hooks: Extract `insert_hooked_blocks()` function
ockham Jan 3, 2024
5350ae2
Introduce hooked_block filter
ockham Jan 3, 2024
e63213c
get_hooked_block_markup: Change argument order
ockham Jan 3, 2024
a891d3c
Fix get_hooked_block_markup tests
ockham Jan 3, 2024
fabd9c2
Make block type name part of filter name
ockham Jan 3, 2024
afd95d0
Set attrs to empty array
ockham Jan 3, 2024
f797f8a
Allow passing inner blocks
ockham Jan 10, 2024
01f30ab
Use correct block type in ignoredHookedBlocks
ockham Jan 10, 2024
962eaf5
Update comment
ockham Jan 11, 2024
1a214a7
Mark insert_hooked_blocks as private
ockham Jan 11, 2024
8b08619
Slight rewording of a comment
ockham Jan 11, 2024
0658500
Clarify PHPDoc
ockham Jan 11, 2024
45ff3da
Fix tests
ockham Jan 11, 2024
520f67e
Cover new hooked_block_type arg in tests
ockham Jan 11, 2024
df3dc99
Add ticket references to tests
ockham Jan 11, 2024
7b9d6f2
Another ticket reference
ockham Jan 11, 2024
c962fc2
Add assertion to insert_hooked_blocks test
ockham Jan 11, 2024
cf1297d
insert_hooked_blocks Test: Turn vars into class consts
ockham Jan 16, 2024
311c2e6
Add more test coverage
ockham Jan 16, 2024
ace54d4
Add ticket references to 59572
ockham Jan 16, 2024
e3df476
Amend PHPDoc
ockham Jan 25, 2024
275a96a
Change variable names to emphasize parsed block array format
ockham Jan 25, 2024
e21c8e9
Remove unnecessary test mocks
ockham Jan 25, 2024
c2912f8
Ooops
ockham Jan 25, 2024
13f1c5d
Add test coverage for filter
ockham Jan 25, 2024
b9f71ad
Add test to cover wrapping block
ockham Jan 25, 2024
30c2e97
Add assertion messages
ockham Jan 25, 2024
5015617
More assertion messages
ockham Jan 25, 2024
3092b85
Coding standards
ockham Jan 25, 2024
47e80e1
More coding standards
ockham Jan 25, 2024
9cac78c
Tweak PHPDoc
ockham Jan 25, 2024
7fdf956
More PHPDoc tweaking
ockham Jan 25, 2024
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
Prev Previous commit
Next Next commit
Set attrs to empty array
  • Loading branch information
@ockham
ockham committed Jan 25, 2024
commit afd95d03ca71bb6484abe3061f2e68f66da6eacb
1 change: 1 addition & 0 deletions src/wp-includes/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,7 @@ function insert_hooked_blocks( &$anchor_block, $relative_position, $hooked_block
foreach ( $hooked_block_types as $hooked_block_type ) {
$hooked_block = array(
Copy link
Member

@gziolo gziolo Jan 16, 2024

Choose a reason for hiding this comment

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

It's a very low-level concept as it's the shape of the parsed block. Are we sure that we want to open innerBlocks plus innerContent? What alternatives do we have here to represent the shape in something more abstract that doesn't impact performance?

Copy link
Contributor Author

@ockham ockham Jan 17, 2024

Choose a reason for hiding this comment

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

Yeah, that's a good question. I was originally only thinking to allow modifying attributes, e.g.

$hooked_block_attributes = apply_filters( "hooked_block_attributes_{$hooked_block_type}", $hooked_block_attributes, $hooked_block_type, $relative_position, $anchor_block, $context );

(Note how $hooked_block_attributes is both the first attribute and the return value. I believe we'd need to do it like that (rather than making $hooked_block_type the first argument and return value), since we'd expect filters to change attributes, while the block type is generally going to remain the same.)

There were a number of reasons against this kind of filter signature; I'll try to list them by relevance:

  1. Discussion over at Block Hooks: Set hooked block's layout attribute based on anchor block's #5811 revealed that extenders might still want to wrap their hooked block in a Group block, in order to get the layout attribute to work properly, as I was cautioned by folks more familiar with Layout block-support (in case it's impossible to add it directly to the hooked block). Additionally, we've heard of other use cases where people would like to inject e.g. two hooked blocks inside of a shared Row block parent. Allowing innerBlocks/innerContent to be set provides at least a low-level escape hatch for these use cases.
  2. Only allowing attributes to be set would raise some questions about the filter signature, if we also want to provide access to the anchor block (which we do). Would we provide the entire parsed anchor block (even though it's more inconsistent with having only the hooked block type and attributes)? Or would we also only provide the anchor block's type and attributes (which is more consistent, but lacks information about inner blocks)?
  3. We already have the parsed block format, and WP_Block class instances; I didn't want to add yet another way of specifying a block 😬

Overall, I agree that the parsed block format isn't exactly pretty; I just found it rather hard to come up with a better format or function signature that worked better. Allowing access to innerBlocks and innerContent is kind of a side effect, but not totally unwanted (see 1.).

But if you do have an idea how we could do this differently, please LMK!

Copy link
Member

@gziolo gziolo Jan 22, 2024

Choose a reason for hiding this comment

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

What if we pass the result of get_hooked_block_markup call instead? In the majority of cases, that would never change, but if plugins would like to produce a different markup, then they can use the filter to rewrite the HTML to whatever they want:

  • change the attributes
  • wrap with a parent block
  • rewrite it to any HTML they want, which is the only risky but the same issue exists with innerBlocks and innerContent as you can put there anything really

I don't think it's any different from what the API currently offers, but we operate on a much higher level without forcing folk to learn the parsed block structure.

Copy link
Contributor Author

@ockham ockham Jan 24, 2024

Choose a reason for hiding this comment

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

What if we pass the result of get_hooked_block_markup call instead?

Hmm, that would be serialized block markup, i.e. something like <!-- my/hooked-block /-->.

In the majority of cases, that would never change, but if plugins would like to produce a different markup, then they can use the filter to rewrite the HTML to whatever they want:

  • change the attributes
  • wrap with a parent block
  • rewrite it to any HTML they want, which is the only risky but the same issue exists with innerBlocks and innerContent as you can put there anything really

But even for a simple operation like changing (or adding) an attribute, they would either have to do some string/RegEx-gymnastics (which can be error-prone if they get the markup wrong), or run parse_blocks again on something that was just parsed and re-serialized.

My thinking is that we want to make it easy enough to do a "simple" (and often-requested) change like set/change attributes (with the current state of this PR: $hooked_block['attrs']['layout'] = $anchor_block['attrs']['layout'];), and a bit harder to wrap with a parent or include inner blocks; it seems reasonable to require that an extender "knows what they're doing" if they want to do something more complex like that. (If they'd like to avoid manually nesting arrays to create the parsed block structure, they could still run parse_blocks on a string literal, e.g. parse_blocks( <-- wp:group /--><div class="wp-block-group"><!-- my/hooked-block /--></div><-- /wp:group -->' ).)

I don't think it's any different from what the API currently offers, but we operate on a much higher level without forcing folk to learn the parsed block structure.

Personally, I see markup as lower-level than parsed block arrays 😅 but that might be subjective. It's true however that extenders might be familiar with (block) markup, but not necessarily with parsed block arrays.

FWIW, some of our filters in the render pipeline -- e.g. render_block or render_block_data -- also pass parsed block arrays as arguments; this was partly the inspiration here 🙂

Copy link
Member

@gziolo gziolo Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, some of our filters in the render pipeline -- e.g. render_block or render_block_data -- also pass parsed block arrays as arguments; this was partly the inspiration here 🙂

Interesting observation. In effect, let's make it clear then and use $parsed_hooked_block for clarity. It isn't that obvious when looking at render_block that exposes $block.

'blockName' => $hooked_block_type,
'attrs' => array(),
);

/**
Expand Down