REST API: Introduce reorder menu endpoint

[TimothyBJacobs]

Is your feature request related to a problem? Please describe.
The Menus endpoint makes it difficult to reorder multiple menu items. This is because the position of each menu item is validated sequentially checking that another menu item doesn't exist with the new position. So if you try to switch the position of two menu items, you'll get an error.

See #21964.

Describe the solution you'd like
As suggested by @spacedmonkey, introduce a REST API endpoint specifically for reordering menu items. Where 50 is the menu item id, and 1 is its order.

POST /wp/v2/menus/<menu_id>/ordering
{
      "50": 1
}

Describe alternatives you've considered
Drop menu item order validation altogether in the endpoint. This isn't validated in WP_REST_Posts_Controller. Though based on my reading of wp_nav_menu, a duplicate menu_order is more of an issue for menus since duplicates will not be printed in the menu.

That being said, AFAICT neither the customizer endpoint nor WP-Admin validate the menu order.

[getdave]

Useful context #21964 (comment)

[getdave]

@spacedmonkey Just flagging this one for you here as you asked me to keep you in the loop on REST Menus API items. This is high on the priority list for the Navigation Editor in Gutenberg and I'm hopeful that someone will be able to take a look into it very soon.

[spacedmonkey]

While thinking over this ticket I had some thoughts.

1. This API design, would only work, if you send all the menu items in one request.

POST /wp/v2/menus/<menu_id>/ordering
{
      "50": 1
}

If allow for random menu items to be admitted, then you may still try and save two menu items in one position. It only works, if we got the whole menu and validate in one got go before saving anything.

2. Regarding transactioning, could we get around this by enabling revisions for menu items and rolling back to the last revision if something go wrong.

3. What should the response look like.
   Return one WP_Error like this

code: "rest_unable_to_save_menu"
data: {status: 400}
message: { "Unable to save, 1, 2, 3 and 4 menu items" }

Should it return multiple WP_Errors, adding additional_errors

code: "rest_unable_to_save_menu"
additional_errors: Array
data: {status: 400}
message: { "Unable to save, 1, 2, 3 and 4 menu items" }

Or should it return an error state with a response like this

POST /wp/v2/menus/<menu_id>/ordering
{
      <menu_id>: true|WP_Error
}

I think the last one is the most usable response, but we don't have this output elsewhere in the REST API.

4. I know we want a menu order endpoint here, but do we need / require parent id as well? When a menu item is reordered, sometimes it is also given new parent id. Should we also validate that menu item parent is valid, in the current menu and two menu items are not parents of each other. Just a thought. For what is worth, this is only other field i would add. It we want to use more than one menu item at the same time, then we can do this with a batch request, as menu item already supports batching.

5. This new menu endpoint, should just using the existing permission modal for menu item edit.

Thoughts @TimothyBJacobs, @getdave @draganescu

[adamziel]

If allow for random menu items to be admitted, then you may still try and save two menu items in one position. It only works, if we got the whole menu and validate in one got go before saving anything.

It surely is easier if we send all the menu items in one go and I think it would be a good first version.

That being said, I think it's possible to make it work with just a partial input. Imagine the following menu:

Homepage (position: 1, ID: 100)
About (position: 2, ID: 101)
Contact (position: 3, ID: 102)
Careers (position: 4, ID: 103)

Then, a request like

POST /wp/v2/menus/<menu_id>/ordering
{
      "102": 1
}

Could follow the following algorithm:

1. Store the current position as position_before
2. Store the target position as position_after
3. Update the position on menu item 102 to something temporary, e.g. null value
4. Find all the menu items where position_after <= position <= position_before
5. Increment their positions by 1. Now we have Homepage on position 2 and About on position 3
6. Set the position on menu item 102 to position_after, so 1

After the entire operation, the menu looks like this:

Contact (position: 1, ID: 102)
Homepage (position: 2, ID: 100)
About (position: 3, ID: 101)
Careers (position: 4, ID: 103)

