Make WordPress Core

Opened 8 weeks ago

Closed 8 weeks ago

Last modified 7 weeks ago

#60580 closed task (blessed) (fixed)

Block Hooks: Allow hooked_block filter(s) to return null

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by:
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Editor Keywords: has-patch has-unit-tests needs-refresh
Focuses: Cc:

Description (last modified by Bernhard Reiter)

The `hooked_block_types` filter allows extenders to conditionally insert a block, based on the anchor block's type.

However, it's currently not possible to base the criterion for insertion on the value of an attribute of the anchor block.

One fairly straight-forward way to implement this would be to allow the hooked_block (and hooked_block_{$hooked_block_type}) filter -- which has access to the entire parsed anchor block, i.e. including attributes -- to return null, which Block Hooks could then interpret as "do not render hooked block". (This would avoid changing the hooked_block_types filter's signature in an awkward way.)

Change History (20)

#1 @Bernhard Reiter
8 weeks ago

  • Description modified (diff)

#2 @Bernhard Reiter
8 weeks ago

One thing to consider when implementing this is that it somewhat muddles the invariant that only the hooked_block_types filter can change which hooked blocks are slated for insertion, whereas the hooked_block (and hooked_block_{$hooked_block_type}) filter cannot -- the latter have so far only been able to modify a given hooked block's attributes, inner blocks, etc.

This might be okay. We just need to be mindful of correctly setting ignoredHookedBlocks if hooked_block or hooked_block_{$hooked_block_type} returns null.

This ticket was mentioned in PR #6143 on WordPress/wordpress-develop by @Bernhard Reiter.


8 weeks ago
#3

  • Keywords has-patch has-unit-tests added

The `hooked_block_types` filter allows extenders to conditionally insert a block, based on the anchor block's type.

However, it's currently not possible to base the criterion for insertion on the value of an attribute of the anchor block.

One fairly straight-forward way to implement this would be to allow the hooked_block (and hooked_block_{$hooked_block_type}) filter -- which has access to the entire parsed anchor block, i.e. including attributes -- to return null, which Block Hooks could then interpret as "do not render hooked block". (This would avoid changing the hooked_block_types filter's signature in an awkward way.)

Trac ticket: https://core.trac.wordpress.org/ticket/60580

@swissspidy commented on PR #6143:


8 weeks ago
#4

This would avoid changing the hooked_block_types filter's signature in an awkward way

You still need to change the signature to indicate the value can be null or an array

@Bernhard Reiter commented on PR #6143:


8 weeks ago
#5

This would avoid changing the hooked_block_types filter's signature in an awkward way

You still need to change the signature to indicate the value can be null or an array

What I meant is I don't need to add a $parsed_anchor_block arg to `hooked_block_types` (when it already has $anchor_block_type).

It's true that I need to allow for hooked_block and hooked_block_{$hooked_block_type} to return null, but that's less of a problem -- they've only been introduced during the 6.5 cycle (unlike hooked_block_types, which has been around since 6.4).

#6 @Bernhard Reiter
8 weeks ago

  • Milestone changed from Awaiting Review to 6.5
  • Version set to trunk

Tentatively setting the milestone to 6.5.

This request has been brought up independently by two people now (@gziolo and https://github.com/joshuatf for who I couldn't find a WP.org profile), so I think it makes sense to add this. It's a fairly contained change to a filter that was introduced during the 6.5 cycle.

#7 @Bernhard Reiter
8 weeks ago

@swissspidy Think this can go into 6.5? If so, does it need to be set to "task (blessed)", and can I do so myself, or do I need someone else to sign off on this?

@Bernhard Reiter commented on PR #6143:


8 weeks ago
#8

FYI @tjcafferkey -- if you'd like to give this a review 😄

#9 follow-up: @swissspidy
8 weeks ago

Since these are new filters I'd prefer to have theme correct from the beginning, so yeah I'm fine with adding this. Are you able to finish this up before beta 2?

Please note that you still need to update the docblock to indicate that $parsed_hooked_block can be an array or null, as per my comment on the PR.

Also, it's @joshuatf on here as well.

#10 @swissspidy
8 weeks ago

  • Component changed from General to Editor
  • Keywords needs-refresh added
  • Type changed from enhancement to task (blessed)

@swissspidy commented on PR #6143:


8 weeks ago
#11

@param array|null $parsed_hooked_block The parsed block array for the given hooked block type or null to remove the block.

@Bernhard Reiter commented on PR #6143:


8 weeks ago
#12

@param array|null $parsed_hooked_block The parsed block array for the given hooked block type or null to remove the block.

@swissspidy Thank you, good point! Added in 556646426d. (I opted to write "to suppress the block" rather than "remove", as the latter would sound a bit like the block has already been inserted and is being removed. LMK if that's okay, or if you'd prefer "remove" after all.)

#13 in reply to: ↑ 9 @Bernhard Reiter
8 weeks ago

Replying to swissspidy:

Since these are new filters I'd prefer to have theme correct from the beginning, so yeah I'm fine with adding this. Are you able to finish this up before beta 2?

From my POV, it's pretty much ready. I've modified both hooked block insertion and setting ignoredHookedBlocks, and covered both with unit tests.

Please note that you still need to update the docblock to indicate that $parsed_hooked_block can be an array or null, as per my comment on the PR.

Thank you! I saw it there and pushed a commit to address.

Also, it's @joshuatf on here as well.

Great find! I should've tried that, rather than just Googling.

@Bernhard Reiter commented on PR #6143:


8 weeks ago
#14

Rebased.

@tomjcafferkey commented on PR #6143:


8 weeks ago
#15

FYI @tjcafferkey -- if you'd like to give this a review 😄

I'm going to try and review this today if required 😄

@swissspidy commented on PR #6143:


8 weeks ago
#16

@tjcafferkey If you'd like to, the sooner the better - beta 2 release party kicks off in around 2hrs.

#17 follow-up: @swissspidy
8 weeks ago

  • Resolution set to fixed
  • Status changed from new to closed

#19 in reply to: ↑ 17 @Bernhard Reiter
8 weeks ago

Replying to swissspidy:

Fixed in https://core.trac.wordpress.org/changeset/57668

Thank you @swissspidy! (Silly me forgot the # before the ticket number in the commit msg.)

#20 @Bernhard Reiter
7 weeks ago

In 57676:

Block Hooks: Make test a bit easier to read.

Follow-up [57668].
Props gziolo.
See #60580.

Note: See TracTickets for help on using tickets.