Save the entire navigation menu at once #22148
Conversation
adamziel
changed the title
|
Size Change: +661 B (0%) Total Size: 834 kB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
ca4e8b2 to
027ede9
Compare
|
CCing @talldan @draganescu @tellthemachines @spacedmonkey @noisysocks since I know you're all working on saving the navigation in one form or another. This PR doesn't really work just yet and needs a lot more work before it's shippable, but it's pretty close to being a functional Proof of Concept. Thoughts and discussions appreciated! I'm not really sure about the request mocking approach - it made the code short as there is no easy way to reuse parts of the codebase. I think it would work as an experiment, but a proper refactoring would be probably a good way to go long-term. |
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
a0668ba to
7cf4963
Compare
|
I am honestly like the idea of this and think the effect put into this. However, I am not sure this PR is going to work. Sites with 200+ menu items, would mean would mean 200+ of mysql updates to the post table and a more update to the post meta table. Not to mention update to caches and any filters that run on update. This means that likely the a request will timeout or error out. As WordPress doesn't really have a system of transactioning and rolling back, with would mean half updated menu items, which we are trying to avoid. Also validation would need to happen before any update is done. How would you report back if they were 100 errors in the request? Of the proof of concerpt of this end was done in the feature plugin. This maybe the best place for this discussion going forward. |
How does the existing |
There was a problem hiding this comment.
MySQL TRANSACTION are not supported by all hosts. This would require a change to WordPress's min requirements. Also the fact isn't fail gracefully here, is not great.
There was a problem hiding this comment.
Agreed, I want to get it to a state where it uses MySQL transactions when they're available, and doesn't use them when they're not. I'm exploring:
- Bulk validation before performing any sql operations
- Only performing SQL queries for menu items that are actually changed, not for all of them
There was a problem hiding this comment.
This is talking about a sizable change to WordPress core. I think this should really be discussed in a ticket on Trac.
There was a problem hiding this comment.
What about if the request times out, what happens? How about all caches that were updated? How are these rolled back?
There was a problem hiding this comment.
@spacedmonkey still exploring - with the current "brute force" way it takes 40s to save 500 menu items which isn't terrible - I think I can get it down to 2-5 with some tweaks.
How about all caches that were updated? How are these rolled back?
Good question - I'm not sure yet, I can imagine a scenario where it's so non-trivial we'll have to abandon this approach for the time being. I have some ideas like disabling any hooks, performing SQL queries, and only notifying them if everything worked properly. Or maybe validating first would be sufficient in most cases?
There was a problem hiding this comment.
Disabling cache invalidation and hooks, waiting for updates to complete, is going to generate a lot of error IMO. The amount of testing to get this into core would far to high and really require a skilled core commiter to comit to this way of doing before we continue with this course of action.
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
523239f to
2223fd9
Compare
@spacedmonkey thank you! I'll check this out tomorrow and follow up |
|
@spacedmonkey do you have an example of a website using over 100 menu items? I don't ask it as proof, I too believe it exists, but I would love to see one as a way to understand the context of that in the wild b/c it might affect other scenarios as well such as converting from existing menus to nav block menus. I can think of a "directory" type of website where people display links using nested nav menus which are programatically maintained. Other than that ... no idea. |
@spacedmonkey do you mean a proof of concept of bulk validation? Would you mind pointing me to the specific line/pr? I can't find it unfortunately. |
Of the top of my head |
The feature plugin can be found here. https://github.com/WP-API/menus-endpoints Work on this endpoint has been going on for over a year on this repo. A change of this later size, should really happen there where the REST API team can approve PRs and discuss the issue. |
|
@spacedmonkey ah that's what you meant, sorry - I thought you mean there's a prototype of bulk validation somewhere in that repo. I think it makes sense! I'll recycle this PR for a bit more just because it makes the development easy, and then will switch over to the feature plugin repo and initiate a proper PR there. |
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
2223fd9 to
ca9327f
Compare
|
I got it to a place where the validation is performed in bulk before saving and the endpoint reliably persists the submitted tree. This is pretty slow if the number of menu items is too large as you predicted @spacedmonkey. I'll try to figure it out tomorrow and move this PR over to the menus-endpoints repository for further discussion. |
The situationThe approach I initially took was to reuse the existing
But even after I pacified
Not good! It's pretty clear that we cannot use What do we do?I can see the following options: 1. Disregard
|
|
The current solution doesn't take into acount sites using object caching. Okay, you can batch the inserts menu items in MySQL, how do you invalidate values stored an object cache? There is no wp_cache_set_multi or wp_cache_get_multi at time of writing. So updates to cache will have to done one at a time and there is no way to roll this back. What happens if transactions are not enabled on mysql? Does this just fail? There would need to be a solution for users without transactions, which would end up being sending one request per menu item, what you are trying to avoid in the first place. Also not firing filters and actions will on menu items, will likely cause breakables which is not okay. I like the idea behind this PR but I think the idea as it stands is a none starter and more time should not be invested in it. |
With multi-insert approach, the only way to invalidate the cache would be to loop over all the items after the query succeeds. I prefer the other two solutions to this one.
Those users who are able to use transactions would get a much better reliability. If transactions are not available, we could just perform all the queries without wrapping them in a transaction and it would still be more bulletproof than sending a single XHR per menu item. Sure, it could still fail after 50% of SQL queries were completed, but the probability of this happening would be much lower than in case of multiple XHRs. For example, batch save wouldn't be affected by network issues between the browser and the server.
As I mentioned above, I don't think we'd have to send one request per item. Out of curiosity - how do you disable transactions in MySQL? Do you mean the storage engine for the tables would be MyISAM?
Agreed - I mentioned this exact problem when considering solution 1. With solutions 2 and 3 it wouldn't be an issue - we would still fire all the filters and actions as they are now. The only unknown is what to do on rollback, but since this is an experimental endpoint then maybe we could just add an action like "menu_save_failed". Even if the rollback would be non trivial to handle, using a batch save still has benefits even without transactions.
I hear you! I'm still pretty optimistic and think it wouldn't be that hard to make it work - let's keep the discussion going! |
|
Thanks for the thorough write-up @adamziel! I think (1) is a no-go as we need to support existing hooks used by plugins and that (2) seems cumbersome from a UX perspective. I agree that we should not avoid batch processing altogether as forcing the frontend to manage all this complexity and make dozens of round-trips isn't practical. (3) is my preference. The disadvantages you listed aren't so hard to work around: we could add code to Core which cleans up dangling menu items, and we could have the Another option not mentioned is to remove the Save Navigation button altogether and have menu items created, updated and deleted as soon as the user creates, updates or deletes a Navigation Link block. I don't like this though as it means the user doesn't have a chance to review their changes before publishing them. It also makes implementing undo/redo really hard. |
|
With regards to 3:
We're already doing this, because in order to save a child item we need to know the id of its parent, so I don't think it should count as a disadvantage 😄 |
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
437928b to
f3476b1
Compare
adamziel
added
[Block] Navigation
adamziel
force-pushed
the
add/navigation-atomic-save
branch
from
e437c4d to
e29b067
Compare
|
As @talldan mentioned - I see these changes as a v1 concept to keep the experimental page going, not something that's supposed to be a clean public API. I added a few unit tests and will add more on Monday. |
I think the primary concern from myself and @spacedmonkey is whether the approach is the right one. I believe Jonny is concerned that this won't scale well and is of limited utility if we can't implement rolling back in a safe way. My concern is that I think this better lives at an infrastructure level, and shouldn't be tied to specifically to the menus controller itself. |
+1 to that |
noisysocks
added
[Feature] Navigation Screen
REST API Interaction
|
I'm closing this PR for now - handling this at the infrastructure level makes sense and it does not seem overly complicated. If it turns out to be, we could always revisit. I'm pondering updating the saving logic to use the existing customizer endpoint for now just to keep the experiment going - it's would be a pretty trivial change and would keep the experiment moving forward. Once the core update lands, switching to the batch endpoint would be trivial as well. |
Description
This PR enables saving the entire navigation using a single HTTP request.
The current behavior is sending one request per change at the moment. If you e.g. add 20 menu items, update 17 and remove 10, it will result in 47 http requests. This is pretty fragile and would easily break under a number of scenarios - validation errors, network failures, accidentally closing the browser, and so on.
The proposed implementation is as follows:
Other notes:
nav-menus.phpscreen.How has this been tested?
Tested locally
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: