Block Hooks: Incorrect context passed when setting ignored hooked blocks metadata

[Bernhard Reiter]

Since [57790], the rest_pre_insert_wp_template and rest_pre_insert_wp_template_part hooks in the Templates Controller are used to call inject_ignored_hooked_blocks_metadata_attributes(), which in turn sets the ignoredHookedBlocks metadata attribute on anchor blocks upon writing to the DB.

The latter function applies the hooked_block_types, hooked_block, and hooked_block_{$hooked_block_type} filters, in order to inform which blocks should be hooked to a given anchor block. Each of these filters is given a $context argument, which in the case of templates (and template parts, respectively) is the containing WP_Block_Template object.

Since the rest_pre_insert_-prefixed filters only pass a stdClass object for the relevant $changes that need to be made to the relevant template (or template part), $context is constructed by fetching the existing template (or part) via get_block_template() (using the template/part ID present in the network $request).

This isn't quite how things should be: hooked_block_types, hooked_block, and hooked_block_{$hooked_block_type} should be given a $context object that reflects the template (part) that is about to be persisted to the database, not the one that's currently stored there. A filter might e.g. look for the presence of a certain block in the template's content; that block might not have been present in the previous version of a template (stored in the DB) but might've been added by the user in the editor (and will be part of the payload that's used to update the DB).

• [ ] Add unit test coverage to demonstrate/cover issue.
• [ ] Fix (probably by casting $changes to WP_Block_Template).

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

cc/ @tjcafferkey

[swissspidy]

@bernhard-reiter could you please clarify the intended milestone here? Thanks!

TypeCasting
I’m not sure we can simply typecast $changes because the posts stdClass and WP_Block_Template have different keys. For example WP_Block_Template has ->content rather than ->post_content

Block template utils
get_block_template() - As already discussed this will build a template object without incoming changes being applied.
_build_block_template_result_from_post() - For templates loaded from the file system this will fail since they dont have IDs.
_build_block_template_result_from_file() - Same problem as get_block_template(), it wont have incoming changes.

Potential solution

In inject_ignored_hooked_blocks_metadata_attributes the stdClass that is $post has limited data unlike the WP_Post object, but it has what we _need_.

One solution I think we could do is build the template using get_block_template and just overwrite that object data with out incoming changes:

$post_to_template_key_map = array(
                'post_content' => 'content',
                'post_title'   => 'title',
                'post_excerpt' => 'description',
                'post_type'    => 'type',
                'post_status'  => 'status',
        );

        // We need to overwrite the built template object with the incoming one from the request.
        // This is so we can provide the correct context to the Block Hooks API which expects the `WP_Block_Template` object.
        foreach ( $post_to_template_key_map as $post_key => $template_key ) {
                if ( isset( $post->$post_key ) ) {
                        $template->{$template_key} = $post->$post_key;
                }
        }

Thank you for your investigation! Good stuff 😄

Potential solution In inject_ignored_hooked_blocks_metadata_attributes the stdClass that is $post has limited data unlike the WP_Post object, but it has what we _need_.

One solution I think we could do is build the template using get_block_template and just overwrite that object data with out incoming changes:

	$post_to_template_key_map = array(
		'post_content' => 'content',
		'post_title'   => 'title',
		'post_excerpt' => 'description',
		'post_type'    => 'type',
		'post_status'  => 'status',
	);

	// We need to overwrite the built template object with the incoming one from the request.
	// This is so we can provide the correct context with up-to-date data on it to the Block Hooks API which expects the `WP_Block_Template` object.
	foreach ( $post_to_template_key_map as $post_key => $template_key ) {
		if ( isset( $post->$post_key ) ) {
			$template->{$template_key} = $post->$post_key;
		}
	}

This makes sense. I wish we didn't need to call get_block_template(), as it ​adds some unneeded overhead (which we've partly worked around by suppressing hooked blocks by adding __return_empty_array as the "last" hooked_block_types filter).

Plus, ironically, when the $changes stdClass object is built by the Templates controller's ​`prepare_item_for_database()` method, it's calling get_block_template() to fill in some of its fields. And now we have to call it again, only to interpolate some of the returned WP_Post object's fields with values from $changes 😅 I wish there was some kind of shortcut...

