Use slug instead of id for ref in Navigation block

[getdave]

What?

In #38291 (comment) we discussed why it is important that the Navigation block uses string based slug as the way for it to "refer" (via an attribute) to a Navigation post containing it's "items".

This PR seeks to address that by adding a new slug attribute and soft deprecating the existing ref attribute. This allows the block to use a slug to refer to a menu (wp_navigation post) and updating the REST API to understand how to handle that.

Please note: this PR is deliberately limited in scope. It does not attempt to implement any fallback behaviour relating to pick the "most appropriate" Navigation to use in the absence of an explicit reference. For example, this PR won't fix the situation whereby you switch Themes and the Navigation Menu isn't carried across the to new theme's Navigation block (although this PR does lay the foundation for such an effort).

Addresses part of work towards #38291

Why?

See #38291 (comment) for full explaination.

How?

This PR:

• adds a custom REST Controller for requests to wp/v2/navigation which adds a new endpoint to handle making requests for individual wp_navigation posts by their post_name (a.k.a slug) or by post_id (for backwards compatibility).
• adds test coverage to the endpoint
• adds a new slug attribute to the block.
• soft deprecates the block's ref attribute.
• updates internals of Nav block to handle using the navigation post's slug (i.e. post_name) as the reference.
• adds temporary fix (relating to Replace the OPTIONS requests with a faster HEAD request in canUser resolver #43703) to OPTIONS permissions requests by overloading implementation of get_post method within the new Navigation REST Controller.

Dependencies

• Replace the OPTIONS requests with a faster HEAD request in canUser resolver #43703 - without this calls to canUser with a slug will not provide the correct permissions. I've created a workaround for this by overloading the get_post method to map slug to ID.

Todo

• Make it possible to continue to be able to retrieve the Nav Menu by post ID as well as slug (for backwards compat reasons).
• We should consider that the endpoint may already be in use. Therefore changing it to expect slugs is considered a breaking change unless we also keep it backwards compatible with IDs.
• Problem using saveEntityRecord for update requests.
• Data migration - do we want to simply force update all existing blocks to have their ref change from ID to slug or do we want to preserve the existing data?
• Ensure rest_get_route_for_post is filtered to account for usage of slug.
• Fix front of site implementation to fetch Navigations by slug.
• Create opt out for data migration from postID to slug (just in case).
• Fix bug with Select menu which doesn't pick the correct menu due to being ID based.

Testing Instructions

Manual

• on trunk create a Navigation Menu using the block. You can use the Post Editor or the Site Editor.
• Check the block has a numerical ref attribute corresponding to the Navigation Post's ID field.
• Publish your Post.
• check out this PR
• re-open the Post you created with the Navigation block's - you should see the block loads the menu and you can manipulate it as per usual (i.e. edit, update, delete...etc).
• Add a new Navigation block and create a new Navigation Menu using that block.
• Check that the block has a string-based ref which corresponds to the slug of the Navigation Post.
• Check you can manipulate the block as per usual (i.e. edit, update, delete...etc).
• Update your Post.
• Reload your post and check that both the ID based and the Slug based Navigation block's load correctly.
• Switch to the front end of your site - check that both Navigations display correctly.

REST

Run tests using

npm run test:unit:php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-gutenberg-rest-navigation-controller-test.php

Screenshots or screencast

Screen.Capture.on.2022-09-02.at.14-09-42.mp4

[getdave]

This handles moving from number to string based ref attribute.

[getdave]

I'm of the opinion that we shouldn't look to land this for WP 6.1. Why?

This change has no user-facing benefit. In fact the whole point of the PR is that nothing changes for the user - otherwise we'd have a regression.

This PR is focused on a longer term goal of allowing Navigation Menus to be carried across to the most appropriate position in a new Theme. However in and of itself it is purely a technical implementation change moving ref being a post.ID to ref being a post.name (i.e. "slug").

Whilst this PR is very important in laying the groundwork for future user facing benefits it won't change anything for the user and (as with all significant changes) it comes with the risk of introducing regressions.

Furthermore, landing this change so close to WP 6.1 would not leave much time for contributors and/or users to test the functionality and provide feedback on regressions.

It might be appropriate to try land this PR if we could also concurrently land a well thought out mechanic which could take a given slug and map that to the most "appropriate" wp_navigation Post (even if there is no exact match). However as this work has not even been started yet it is unlikely to make the feature freeze, though that has not been confirmed.

Therefore I'm recommending we don't merge this PR unless we can also land the additional functionality and we wait until the WP 6.2 cycle.

What do you think?

[costdev]

@getdave I agree with your assessment that sufficient testing so close to 6.1 is unlikely to happen and, if it does/a mechanic is developed, it may be done in a crunch-like manner (i.e. stress). Given that this has no user-facing benefit, it makes sense to land this in 6.2 instead with the confidence that no regressions have been introduced.

[mtias]

@getdave I'm a bit confused why we are trying to retrofit a slug in the "ref" attribute instead of just using "slug":"header" and soft-deprecating "ref". When we use slugs elsewhere (like with template parts) the attribute name is a slug and we should strive for being consistent there.

[scruffian]

I rebased this.

[getdave]

@getdave I'm a bit confused why we are trying to retrofit a slug in the "ref" attribute instead of just using "slug":"header" and soft-deprecating "ref". When we use slugs elsewhere (like with template parts) the attribute name is a slug and we should strive for being consistent there.

Honestly I've considered this route several times but never voiced it because I felt it would be a "developer convenience" to simply add a new attribute to work around the problem that currently menus are referenced by IDs.

Bear in mind that the Nav block has already had one migration from navigationMenuId to ref already. So what we're proposing is adding another attribute which is used as the "reference" but this time it's called slug.

The pros of this approach are clearly:

• it preserves the users' original data (the ref is left alone untouched)
• no need for a deprecation because we're just adding a new attribute
• slug is clearer than ref than can be either numeric or a string
• aligns with other similar implementations such as template part

Cons are:

• it's yet another "reference" type attribute
• might not be clear which is considered the canonical "reference" when both attributes are present

That all said, as I see it, it should be pretty simple to rewrite the frontend to use a new slug attribute and have ref soft deprecated.

The process is to request the menu by the ref and then once retrieved set the slug attribute to the slug returned from the API. From that point ref is soft deprecated on that block instance and slug is used to do the lockup. The work on the REST API in this PR can stay "as is".

For new blocks, we simply use slug and ref is never set.

I'll do this work tomorrow which will hopefully somewhat simplify the PR. From there we can decide whether it's possible to land a follow up to this PR which implements the "lookup" part of the plan which would allow the Navigation to be carried across on Theme switch.

I know @draganescu has been looking at this but I believe we should be careful to be sure that landing such a feature without lots of testing this close to 6.1 is a good idea.

[draganescu]

I've explored how having a slug for navigation block would improve the navigation preservation experience. I found that:

• the template part block can expose its slug via block context (or better yet via entity context - thanks @talldan)
• the navigation block can look up a slug from context
• if a template part is the parent of a navigation block we set the slug (post_name/name) of the navigation post it references to that of the parent template part
• when theme is switched:
  • in the editor we get the slug from context and fall back to that navigation post (instead of most recent)
  • on the front end we get the slug from context and fall back to that navigation post (instead of most recent)
  • if the user will interact with the block we prompt them to save the content of the template part with the slug set
• getting the best slug via entity context would be better as it would allow us to set the slug to the semantic area name not the slug of the template part

The idea is that navigation preservation on theme switch based on slug works.

But it is not perfect:

• we can't build a better heuristic yet, say based on the parent of parent, so nested template parts can easily break what the system expects (e.g. having a hero in a header?)
  • the main blocker here is that in the isolated editor we can't get yet the parent of the root
  • a secondary blocker is that if we get parent slugs from context, I don't think this can contain the same thing on multiple levels - so ideally we'd have access to the tree of blocks at render time or implement a stack of contexts - bot way outside of the purpose of this work, and unlikely to land well just before a WordPress release
• this PR uses template part slug as the reference for the name of the navigation post, but the template parts are descendants of a semantic area which could be used instead, therefore solving the problem of having a hero in a header and needing some system to compose this hierarchy.

Some uncertainties:

• does this add a real and consistent benefit on the short term considering that:
  • the navigation block defaults to a list of pages if there are no navigation posts.
  • the navigation block defaults to the most recently created navigation post, if found.
  • most websites have one menu therefore, because of the above behavior, the preservation of navigation on block theme switch already happens.
• if the navigation post is manually placed in a navigation block that has a parent template part with a different slug do we update the slug of the navigation post to match the slug of the template part?
• what happens when the navigation post is referenced by two navigation blocks in two different template parts?
• is the stack of contexts straightforward to implement or does it raise unknown complications?

Given the above, even if we land this work it is unlikely that we can produce any major user visible enhancement of UX based on this PR.

Therefore it seems that despite @mtias ’s suggestion of just adding a slug instead of migrating from ref to slug - which would simplify the implications of this PR - we still find ourselves in a situation where the improvement for users is minimal and it's based on big code updates.

[talldan]

getting the best slug via entity context would be better as it would allow us to set the slug to the semantic area name not the slug of the template part

Plus it would also work in template part focus mode.

The blockers are (from what I understand):

• Template part blocks don't seem to provide their entity using EntityProvider (easy fix)
• EntityProvider isn't hierarchical. Only the inner-most entity of a particular type is provided. This means there's no way to search through ancestor entity providers to find the one that has an 'area' when there are nested template parts. (harder fix, but not impossible)

Another option is a completely separate AreaProvider, but that would be inventing something new. It feels like it should be possible through the existing entity system.

[getdave]

I agree with Andrei as I can't see a suitably robust system landing prior to WP 6.1 feature freeze.

What we cam do now is hold off merging this PR until after the feature freeze thereby ensuring it does not end up in WP 6.1. Then we can look to merge and continue working on the template part hierarchy and lookup work.

I think we should have an open-invite project kick off where the options for the lookup work are discussed and next steps are agreed. We should also agree to have regular sync checkins with stakeholders (i.e. anyone with an active interest in the work) to ensure that whoever is developing the PR has sufficient input and opinion to avoid things going off track.

Therefore it seems that despite @mtias ’s suggestion of just adding a slug instead of migrating from ref to slug - which would simplify the implications of this PR

Just noting that this work is already complete. So this now hangs on what you've concluded re: template part hierarchy and lookups.

[mtias]

@getdave id and slug are different things, and it'd stand to reason that we use different attributes for them. We have used ref in place of id in other places, so that's what we should get it to mean. The alignment with template parts on its own should also be a deciding factor.

The process is to request the menu by the ref and then once retrieved set the slug attribute to the slug returned from the API. From that point ref is soft deprecated on that block instance and slug is used to do the lockup. The work on the REST API in this PR can stay "as is".

It might be fine to just leave ref requests working with an ID and not try to switch them. There can be value for a developer or site maintainer with complex menus to reference them by ids for maintenance reasons. We should just ensure new menus are created using slugs first with the template context first.

[getdave]

@getdave id and slug are different things, and it'd stand to reason that we use different attributes for them. We have used ref in place of id in other places, so that's what we should get it to mean. The alignment with template parts on its own should also be a deciding factor.

Yes and I've already updated the implementation as such 👍🏽. My instinct was to see them both as a form of "reference" thus the urge to normalize them but I have taken on board your well justified arguments and I don't feel any reason not to go ahead on that basis.

It might be fine to just leave ref requests working with an ID and not try to switch them. There can be value for a developer or site maintainer with complex menus to reference them by ids for maintenance reasons. We should just ensure new menus are created using slugs first with the template context first.

This is the question. If we want to leave ID-based "references" alone then we'll need to test this PR and check there are no scenarios where that occurs. My concern was always that there will be use cases already in the open whereby folks expect their reference to stay the same. If we suddenly start migrating them to slug I could imagine that being rather unpopular.

[mtias]

Yeah, I wouldn't mind leaving refs untouched until users reset a template, switch themes, or remove/add navigation.

[getdave]

I haven't forgotten about this. Contributors are focused on bugs for WP 6.1 at the moment but I will pick it up again asap.

[getdave]

This PR is too large. I've extracted a sub PR to deal with the REST API parts. As this is fully backwards compatible we can merge it and then rebase this PR to remove all the REST API bits.