Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Navigation: Avoid content loss when only specific entity fields are edited #60071

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Navigation: Avoid content loss when only specific entity fields are edited #60071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/block-library/src/navigation/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,6 +1520,14 @@ function block_core_navigation_update_ignore_hooked_blocks_meta( $post ) {
return $post;
}

/**
* Skip meta generation when consumers intentionally update specific Navigation fields
* and omit the content update.
*/
if ( ! isset( $post->post_content ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that this is stdClass because it’s hooked to https://developer.wordpress.org/reference/hooks/rest_pre_insert_this-post_type/.

return $post;
}
Comment on lines +1523 to +1529
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want to generate meta when the requests skip content updates?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this correctly, the hook looks at post_content and calculates some derived values from it. The _wp_ignored_hooked_blocks post meta, and it also modifies the post_content itself.

If the REST request doesn't modify post_content, the existing values are OK and the hook doesn't need to do anything.

I'm more curious about the if ( empty( $post->ID ) ). If I'm creating a wp_navigation by POST-ing to the endpoint and not specifying an ID, who will update the _wp_ignored_hooked_blocks and the post_content? It seems like it will be skipped, although it shouldn't be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. We could also use the if ( empty( $post->post_content ) )) check; it would also cover the case when the post content is set but empty, and the hook doesn't need to do anything.

Unfortunately, I'm not very familiar with the internals of Block Hooks API. @ockham and @tjcafferkey can provide more details.

The $post->ID check was introduced in #59875.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty() is bad, because it would miss the cases where the post_content is legitimately changed to an empty string. isset() is better.

The $post->ID check was introduced in #59875.

Ideally the post_content should be modified in the rest_pre_insert hook and the post meta updated in the rest_insert hook.

Because rest_pre_insert hook runs before the post was inserted/updated in the database, and the $post argument is only the partial post with fields specified in the request. It's a stdClass, not WP_Post. It might not have an ID yet. And you want to modify the post_content before it's written into the database, instead of having to write it twice.

rest_insert sees the real WP_Post as $post, it's the one read from the database and it always has an ID. It's a good place where to update the post meta.

I believe the fix in #59875 is not 100% correct. The code needs the $post->ID only to read the existing ignored_hooked_blocks meta and then write it into a Navigation block attribute in the serialized post_content. But when a wp_navigation is freshly created, there is no existing meta.

The current code will fail to set the ignored_hooked_blocks meta when the "create" request has a post_content where the root nav block has a $root_nav_block['attrs']['metadata']['ignoredHookedBlocks'] value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both. You are right @jsnajdr in saying this.

The current code will fail to set the ignored_hooked_blocks meta when the "create" request has a post_content where the root nav block has a $root_nav_block['attrs']['metadata']['ignoredHookedBlocks'] value.

The fix in #59875 was accepted to cover a fatal error happening in what we considered to be an edge-case. It's not 'ideal', and I'm sure there's a solution in what you mentioned above. Unfortunately time was not on our side when this was reported and considered it more important to fix the error.

I'm going to create a follow-up issue to look into a better solution.

cc @ockham incase I've misspoken or there's something further to add 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add to this.

when the "create" request has a post_content where the root nav block has a $root_nav_block['attrs']['metadata']['ignoredHookedBlocks'] value.

We will never get this data in the request since the Navigation block (which contains the metadata) and its inner blocks are detached, the POST request would only contain inner block data.

In the function above we have to mock the Navigation block in order to generate the necessary meta data to save. We do this because we assume the incoming $post data is coming from the Site Editor where the Block Hooks algorithm has already run when preparing the response for the Editor to insert the hooked blocks.

Still, a follow-up issue should be created to look if there's a better solution to tackle the side effect detailed in the description of #59875.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, I've only seen this now, since I wasn't working on Friday.

Anyway, thanks a lot @jsnajdr for reporting #59991, @Mamaduka for this fix, and @tjcafferkey for the follow-up clarification!

I think your reasoning about the issue is spot-on, as is the fix. I'll try to address some of the concerns that Jarda raised about #59875:

https://github.com/WordPress/gutenberg/pull/60071/files#r1533833240:

I'm more curious about the if ( empty( $post->ID ) ). If I'm creating a wp_navigation by POST-ing to the endpoint and not specifying an ID, who will update the _wp_ignored_hooked_blocks and the post_content? It seems like it will be skipped, although it shouldn't be.

This was covered by Tom in the PR description of #59875:

A known side effect of this means that the next time this navigation is loaded (if the user is creating it via the REST API outside of the site editor), if it has any anchor blocks it will proceed to insert the hooked blocks since we won't have run the ignored hooked blocks logic for reasons mentioned above. Whether this is a 'bug' or not I think is still a bit of a grey area but given that I believe this is an edge case I think its acceptable until we decide a better course of action.

https://github.com/WordPress/gutenberg/pull/60071/files#r1535417586:

Ideally the post_content should be modified in the rest_pre_insert hook and the post meta updated in the rest_insert hook.

Because rest_pre_insert hook runs before the post was inserted/updated in the database, and the $post argument is only the partial post with fields specified in the request. It's a stdClass, not WP_Post. It might not have an ID yet. And you want to modify the post_content before it's written into the database, instead of having to write it twice.

Yes, that's something we learned the hard way (as writing twice even causes an issue with encoding): #59561.

Aside, with some background.

What makes things even trickier is that the mechanism that injects Block Hooks (and the related ignoredHookedBlocks metadata) includes some filters that receive a $context argument that reflects the containing wp_navigation post object, or the containing WP_Block_Template. This allows filters to conditionally enable or disable insertion of a given hooked block depending on some properties of that $context; e.g. they might check if the $context is a single template, a header template part. However, they might also check for the presence of a block in the content of the template/part/navigation object. In the latter case, we'd want $context to reflect the updated content (coming in from the REST API as part of the stdClass object you mentioned), rather than whatever's already stored in the DB. For templates and template parts (where the ignoredHookedBlocks metadata is stored as an attribute on individual blocks rather than in post meta), this has lead to a fairly complex solution where we emulate what the updated post object will look like after it's been inserted into the DB by merging the stdClass on top of the existing WP_Post object: WordPress/wordpress-develop#6278.

Note that this is only a problem for the filter $context; the actual insertion of hooked blocks (and of the ignoredHookedBlocks metadata attribute, in case of templates and template parts) is always performed on the stdClass object's content.

rest_insert sees the real WP_Post as $post, it's the one read from the database and it always has an ID. It's a good place where to update the post meta.

This would however require us to communicate a bunch of data computed in rest_pre_insert to rest_insert, or to recompute that data. My alternative suggestion would be to use the meta_input field on the stdClass object (which is used to communicate that the post meta for the post object should be updated).


/*
* We run the Block Hooks mechanism to inject the `metadata.ignoredHookedBlocks` attribute into
* all anchor blocks. For the root level, we create a mock Navigation and extract them from there.
Expand Down