REST API: Add locations to the menu response #31656
Merged
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
Mamaduka
added
[Type] Bug
Comment on lines
+192
to
+194
| if ( in_array( 'locations', $fields, true ) ) { | ||
| $data['locations'] = $this->get_menu_locations( $nav_menu->term_id ); | ||
| } |
There was a problem hiding this comment.
Why is this needed? You can get menu locations from embedding request?
?_embed=menu-location at the end of the request.
There was a problem hiding this comment.
Also, you can just add locations like this. You would need to schema.
There was a problem hiding this comment.
Primarily because of consistency when using with entities on the JS side. You can set locations using editEntityRecord, but the getEntityRecord doesn't return them.
The locations is also defined in the schema but never returned.
?_embed=menu-location at the end of the request.
I just tested this, and it doesn't embed menu locations for some reason.
There was a problem hiding this comment.
In a way, this is similar to how a post response has an array of category/tag IDs.
| /** | ||
| * Returns names of the locations assigned to the menu. | ||
| * | ||
| * @param int $menu_id The menu id. |
There was a problem hiding this comment.
Add a since and a unit test.
There was a problem hiding this comment.
Won't be since tags updated if/when this endpoint gets merged into WP core?
I will work on unit tests.
|
Size Change: +776 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
fix/rest-api-menu-locations
branch
from
ac3c7dd to
1e11daf
Compare
|
IIRC we intentionally omitted the locations from the menu object. The locations are a property of the theme, and should be managed through the |
|
Currently, you can only get locations using the The primary reason I created this PR was inconsistency with It's also confusing to have an item in the schema but not in the response IMO. |
|
Huh, I could have sworn it was the opposite, that updating was done thru the menu locations endpoint. If we're doing updating in the menus endpoint, then yes we definitely need to surface that value in the response. |
|
Do you think we should fully move menu location management to the |
|
Yes, my preference would be to put it in the locations endpoint, but I don't think it's a deal breaker. Actually, thinking about it more. I think it'd probably make the most sense to have the menu locations be part of the theme's endpoint itself and use that for both retrieving and editing. |
|
Looking at the Maybe we should use the same logic for |
|
Hi, @spacedmonkey, @TimothyBJacobs Is it okay to move forward with this solution? Thanks |
|
Yeah, the sidebars endpoint is definitely something to look at. I think a difference would be the complexity of data, the sidebars have a lot of complex things to manage, while the menu locations are fairly straightforward and are basically just a wrapper around the theme mod api. But let's keep things simple and roll with this PR for now. |
TimothyBJacobs
approved these changes
May 20, 2021
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
Fixes #31648.
How has this been tested?
/wp-json/__experimental/menus/:idlocationsproperty with assigned menu locations.Screenshots
Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).