Block Hooks: Set `ignoredHookedBlocks` metadata upon saving #6087

ockham · 2024-02-12T13:51:17Z

Based on an idea by @gziolo. Alternative approach to WordPress/gutenberg#58622.

Per #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).

WordPress/gutenberg#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: #5712 (comment)

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 WordPress/gutenberg#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 Block Hooks: Set ignoredHookedBlocks metada attr upon insertion gutenberg#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 WordPress/gutenberg#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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

ockham · 2024-02-12T14:10:15Z

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.

github-actions · 2024-02-12T14:19:01Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

ockham · 2024-02-12T16:44:16Z

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 😬

(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 😬

ockham · 2024-02-13T09:02:15Z

[...] 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.

src/wp-includes/default-filters.php

tjcafferkey · 2024-02-13T10:49:47Z

src/wp-includes/blocks.php

+/**
+ * Adds a list of hooked block types to an anchor block's ignored hooked block types.
+ *
+ * This function is meant for internal use only.


I think we can denote this with @internal

Ah, interesting -- I was unaware of @internal.

There are a few other functions I'd added for Block Hooks in the past that were also only meant for internal use only, and folks asked me to add this note, plus @access private (which I'm also adding here). Since I didn't add @internal to any of those, I'm inclined to omit it here as well for consistency's sake. I will however inquire about it in Core Slack 🙂

Makes sense! I think what you've done for the time being is clear too, just pointing it out incase its whats expected.

tjcafferkey · 2024-02-13T10:51:46Z

src/wp-includes/default-filters.php

@@ -752,4 +752,37 @@
 add_action( 'before_delete_post', '_wp_before_delete_font_face', 10, 2 );
 add_action( 'init', '_wp_register_default_font_collections' );

+// TODO: This should probably go somewhere else.


Perhaps this function might be best placed in block-template-utils.php

Ah, good call!

As an alternative, maybe it's worth putting directly into WP_REST_Templates_Controller after all? I was hesitant to have it directly inside a controller when I thought that templates were handled by WP_REST_Posts_Controller (as it seemed too specific to the wp_template and wp_template_part post types there), but realizing that we have a specialized controller, it might not feel so out of place there... 🤔

Yeah, that is actually a better place IMO!

Moved the function into block-template-utils.php in dfb6590 (but still using it as an action hooked into rest_after_insert_wp_template and rest_after_insert_wp_template_part, respectively).

Considering leaving as-is for now, as it's not as straight-forward to find the right spot inside the controller for running this logic. (Both create_item and update_item methods call prepare_item_for_database which does a lot of the heavy lifting, but the way it works is that it prepares a $changes object which is just a stdClass. There are some stages --- especially when first creating the template from a theme file for subsequent insertion into the DB -- where some fields are in an in-between state (e.g. still claiming to be a file-based template when we're already about to insert into the DB). This might give some misleading $context information to the filter).

src/wp-includes/default-filters.php

tjcafferkey · 2024-02-13T11:01:18Z

src/wp-includes/blocks.php

+	/** 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 ) ) {
+		return '';


I don't know what the convention is but do we have to explicitly return a value? E.g. can we not just return;?

The function that this is part of -- set_ignored_hooked_blocks_metadata -- is used as a callback arg for make_{before|after}_block_visitor. Its signature needs to be consistent with insert_hooked_blocks, which is the default value for the callback.

Previously, when insert_hooked_blocks would take care of both inserting hooked blocks and setting the anchor block's metadata.ignoredHookedBlocks attribute, it needed a way to do both. I decided to pass the anchor block as a reference, and to have insert_hooked_blocks return the markup generated for the inserted hooked blocks as a string.

It's still doing the latter; and make_{before|after}_block_visitor expects a string as a return value, which it'll happily prepend or append to the current block, respectively.

I could maybe change the visitors to allow for null or falsey return values, causing the visitor to ignore the return value 🤔 It's just that an empty string was kinda easier to do 😅

Ah I see. I think leaving it as is, is fine. My question came from a place of curiosity which this answers. Thank you!

tjcafferkey · 2024-02-13T11:01:29Z

src/wp-includes/blocks.php

+	);
+
+	// Markup for the hooked blocks has already been created (in `insert_hooked_blocks`).
+	return '';


Same as https://github.com/WordPress/wordpress-develop/pull/6087/files?diff=unified&w=0#r1487632150

(see #6087 (comment))

src/wp-includes/blocks.php

github-actions · 2024-02-13T14:52:56Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props bernhard-reiter, tomjcafferkey.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

…ert_hooked_blocks

…plate-utils.php

ockham · 2024-02-13T15:20:54Z

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 WordPress/gutenberg#57754 on are no longer valid. But that should be fine to have ready in time for Beta 2.

Update the Block Hooks mechanism as used in the Navigation block upon writing the REST API response received from a client to the DB. The Block Hooks mechanism recently underwent some overhaul (in WordPress/wordpress-develop#6087), which decoupled hooked block injection from setting the `ignoredHookedBlocks` metadata. The latter is no longer done by `insert_hooked_blocks` but by a separate function called `set_ignored_hooked_blocks_metadata` (that can be passed as a callback to the `make_{before|after}_block` visitor factories). Since the implementation of the Navigation block relied on the previous functionality, it needs to be updated to work properly: We need to run block tree traversal with `set_ignored_hooked_blocks_metadata` rather than `insert_hooked_blocks` on the template content received from the endpoint for the Navigation post type, upon persisting to the DB. Co-authored-by: ockham <[email protected]> Co-authored-by: tjcafferkey <[email protected]> Co-authored-by: gziolo <[email protected]>

ockham self-assigned this Feb 12, 2024

tjcafferkey reviewed Feb 13, 2024

View reviewed changes

src/wp-includes/default-filters.php Outdated Show resolved Hide resolved

tjcafferkey reviewed Feb 13, 2024

View reviewed changes

src/wp-includes/default-filters.php Outdated Show resolved Hide resolved

tjcafferkey reviewed Feb 13, 2024

View reviewed changes

ockham commented Feb 13, 2024

View reviewed changes

src/wp-includes/blocks.php Outdated Show resolved Hide resolved

ockham mentioned this pull request Feb 13, 2024

Revert "Block Hooks: Set ignoredHookedBlocks metada attr upon insertion" WordPress/gutenberg#58969

Merged

ockham marked this pull request as ready for review February 13, 2024 14:52

ockham added 15 commits February 13, 2024 15:53

Don't set ignoredHookedBlocks metadata upon read

b5aaa44

Don't pass anchor_block by reference

2e28810

Update PHPDoc

40fa3fb

Add new set_ignored_hooked_blocks_metadata() function

1f0d087

Whitespace

c434d75

Fix first batch of unit tests

b63d367

Fix remaining unit tests

36c0fc3

There can never be enough callbacks

2358241

Tweak set_ignored_hooked_blocks_metadata to match callback signature

0330473

Wire it all up

e6eea6e

Whitespace

eb1daa1

Bail early

d057ece

Actually update post

ab6a8c9

Use correct action

998147f

Add action for template parts

1296344

ockham added 14 commits February 13, 2024 15:53

Add note on action vs filter

27724fc

Clarify comment

012004b

Fix more unit tests

27a2ec8

Start adding test coverage for set_ignored_hooked_blocks_metadata

12ccd0d

Add more test coverage and a small fix

1128b5f

Add test coverage for hooked block added by filter

5817b49

Add test to cover context-aware filter

60b2732

Remove obsolete comment about post_filtered_content

61847cb

Move set_ignored_hooked_blocks_metadata function definition below ins…

712260f

…ert_hooked_blocks

Inline get_hooked_block_markup

7c7f01e

Coding Standards in test

f4f856b

Add test coverage to verify hooked blocks aren't added if ignored

7d43f21

Move set_ignored_hooked_blocks_metadata_upon_rest_insert to block-tem…

3c9aa4b

…plate-utils.php

Rename to inject_ignored_hooked_blocks_metadata_attributes

51832fe

ockham force-pushed the update/set-ignored-hooked-blocks-metadata-upon-saving branch from 45914d4 to 51832fe Compare February 13, 2024 14:53

ockham closed this Feb 13, 2024

ockham deleted the update/set-ignored-hooked-blocks-metadata-upon-saving branch February 13, 2024 15:23

This was referenced Feb 13, 2024

Block Hooks: Add controller to expose hooked blocks for a given anchor block WordPress/gutenberg#58622

Closed

Block Hooks: Fix in Navigation block WordPress/gutenberg#59021

Merged

Epic: Block Hooks Features for WP 6.5 WordPress/gutenberg#54904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Hooks: Set `ignoredHookedBlocks` metadata upon saving #6087

Block Hooks: Set `ignoredHookedBlocks` metadata upon saving #6087

ockham commented Feb 12, 2024 •

edited

Loading

ockham commented Feb 12, 2024

github-actions bot commented Feb 12, 2024

ockham commented Feb 12, 2024

ockham commented Feb 13, 2024

tjcafferkey Feb 13, 2024

ockham Feb 13, 2024

tjcafferkey Feb 13, 2024

tjcafferkey Feb 13, 2024

ockham Feb 13, 2024

tjcafferkey Feb 13, 2024

ockham Feb 13, 2024

tjcafferkey Feb 13, 2024

ockham Feb 13, 2024 •

edited

Loading

tjcafferkey Feb 13, 2024

tjcafferkey Feb 13, 2024

ockham Feb 13, 2024

github-actions bot commented Feb 13, 2024

ockham commented Feb 13, 2024

Block Hooks: Set ignoredHookedBlocks metadata upon saving #6087

Block Hooks: Set ignoredHookedBlocks metadata upon saving #6087

Conversation

ockham commented Feb 12, 2024 • edited Loading

ockham commented Feb 12, 2024

github-actions bot commented Feb 12, 2024

Test using WordPress Playground

Some things to be aware of

ockham commented Feb 12, 2024

ockham commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 13, 2024

ockham commented Feb 13, 2024

Block Hooks: Set `ignoredHookedBlocks` metadata upon saving #6087

Block Hooks: Set `ignoredHookedBlocks` metadata upon saving #6087

ockham commented Feb 12, 2024 •

edited

Loading

ockham Feb 13, 2024 •

edited

Loading