Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
When moving menu items within the tree the new menu item position is not always saved, i.e. the plid of the moved item is not updated.
Steps to reproduce
- Browse to admin/structure/menu/manage/main-menu
- Create 4 menu items
- Set it up so that you have 2 parent links each with a child link and save the menu
- The 2 parent links should display the 'Show children' link
- Expand the 1st parent
- Expand the 2nd parent
- Drag the child of the 1st parent so that it is set as a child of the 2nd parents child item
- Save the menu again
- When the menu reloads the position of the moved child item has not been saved
It appears that when the ajax call is made to fetch the menu slice, as a menu link is expanded to display its children, the form is cached. When you save the menu the form is posted with the form_build_id that was previously cached and the form object is retreived from the cache within drupal_build_form. If this cached form does not contain the mlid element that is being moved then the item will not save it's new parent. This is because the only field values being checked within menu_overview_form_submit are the mlid elements within the form object.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | bigmenu-moved-menu-items-not-saving-1965350-3.patch | 1.83 KB | mike.darke |
Comments
Comment #1
dman commentedGreat issue report and analysis.
IIRC, this was an issue encountered early on, but there should be a flag, something like $form['#rebuild'] = TRUE or something (OTTOMH) that was supposed to deal with that.
Some of FAPI may have undulated in the background in the course of history.
There should be a way for in-place AJAX calls to invalidate the cache of the parent form in this way - if your analysis is correct - but I'll admit that a full understanding of the right way to do this may have passed me by at the time.
Hm.
Comment #2
mike.darke commentedThanks for the info. I have been investigating it a bit more.
The AJAX callback is rebuilding the form by manually calling drupal_rebuild_form, which then calls the bigmenu_overview_form handler, I think this includes the menu items for the current slice. However this doesn't include any links from previously expanded menu slices, so I'm not sure if $form['#rebuild'] = TRUE would help in this instance.
I've been able to create a fix for the issue (but it may not be the best approach!) by fetching the cached build of the form within the form handler bigmenu_overview_form, extracting the menu items that are in the previous build and merging them with the ones which have been fetched from the DB query. I'm going to have more of a test with it to see if I've introduced any issues and then if it seems ok, I will submit a patch for you to look at.
Comment #3
mike.darke commentedThis patch has fixed the issue that I was experiencing, but there may be a better solution. As I mentioned previously this is extracting the menu items from the cached version of the form (if it exists) which will contain any menu items that were fetched previously when expanding a parent item. This then persists the changes of any visible menu item.
Comment #4
acbramley commentedVery strange, this must have been some change in core that broke this functionality as it was certainly working before! I will investigate further, not sure if the cached form thing is the best way to go as it wasn't previously required in 6.x or 7.x
Comment #5
dman commentedThinking about it, I remember most of my standard testing being of the form:
Open tree 1
Open tree 2
Copy child of tree 2 into tree one (because that's where my mouse was)
Save.
This would leave the most recently loaded tree in active form memory, and its plid position in tree 1 is a property of the menu item being moved (tree 1 need not exist as form submission data)
But the opposite order - shifting an item from tree 1 into tree 2 MAY be invalidated if the theory is correct that the earlier expanded tree items gets lost from from cache. If this patch does fix the problem, it's strong evidence that this really is what is happening.
But yeah, I'm pretty sure that at other times a lot more random ordering must have been tested as well. It is odd that this behavior could have slipped through.
Comment #6
acbramley commentedYeah not really sure how this occured (I'm sure I would've tested this case when porting this module). Anyhow, patch works nicely and makes sense as we need to add the cached mlids to the $form so that the items are passed through to the submit function for saving. I've done some pretty aggresive testing with it (opening several levels at a time plus other parents, moving 2 or 3 items multiple levels deep etc).
Committed to 7.x-1.x, should this be backported to 6.x?
Comment #7
acbramley commentedClosing as fixed, committed and released new stable version