I gave this some more thought. I think ​your approach (​https://github.com/WordPress/wordpress-develop/pull/6254/commits/6de2de8bd2bb8ba2da1d7d3f7229a0e451ad98d0) is fine; the one thing I'm still a bit worried about is that we might miss some data.

The crux is that we want a WP_Block_Template object that reflects all the changes that are being made to it based on the incoming network request -- essentially, what the template object in the DB will look like _after_ those changes have been applied.

Now preparing those changes is exactly what
​`WP_REST_Templates_Controller::prepare_item_for_database` does; but unfortunately, we can't use the stdClass format it returns.

Creating a WP_Block_Template object based on what's currently in the DB, and then applying those changes -- as you're doing -- then seems like a reasonable approximation. The one thing we should probably do is increase test coverage to make sure that our template object reflects them. To start, we can look at all the fields of the $request object that WP_REST_Templates_Controller::prepare_item_for_database, and mock them in the $request object that we create for the test.

We can then look at all the fields in the $changes return value that WP_REST_Templates_Controller::prepare_item_for_database is setting, and make sure that they're updated in our template object as well. (There are some subtleties, e.g. the meta_input and tax_input fields are there to update the post meta and terms, respectively.)

Finally, for the WP_Post to WP_Block_Template correspondence, we should probably also look at _build_block_template_result_from_post to make sure we're carrying over values correctly.

@ockham could you update the ticket's intended milestone when you get a chance? thanks!

[Bernhard Reiter]

Nominating this for 6.5. It's a regression that was introduced during the 6.5 cycle. It has the potential of breaking hooked blocks that are added by a filter that depends on the $context argument.

$context should be representative of what is about to be persisted to the database, not what is already there. Not only that but we need to ensure the $context is consistent in its type. To that end we need to build up a WP_Block_Template object for the incoming $post (which has a type of stdClass).

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

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/6278.

[Bernhard Reiter]

It was ​discussed today in Slack that this isn't going to be part of 6.5.0 but ​into 6.5.1 instead.

I don't see a 6.5.1 milestone available in Trac yet, so I'll leave it set to 6.5 for now. Feel free to adjust!

Rebased.

I've added some more unit test coverage, moved tests for inject_ignored_hooked_blocks_metadata_attributes to a separate file, and updated the PR description to capture the changes. This should now be ready for a proper review!

cc/ @gziolo

I need to have another look at the code, because the refactoring applied has a lot of nuances. However, the approach taken makes perfect sense, so it looks nearly ready to go.

Rebased to include the ​`docker-compose` fix required to make tests pass more reliably.

[Bernhard Reiter]

In 57919:

Block Hooks: Pass correct context to filters.

The $context argument passed to filters such as hooked_block_types, hooked_block, and hooked_block_{$hooked_block_type} allows them to conditionally insert a hooked block. If the anchor block is contained in a template or template part, $context will be set to a WP_Block_Template object reflecting that template or part.

The aforementioned filters are applied when hooked block insertion is run upon reading a template (or part) from the DB (and before sending the template/part content with hooked blocks inserted over the REST API to the client), but also upon writing to the DB, as that's when the ignoredHookedBlocks metadata attribute is set.

Prior to this changeset, the $context passed to Block Hooks related filters in the latter case reflected the template/part that was already stored in the database (if any), which is a bug; instead, it needs to reflect the template/part that will result from the incoming POST network request that will trigger a database update.

Those incoming changes are encapsulated in the $changes argument passed to the reset_pre_insert_template and reset_pre_insert_template_part filters, respectively, and thus to the inject_ignored_hooked_blocks_metadata_attributes function that is hooked to them. $changes is of type stdClass and only contains the fields that need to be updated. That means that in order to create a WP_Block_Template object, a two-step process is needed:

• Emulate what the updated wp_template or wp_template_part post object in the database will look like by merging $changes on top of the existing $post object fetched from the DB, or from the theme's block template (part) file, if any.
• Create a WP_Block_Template from the resulting object.

To achieve the latter, a new helper method (_build_block_template_object_from_post_object) is extracted from the existing _build_block_template_result_from_post function. (The latter cannot be used directly as it includes a few database calls that will fail if no post object for the template has existed yet in the database.)

While somewhat complicated to implement, the overall change allows for better separation of concerns and isolation of entities. This is visible e.g. in the fact that inject_ignored_hooked_blocks_metadata_attributes no longer requires a $request argument, which is reflected by unit tests no longer needing to create a $request object to pass to it, thus decoupling the function from the templates endpoint controller.

Unit tests for inject_ignored_hooked_blocks_metadata_attributes have been moved to a new, separate file. Test coverage has been added such that now, all three relevant scenarios are covered:

• The template doesn't exist in the DB, nor is there a block theme template file for it.
• The template doesn't exist in the DB, but there is a block theme template file for it.
• The template already exists in the DB.

Those scenarios also correspond to the logical branching inside WP_REST_Templates_Controller::prepare_item_for_database, which is where inject_ignored_hooked_blocks_metadata_attributes gets its data from.

Props tomjcafferkey, bernhard-reiter, gziolo, swissspidy.
Fixes #60754.

Committed to Core trunk in https://core.trac.wordpress.org/changeset/57919.
Will re-open the ticket to nominate for backporting to the 6.5 branch.

[Bernhard Reiter]

Reopening to request approval from another Core Committer to backport to the 6.5 branch.

[davidbaumwald]

Milestone renamed

[jorbin]

@gziolo, @swissspidy since both of you got props and are more familiar with this code, would one of you be able to get the second review for this change to be backported to 6.5?

[gziolo]

I reviewed the changes earlier and it all looked good. All the necessary use cases I could think of were covered with tests. I’m fine landing it in the minor release.

[jorbin]

Thanks @gziolo!

[Bernhard Reiter]

In 58041:

Block Hooks: Pass correct context to filters.

The $context argument passed to filters such as hooked_block_types, hooked_block, and hooked_block_{$hooked_block_type} allows them to conditionally insert a hooked block. If the anchor block is contained in a template or template part, $context will be set to a WP_Block_Template object reflecting that template or part.

The aforementioned filters are applied when hooked block insertion is run upon reading a template (or part) from the DB (and before sending the template/part content with hooked blocks inserted over the REST API to the client), but also upon writing to the DB, as that's when the ignoredHookedBlocks metadata attribute is set.

Prior to this changeset, the $context passed to Block Hooks related filters in the latter case reflected the template/part that was already stored in the database (if any), which is a bug; instead, it needs to reflect the template/part that will result from the incoming POST network request that will trigger a database update.

Those incoming changes are encapsulated in the $changes argument passed to the reset_pre_insert_template and reset_pre_insert_template_part filters, respectively, and thus to the inject_ignored_hooked_blocks_metadata_attributes function that is hooked to them. $changes is of type stdClass and only contains the fields that need to be updated. That means that in order to create a WP_Block_Template object, a two-step process is needed:

• Emulate what the updated wp_template or wp_template_part post object in the database will look like by merging $changes on top of the existing $post object fetched from the DB, or from the theme's block template (part) file, if any.
• Create a WP_Block_Template from the resulting object.

To achieve the latter, a new helper method (_build_block_template_object_from_post_object) is extracted from the existing _build_block_template_result_from_post function. (The latter cannot be used directly as it includes a few database calls that will fail if no post object for the template has existed yet in the database.)

While somewhat complicated to implement, the overall change allows for better separation of concerns and isolation of entities. This is visible e.g. in the fact that inject_ignored_hooked_blocks_metadata_attributes no longer requires a $request argument, which is reflected by unit tests no longer needing to create a $request object to pass to it, thus decoupling the function from the templates endpoint controller.

Unit tests for inject_ignored_hooked_blocks_metadata_attributes have been moved to a new, separate file. Test coverage has been added such that now, all three relevant scenarios are covered:

• The template doesn't exist in the DB, nor is there a block theme template file for it.
• The template doesn't exist in the DB, but there is a block theme template file for it.
• The template already exists in the DB.

Those scenarios also correspond to the logical branching inside WP_REST_Templates_Controller::prepare_item_for_database, which is where inject_ignored_hooked_blocks_metadata_attributes gets its data from.

Reviewed by gziolo.
Merges [57919] to the to the 6.5 branch.

Props tomjcafferkey, bernhard-reiter, gziolo, swissspidy.
Fixes #60754.

Backported to the 6.5 branch in https://core.trac.wordpress.org/changeset/58041.

[Bernhard Reiter]

Looks like we'll need to update a few @since PHPDoc fields.

[Bernhard Reiter]

In 58042:

Block Hooks: Fix @since and deprecated versions.

Two @since PHPDoc fields, and the version argument to one _deprecated_argument() incorrectly stated 6.5.1 as the relevant WordPress version where a change was introduced.

This changeset fixes them by setting them to 6.5.3 instead.

Follow-up to [57919].
See #60754.

[Bernhard Reiter]

Replying to Bernhard Reiter:

In 58042:

Block Hooks: Fix @since and deprecated versions.

Two @since PHPDoc fields, and the version argument to one _deprecated_argument() incorrectly stated 6.5.1 as the relevant WordPress version where a change was introduced.

This changeset fixes them by setting them to 6.5.3 instead.

Follow-up to [57919].
See #60754.

Do I need sign-off by another Core Committer for backporting this to the 6.5 branch?

[swissspidy]

Consider it signed off :)

[Bernhard Reiter]

In 58043:

Block Hooks: Fix @since and deprecated versions.

Two @since PHPDoc fields, and the version argument to one _deprecated_argument() incorrectly stated 6.5.1 as the relevant WordPress version where a change was introduced.

This changeset fixes them by setting them to 6.5.3 instead.

Reviewed by swissspidy.
Merges [58042] to the to the 6.5 branch.

Follow-up to [58041].
See #60754.