Make WordPress Core

Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#60574 closed task (blessed) (fixed)

Block Hooks: Add new `hooked_block` filter

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

Description

In [57354], a new hooked_block_{$block_type} filter was introduced that allows extenders to specify attributes and inner blocks for hooked blocks.

It was pointed out that for filters with a dynamic name portion like this, it is customary to have a "non-dynamic" counterpart filter (i.e. hooked_block, in this case).

There are a few concerns around the implications for the function signatures for each filter. We can discuss them here (or on a PR that implements this feature) and decide how to proceed.

Change History (10)

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


2 months ago
#1

  • Keywords has-patch added

In `[57354] (https://github.com/WordPress/wordpress-develop/pull/5835), a new hooked_block_{$block_type}` filter was introduced that allowed extenders to specify attributes and inner blocks for hooked blocks.

@swissspidy pointed out that for filters with a dynamic name portion like this, it is customary to have a "non-dynamic" counterpart filter (i.e. hooked_block, in this case).

There are a few concerns around the implications for the function signatures for each filter. We can discuss them here (or on a PR that implements this feature) and decide how to proceed.

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

@Bernhard Reiter commented on PR #6136:


2 months ago
#2

To quote my own concerns:

So the signature for the non-dynamic version would have to be something like

apply_filters( 'hooked_block', $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context )

which then raises the question if we also need to adjust the dynamic version to include the type (for consistency)?

apply_filters( "hooked_block_{$hooked_block_type}", $parsed_hooked_block, $hooked_block_type, $relative_position, $parsed_anchor_block, $context )

Note that this duplicates $hooked_block_type (which is already present in the filter name) in a somewhat ugly way; plus given the way that filters work, that arg ends up in the position _after_ $parsed_hooked_block, which is also a bit awkward.

#3 @Bernhard Reiter
2 months ago

@swissspidy Apologies, took me a bit to get back to this.

LMK if you think we should set the milestone for this to 6.5 :)

This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.


2 months ago

#5 @swissspidy
2 months ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 6.5
  • Type changed from enhancement to task (blessed)
  • Version set to trunk

Re-sharing the gist of that Slack conversation here for improved visibility, as Slack requires signup.

My main input was that it's common for dynamic filters in WordPress to have a non-dynamic counterpart, see e.g. pre_option_{$option} and pre_option filters.

The question about the signature for the dynamic version is really independent of that.

Usually when we have a dynamic filter such as hooked_block_{$hooked_block_type}, $hooked_block_type is often then added as an additional argument to the filter. This makes it easier for devs to add a filter for multiple block types and then figuring out which block type the current invocation is for.

As for the exact signature, the argument could also come in last, e.g. apply_filters( "hooked_block_{$hooked_block_type}", $parsed_hooked_block, $relative_position, $parsed_anchor_block, $context ) and apply_filters( "hooked_block", $parsed_hooked_block, $relative_position, $parsed_anchor_block, $context, $hooked_block_type ).

That seems like a reasonable change to me.

Moving to milestone for consideration as a blessed task, as it's a new filter in 6.5 so now is the best opportunity to make bigger changes to it. However, adding a new filter & adding a new argument at the end is a non breaking change that could also be added later as well.

@swissspidy commented on PR #6136:


2 months ago
#6

Note that this duplicates $hooked_block_type (which is already present in the filter name)

"Duplicating" the variable again is the whole point of it :)

Just left this as a comment on the ticket as well, but about moving $hooked_block_type to the end of the arguments list? Would that be less awkward?

@Bernhard Reiter commented on PR #6136:


2 months ago
#7

Just left this as a comment on the ticket as well, but about moving $hooked_block_type to the end of the arguments list? Would that be less awkward?

Given the choice, I think I'd swallow the pill and would put it in the second position (i.e. after $parsed_hooked_block, as per the current diff).

The chief reason being that substituting for parameters, it's at least somewhat natural to read:

..., $hooked_block_type, $relative_position, $parsed_anchor_block, ...

becomes e.g.

..., 'ockham/like-button`, `after`, `core/post-content`,...

which does exactly what it says: It inserts a Like Button block after the Post Content block.

(Furthermore, we have a convention of sorts that $context is the last argument in Block Hooks related functions and filters, e.g. in the `hooked_block_types` filter (introduced in 6.4).)

#8 @swissspidy
2 months ago

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

In 57660:

Block Hooks: Introduce a new hooked_block filter.

This is a counterpart to the dynamic hooked_block_{$block_type} filter introduced in [57354],
which makes it easier to modify all hooked blocks prior to insertion.

Also adds the hooked block type as an additional argument to both filters for consistency.

Props bernhard-reiter, swissspidy.
Fixes #60574.

@Bernhard Reiter commented on PR #6136:


2 months ago
#10

Thank you for landing this @swissspidy!

Note: See TracTickets for help on using tickets.