Assign a unique slug to navigation blocks and wp_navigation posts #36828
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamziel
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
adamziel
changed the title
|
I'm fine with using IDs in the first rollout and switch to slugs down the road. The uniqueness of a slug based on a name should be enforced by the post type APIs when the menu is saved server-side, though. |
|
Thank you for proposing this and thinking carefully about the longer term health of the block 👏
So the benefit of this is that it's a DB attribute and thus if necessary we can write a migration for it in the future? I'm trying to get at why using |
adamziel
force-pushed
the
try/store-slug-in-nav-block
branch
from
4c048f5 to
c596381
Compare
|
Prompted by your comments, I did some more thinking and it turned out that title-based slug is already available at the same time as ID. I refactored this PR from merely a migration path to a decent and potentially long-term solution. |
|
Size Change: +21 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
getdave
reviewed
Nov 25, 2021
There was a problem hiding this comment.
I tried this and the slug was auto generated correctly.
However I found a bug.
- Delete all menus.
- Site Editor.
- Create Nav block and create a Menu adding 1 item.
- Save.
- Go back to Nav block and
Select menu -> Create new. - Add some items to this new Menu.
- Save.
- Notice how both menus are now marked as dirty.
- Saving causes the original / first menu to loose all its items.
|
@getdave good catch! It is a bug in trunk, let's address it separately. |
adamziel
force-pushed
the
try/store-slug-in-nav-block
branch
from
c596381 to
034ab76
Compare
getdave
approved these changes
Nov 25, 2021
|
After some more discussion, it will be easy for the two values to go out of sync. I could change the ID in the code editor, for example. This would only create more trouble in the future. Let's close this one for now and stick to numeric IDs. The migration will still be possible, and maybe even easier than if there were two sources of truth instead of just one. |
|
Sounds logical. Small steps. Thank you for exploring though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related to #36524.
This PR sets the ground for using slugs in the navigation block in WordPress 6.0. Right now, the navigation block refers to the underlying data using a numerical ID, like
684:<!-- wp:navigation {"menuId": 642 } /-->With slugs, it would refer to it using a string:
<!-- wp:navigation {"slug": "header-menu" } /-->This would enable an automated transfer of menus between themes. It's the same idea as in template parts.
The problem is that WP 5.9 will use numerical IDs, which means there will be many "legacy" blocks created by the time WP 6.0 is released. How could they be migrated to slugs in 6.0? Enter this PR.
I propose setting
post_nameof every createdwp_navigationpost to the slug fetched from the backend on wp_navigation post creation. This way, in WP 6.0 we could migrate this:<!-- wp:navigation {"menuId": 642, "slug": "header-navigation" } /-->to this:
<!-- wp:navigation {"slug": "header-navigation" } /-->Even if slugs won't be shipped in the end, that's okay – the migration would just go in this direction insted:
<!-- wp:navigation {"menuId": 642 } /-->Test plan
cc @getdave @tellthemachines @talldan @noisysocks @mtias @draganescu