Block Hooks: Return early from saving meta data for the navigation without a $post->ID

[tjcafferkey]

What?

If the user is creating a navigation menu via a POST request to the REST API then we need to return early since we won't have a $post->ID to store the ignored hooked blocks meta against.

Why?

There are a number of reasons for this change but primarily because it returns an error since it incorrectly assumes there's a $post->ID in a scenario when there's not.

Another reason is because if the user is creating the navigation via the REST API they likely haven't got any hooked blocks in their content since we insert these when the response is being prepared for the site editor.

The only other way a user can create a new navigation is by duplicating an existing one. In which case the hooked blocks markup and meta data will be duplicated along with it and will retain the intended behaviour.

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.

Fixes #59867

How?

Check for the existence of a $post->ID and if not found, return $post early.

Testing Instructions

1. Regression test the navigation if we feel its required.
2. Create a navigation via the REST API similar to how is done in accompanying tests and ensure you get a successful response.

[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: tjcafferkey <tomjcafferkey@git.wordpress.org>
Co-authored-by: ockham <bernhard-reiter@git.wordpress.org>
Co-authored-by: Hug0-Drelon <hugod@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>

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

[ockham]

@youknowriad @getdave This fixes #59867, which is bad enough to warrant including the fix in RC3 IMO.

[tjcafferkey]

@getdave the newly added unit test is failing on this PR but passes locally. I suspect it's because the test is running against the built version of this change, and in the CI that isn't done? Wondering if my hunch is right and if so, is there a standardized way to approach this?

Edit: You can now ignore this, @getdave

[tjcafferkey]

@getdave the newly added unit test is failing on this PR but passes locally. I suspect it's because the test is running against the built version of this change, and in the CI that isn't done? Wondering if my hunch is right and if so, is there a standardized way to approach this?

Actually it looks like npm run build is run in the workflow prior to tests, which should have built the navigation.php file 🤔

[tjcafferkey]

Looking at the test report, it looks like its failing on /var/www/html/wp-includes/blocks/navigation.php:1479 which is the block file from WP core (not GB) and likely won't have this change present. Perhaps a different way to write the test is needed.

[ockham]

LGTM! Thank you for taking care of this, @tjcafferkey!

[tjcafferkey]

@getdave I'm unfamiliar with the process of backporting here (are there any docs explaining the process?). Is there anything further required of me post-merge? 🙏🏻 thanks.

[youknowriad]

Since this change is in the block library code, there's nothing else that should be done aside adding the backport label. At this point in the release cycle, the label should only be added to the regressions that happened during 6.5 cycle and it seems to be the case for the current PR.

[youknowriad]

I just cherry-picked this PR to the update/packages-rc2-6.5 branch to get it included in the next release: ce4250a

[getdave]

@getdave I'm unfamiliar with the process of backporting here (are there any docs explaining the process?). Is there anything further required of me post-merge? 🙏🏻 thanks.

Hi @tjcafferkey. Thanks for reaching out - looks like Riad got here before me with all the correct info 👍

I agree the backporting process can be pretty confusing. As a result I've spent time during this release documenting which files need manual backporting and which will be handled as part of the standard release process.

I'd greatly value your feedback on this in order that it can be made even clearer for future releases. This will help all contributors become aware of the process and make the task of managing the release slightly easier for release leads.