Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 11 months ago

#60506 closed defect (bug) (fixed)

Block Hooks: Newly inserted anchor block doesn't ignore hooked blocks set by filter

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

Description

Per [57594], hooked blocks will be injected into modified templates and template parts. This has one side effect: If a new instance of an anchor block is inserted into the editor, it either needs its hooked blocks inserted alongside it, or at least its ignoredHookedBlocks attribute set (to prevent the Block Hooks mechanism running on the frontend, and inserting hooked blocks that aren't present in the editor, which would violate editor/frontend parity, a core tenet for Block Hooks).

https://github.com/WordPress/gutenberg/pull/58553 (which is included in GB 17.7 RC1 and thus will be in WP 5.6) partly resolves this issue by setting the ignoredHookedBlocks attribute for newly inserted anchor blocks based on the blockHooks field set in a block's block.json (and exposed via the block-types REST API endpoint).

However, that approach has one shortcoming: It is unaware of any hooked blocks added conditionally, i.e. via the hooked_block_types filter, as the latter isn't reflected in the block-types endpoint (and cannot be by design, as it can limit hooked blocks to a given context -- such as a specific template or template part).

As a result, the ignoredHookedBlocks metadata will not be set on a newly inserted anchor block for any hooked blocks that were added by the hooked_block_types filter, resulting in a violation of aforementioned parity. In practice, this means that the hooked block will not show up in the editor, but will be present in the frontend. Editing the affected template can then also be somewhat confusing. Here's a small screencast to illustrate: https://github.com/WordPress/wordpress-develop/pull/5712#issuecomment-1838758546

Change History (11)

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


12 months ago
#1

  • Keywords has-patch added

_Based on an idea by @gziolo. Alternative approach to https://github.com/WordPress/gutenberg/pull/58622._

Per https://github.com/WordPress/wordpress-develop/pull/5726, hooked blocks will be injected into modified templates and template parts. This has one side effect: If a new instance of an anchor block is inserted into the editor, it either needs its hooked blocks inserted alongside it, or at least its ignoredHookedBlocks attribute set (to prevent the Block Hooks mechanism running on the frontend, and inserting hooked blocks that aren't present in the editor, which would violate editor/frontend parity, a core tenet for Block Hooks).

https://github.com/WordPress/gutenberg/pull/58553 (which is included in GB 17.7 RC1 and thus will be in WP 5.6) partly resolves this issue by setting the ignoredHookedBlocks attribute for newly inserted anchor blocks based on the blockHooks field set in a block's block.json (and exposed via the block-types REST API endpoint).

However, that approach has one shortcoming: It is unaware of any hooked blocks added _conditionally_, i.e. via the hooked_block_types filter, as the latter isn't reflected in the block-types endpoint (and cannot be by design, as it can limit hooked blocks to a given context -- such as a specific template or template part).

As a result, the ignoredHookedBlocks metadata will _not_ be set on a newly inserted anchor block for any hooked blocks that were added by the hooked_block_types filter, resulting in a violation of aforementioned parity. In practice, this means that the hooked block will not show up in the editor, but will be present in the frontend. Editing the affected template can then also be somewhat confusing. Here's a small screencast to illustrate: https://github.com/WordPress/wordpress-develop/pull/5712#issuecomment-1838758546

One idea to work around this is to write a hooked-blocks endpoint that allows querying hooked blocks for a given anchor block _and_ for a given context. This is being explored in https://github.com/WordPress/gutenberg/pull/58622. However, this approach has various downsides:

  • It introduces a new REST API endpoint (which includes a lot of boilerplate, such as schema and permissions).
  • It will need changes on the editor side to connect the getHookedBlocks selector (introduced in https://github.com/WordPress/gutenberg/pull/58553) to the new endpoint. _We're already past the deadline for new GB features to be included in WP 6.5, so we'd likely need to request "task (blessed)" status.
  • Generally, it adds a lot of logic to the client side, whereas most of the Block Hooks logic has so far existed on the server-side only, thus leading to more fragmentation.

This PR explores an alternative, based on a suggestion by @gziolo, and prior art by @tjcafferkey for the Navigation block in https://github.com/WordPress/gutenberg/pull/57754. At its core, the idea is to decouple setting the metadata.ignoredHookedBlocks attribute from inserting hooked blocks. Instead, the attribute will only be set when storing a (modified) wp_template to the DB.

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

@Bernhard Reiter commented on PR #6087:


12 months ago
#2

As a first step, I've removed the setting of metadata.ignoredHookedBlocks from get_hooked_block_markup. As expected, all unit tests that are checking for that are now failing. I'll update them.

#3 @Bernhard Reiter
12 months ago

  • Milestone changed from Awaiting Review to 6.5

#4 follow-up: @poena
12 months ago

Hi

To make it easier for contributors and for anyone reading this in the future, can you please include information about the concept of "anchor blocks". For example by linking to the previous tickets where this was introduced.

Because right now the title and description can be read as inserting a literal block called anchor. Which at this point in time does not exist in core, but as a separate plugin.

@Bernhard Reiter commented on PR #6087:


12 months ago
#5

I have to wrap up for tonight. The basic logic is in place, but I didn't do much testing along the way, and as a result, everything is currently broken 😬

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/96308/b136d44e-11ef-45fe-9c68-4a04b8dade80

(That's not even the theme I'm using.) The error looks a bit familiar; we've had something like that in the past when _inject_theme_attribute_in_template_part_block() didn't inject the theme name into template part blocks.

Or maybe I just really screwed up parsing/reserialization 😬

#6 in reply to: ↑ 4 @Bernhard Reiter
12 months ago

Replying to poena:

Hi

To make it easier for contributors and for anyone reading this in the future, can you please include information about the concept of "anchor blocks". For example by linking to the previous tickets where this was introduced.

Because right now the title and description can be read as inserting a literal block called anchor. Which at this point in time does not exist in core, but as a separate plugin.

Right, my apologies about that. That's Block Hooks lingo -- an anchor block is the block that a hooked block chooses to be inserted next to. This is covered e.g. in this introductory post.

For example, in my Like Button block plugin's block.json, I have

"blockHooks": {
	"core/comment-template": "lastChild"
}

which will cause it to be inserted as the last child (i.e. inner block) of every Comment Template block found; meaning that in this case, the Comment Template block is the anchor block :)

@Bernhard Reiter commented on PR #6087:


12 months ago
#7

[...] everything is currently broken 😬

Ah, disregard; turns out I'd switched to a newer MySQL version, and as a result, my DB hadn't been primed. So it looks like it's actually not that terribly broken.

#8 @Bernhard Reiter
12 months ago

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

In 57627:

Block Hooks: Set ignoredHookedBlocks metadata upon saving.

Decouple hooked blocks insertion from setting the metadata.ignoredHookedBlocks attribute on anchor blocks, and perform the latter step upon saving a template or template part to the database. This ensures that anchor blocks that have been newly added to a template (or part) on the client side will also get ignoredHookedBlocks metadata set correctly, thus preserving editor/front-end parity. Hooked block insertion, on the other hand, will continue to happen upon reading a template (or part).

Props gziolo, tomjcafferkey.
Fixes #60506.

@Bernhard Reiter commented on PR #6087:


12 months ago
#9

Committed to Core in https://core.trac.wordpress.org/changeset/57627 🚀

Thank you @tjcafferkey and @gziolo! With this, the last known bug related to Blocks Hooks insertion into modified templates and parts should be fixed 😄

We'll need to tweak the Navigation block a bit now, as some of the assumptions we based https://github.com/WordPress/gutenberg/pull/57754 on are no longer valid. But that should be fine to have ready in time for Beta 2.

#10 @Bernhard Reiter
12 months ago

In 57666:

Block Hooks: Fix actual/expected order in test assertions.

In test assertions for the set_ignored_hooked_blocks_metadata function, the actual value was erroneously passed as the first argument, and the expected value as the second. This changeset rectifies this by swapping the arguments.

Follow-up [57627].
See #60506.

#11 @desrosj
11 months ago

In 57771:

Coding standards: Apply some changes after composer format.

Follow up to [57565], [57627], [57755],

See #60233, #60506, #60524.

Note: See TracTickets for help on using tickets.