Reordering fails with more than 100 items in a menu

Sounds serious...

I've run into this issue, and have some insight. I've been using a contrib module called Taxonomy Menu that generates a menu from a taxonomy. I had a taxonomy with more than 100 terms, so the menu it created fell victim to the hardcoded delta=50 problem (basically, the weights in the database were stored appropriately, but when the menu was edited, the select dropdowns were capped at -50..50, so saving the menu from the editor overwrote all menu item weights above 50 with -50, destroying the previous order).

For more details, you can see the discussion on the issue in the Taxonomy Menu queue.

The practical solution here is to mimic the code used to build #delta in the taxonomy core module, which does not have a hardcoded value; instead, it has a do-while loop that increments delta until it's big enough to fit all the items in the taxonomy. The relevant code is in the taxonomy_overview_terms() function (I'm referencing Drupal 7 here, but it should be the same in 8).

The relevant functions in the menu module should be menu_overview_form() and _menu_overview_tree_form() (the latter contains the hardcoded #delta=50).

Here's a first shot. I patched against Drupal 7.10, but I believe the relevant functions are the same in 8. Please give me some guidance if I've done something wrong.

I basically added a parameter ($delta) to the recursive helper function _menu_overview_tree_form() which builds the menu tree form. $delta first gets set in menu_overview_form() to the total number of links in the menu.

Let's try that again. Patching against 8.x this time, properly using git. Changes against 8.x are the same as for 7.10 (except for the file location).

+++ b/core/modules/menu/menu.admin.incundefined
@@ -88,8 +89,10 @@ function menu_overview_form($form, &$form_state, $menu) {
+ *   A number indicating the total number of items in this menu; used to generate the range for the menu weight selector.

Comments can't exceed the 80 character limit.

Looks good otherwhise on the PHP side.

Re-rolled the patch with the offending comment shortened to fit the 80 char limit.

Thanks @figureone. Looks like something we'd need to fix in D7, so we should make sure it's backportable:

+++ b/core/modules/menu/menu.admin.incundefined
@@ -88,8 +89,10 @@ function menu_overview_form($form, &$form_state, $menu) {
  * @param $tree
  *   The menu_tree retrieved by menu_tree_data.
+ * @param $delta
+ *   The total number of items in this menu; used in the menu weight selector.
  */
-function _menu_overview_tree_form($tree) {
+function _menu_overview_tree_form($tree, $delta) {
   $form = &drupal_static(__FUNCTION__, array('#tree' => TRUE));
   foreach ($tree as $data) {
     $title = '';
@@ -108,7 +111,7 @@ function _menu_overview_tree_form($tree) {

@@ -108,7 +111,7 @@ function _menu_overview_tree_form($tree) {
       );
       $form[$mlid]['weight'] = array(
         '#type' => 'weight',
-        '#delta' => 50,
+        '#delta' => $delta,
         '#default_value' => $item['weight'],
         '#title_display' => 'invisible',
         '#title' => t('Weight for @title', array('@title' => $item['title'])),

In order for this change to be backportable, we'd need to provude a default value of 50 for the $delta parameter.

Also, we'll need an automated test that duplicates the bug. It should fail without the patch here and pass with it. Thanks!

Oh, actually--I just looked at #589440: Reordering fails with more than 31 book pages in a book and it sounds like that is no longer an issue in D7 or D8. Can someone confirm whether this is still an issue in 7.12?

If not, we'll bump it back to D6 (and not worry about the tests).

Tagging.

Manual test on a clean 7.12 (only Devel module installed) results are fine. So I made a menu with 121 items with depths up to 3 levels. All reordering went on fine, saved properly and all. After doing the same for taxonomy and taxonomy menu, same behaviour, no errors.

1. Did a clean install of D7.12, Standard, English
2. Add 121 menu items into one menu, max 3 levels deep using Devel
3. Stay lazy, do not disable Devel
4. Do a bunch of reorders using the drag & drop controls
5. Save after each, check if they display OK on admin page and on a test page
6. Everything OK in every instance
7. Add an empty menu
8. Enable taxonomy menu
9. Devel 150 tax items
10. Build a taxonomy menu
11. Reorder tax terms apply to menu
12. Revert to alphabetical, sync menu

Thanks @Ranko!

Verified: it's been fixed in Drupal 7.12. The error was being caused in the menu editor by the html select form element being limited to a range of -50..50 (because delta was hardcoded to 50). If a menu item weight was outside that range, overflow would occur.
In Drupal 7.12, the select form element has been replaced by an input[text] form element, which has no meaningful bounds.

EDIT: Scratch that, sorry for my part in making this thread confusing. It appears the issue remains in 7.12 unless my patch from #7 is applied. I'm going to try to reproduce what Ranko did in #12 and see if I can figure out where we are having a miscommunication. More in a bit...

I wonder if maybe we can simply remove the hardcoded delta in D7/8? That would be a non-major cleanup/followup issue, though.

Note that the delta change should also be applied to menu_edit_item() form.
+1 for removing the hardcoded delta.

Followup: issue still exists in Drupal 7.12.

I reproduced the steps Ranko tried in comment #12 and found that his steps did not reproduce the problem. Using devel_generate to create a menu of 121 menu items or 150 taxonomy items will not create a menu or taxonomy with more than 100 items at any given depth. In fact, I don't see a way to make devel_generate do that--it always tries to create less than 50 items at any tree depth.

To clarify, the problem occurs when more than 100 items exist in the root of the menu or under a single parent item, or items exist with a weight outside of -50..50 (causing overflow when trying to fit that weight into the select form element).

Further problems occur in conjunction with the Taxonomy Menu module, which relies on the taxonomy item weights which are not bound by delta=50 like they are in menu. See #2 for more details on that one.

I'm going to update the bug version to 8.x-dev again, and we should probably add back in the "needs backport to D7" and "Needs tests" tags. I'll work on updating the patch to include tests, a default value for $delta, and hunt around for other occurrences of delta elsewhere in the menu module.

I'm pretty sure devel can do a lot of items, from the test datbase:

But let's do this the other way around. Can you give me steps to reproduce the problem, one by one? Heck, let's get silly and I'll do my best to emulate it on your OS and browser version.

Gotcha, here are the steps to reproduce (I found a way to do it with devel_generate using a 4th argument).

1. Clean install of Drupal 7.12 (on CentOS 5.2, browsing using Chrome 17.0.963.46 on OSX Lion)
2. Install devel module and devel_generate
3. Install drush
4. Use drush to create 1 menu with 200 items, 1 level deep, all 200 at the first level:
   
   drush genm 1 200 1 200
5. Edit the menu via the browser: /admin/structure/menu/manage/devel-{yourmenuname}
6. Click "Show row weights" to see the select form element for each item (the one that ranges from -50..50). Notice that some weights are the same for different menu items, because there are more menu items (200) than available weights (101).
7. Go ahead and save the menu. You'll now notice that the weights start at -50 for the first menu item, and count up by one for each menu item. Once you reach 50, you'll notice that all the remaining menu items are at a weight of 50. Now all the menu items with a weight of 50 will be sorted alphabetically, and trying to change their order (via tabledrop) does not work properly.

Hope that's clear!

I think this part is the cause of the issue: Show row weights, at that point the default values show up and mess things up. I was looking at my data in the database directly and I could see values of above 50 (100+). Once I display the numbers and save, those values go in. I'm a bit overbooked now, but I'll try to verify on Wednesday.

Unrelated, and I know not how it works in Drupal so it might not apply: the drop down values should be dynamically generated from the number of values in that particular taxonomy, menu or other sortable element. IIRC SharePoint does it like that in appropriate places (column ordering of lists, document libraries, etc.) and at least a few custom systems I work with do.

Yes, Show row weights is at the heart of the issue. The html select element (dropdown menu) for the row weight is created based on the "delta" value discussed in the thread above. That value is currently hardcoded at 50, so the select element ranges from -50..50. When you have an element with a weight larger than 50, it's an overflow error, and making any modifications to the menu from the editor interface will overwrite the out-of-bounds row weights.

I've updated my patch to include backportability and added test cases (thanks xjm). The new test cases (I just added them to menu.test) try to create a menu of 102 items and assign increasing weights to each element, and fails when trying to set a weight of 51 because the select element caps at 50 (because delta=50). The patched code (in menu.admin.inc and menu.module) removes the hardcoded 50, and adjusts it to match the total number of items already in a menu (or 50 if there are less than 50 items).

Patch is against 8.x-dev, but should be directly applicable to Drupal 7.x-dev and 7.12. Note that this is my first time submitting a patch to core, and first time submitting test cases, so please let me know if I haven't followed protocol.

I'll try to test it ASAP, but wanted to ask if it would be possible and not a bad idea to drop the whole -50 to 50 scale and just go for the current taxonomy/menu/whatever is being weighed count?

Yeah, that definitely seems like a better idea... we should look at how taxonomy, book, etc. do this and do it the same way.

That's what the patch in #22 does--it calculates delta based on the size of the menu in question. It does, however, fall back to a delta of 50 if the size of the menu is under 50 items.

Additionally, I've noticed that the select form element for row weights will change to an input[type=text] if delta is large. I haven't traced the code to see at what point this happens (mostly because I have no problem with the form element changing).

Additionally, I've noticed that the select form element for row weights will change to an input[type=text] if delta is large. I haven't traced the code to see at what point this happens (mostly because I have no problem with the form element changing).

This is a performance feature added in #1346760: Add a scalable weight select element, so it will happen automatically when you use the weight element.

Here's how it's done in other modules:

• _book_admin_table_tree()
• taxonomy_overview_terms()

Bumping this issue--I think the patch in #22 is ready to be committed into core. xjm and Ranko, agree?

How's the backport to D7 doing? Latest release 7.14 still has this issue.

Here's the D7 backport of #22. All I did was change the paths in the patch (i.e., core/modules/menu -> modules/menu); the D8 patch applies cleanly to D7.

P.S. I followed the backporting instructions here, please let me know if I did anything wrong.

This still needs manual testing for D8. Attaching a D7 backport now just confuses the testbot.

Reroll for PSR-0 tests.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -244,6 +244,14 @@ class MenuTest extends WebTestBase {
+    // item's weight doesn't get changed because of the old hardcoded delta=50

Missing a full stop, the "delta=50" should probably have quotes or spaces or something

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -287,9 +295,10 @@ class MenuTest extends WebTestBase {
+   * @param string $weight Menu weight

All of these are against coding standard, not sure if this should stay the same or not.

+++ b/core/modules/menu/menu.admin.incundefined
@@ -61,6 +61,7 @@ function menu_overview_form($form, &$form_state, $menu) {
+  $delta = count($links) > 50 ? count($links) : 50;

Why not $delta = max(count($links), 50);

+++ b/core/modules/menu/menu.admin.incundefined
@@ -90,8 +91,11 @@ function menu_overview_form($form, &$form_state, $menu) {
+ *   The total number of items in this menu; used in the menu weight selector.
+ *   Note: Defaults to 50 for backportability.

This should be combined into once sentence, the second half being ", defaults to 50."

+++ b/core/modules/menu/menu.admin.incundefined
@@ -352,10 +356,21 @@ function menu_edit_item($form, &$form_state, $type, $item, $menu) {
+  // Get number of items in menu so the weight selector is sized appropriately

Missing full stop

+++ b/core/modules/menu/menu.admin.incundefined
@@ -352,10 +356,21 @@ function menu_edit_item($form, &$form_state, $type, $item, $menu) {
+  if ($delta < 50)
+    $delta = 50; // Old hardcoded value

This needs curly braces, and the comment shouldn't be inline, and needs a full stop.

+++ b/core/modules/menu/menu.moduleundefined
@@ -704,10 +704,21 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  // Get number of items in menu so the weight selector is sized appropriately

Missing full stop

+++ b/core/modules/menu/menu.moduleundefined
@@ -704,10 +704,21 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  if ($delta < 50)
+    $delta = 50; // Old hardcoded value

This needs curly braces, and the comment shouldn't be inline, and needs a full stop.

I followed the steps posted in http://drupal.org/node/1007746#comment-5586670 with 125 items in a menu and it worked as expected. I was able to reorder the menu items by both drag and drop and by showing the weights. The weights shifted to accommodate the new order on save, increasing to over 50 where necessary.

Mickey

Here is the patch with changes based on tim.plunkett's comments. I didn't address his second note as I couldn't come up with anything solid for that. I did add a note to the issue summary to give this more attention. Everything else has been addressed.

Oops sorry all, I hadn't pulled recently, let's try this again.

#33 shows the automated test works, and it was manually tested in #35.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -287,9 +295,10 @@ class MenuTest extends WebTestBase {
    * @param integer $plid Parent menu link id.
    * @param string $link Link path.
    * @param string $menu_name Menu name.
+   * @param string $weight Menu weight
    * @return array Menu link created.

I'm ignoring this because fixing it would mean fixing all the code around it, and that's out of scope.

Committed and pushed to 8.x. Thanks! Marking to 7.x for backport.

Here's the D7 backport of #37.

Looks like a good backport!

This patch is going to cause some disruption since it will change select boxes to textfields in certain places, and I think we need to minimize that. But currently, the patch doesn't do that and actually seems to be calculating the deltas incorrectly...

As an example, on a fresh installation of Drupal 7 with the standard profile and this patch applied, the Management menu has a range of weights that goes from -60 to 50. Thus, at least for the menu_edit_item() and node forms, I would think the delta for that menu should be 60 (or at most 61). This would put it under the limit to keep it as a select box, and still allow you to choose any relevant weight. However, the code in this patch results in a delta of 228 (total number of items in the menu) which doesn't make sense to me, and therefore results in the weight field turning into a textfield when it doesn't seem like it should need to.

For the menu_overview_form() case, since that shows all menu links on the same form I guess it might still be necessary to base this on the total number of menu links (to deal with the pathological case where someone tries to take a hierarchical menu like Management and rearrange a bunch of links to all be on the same level all at the same time - in that case they do need the full range of weights). But even then, shouldn't it be based on count($links) / 2 rather than count($links)? And shouldn't it also check the actual min/max of the menu items in the database as a fallback rather than only checking their number (to guard against a random weight being a lot lower or higher than the others)? In short, shouldn't it do something a lot closer to what _book_admin_table_tree() is currently doing?

Also:

+  // Get number of items in menu so the weight selector is sized appropriately.
+  $sql = "SELECT COUNT(*) FROM {menu_links} WHERE menu_name = :menu";
+  $result = db_query($sql, array(':menu' => $item['menu_name']), array('fetch' => PDO::FETCH_ASSOC));
+  foreach ($result as $row) {
+    foreach ($row as $menu_item_count) {
+      $delta = $menu_item_count;
+    }
+  }
+  if ($delta < 50) {
+    // Old hardcoded value.
+    $delta = 50;
+  }

I was pretty confused by this; it gives the impression that $delta could wind up undefined after the foreach() loop (although in practice maybe it can't).

Is there a reason not to simply use db_query()->fetchField() instead? Although with other things I pointed out above, it would be more like this in the end:

  $weight_info = db_query("SELECT MAX(weight) AS max_weight, MIN(weight) as min_weight FROM {menu_links} WHERE menu_name = :menu", array(':menu' => $item['menu_name']))->fetchObject();
  $delta = max(abs($weight_info->min_weight), abs($weight_info->max_weight));
  if ($delta < 50) {
    // Old hardcoded value.
    $delta = 50;
  }

Also some minor things, not enough to hold up the patch on its own, but anyway:

+    // Add 102 menu links with increasing weights, then make sure the last-added
+    // item's weight doesn't get changed because of the old hardcoded delta=50

This seems to be missing @tim.plunkett's feedback from #34 (delta=50 missing either spaces or quotes, and no period at the end of the sentence).

+ * @param $delta
+ *   The default number of menu items used in the menu weight selector is 50.

There's something odd about this sentence - it's describing the parameter and default all in one breath. Maybe change to "The number of items to use in the menu weight selector. Defaults to 50."

Here's David_Rothstein's comments rolled into a patch against D8. If this gets the go-ahead, I can modify the D7 backport patch so we can finally get this into production.

Argh, I truncated a line in that previous patch, sorry. Here it is again.

Let's do this one more time. It's been one of those days...

I am curious if there's been any progress on this. I ran into this problem when upgrading a D6 site to D7.17 (it had a very long secondary menu). I applied the patch in comment #41 but it did not resolve the problem with the menu reordering. Menu items cannot be reordered or disabled.

Fixing version.

The patch in #41 for D7 does not work when the number of menu items exceeds about 400.

I tried a few of these patches against Drupal 7.19 but ran into some "patch failed" errors... granted, it's probably due to it being 7.19.
It appears this is not yet in core. If this works, how can we get it into 7.20?

This issue needed a rerole against the menu_link entity and the entity form controllers.
It seems to be like this is the first instance of a aggregated EQ in core.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -567,6 +567,9 @@ protected function moveChildren(EntityInterface $entity) {
+   * @return int
+   *   Returns the total number.

number.. of what? :P

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -575,4 +578,23 @@ public function countMenuLinks($menu_name) {
+   * Returns the maximum range in weights for a given menu..

Extra trailing period.

Other than those small doc nitpicks, looks fine to me.

Thanks for the review!

All good now.

This patch is going to cause some disruption since it will change select boxes to textfields in certain places, and I think we need to minimize that. But currently, the patch doesn't do that and actually seems to be calculating the deltas incorrectly...

So does #60 fix a bug then? Does said bug need tests? It seems like we're doing some normal cleanups on the committed D8 patch, and then backporting the cleaned-up major to D7?

So, this does need test coverage:

This patch is going to cause some disruption since it will change select boxes to textfields in certain places, and I think we need to minimize that. But currently, the patch doesn't do that and actually seems to be calculating the deltas incorrectly...

As an example, on a fresh installation of Drupal 7 with the standard profile and this patch applied, the Management menu has a range of weights that goes from -60 to 50. Thus, at least for the menu_edit_item() and node forms, I would think the delta for that menu should be 60 (or at most 61). This would put it under the limit to keep it as a select box, and still allow you to choose any relevant weight. However, the code in this patch results in a delta of 228 (total number of items in the menu) which doesn't make sense to me, and therefore results in the weight field turning into a textfield when it doesn't seem like it should need to.

If we're fixing that bug, then we need expand the test coverage to catch that bug. We should also add a unit test for the new method.

Also, @Dries suggested that the method could use an inline comment to explain the logic.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -575,4 +578,23 @@ public function countMenuLinks($menu_name) {
+  /**
+   * Returns the maximum range in weights for a given menu.
+   *
+   * @param string $menu_name
+   *   The unique name of a menu.
+   *
+   * @return int
+   *   Returns the needed range.
+   */
+  public function menuLinksRange($menu_name) {
+    $query = entity_query_aggregate($this->entityType)
+      ->aggregate('weight', 'MAX')
+      ->aggregate('weight', 'MIN')
+      ->condition('menu_name', $menu_name);
+    $result = $query->execute();
+    $item = reset($result);
+    return max(abs($item['weight_min']), abs($item['weight_max'])) + 1;

Four core developers just spent 15 minutes talking on the phone through what this method actually does, so we could really use some documentation here. ;) I'd suggest more detailed information in the docblock and for the return value, as well as some inline comments (particularly a comment explaining the last line that makes the range symmetric). Also, the word "range" does not really capture what it is that this function returns--a recommended size to use for a select widget--so we should probably change the method and parameter names to clarify this somehow.

Finally, @Dries also suggested that it's a bit confusing for the logic related to this to be happening in two places. We have this function to return one recommended range, and then we do another check in menu_overview_form() and elsewhere in the menu system to compare it to 50, and then the form API implements yet more magic for form_process_weight(). It would be better to have all the magic happen in one place that could be reused throughout the menu system and also for the other places in core this same bug has been fixed (_book_admin_table_tree() and taxonomy_overview_terms(). Since the query is menu-specific, maybe we need a function or method that takes a minimum and maximum value and returns the recommended weight widget size?

Also, let's update the issue summary here with the current status, including what I eventually figured out in #62 and #63. ;)

Sorry for wasting 15min, sometimes communication is the strategy to save time :(

Let's write a test first and explain it better. It's actually suprising how this got in without explicit test coverage for the menu link storage controller.

Finally, @Dries also suggested that it's a bit confusing for the logic related to this to be happening in two places. We have this function to return one recommended range, and then we do another check in menu_overview_form() and elsewhere in the menu system to compare it to 50, and then the form API implements yet more magic for form_process_weight(). It would be better to have all the magic happen in one place that could be reused throughout the menu system and also for the other places in core this same bug has been fixed (_book_admin_table_tree() and taxonomy_overview_terms(). Since the query is menu-specific, maybe we need a function or method that takes a minimum and maximum value and returns the recommended weight widget size?

It seems to be useful to at least have this method, as it has one single purpose and get rid of the +1 on there.
On top of that we could add the logic which exists in various places?

I was thinking maybe a generic procedural function that takes a minimum value and a maximum value and returns the recommended FAPI delta based on that.

edited comment

Updated issue summary.

#65: drupal-1007746-65.patch queued for re-testing.

Just fixes the other tests, though I'm not really sure how the perfect solution looks like.

Is #41 the latest patch for D7?

Yes, #41 is the one I run on my production websites since this issue is having trouble making it into core.

Thanx Paul. I needed to point people from #2022141: Menu ordering not saved to a place with a working patch in order to see if the cause of the issue over there is actually this core bug here. Your confirmation that this works for you in production is encouraging for others to test the available patch. Thanx again.

Just for anyone on D7 struggling with this problem, this module helps you to circumvent it: https://drupal.org/project/menuadminsplitter

This is terrific -- not only solved the problem, but makes administering huge menus much easier. Thanks!

Let's reroll the patch.

That was easy to fix. In general I am wondering whether this is really to be considered as a major bug.

why does taking out the schema for fields help?

why does taking out the schema for fields help?

Because those tables do no exist anymore.

And a review:

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
@@ -107,4 +107,18 @@ public function countMenuLinks($menu_name);
+   * So for example there is a menu with a link with weight -5 and one with 4,
+   * it should return "5".

Shouldn't it return 4? At least that's what the dedicated test seems to expect :)

I think we can leave "so" from documentation sentences, how about "For example: if a menu contains two links, one with a weight of '-5' and the other with a weight of '4', it returns '4'."

But, after reading all this docblock, I think this function should actually be called getMaxWeight() because it has nothing to do with the menu size.

(Ignore me, I just need a 7.x version of this patch)

adding issue summary

Edit: I, hefox, did not write this comment or edit the issue summary.

We do use the following code in the UI fnow:

    // Determine the delta; the number of weights to be made available.
    $count = function(array $tree) {
      $sum = function ($carry, MenuLinkTreeElement $item) {
        return $carry + $item->count();
      };
      return array_reduce($tree, $sum);
    };
    $delta = max($count($tree), 50);

so I consider this as be fixed.

I don't think it's fixed in d7 (has needs backport to D7 tag)?

I agree I'd love a backport if possible

I "hopefully" queued up the patch that hefox wrote. I just added that to D7 and works like a charm. Please can we port this on the latest Drupal release!

I tried patch #86 but it doesn't seem to work for D7 (7.31). Is there any solution or more steps to fix this issue? Thanks.

Patch #86 seems to be an incomplete re-roll of patch #41. In particular, #86 only modifies menu.admin.inc, and not menu.module and menu.test.

I've just run into this issue, in a D7.32 installation.
I'm taking a look at the #41 patch and the #44 comment.

The patch at #41 seems incomplete as #94 comment says, but it looks like a good starting point to look at, isn't it?

Reading this comment #44:

This patch is going to cause some disruption since it will change select boxes to textfields in certain places, and I think we need to minimize that. But currently, the patch doesn't do that and actually seems to be calculating the deltas incorrectly...

That happens because system.module defines DRUPAL_WEIGHT_SELECT_MAX = 100. and form_process_weight() in form.inc takes in account that define, so, it shouldn't be a problem to have those fields as text, right?

Do you guys think that #41 patch is a good aproach to work further on? And maybe complete that patch taking in account the menu.module and menu.test?

I'm also wondering if the drupal instalation should take in account (if possilble) to set the variable 'drupal_weight_select_max' (DRUPAL_WEIGHT_SELECT_MAX) to a proper number, maybe based on the current created menus, and then increase that variable again (if needed) every time we use the menu UI?

What do you think?

Hi,
I've just create a patch taking in account all work from @figureone (awesome work) and trying to get in account comments from @David_Rothstein and @dawehner.
Probably I missed some things, but I'll try to sumarize the issue, so you guys can test it and comment it and hopefully change it to a final commit.

The main problem is that the delta value of the weight "property" is hardcoded to 50, so if we have more than 100 elements (-50 to 50) we won't be able to sort items in the menu for higher weights.

There is different approches to get the delta value from the database, some involve to query "SELECT COUNT(*) FROM {menu_links} WHERE menu_name = :menu" which give us a count of all links in this table for a certain menu, which is probably more than 200 results in a clean instalation for the management menu leading to have a delta value higher than 100 which leads to have the weight widget as a text field instead of a select list @see form_process_weight() in form.inc file and DRUPAL_WEIGHT_SELECT_MAX in system.module for more info about it.

Because I would like to have a select list for selecting weights in a clean drupal instalation, I've chosen the other aproach, wich is quering the min and max values from the wheight field in the menu_links table, then make the abs value and sum 1 more "point" to that weight, just in case I need to set one item above or bellow the max or min value as @David_Rothstein recommended in #44.

I'm also doing another two things:

1. Use db_query and -> fetchObject() for getting the results instead of 'fetch' => PDO::FETCH_ASSOC. I'm not pretty sure why we should use one or another, so feel free to change it.
2. Take in account all possible parent (root) menus that an item could have since, sometimes we can add an item to a menu different that the one we are editing (i.e. go to "/admin/structure/menu/manage/main-menu" press "add link", and place the link into the management menu).

Patch will comprises these three files:
menu.admin.inc

• menu_overview_form()
  Here we need to get the max and min weights for the menu we are viewing and set delta value to abs(max, min) + 1, just in case we need to set one item to a max weight + 1 or min weight - 1. Then call to _menu_overview_tree_form with the proper delta value passed as parameter.
• _menu_overview_tree_form()
  Just add our new delta value to weight, instead of the old hardcoded value (which was 50)
• menu_edit_item()
  Here we are editing an item which could have different parent (root) menus, so we take in account all those and get the max min value, Then, as before, set our "dinamic" delta to the form weight.

----------------------
menu.module

• menu_form_node_form_alter()
  When adding a node we have the choice (if user perms and node type config allow it) to add a menu link for that node. Here we also could choose the weight, and we should be able to set it. Then we need same logic as before. Get all posible menu parents and then calculate their max min abs weights + 1, so the node could fit in the right position.

----------------------
menu.test

• doMenuTests()
  Which will add 102 menu links and see if the weight is correct for the number 51 (it should be 51), so we make sure that the old code is being correctly replaced.
• addMenuLink()
  We just add a new parameter to this function (weight) for making the call from doMenuTests and take in account the new weight variable wich is now dinamic, because before it wasn't tested.

Finally I don't know much about testing so I've tried to add the code for the menu.test but I'm not confidence about it, so, feel free to change it, test anything or change anything. Comments will be very welcome and let see if we can have a drupal menu with more than 100 elements properly ordered, which could be really good, although I don't know if menus whith that amount of elements are good... ;)

Hi,
I just realize (after 2 hours) that the menu_overview_form that I commited had one error, condition for delta < 50 was not added.
But most important thing is that we can change the select for selecting those link_path that don't have placeholder "%" since those will never be in the menu (UI) AFIK. Then we can make a select taking in account that condition (escape '%' is needed), and the fallback for max min weights just in case.

So code will be like:

<?php
function menu_overview_form($form, &$form_state, $menu) {
  global $menu_admin;
  $form['#attached']['css'] = array(drupal_get_path('module', 'menu') . '/menu.css');
  $sql = "
    SELECT m.load_functions, m.to_arg_functions, m.access_callback, m.access_arguments, m.page_callback, m.page_arguments, m.delivery_callback, m.title, m.title_callback, m.title_arguments, m.type, m.description, ml.*
    FROM {menu_links} ml LEFT JOIN {menu_router} m ON m.path = ml.router_path
    WHERE ml.menu_name = :menu
    ORDER BY p1 ASC, p2 ASC, p3 ASC, p4 ASC, p5 ASC, p6 ASC, p7 ASC, p8 ASC, p9 ASC";
  $result = db_query($sql, array(':menu' => $menu['menu_name']), array('fetch' => PDO::FETCH_ASSOC));
  $links = array();
  foreach ($result as $item) {
    $links[] = $item;
  }
  $link_count = db_query("SELECT COUNT(*) AS counter FROM {menu_links} WHERE menu_name = :menu AND link_path NOT LIKE :link_path", array(':menu' => $menu['menu_name'], ':link_path' => '%\%%'))->fetchObject();
  $counter = intval($link_count->counter / 2 ) + 1;
  $weight_info = db_query("SELECT MAX(weight) AS max_weight, MIN(weight) as min_weight FROM {menu_links} WHERE menu_name = :menu", array(':menu' => $menu['menu_name']))->fetchObject();
  $fallback = max(abs($weight_info->min_weight), abs($weight_info->max_weight)) + 1;
  $delta = $fallback > $counter ? $fallback : $counter;
  if ($delta < 50) {
    // Old hardcoded value.
    $delta = 50;
  }

  $tree = menu_tree_data($links);
//...
?>

For the menu_edit_item and the menu_form_node_form_alter I think the code is ok, since those functions will only acept a weight based on the current items, so there is no problem to take in account only max min weights...

I added the patch with these changes, and I think it is working fine.

Sorry I missed to change '#delta' => 50 in menu_form_node_form_alter, but now... YES!

Please be aware of #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items), since if you have a big menu, it will probably affect you. You will notice of this issue because menu overview will not submit the changes (i.e. you change the position of one item).

The latest patch #98 works well on my menu with over 100+ menu items. Saving the new order does not break the ordering. Thanks!

looks like the recent patch is a backport to 7 so removing the needs tag.

Our team ran in to this issue and really didn't want to hack core to get a solution, so we thought about other ways to attack the problem. As a temporary workaround, we found a hook_form_alter seemed to do the job rather well. It doesn't address the max_input_vars issue, but that can pretty easily be changed in the .htaccess or php.ini.

It requires an additional database query per load of an admin menu page. Seeing as that is a pretty infrequent task, and we are talking about sites with lots of content, that tradeoff seemed reasonable.

We posted the code up here: https://www.drupal.org/project/menu_admin_fix

As far as I can see, this issue is related: #2332785: Textfield weight increases until int(11) size is reached when tabledrag is used with a passing 676 bytes slim patch.

stephen.colson: Patching core is not hacking core -- it allows the patch to be tested/reviewed by more people, increasing the chance it gets into core, which benefits all who would have encountered the issue!

Oh, don't get me wrong, a patch is absolutely critical to fixing the core issue. But in the mean time, plenty of people don't want to have to own re-patching every time there is a core update. So the module we put up helps bridge that gap.

FYI...I tried the patches in #41 and #98, but neither worked for me on my D7.36 site with 373 menu items.

Patch #98 no longer works for us either unfortunately, so I think we still need work here.

@gmclelland And @jamesdixon

Could you two provide a bit more details in how exactly it's not working now? Maybe steps to reproduce would help others who are creating patches to resolve this issue.

@joelpittet - I'm going to /admin/structure/menu/manage/cmf-sitenav and then I reorder one menu item up in the hierarchy and click "Save". After the page reloads, it's as if I never made any changes. FYI.. between testing the patches I did run drush cc all a couple of times.

I'm running Acquia Dev Desktop that is using php 5.3.18(mod_php), MySQL 5.1.66, and testing with Google Chrome on a mac.

I've also tried using PHP 5.4.8(mod_php) with no luck, but I did see the following warning with this php version:
Warning: Unknown: Input variables exceeded 1000. To increase the limit change max_input_vars in php.ini. in Unknown on line 0

No js errors are reported in the console and no php errors/notices reported at /admin/reports/dblog.

I also tried https://www.drupal.org/project/menu_admin_fix with no luck.

So far the only way I can reorder menu items is by using https://www.drupal.org/project/bigmenu or editing a menu item individually and changing it parent.

Hope that helps

With PHP 5.4.8 I was able to change the php.ini file setting from max_input_vars=1000 to max_input_vars=50000.

After that I restarted apache and I was able to reorder menu items with just Drupal core, but I only have 373 menu items in this menu. I'm not sure how it would behave with a lot more menu items.

Thanks @TuWebO - thanks for pointing out #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)

@gmclelland the scaling issue may be related but likely a different issue than the weight and may need a new issue for that.

Currently I use this module to help with scaling both from UX and scaling: big_menu

And thank you for the details!

@joelpittet just a little plug, these also help UX-wise with large menus:

https://www.drupal.org/project/menu_link_weight
https://www.drupal.org/project/hierarchical_select or https://www.drupal.org/project/shs

Hi,
Thanks for the review. Comments from #105 and #106, @gmclelland and @jamesdixon, could we trace a little what happened?

1. If reorder of the menu items fails in the UI (when you save the form, the menu items does not change the order) could be related to the issue commented on #98 (#1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)). This happened to me because I was wrong about the menu items I really had (it was a multilingual site).
2. If reorder the menu items is ok when you save the form but the maximun and minimun weights are wrong (no more/less than +-50 when they should) the problem could come from this issue/patch. You can test this situation by editing again the form and clicking on "Show row weights" for checking them out.

Is it happening any of these two things to you guys, or something different?

@TuWebO - Just to be clear, yes the problem I was having in #105 is only related to #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items) and not this issue.

Hi gmclelland,
thanks for the feedback,
Since it's been a week or so and @jamesdixon has not provided any feedback, I will set the status to were it was before, feel free to change it if you thik so.

Thanks for the patch, #98 works for me as well.

Have there been any recent updates on this issue? I tried the patch in #98 and it doesn't seem to work in my D7 site. I have a menu with ~220 items in it.

Hi @ekidman,
It should work as far as I know.
If it doesn't work for you, could you tell us exactly what happend or the steps to reproduce it?
Also make sure to double check this issue (linked to the most relevant comment) #1517092-6: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)

Thanks for the review.

I actually ended up solving the issue, but didn't get a chance to post back. In my case, I was using the Big Menu module. I had to go into the bigmenu.admin.inc file and increase the delta value there.

#99 is working for me. I noticed that the limit is not absolute at 100 items, but seems to be 100 items in a menu level. You could have 200 items evenly spread out in a 4 level menu and not need this patch.

#100 is max for value that we assign to manage the weight. Anyone suggest if menu items are more then 500 or more than that how we manage them.

Will this make it into core at some point? It's clearly been a long time coming, and the final patch (#98) seems to address the issue with no ill effects.

Unfortunately, the last patch does not solve the issue.

But this https://www.drupal.org/node/1517092#comment-6945642 is working for me.

See discussion above - that's a different bug than what the patch here is trying to fix.

Added #2662266: Form submit with truncated $_POST re-renders silently for the max_input_vars related fix.

In our case it seems that one particular parent menu item was acting kind of problematic. Deleting that parent item and adding a new one resolved this.

This seems good and solves a major issue. Any other problem?

Only thing: code to calculate delta should be in a function, probably a private one.

$link_count = db_query("SELECT COUNT(*) AS counter FROM {menu_links} WHERE menu_name = :menu AND link_path NOT LIKE :link_path", array(':menu' => $menu['menu_name'], ':link_path' => '%\%%'))->fetchObject();
  $counter = intval($link_count->counter / 2 ) + 1;
  $weight_info = db_query("SELECT MAX(weight) AS max_weight, MIN(weight) as min_weight FROM {menu_links} WHERE menu_name = :menu", array(':menu' => $menu['menu_name']))->fetchObject();
  $fallback = max(abs($weight_info->min_weight), abs($weight_info->max_weight)) + 1;
  $delta = $fallback > $counter ? $fallback : $counter;
  if ($delta < 50) {
    // Old hardcoded value.
    $delta = 50;
}

#98 Works for me.

#98 did not work for me. I had to resort to #109

Hi crisgross. Several people have reported that patch work for them but you say in #130 that patch didn't work for you. Could you explain why is failing? Errors, unexpected behavior, warnings, etc.

@tunic. No change in behavior from before the patch. PHP Warning: Unknown: Input variables exceeded 1000. To increase the limit change max_input_vars in php.ini. in Unknown on line 0,

Thanks for all the work on this, this makes a lot of sense.

Please add a helper function to menu.module called something like:

_menu_get_maximum_delta($menu_name);

or something like that.

@chrisgross - see #112 and related comments. You're likely running into a problem with #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items), not this issue.

Has a solution been found for this? The patch in 98 doesn't seem to help in my case. I have a website that uses domain_access to manage several domains that might have one or more languages. I am using menu translation (i18n_menu) for multilingual aspect and menu language filter module for filtering menus for different languages.

The problem is that sorting say the main menu for one domain might end up shuffling the main menu for another, and fixing it for the other one might rearrange the main menu for some other. Not sure yet if saving a new node with a menu enabled has any effect or not. Is the only way to use menu_blocks or is it possible to fix it? If the issue is simply the weight field, would increasing its range help? Or in my particular case of multilingual menu, would it not help to include language as a filter criteria before rearranging?

Any help in this regard would be much appreciated.

#135: That sounds to me like a bug in domain module.

If you have less than 50 items in a menu and the bug still happens, then it certainly is not this issue.

Hei Fabianx, thanks for the prompt reply. I don't have less than 50 items in the menu, on the contrary I have hundreds since every menu item goes in the same 'main menu' regardless of the language.

@rockaholiciam
see 134.
There's 1) the issue with drupal and 2) the issue with default php config

Hei rv0, I have looked at it and already encountered and resolved the php issue at some point by increasing max_input_vars. Then, the form would simply refuse to do anything. However, as of now, it does submit it, it does reorder the menu in the particular language you want, but it can shuffle some other languages menu around at the same time since they all lie at the root of main menu.

One workaround I can think of is to add an additional menu item at root of each language menu and moving each languages menu under it signifying what language it belongs to, and stripping it off before render. That would likely work but not a clean solution.

I ran into this issue today after adding about 5 more menu items, although I'm only at 67 items, and I am having trouble getting items to go where I need them to.

Reroll patch from #98 but moving repeated code to functions, so it has no functionality changes (I hope). See my comment on #128.

Come on, this issue should be about to be closed....

@tunic should it? It seems to me that a change to php.ini is still necessary, which is a workaround, not a permanent solution. I definitely have fewer than 1000 menu items, but I still had to change the php setting. Shouldn't this be mitigated in core itself, perhaps through batch processing or something?

Hi,
@chrisgross please read carefully comments on #112, and your own comments in #132 and the response on #134, otherwise people will get confused.

This issue is nothing to do with 1000 menu items, for that one take a look at #1517092-6: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)

Hope this helps.

@TuWebO I am not confusing the issues at all. The other one is for menus with 1000 menu items, yet the solution there is the only one that worked for me, even though I don't have that many. That means that the patches in this issue are not sufficient to fix this problem.

@chrisgross, from your comment #132:

@tunic. No change in behavior from before the patch. PHP Warning: Unknown: Input variables exceeded 1000. To increase the limit change max_input_vars in php.ini. in Unknown on line 0,

That erorr is not related with present issue. Your error (Input variables exceeded 1000) comes from the fact that PHP has a limit of 1000 elements in POST requests. This issue I'm commenting it's about more than 100 items in the menu. Although if you have more than 100 items in the menu you can hit the 1000 limit are separated issues. Iin your case, probably, you have to resolve two issues: this issue (sorting more than 100 menu items) and the other issue (1000 POST elements limit).

+++ b/modules/menu/menu.module
@@ -617,6 +617,63 @@ function _menu_parent_depth_limit($item) {
+function _get_menu_link_weight_info($menu_name) {
...
+function _get_multiple_menu_link_weight_info($menu_options) {
...
+function _get_menu_weight_delta($menu_name, $max_delta = NULL) {

Lets name the functions better using the _menu prefix

Other than that this is good to go

Lets name the functions better using the _menu prefix

I can't believe I missed that :D

thank you!

Thank you for a great patch and contribution, unfortunately the code organization will need some more work.

Please see below:

1. +++ b/modules/menu/menu.module
   @@ -617,6 +617,63 @@ function _menu_parent_depth_limit($item) {
   + * Helper function to get a menu option max and min weights.
   + *
   ...
   +/**
   + * Helper function to get the max and min weights of a group of menu options.
   + *
   

   I am not sure we need the helper functions at all as they are just called once anyway and we should only use the multiple case.

2. +++ b/modules/menu/menu.module
   @@ -617,6 +617,63 @@ function _menu_parent_depth_limit($item) {
   +function _menu_get_menu_link_weight_info($menu_name) {
   

   I don't think we need both helpers.

3. +++ b/modules/menu/menu.module
   @@ -617,6 +617,63 @@ function _menu_parent_depth_limit($item) {
   + * @param array $menu_options
   + *   Array of string with menu the names to get its weight info.
   ...
   +function _menu_get_multiple_menu_link_weight_info($menu_options) {
   

   menu_names vs. menu_options feels a littel strange API wise.

4. +++ b/modules/menu/menu.module
   @@ -617,6 +617,63 @@ function _menu_parent_depth_limit($item) {
   +function _menu_get_menu_weight_delta($menu_name, $max_delta = NULL) {
   +
   +  $weight_info = is_array($menu_name) ? _menu_get_multiple_menu_link_weight_info($menu_name) : _menu_get_menu_link_weight_info($menu_name);
   

   Usually we use the multiple parameter argument, e.g. $menu_names, then do:

   if (is_string($menu_names)) {
     $menu_names = array($menu_names);
   }
   

Thanks for the review Fabianx.

I think that even _menu_parent_depth_limit is called just one time it's good to have a function. This way code is more self-explanatory and less cluttered. Anyway, I can remove it if it's mandatory.

I'll address the issues in short, thanks again!

#153: It is not mandatory, but we also don't want to have too many helpers that no one else should call lying around. That usually creates confusion for Developers, even if marked with _ as internal.

Helpers removed, fixed param name, use is_string to preprocess param and make sure it's an array.

Thanks for your continued work on this!

Interdiff looks good, but the patch contains unrelated changes, which makes it hard to review.

Ooops, I created the patch using diff against my local 7.x after rebasing my local issue branch to origin/7.x so it incluides already commited changes, sorry.

Fixed patch attached. Interdiff from #155 is ok.

Strange, on my local box all tests pass. One difference I use PHP 7.

It seems a temporal test bot error, now 5.5 pass and 7 shows HEAD's 3 fails.

Drupal 7 is still supported, why close this bug as outdated?

I have the problem on a Drupal 7.52 / Php 7
The website uses 11 languages. The list of items is very long and I can't reorder menu links.

This bug has been tagged as outdated, but the bug is still there.

To state officially:

This was never meant to be closed as outdated - at least not from any official reason. I think it was just an accident.

---

Steps needed here:

- Reviews (back to RTBC)
- Testing
- Issue summary update

Hello All,
I've applied the patch from #157 cleanly to D 7.52, but I'm still unable to reorder my menu.

Other than apply the patch and clear caches am I supposed to do anything? or is there any way I can help?
thanks,
Stephanie_42

Hi Stephanie_42, checkout comment #112, maybe you are running into this issue too #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)

This patch is working in many projects I've worked on. Let's see if we can get this patch in before Drupal 7 gets deprecated.

I've re-queued #157

All tests still pass:

PHP 5.3 & MySQL 5.5 2,035 pass most recent, 21 Jun 2017 at 23:27 CEST

#157 works great and fixes the issue. Great job, @tunic. Now let's get his fix into core!

Hi,

Just had to use this patch and it's working great, thanks.

Removing tag as it doesn't seem relevant.

Here it is 7 years after the original issue, and I am still having this problem in a Drupal 7.53 installation.
We are very reliant on the row weights being correct as we are using them in a web service API for consumption in a mobile app. 2 of our books are apparently still under the threshold, and re-ordering works fine. A third one has many more pages and re-ordering of course does not work.

I have applied the patch in #157, and I have raised max_input_vars to 50000, but no joy still.

I tried Outline Designer, but that did not help, as well as having its own issues.

Can anyone please help me figure out why the patch did not fix the problem?

@timmay I suggest to try upgrading to 7.56 first, then apply the patch. Also, have a look at your phpinfo to make sure that your expected configuration matches the max_input_vars limit you specified.
also see the related issue #1517092: admin/structure/menu/manage/MENU_NAME cannot be submitted on PHP 5.3.9+ for large menus (>~1000 items)

So, of course I figured out the dumb mistake right after I posted this...I forgot to verify that max_input_vars had indeed been raised to 50000, and it hadn't. So once we took care of that for real, it fixed the problem.

Thanks @joseph.olstad. You hit the nail right on the head!

deleted my own comment

Does the issue below relate to the same problem?
https://www.drupal.org/project/drupal/issues/2973578
Desperately seeking a solution in Drupal 7.59...

see comment #178 #1007746-178: Reordering fails with more than 100 items in a menu
try the RTBC core patch 157 above , should still apply to 7.59

Thanks Joseph! Could you maybe post the patched file menu.admin.inc for 7.59 here?

I managed to apply the patch, but uploading the new menu.admin.inc did not solve my problem re-ordering taxonomy items.

@Tino #2973578: Taxonomy drag and drop reorder not permanently saved is likely related. But this issue is solely about menus. Since you are desperate as per #2973578, Taxonomy Manager may help you.

I applied the patch #157, but it didn't work for me on drupal 7.59

Stopopol,
Please review comment #178 and #179 above

Also, I just queued a 7.x-dev php7.2 test for patch #157. Next release supports php 7.2.x

@joseph.olstadt I reviewed both #178 and #179. I increased max_input_vars accordingly and those changes are reflected in admin/reports/status/php
Still not working

@stopopol, please verify that you adjusted the correct php.ini (alternatively can be done on the vhost)
there is one for the command line and another for the web server. If you only adjusted max_input_vars for the command line it will only work with drush, but if you did the web server php.ini it will affect Drupal.
alternatively there is ways to put directives into vhosts as well.

this fix works, lots of others reported this fix and I have done it as well.

this didn't make it into 7.60, bumping to 7.61

Two years and a half, tests still pass.

I still have faith.

@joseph.olstad
I finally found out that my issue was a corrupt user menu. I found the solution here:
https://www.drupal.org/project/drupal/issues/991778

Bumping to 7.68.

Tests still pass.

Maintain RTBC
1) Has tests
2) Passes tests
3) Fix is already in D8

Drupal 7 is going to be dropped before this gets committed isn't it?

see justification in my last comment above.
repeating again:

1) Has tests
2) Passes tests
3) Fix is already in D8

1. Has tests
2. Passes tests
3. Fix is already in D8/D9

Created the merge request. I hope this will help with the final review.
Also, here the link for the patch based on the changes from the merge request:
https://git.drupalcode.org/project/drupal/-/merge_requests/21.patch

Justification:
1) Already in D8
2) has tests
3) RTBC as requested by FabianX

one last thing is needed that FabianX requested:
- Issue summary update

I've tried to update the summary update. I hope it is ok.

I don't like doing this, but I'm removing the "Pending Drupal 7 commit" tag for now as this hasn't got to that stage with the current D7 maintainers' reviews yet.

Also, personally I do not find it aids review to open an MR on a long / old issue with lots of comments and patches. For me, it's adding more noise where there is already plenty.

I'll try to get this issue reviewed properly ASAP, but having issues that I've not reviewed show up in the listing for the "Pending Drupal 7 commit" tag is actually more of a hindrance than a help for me at the moment as I'm having to check which of them I've already looked at. This was not one of those.

This is not set in stone (and has already evolved), but we drafted a proposed workflow for D7 here:

https://www.drupal.org/project/drupal/issues/3093258

Please could everybody avoid using the "Pending Drupal 7 commit" tag. I know you're keen to get things committed, but this is actually making it less likely that issues will get into the next release as I'm having to spend time checking which issues I've already reviewed when I could be reviewing :)

Thanks.

There's (currently) no difference between the MR and #157 (note to self and anyone else reviewing).

I've verified this locally, but just to be sure here's a test_only patch.

I've also tweaked #157's comments slightly, but no code changes.

LGTM.

Thank you for your patience :)

RTBC + 1

Thank you very much to everyone that contributed!

A dream from a decade comes true haha! Thank you for your work on this issue.

I am using Drupal 7.92 on PHP7.4.32 and with a menu of >400 items. My php.ini (#144 ) had max_input_vars=1000 and I could not reorder. Upped variable to 50000 and now can reorder as expected.

#225 That issue doesn't seem related to this issue. See #145 and #147.

Hi tunic, I see what you mean, but I did not discover the other link, it was the comments in this issue that lead me to solve my problem, and I was highlighting the continued need for high max_input_vars even with latest code - in case it helps someone else. Thanks.