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

[Mamaduka]

What?

Fixes #59991.

PR fixes the Navigation block content loss when renaming them in the Site Editor. It also fixes PHP warnings when post content isn't set.

PHP Warning: Undefined property: stdClass::$post_content in ~/gutenberg/wp-content/plugins/gutenberg/build/block-library/blocks/navigation.php on line 1527

PHP Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in ~/gutenberg/wp-includes/class-wp-block-parser.php on line 252

PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in ~/gutenberg/wp-includes/class-wp-block-parser.php on line 324

Why?

The entities can choose to update only specific fields (See __experimentalSaveSpecifiedEntityEdits) via REST API, and the prepared entity update will only contain those properties.

How?

Bail early when post content isn't set for the prepared update.

Testing Instructions

1. Open the Site Editor.
2. Navigate to Design > Navigation.
3. Pick any navigation.
4. Click on "Actions" and rename it.
5. Confirm that navigation block content stays intact after renaming.

Testing Instructions for Keyboard

Same.

[Mamaduka]

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

[jsnajdr]

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.

[Mamaduka]

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.

[jsnajdr]

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.

[tjcafferkey]

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 😅

[tjcafferkey]

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.

[ockham]

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

[github-actions]

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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>

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

[jsnajdr]

This fixes the bug for me, but I must apply the patch to the Core files (wp-includes/blocks/navigation.php), because the Gutenberg plugin adds the filter only if your Core is old and hasn't added it already.

Should be a backport to 6.5 RC

[getdave]

but I must apply the patch to the Core files (wp-includes/blocks/navigation.php

Noting that this file will be automatically synced to WP Core as part of the packages release process if this PR is picked for inclusion in the release. So you shouldn't need to do any manual backports.

[ockham]

@tjcafferkey We should probably add some unit test coverage for this (akin to this, but with a pre-existing DB object, and with $changes->post_content unset (and probably some other field set instead, to make sure a DB update is performed)).

[getdave]

It would be helpful if we did have test coverage for this. It may also aid in being able to land in WP 6.5.

[getdave]

This PR was triaged by Editor release leads and contributors in WP Slack as part of the final WordPress 6.5 process.

See https://wordpress.slack.com/archives/C02QB2JS7/p1711369267337079 (requires registration).

The criteria used was:

• Was this bug identified during the RC cycle for WP 6.5?
• Is this bug directly related to one of the blessed features in this release?
• Would this bug cause existing website’s to break/fail upon upgrade to WP 6.5?
• What is the approximate scope of the impact? Scale: (Minimal) 1 - 5 (Severe).
• Is the fix introducing any new features or changes?

Conclusion: we propose that this fix is included in WP 6.5.

Reason: Content loss for users.

We now await decision on this from the full release team.

[ockham]

It would be helpful if we did have test coverage for this. It may also aid in being able to land in WP 6.5.

I'm working on one now 👍

[ockham]

It would be helpful if we did have test coverage for this. It may also aid in being able to land in WP 6.5.

I'm working on one now 👍

#60189

[bph]

I just cherry-picked this PR to the release/18.0 branch to get it included in the next release: 96ffcf1

[getdave]

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

[getdave]

I just cherry picked this into WP 6.5 RC 4 branch 89f4782