Navigation: Only fetch wp_navigation menus once #49143
Closed
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
|
Size Change: -4 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 81e9999. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4440160008
|
getdave
reviewed
Mar 16, 2023
| { per_page: -1, status: [ 'publish', 'draft' ] }, | ||
| { per_page: -1, status: 'publish,draft' }, |
There was a problem hiding this comment.
@adamziel Do you have any idea why this might be the case? Seems like a bug in the underlying implementation of Core Data.
There was a problem hiding this comment.
I looked into what might be going on here. It doesn't seem to make a difference in the final query before the rest api WP_Query happens. The comma separated string gets transformed into an array before the WP_Query. This image shows the status array query vs string query end up being identical.
I dumped the $query_args with die() after it if it was a navigation request to see the params here.
There was a problem hiding this comment.
I may be misunderstanding where to look test this at. I would add a new post, then add a navigation block to the page.
getdave
reviewed
Mar 16, 2023
| @@ -71,8 +71,9 @@ function selectNavigationMenus( select ) { | |||
| const args = [ | |||
| 'postType', | |||
| 'wp_navigation', | |||
| { per_page: -1, status: [ 'publish', 'draft' ] }, | |||
| { per_page: -1, status: 'publish,draft' }, | |||
There was a problem hiding this comment.
@scruffian If we do this we will need a comment to explain why it has to be a string for now.
|
Well this is very strange. Looks like this also triggers a number of test failures so we'll need to get to the bottom of those as well. |
|
@scruffian, in which editor are you testing? This change shouldn't have any effect on request numbers. Looking at the requests in the Site Editor, we're making two for navigation. One is from Sidebar, and the Navigation block triggers the other. We can delay the first request once the suggestion from #48906 is implemented. The request triggered by the Navigation block is only critical for fallback logic. Can we try to trigger that request if the block has no Screenshot
|
3 tasks
The Site Editor |
|
Sorry I must have been testing something incorrectly yesterday, this doesn't resolve anything. Closing. |
|
I think I found the issue this time: #49161 |
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?
As part of #48683 we noticed that there are two API requests being made for wp_navigation entities - one for published and draft statuses, and one for just draft statuses. This seems to be a feature of the way that
getEntityRecordsworks whenstatusis set to an array. If change it a string with the valuepublish,draftthen it does one request to get all wp_navigation entities.Why?
This saves one unnecessary API request. It should also simplify #48683.
How?
Change the status from an array to a string
Testing Instructions
navigation/wp-json/wp/v2/navigation/wp-json/wp/v2/navigation