Remove parent and position validation from menu item REST API endpoint #34672
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
spacedmonkey
requested review from
draganescu,
getdave,
talldan and
TimothyBJacobs
spacedmonkey
added
[Feature] Navigation Screen
Core REST API Task
|
Thanks for working on this. Looks like just the PHP Unit tests to update and this should be good to go. |
talldan
approved these changes
Sep 10, 2021
Comment on lines
277
to
294
| public function test_create_item_change_position() { | ||
| wp_set_current_user( self::$admin_id ); | ||
| $new_menu_id = wp_create_nav_menu( rand_str() ); | ||
| for ( $i = 1; $i < 5; $i ++ ) { | ||
| $request = new WP_REST_Request( 'POST', '/__experimental/menu-items' ); | ||
| $request->add_header( 'content-type', 'application/x-www-form-urlencoded' ); | ||
| $params = $this->set_menu_item_data( | ||
| array( | ||
| 'menus' => $new_menu_id, | ||
| ) | ||
| ); | ||
| $request->set_body_params( $params ); | ||
| $response = rest_get_server()->dispatch( $request ); | ||
| $this->check_create_menu_item_response( $response ); | ||
| $data = $response->get_data(); | ||
| $this->assertEquals( $data['menu_order'], $i ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should this test be removed? It doesn't seem to trigger validation.
There was a problem hiding this comment.
It failed because it resulted in the following sequence of positions: 0, 2, 3, 4, 5. According to https://core.trac.wordpress.org/ticket/28140, menu position 0 is invalid and breaks things so it would be good to get it fixed. I proposed an adjustment in 4e97c84 (feel free to roll it back) – what do you think @spacedmonkey ?
There was a problem hiding this comment.
I think it might be better change the schema to make the min 1 and default 1.
gutenberg/lib/class-wp-rest-menu-items-controller.php
Lines 958 to 964 in 8e4ebfa
There was a problem hiding this comment.
I would also think about making the position a required field.
There was a problem hiding this comment.
Nice! Yes, let's do that instead
talldan
changed the title
|
Renamed the PR to be a little clearer as the title is what appears in the Gutenberg changelog. |
adamziel
approved these changes
Sep 10, 2021
|
Please do not merge until this is resolved - #34672 (comment) |
…g into fix/remove-validation
spacedmonkey
commented
Sep 11, 2021
spacedmonkey
commented
Sep 11, 2021
spacedmonkey
commented
Sep 11, 2021
|
@spacedmonkey, thanks for working on this. I see the following REST API response error when reordering menu items: ErrorI think we should update Current PHP Unit Tests failure also looks like to be related to the recent changes - https://github.com/WordPress/gutenberg/runs/3574827241. |
|
@adamziel @TimothyBJacobs @Mamaduka There seems to be some confusion here, on if we should not allow 'menu_order' => 0. Seems to lots of backwards and forwards on this one. According to this, 0 is not a validation position. I am not bother either way. I will defer to y'all on this one. |
TimothyBJacobs
approved these changes
Sep 13, 2021
There was a problem hiding this comment.
Let's stick with as you have it now @spacedmonkey. I haven't tested the editor for regressions, but if all is working this looks good to merge to me.
spacedmonkey
requested review from
kevin940726 and
tellthemachines
as code owners
|
Size Change: 0 B Total Size: 1.06 MB ℹ️ View Unchanged
|
adamziel
approved these changes
Sep 14, 2021
fullofcaffeine
added a commit
that referenced
this pull request
Sep 16, 2021
* trunk: (74 commits) Update docs for ClipboardButton component (#34711) Post Title Block: add typography formatting options (#31623) Bump plugin version to 11.5.0 Navigation Screen: Adjust header toolbar icon styles (#34833) Fix the parent menu item field in REST API responses (#34835) Rewrite FocusableIframe as hook API (#26753) Create Block: Remove wp-cli callout since not recommended and outdated (#34821) Global Styles: Fix dimensions panel default controls display (#34828) [RNMobile][Embed block] Enable embed preview for Instagram and Vimeo providers (#34563) Increase Link UI search results to 10 on Nav Editor screen (#34808) Prevent welcome guide overflow x scroll (#34713) Enable open on click for Page List inside Navigation. (#34675) [RNMobile] [Embed block] - Unavailable preview fallback bottom sheet title update (#34674) Add missing field _invalid in menu item REST API (#34670) Fix Dropdown/DropdownMenu toggle closing in all UAs (#31170) Navigation submenu block: replace global shortcut event handlers with local ones (#34812) Navigation Screen: Consolidate menu name and switcher (#34786) Remove parent and position validation from menu item REST API endpoint (#34672) Clean theme data when switching themes in the customizer (#34540) Components: add reset timeout to ColorPicker's copy functionality. (#34601) ...
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
As noted in #25093 (comment), we need to remove this validation, so that we can make batch requests work. This PR is simple, remove validation for parent and position.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).Fixes #25093