Block Hooks: Pass correct context to filters #6254

ockham · 2024-03-12T11:19:08Z

Add unit test coverage to demonstrate/cover issue.
Fix (probably by ~~casting $changes to WP_Block_Template~~ building. WP_Block_Template object from $changes).
Transform object comparison in unit test to individual assertions! One for each field. (Like we had before ce3c67b. Will require adding a lot of individual assertions.)
Add unit test to the controller. This will allow us to provide a sparse $request (whose missing fields will be set by the controller), rather than a "full" $changes object, as we have to do in the block-template-utils.php unit test.
- To allow for missing fields and emulate the template that will result from the database write, we should ideally "merge" the data from $changes into the existing $template. (Done in ac59c76.)
Add unit tests for newly created template (that doesn't exist in the DB yet). Both for controller, and for the isolated filter.
Add unit test for template parts to block-template-utils.php. (Cover setting of area!)

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

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.

…ters

ockham · 2024-03-12T11:19:21Z

cc/ @tjcafferkey

ockham · 2024-03-12T11:20:13Z

tests/phpunit/tests/block-template-utils.php

+		$context           = $args[0][3];
+
+		$this->assertSame( 'tests/anchor-block', $anchor_block_type );
+		$this->assertInstanceOf( 'WP_Block_Template', $context ); // FIXME: Currently failing :/


This one threw me off; it's currently failing, saying $context is null.

ockham · 2024-03-12T11:21:19Z

tests/phpunit/tests/block-template-utils.php

+		$this->assertSame(
+			$changes->post_content,
+			$context->post_content,
+			'The context passed to the hooked_block_types filter doesn\'t match the template changes.'
+		);


This one is expected to fail currently; I'd expect the actual argument to be Content (which is the post_content set up in the test's template object fixtures).

This test will always fail unless changed to $context->content since WP_Block_Template doesn't have a post_content property which is the expected context here.

github-actions · 2024-03-12T11:32:24Z

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-03-12T12:00:23Z

tests/phpunit/tests/block-template-utils.php

@@ -412,7 +412,7 @@ public function test_wp_generate_block_templates_export_file() {
 	public function test_inject_ignored_hooked_blocks_metadata_attributes_into_template() {
 		global $wp_current_filter;
 		// Mock currently set filter. The $wp_current_filter global is reset during teardown by
-		// WP_UnitTestCase_Base::_restore_hooks() in tests/phpunit/includes/abstract-testcase.php.
+		// the unit test base class.


This is in response to this comment:

Nit for [57799]: don't see a reason for mentioning the tests/phpunit/includes/abstract-testcase.php file path in the new comment there. That will just become stale with any refactor, and any IDE will allow you to quickly jump to the definition. But it is what it is :)

tjcafferkey · 2024-03-13T11:15:14Z

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 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;
		}
	}

ockham · 2024-03-13T11:23:29Z

src/wp-includes/block-template-utils.php