Rinse and repeat for each reordered menu item (well that could be slow, but there's also a lot of optimization avenues such as doing the computations in memory first).

For invalid inputs such as

POST /wp/v2/menus/<menu_id>/ordering
{
      "102": 1,
      "103": 1
}

We would just throw a validation error.

---

I know we want a menu order endpoint here, but do we need / require parent id as well?

Good point! So now we have two use cases:

1. Reordering within the same parent
2. Moving to another parent

Since moving to another parent requires you to choose a position, even if the last one, I would say they are the same use case called "Moving a menu item to a specific position under a specific parent node".

In which case the input would maybe look more like:

POST /wp/v2/menus/<menu_id>/ordering
[
    { "itemId": "102", "parentId": "101", "position": 1},
    { "itemId": "103", "parentId": "101", "position": 2},
     // ...
}

Now this looks "batchable", but I vote against using batch requests here. We need to reason about the input as a whole (at least to validate it) and that's not what batch requests are for.

[adamziel]

My biggest problem here is this:

We have one tree stored on the backend and another one in the editor where some items were added, some are missing, we also have some new parent nodes with both new and existing menu items. We want to take the editor tree and save it in the backend.

One way to do it would be having an endpoint to diff the input tree against the database tree. Another way would be to have an endpoint that scrapes all the stored menus and create new ones every time.

And then, the easy way would be:

1. For all created items and updated: send a series of batched POST requests, but don't set any parent_id and such
2. Once all the IDs are available, use the /ordering endpoint to give all the menu items a proper structure
3. Send over any DELETE requests required
4. "Commit" everything to guarantee consistency – no changes are visible to the users without this step

If then, in the /ordering we require sending over all the menu items, we can just unset all the positions and parents and set them all over again – thus avoiding tree diffing.

If we continue to create new draft menu items in the editor right when the user clicks + and have all the IDs available early, we could simplify it even more. We could pack all the POST, /ordering, and DELETE requests into the same batch. Then we wouldn't even need the commit step. Well we would, but now it's not a new mechanics but the same general feature request related to batches we already know about.

[TimothyBJacobs]

POST /wp/v2/menus/<menu_id>/ordering
[
    { "itemId": "102", "parentId": "101", "position": 1},
    { "itemId": "103", "parentId": "101", "position": 2},
     // ...
}

This format makes sense to me, though we'll need to use snake_case.

In terms of error response, I think we should probably reuse the way parameter validation works. So have in the data property something named details or similar which is a map of the menu ids to error messages.

Another way would be to have an endpoint that scrapes all the stored menus and create new ones every time.

I don't think we can really do this. That'll cause a lot of entities to be deleted and recreated. Any references plugins are making to those menu items would also end up being lost.

If we continue to create new draft menu items in the editor right when the user clicks + and have all the IDs available early, we could simplify it even more. We could pack all the POST, /ordering, and DELETE requests into the same batch. Then we wouldn't even need the commit step.

This makes the most sense to me. We may want to allow creating them as auto-draft so they get automatically cleaned up by WordPress.

[spacedmonkey]

I have been considering this endpoint over the last couple of days.

Originally, when I built this endpoint, I wanted avoid doing more than one ( two with post meta ) database inserts / updates in a single REST API request. As in, one request = one database operation = one menu item. But this basically means that validation against other menu items is impossible.

When considering the a reorder endpoint, by it's nature, it will update multiple rows, as more than one menu item can / will be updated in one request. At the moment, we are only updating menu order field and maybe parent id. Why this, point, if we are going to do a update, when why update other fields? Consider the following.

1. Send one batch request, to create new menu items (as drafts). Confirm menu items are created. Get ids of new items.
2. Send a request to a new "multiple update" endpoint. Get a array of all updates, validate changes. Error if there is a problem. This update existing menu items, order, title other fields. Also update new menu items to published.
3. Send one batch request to delete menu items.

Or a much simpler solution, would be a do all the create, delete and update in one request. This is currently how the customizer admin ajax works. This has been in core for a long time and seems to have worked well. We could copy / reimplement the same code.

I think we can only prepare for database failure up to a point. Databases failing is an edge case, for the most part, it should not block us for building a solution.

[TimothyBJacobs]

Or a much simpler solution, would be a do all the create, delete and update in one request. This is currently how the customizer admin ajax works. This has been in core for a long time and seems to have worked well. We could copy / reimplement the same code.

We can do this now with the batching endpoint as it exists if we drop menu order validation. From the initial issue description:

Drop menu item order validation altogether in the endpoint. This isn't validated in WP_REST_Posts_Controller. Though based on my reading of wp_nav_menu, a duplicate menu_order is more of an issue for menus since duplicates will not be printed in the menu.

That being said, AFAICT neither the customizer endpoint nor WP-Admin validate the menu order.

[spacedmonkey]

Okay, if existing menu screens do not do this level of validation, then I think we do not bother with this endpoint. I think we should work on converting the current screen to using existing apis with batching. From there we can do some testing, to see if this endpoint is even needed.

I would recommend doing validation like this in client. Maybe before making request, saving a version of menu data in local storage before making API requests, so if browser crashes, we can recover the data. After all the batch requests are completely, it would be simple enough to get all the menu items ( with a rest api request) again and comparing to local state to ensure they match.

[TimothyBJacobs]

That works for me. Do you want to open a PR to remove that validation?

[spacedmonkey]

That works for me. Do you want to open a PR to remove that validation?

Let's create a ticket first, then I will work on the PR.

[adamziel]

This sounds like a good version 1. My only concern is the usual suspect: network problems and lost updates. Unlike widgets, it's not uncommon to have large menus. This means more requests to batch and, in turn, more actual HTTP requests. But that's not specific to this ticket – it's a general thing related to batching.

It may be a good reason to add a "preflight" validation check: If we need to split a batch into two or more API requests, we'd validate all the payloads first and only pass them to handlers if that worked. So for every "bundle" of n API requests, we'd actually run n * 2 requests: n for pre-validation, and n for processing. I believe the term is "dry run".

Or we could just accept that we're not transactional for now, and defer this to some later point where we'd discuss transactions or changesets in some form.

[draganescu]

I was wondering, since we don't support the built in transaction mechanism in Mysql, if there is an option that for batched requests to be implementing a simulated transaction:

• we send the batch objects
• we also send the length of the total batch operation
• we keep sending batch objects in subsequent calls
• when there are as many batch objects as the length initially received the batch is executed, otherwise we error out

For this we'd need to store the batch data up to length in some option or CPT. So by having this approach we are free to send hundreds of requests and not fear that some of them will be lost which in turn breaks the logic of the bigger batch that happens to contain more than 25 steps.

By storing the batch objects we can even retry things if they go awry because we can start again.

How awful is this idea?

[spacedmonkey]

Just a friendly reminder here, you can only send 25 request at a time in a batched request. See this line. Meaning only 25 menu items could be updated / deleted / created in one request. This means for a super large merge menu, like 200+ items, would still have to break this into at least 8 https / api requests. This also means, that if one of the API requests fails, it is easier to find where something went wrong, you can retry and even try sending one request at a time, it re-sending the batch request fails.

It is worth thinking about how menu items could be broken into groups. For example, could we group of the delete, creates and edits into one request? Or just group them by menu order or id?

[spacedmonkey]

we send the batch objects
we also send the length of the total batch operation
we keep sending batch objects in subsequent calls
when there are as many batch objects as the length initially received the batch is executed, otherwise we error out

In response to this. Let's think of responses why a REST API call might fail.

1. Database is down. Well if we batch requests, 25 a time. Some of them may work, but if the database is down, all the requests will fail. This should be highlighted to the user.
2. Invalid data in request, like a special character in the title or something, that makes one or more REST API request fail. This would only affect that one item. So save everything else, and make a single REST API request on the failed one. Maybe even retry a couple of times. Let's try to save as much as we can. Highlight to the user the parts that failed.
3. If invalid request is sent, then again, highlight to the user the parts that failed.

Batching requests to 25 items is by design. There are a number of limitations in PHP, 30 second execution time for one, some hosts limit the number database operations per quest etc. By batching this, it is much more likely that the saving of menu items will save correctly and not fail. It is also worth noting that existing menus screen as had long standing issues, with number of posted variables.

It may be a good reason to add a "preflight" validation check.

A preflight validation check, guesses that data provided might be invalid. Why would the data generated by the React code be invalid? Doesn't that seem very unlikely?

[draganescu]

I've given this some more thought (along with Jonny and Adam) in the context of eventual consistency problems b/c of batching limitations. In the end, I think moving ahead with the most common usecase is probably the best idea in terms of the API's resilience to failure.

Not many menus are outside of the 25-50 items bracket. While the number of operations can be higher than the number of menu items, it's still just a few batch requests.

While batching in fixed numbers of operations runs the risk of having half the operations saved and half not saved, buffering all batch calls runs the same risk if the server times out. Plus, the current screen already has the problem of timeouts. And the only way to have a sort of "rollback" is to use the Customizer and rely on changesets, but even then, it's not guaranteed that unusually complex menus in unusual server or connection situations won't end up saved incomplete.

Therefore, let's move forward with batching as is. One thing it has going for it is that the number of operations to be sent at one time can be configured at implementation time, so unusually complex systems of great hardware can just increase this limit.

Considering that it looks like we're going to drop the validation for order, hence we won't need this ordering endpoint, I'll leave it to @spacedmonkey to close this one.

[TimothyBJacobs]

Just giving a +1 to what @spacedmonkey said.

[adamziel]

As for the way forward now, I agree. Let's do most out of what we already have and then iterate based on what we learn. I started a PR to migrate the editor to current REST API endpoints: #34541

As for the top-down design discussion, some additional points:

In response to this. Let's think of responses why a REST API call might fail.

Also network connectivity issues could lead to losing one of the requests.

Highlight to the user the parts that failed.

We can't do that if "Move menu item 2 from parent 4 to parent 5" fails, but "Delete parent 4" succeeds. Also, I'm not sure how to tell the user that all his updates went through except for the 4 menu items here. Do we rollback that part of the UI? Do we mark these items using red? What if the partial failure made the website non-navigable, even if for a few minutes?

Why would the data generated by the React code be invalid? Doesn't that seem very unlikely?

Bugs happen, invalid inputs happen – backend validation reduces the chances of corrupting the data.