Create a fallback navigation menu as one time operation on install and Theme switch #48625
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
scruffian
force-pushed
the
update/navigation-fallback-simplify
branch
from
ab30218
to
5039150
Compare
|
Size Change: +209 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d59cde3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4344616141
|
getdave
reviewed
Mar 1, 2023
scruffian
commented
Mar 1, 2023
scruffian
commented
Mar 1, 2023
|
Tests look good to me, thanks @getdave |
getdave
reviewed
Mar 1, 2023
This was referenced Mar 2, 2023
draganescu
reviewed
Mar 2, 2023
|
I think we should reach out to folks from WordPress Core before merging this one as it will form the basis for work on the Nav block going forward. We need some certainty it won't need to be rolled back. |
UpdateI've changed the model here to only create the Nav Menu on "one time" operations:
That means you can now choose to delete the default Nav Menu and no writes happen on render. But we still retain the auto-creation behaviour. I've added tests for Theme Switch |
getdave
force-pushed
the
update/navigation-fallback-simplify
branch
from
56a209f
to
343cc6e
Compare
spacedmonkey
reviewed
Mar 6, 2023
| @@ -446,8 +447,8 @@ function block_core_navigation_get_default_pages_fallback() { | |||
| $wp_insert_post_result = wp_insert_post( | |||
| array( | |||
| 'post_content' => $default_blocks, | |||
| 'post_title' => 'Navigation', // TODO - use the template slug in future. | |||
| 'post_name' => 'Navigation', // TODO - use the template slug in future. | |||
| 'post_title' => 'Navigation', | |||
There was a problem hiding this comment.
I pulled out a separate PR in case this one doesn't get merged #48773
getdave
reviewed
Mar 6, 2023
| $this->assertCount( 1, $navs_in_db ); | ||
| } | ||
|
|
||
| public function test_should_auto_create_navigation_menu_on_theme_switch() { |
There was a problem hiding this comment.
We need a test for when we switch to a Classic Theme. No menu should be created.
getdave
reviewed
Mar 6, 2023
|
|
||
| private function mock_wp_install() { | ||
| $user = get_current_user(); | ||
| do_action( 'wp_install', $user ); |
There was a problem hiding this comment.
I checked with @anton-vlasenko and @ironprogrammer about this and they felt confidence it would be safe to trigger on a mock basis here.
There was a problem hiding this comment.
Where do this unit test setup the current user?
There was a problem hiding this comment.
Ah...looks like they don't which is what I understand you are alluding to.
Question: do we need to pass the User argument? If so would you be able to point me in the direction of a test which does this setup step?
Much appreciated.
There was a problem hiding this comment.
I looked again at https://developer.wordpress.org/reference/functions/wp_get_current_user/.
It says:
Will set the current user, if the current user is not set...If no user is logged-in, then it will set the current user to 0, which is invalid and won’t have any permissions.
AKAIK our tests have no dependency on the user object so either:
- We cannot remove the
get_current_user()call - Allow it to run and set the current user to
0.
If that's incorrect I'd be grateful if you could explain the best practice so I'm aware for future reference.
getdave
force-pushed
the
update/navigation-fallback-simplify
branch
from
422369f
to
3ddf065
Compare
There was a problem hiding this comment.
Thank you for adding the @covers tags and adding messages to the assertion methods. I appreciate it.
I only have 1 small suggestion: #48625 (review)
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
getdave
reviewed
Mar 6, 2023
| } | ||
| // Run on switching Theme and when installing WP for the first time. | ||
| add_action( 'switch_theme', 'block_core_navigation_create_fallback' ); | ||
| add_action( 'wp_install', 'block_core_navigation_create_fallback' ); |
There was a problem hiding this comment.
Discussing away from Github with @azaozz I'm concerned that hooking to wp_install might be a bad option because it's likely to mean all sites will get a wp_navigation even if no block Theme is active.
|
Some things I'm looking at and considering:
|
getdave
changed the title
|
We should also consider doing this via a REST endpoint, like this: #48698 |
|
Marking this as blocked while we consider the REST approach. |
scruffian
added
the
[Status] Blocked
32 tasks
This was referenced Mar 17, 2023
|
Here is the PR which uses the REST API to achieve a similar outcome. |
|
I think this should be closed in favor of getting #48698 across the finish line. Or do we still think that one has limitations? |
|
I'd be happy to close it and focus efforts on REST. It seems like the better option to me. We always have the code here to reference if needed. |
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.
What?
Improves Nav block fallback performance and UX by auto-creating a Navigation Post (
wp_navigation). This is done on:switch_themewp_installRemoves redundant code from editor and front of site.
Why?
TL;DR
Creating Navigation menus in the client side is very susceptible to bugs - when we fire multiple requests to create navigation menus we can't know if others are still pending. To reduce the complexity of this we should just always ensure that a navigation menu CPT exists.
Longer
For a long while contributors working on the Nav block have been implementing various mechanics to handle the fallbacks for the Nav block.
As a re-fresher:
wp_navigationif found.Handling the asynchronicity of this in a single block is somewhat feasible (indeed we've been doing this for a while).
However....doing it across potentially multiple blocks on a given page is extremely complex, susceptible to race conditions and generally very difficult to test. This is compounded by the fact that the same logic has to be repeated for the dynamic block rendering on the PHP side introducing yet another surface area for bugs and regressions.
In addition, there has been a drive to reduce the friction introduced by "loading" UX. This was apparent due to network when transitioning from a "placeholder" block (one that isn't saved to a
wp_navigation) and a synced block (one whose contents is saved to awp_navigationpost). To work around this functionality was introduced to auto-create a Nav menu if one didn't exist.These two elements combined have introduced some nasty bugs into the Nav block. Perhaps the most obvious is multiple menus being created when mulitple empty Nav blocks are on a given page (if you think this will never happen think again because simply opening the Patterns inserter and viewing the
Headerpatterns causes this to occur).We can mimic this behaviour by checking out
trunk, clearing all Navigation Menus and Classic Menus and inserting multiple empty Nav block's into a post:The result is 5
wp_navigationposts get created when we only wanted a single fallback to be auto-created.This PR attempts to resolve this problem by pre-creating a Navigation post on
render_block_data(if one doesn't exist). This mechanic:How?
Add an action on
switch_themeandwp_installto create a fallback navigation menu if one does not exist and if the active Theme is a block theme.Also adds a new filter called
block_core_navigation_skip_fallbackwhich allows developers to skip the creation of a navigation if they want to.Testing Instructions
Automated Testing
Run
Manual Testing
wp-admin/edit.php?post_type=wp_navigation)wp-admin/edit.php?post_type=wp_navigation)wp-admin/edit.php?post_type=wp_navigation).add_filter( 'block_core_navigation_skip_fallback', '__return_true' );Co-authored-by: Andrei Draganescu 107534+draganescu@users.noreply.github.com
Co-authored-by: Dave Smith 444434+getdave@users.noreply.github.com