@@ -1468,6 +1468,22 @@ function inject_ignored_hooked_blocks_metadata_attributes( $post, $request ) {
 	$template = $request['id'] ? get_block_template( $request['id'], $post_type ) : null;


This call to get_block_template results in an extra invocation of the hooked_block_types filter.

As @tjcafferkey has mentioned in DM to me, this is visible in the newly added unit test, where the mock filter is run by the Block Hooks logic which is attempting insertion of hooked blocks into the mock template's content (which is just a Content string literal, i.e. no blocks).

ockham · 2024-03-13T11:32:25Z

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

ockham · 2024-03-13T14:12:39Z

tests/phpunit/tests/block-template-utils.php

@@ -24,7 +24,7 @@ public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
 				'post_type'    => 'wp_template',
 				'post_name'    => 'my_template',
 				'post_title'   => 'My Template',
-				'post_content' => 'Content',
+				'post_content' => '<!-- wp:tests/anchor-block -->Hello<!-- /wp:tests/anchor-block -->',


I'd like to keep these distinct from the updated value that "comes in from the wire" in the new unit test. Otherwise, the test loses some value, as we won't be able to tell if the correct template object was given to the filter (i.e. one that's based on the $changes or the one that was already present in the DB).

If we really need a block here to make sure it's traversed, we can maybe add a different one; although I'd hope we can maybe just revert to Content and instead change the test (so it uses end( $args ) to look at the arguments from the last function call, rather than $args[0]).

instead change the test (so it uses end( $args ) to look at the arguments from the last function call, rather than $args[0]).

This would work and satisfy the test.

Done in d5d8b9a

ockham · 2024-03-13T15:15:25Z

I gave this some more thought. I think your approach (6de2de8) 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.

src/wp-includes/block-template-utils.php

tjcafferkey · 2024-03-14T08:29:41Z

src/wp-includes/block-template-utils.php

@@ -1468,6 +1468,37 @@ function inject_ignored_hooked_blocks_metadata_attributes( $post, $request ) {
 	$template = $request['id'] ? get_block_template( $request['id'], $post_type ) : null;
 	remove_filter( 'hooked_block_types', '__return_empty_array', 99999 );

+	if ( $template ) {
+		// TODO: Should maybe make this static. E.g. in the Templates Controller?


Hmm, for my own benefit can you explain how you see this TODO being implemented a little more please? I'm not quite following.

I mostly meant that it'd be nice if the mappings were (more) publicly available, since I ended up duplicating them in the unit test.

(Making them static was a poor choice of words on my part, where I conflated the means with the end.)

I realize now that a publicly available mappings array is probably not totally what we'd want, since there are some fields for which there isn't a 1:1 mapping from our $changes object to WP_Block_Template, so it'd have to be a function at least.

In the long run, this might be something we could use inside of _build_block_template_result_from_post (it'd need some adjusting though to account for the differences between mapping from WP_Post -- rather than from our $changes stdClass -- to WP_Block_Template). This also means that it should not be a static member of the Templates controller after all, as functions such as _build_block_template_result_from_post are clearly lower-level than the controller.

I'll rephrase the TODO comment accordingly.

ockham · 2024-03-15T09:43:29Z

src/wp-includes/block-template-utils.php

+	// We also need to mimic terms and meta for the post based on the corresponding
+	// `terms_input` and `meta_input` properties in the changes object.
+
+	$terms_filter = function( $terms, $post_id, $taxonomy ) use ( $changes ) {
+		if ( 'wp_theme' !== $taxonomy || ! isset( $changes->tax_input['wp_theme'] ) ) {
+			return $terms;
+		}
+
+		// TODO: Verify it's not an error object.
+		// TODO: Verify that $post_id matches (or isn't set).
+		$term       = new stdClass;
+		$term->name = $changes->tax_input['wp_theme'];
+
+		$term = new WP_Term( $term );
+		return array( $term );
+	};
+
+	$meta_filter = function( $value, $post_id, $meta_key, $single ) use ( $changes ) {
+		if ( 'origin' === $meta_key && isset( $changes->meta_input['origin'] ) ) {
+			return $single ? $changes->meta_input['origin'] : array( $changes->meta_input['origin'] );
+		}
+
+		if ( 'is_wp_suggestion' === $meta_key && isset( $changes->meta_input['is_wp_suggestion'] ) ) {
+			return $single ? $changes->meta_input['is_wp_suggestion'] : array( $changes->meta_input['is_wp_suggestion'] );
+		}
+
+		return $value;
+	};
+
+	add_filter( 'get_the_terms', $terms_filter, 10, 3 );
+	add_filter( 'get_post_metadata', $meta_filter, 10, 4 );


This is a bit too wild, and I don’t think I want to land it like this. However, it has helped inform a strategy in that it has made clear that a WP_Block_Template needs more information than is solely contained in a WP_Post (which it fetches from meta and terms). This becomes a problem when we try to pass $changes to _build_block_template_result_from_post, as it doesn’t have that meta and terms in the DB available yet; instead, it uses the meta_input and tax_input fields to set those upon insertion into the DB.

Let’s extract a helper function from _build_block_template_result_from_post that acknowledges this and accepts as its arguments a WP_Post, and additionally, all the required information that’s not included in it, i.e. a $theme, an $origin, an $is_wp_suggestion, etc. That way, we’ll have a function that accurately represents the mapping from WP_Post object to WP_Block_Template; and in our inject_ignored_hooked_blocks_metadata_attributes, we can provide the additional required arguments by reading them from meta_input and tax_input.

gziolo · 2024-03-18T11:09:34Z

src/wp-includes/block-template-utils.php

+		$post = get_post( $changes->ID );
+	} else {
+		// This means that there's not post for this template in the DB yet.
+		$post = new stdClass; // Is this correct? Might instead want to init a WP_Post with some fields set.


In the post editor it’s being set to the post object with some initial data. Let me find the code.

I think this is what gets called to create a new draft post:

wordpress-develop/src/wp-admin/post-new.php

Line 66 in 3d3d510

$post = get_default_post_to_edit( $post_type, true );

I was thinking about the following logic that is rather unrelated:

wordpress-develop/src/wp-admin/edit-form-blocks.php

Lines 91 to 105 in 3d3d510

if ( 'auto-draft' === $post->post_status ) {

$is_new_post = true;

// Override "(Auto Draft)" new post default title with empty string, or filtered value.

if ( post_type_supports( $post->post_type, 'title' ) ) {

$initial_edits['title'] = $post->post_title;

}

if ( post_type_supports( $post->post_type, 'editor' ) ) {

$initial_edits['content'] = $post->post_content;

}

if ( post_type_supports( $post->post_type, 'excerpt' ) ) {

$initial_edits['excerpt'] = $post->post_excerpt;

}

}

I don't know if the get_default_post_to_edit is a good way to go here.

ockham · 2024-03-19T14:22:46Z

Closing in favor of #6278.

ockham added 2 commits March 12, 2024 12:14

Block Hooks: Remove references to files in test

c8031bb

Block Hooks: Add test to verify that correct context is passed to fil…

d9448a7

…ters

ockham self-assigned this Mar 12, 2024

ockham commented Mar 12, 2024

View reviewed changes

ockham and others added 2 commits March 12, 2024 14:26

Coding Standards

700c220

Fix failing block-template-utils test

6329e2c

Map incoming data to build object

6de2de8

ockham commented Mar 13, 2024

View reviewed changes

tjcafferkey added 2 commits March 13, 2024 14:55

Revert mock post_content values back to 'Content'

d5d8b9a

Coding standards

12d18dc

ockham added 12 commits March 13, 2024 16:22

Fix test

0644556

Add some more coverage

54a5ab9

Remove commented-out line

52a5442

Move global to top of function

5ac163a

Rearrange comment

2ad0ad2

Include author

944b367

Rewrite assertion

ce3c67b

Add TODO comment

97a8189

Make sure template exists

666c119

Cover origin

b3df9d2

Test post type

1d96a9b

Cover post name

d534c06

Cover theme and ID

dc47998

This comment was marked as resolved.

Sign in to view

tjcafferkey reviewed Mar 14, 2024

View reviewed changes

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved

Remove whitespace

86a3cf6

tjcafferkey reviewed Mar 14, 2024

View reviewed changes

ockham added 10 commits March 14, 2024 10:58

Rephrase comment

c3d78c6

Whitespace, my raison d'etre

538ad60

Madness

82f65e4

Rewrite a bit

f1f92fd

Add ticket number to test

529964b

Get test to pass

1f615eb

Rename static post var to template_post

651d0cc

Attempt fixing controller test

e3d6dc1

Rename argument to changes

34f0126

Remove temp var

5319235

ockham commented Mar 15, 2024

View reviewed changes

ockham added 3 commits March 15, 2024 10:45

Polyfill missing information in changes from existing template post

ac59c76

Revert now-obsolete change to the controller

2064650

Remove now-obsolete test fixtures

5a791d2

ockham mentioned this pull request Mar 18, 2024

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

Closed

16 tasks

gziolo reviewed Mar 18, 2024

View reviewed changes

WPCS

c3c3009

ockham closed this Mar 19, 2024

ockham deleted the fix/block-hooks-context branch March 19, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Hooks: Pass correct context to filters #6254

Block Hooks: Pass correct context to filters #6254

ockham commented Mar 12, 2024 •

edited

Loading

ockham commented Mar 12, 2024

ockham Mar 12, 2024

ockham Mar 12, 2024

tjcafferkey Mar 13, 2024

github-actions bot commented Mar 12, 2024

ockham Mar 12, 2024

tjcafferkey commented Mar 13, 2024 •

edited

Loading

ockham Mar 13, 2024

ockham commented Mar 13, 2024

ockham Mar 13, 2024

tjcafferkey Mar 13, 2024

tjcafferkey Mar 13, 2024

ockham commented Mar 13, 2024

This comment was marked as resolved.

tjcafferkey Mar 14, 2024

ockham Mar 14, 2024

ockham Mar 14, 2024

ockham Mar 15, 2024

gziolo Mar 18, 2024

gziolo Mar 18, 2024

ockham commented Mar 19, 2024

		@@ -1468,6 +1468,22 @@ function inject_ignored_hooked_blocks_metadata_attributes( $post, $request ) {
		$template = $request['id'] ? get_block_template( $request['id'], $post_type ) : null;

	if ( 'auto-draft' === $post->post_status ) {
	$is_new_post = true;
	// Override "(Auto Draft)" new post default title with empty string, or filtered value.
	if ( post_type_supports( $post->post_type, 'title' ) ) {
	$initial_edits['title'] = $post->post_title;
	}

	if ( post_type_supports( $post->post_type, 'editor' ) ) {
	$initial_edits['content'] = $post->post_content;
	}

	if ( post_type_supports( $post->post_type, 'excerpt' ) ) {
	$initial_edits['excerpt'] = $post->post_excerpt;
	}
	}

Block Hooks: Pass correct context to filters #6254

Block Hooks: Pass correct context to filters #6254

Conversation

ockham commented Mar 12, 2024 • edited Loading

ockham commented Mar 12, 2024

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 Mar 12, 2024

Test using WordPress Playground

Some things to be aware of

Choose a reason for hiding this comment

tjcafferkey commented Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

ockham commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Mar 13, 2024

This comment was marked as resolved.

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 commented Mar 19, 2024

ockham commented Mar 12, 2024 •

edited

Loading

tjcafferkey commented Mar 13, 2024 •

edited

Loading