The menus are sorted by machine name and not by label

Here's an initial patch.

Adding test coverage, although I'm having trouble with it NOT failing. I know I saw this bug locally, so I need to rework the test to actually demonstrate the problem.

I forgot that the paths are prefaces with /subdirectory/ on testbot:

-    $this->assertEquals('/admin/structure/menu/manage/zzz', $link1->getAttribute('href'));
+    $this->assertContains('/admin/structure/menu/manage/zzz', $link1->getAttribute('href'));

This fixes failures like this:

--- Expected
+++ Actual
@@ @@
-'/admin/structure/menu/manage/zzz'
+'/subdirectory/admin/structure/menu/manage/zzz'

Of course, this was my test, case that was supposed to fail, so back to work on this.

patch #2 has unexpected results testing locally on Drupal 8.8:

trying a test like this:

   $menus = [
      'aaa' => 'lll',
      'bbb' => 'kkk',
      'ccc' => 'jjj',
      'ddd' => 'iii',
      'eee' => 'hhh',
      'fff' => 'ggg',
      'ggg' => 'fff',
      'hhh' => 'eee',
      'iii' => 'ddd',
      'jjj' => 'ccc',
      'kkk' => 'bbb',
      'lll' => 'aaa',
    ];

    foreach ($menus as $machine_name => $label) {
      $menu = Menu::create([
        'id' => $machine_name,
        'label' => $label,
      ]);
      $menu->save();
    }


    $this->drupalLogin($this->adminUser);

    $results = $this->getSession()->getPage()->findAll('xpath', '//a[contains(@href, "/admin/structure/menu/manage")]');

    $links = [];
    foreach ($results as $result) {
      $links[] = $result->getAttribute('href');
    }
    $this->assertEquals(['test' => 'test'], $links);

without the patch:

 0 => '/admin/structure/menu/manage/admin'
+    1 => '/admin/structure/menu/manage/admin/add'
+    2 => '/admin/structure/menu/manage/footer'
+    3 => '/admin/structure/menu/manage/...er/add'
+    4 => '/admin/structure/menu/manage/account'
+    5 => '/admin/structure/menu/manage/...nt/add'
+    6 => '/admin/structure/menu/manage/ggg'
+    7 => '/admin/structure/menu/manage/ggg/add'
+    8 => '/admin/structure/menu/manage/...delete'
+    9 => '/admin/structure/menu/manage/fff'
+    10 => '/admin/structure/menu/manage/fff/add'
+    11 => '/admin/structure/menu/manage/...delete'
+    12 => '/admin/structure/menu/manage/eee'
+    13 => '/admin/structure/menu/manage/eee/add'
+    14 => '/admin/structure/menu/manage/...delete'
+    15 => '/admin/structure/menu/manage/ddd'
+    16 => '/admin/structure/menu/manage/ddd/add'
+    17 => '/admin/structure/menu/manage/...delete'
+    18 => '/admin/structure/menu/manage/ccc'
+    19 => '/admin/structure/menu/manage/ccc/add'
+    20 => '/admin/structure/menu/manage/...delete'
+    21 => '/admin/structure/menu/manage/bbb'
+    22 => '/admin/structure/menu/manage/bbb/add'
+    23 => '/admin/structure/menu/manage/...delete'
+    24 => '/admin/structure/menu/manage/aaa'
+    25 => '/admin/structure/menu/manage/aaa/add'
+    26 => '/admin/structure/menu/manage/...delete'

with the patch:

 0 => '/admin/structure/menu/manage/admin'
+    1 => '/admin/structure/menu/manage/admin/add'
+    2 => '/admin/structure/menu/manage/footer'
+    3 => '/admin/structure/menu/manage/...er/add'
+    4 => '/admin/structure/menu/manage/main'
+    5 => '/admin/structure/menu/manage/main/add'
+    6 => '/admin/structure/menu/manage/tools'
+    7 => '/admin/structure/menu/manage/tools/add'
+    8 => '/admin/structure/menu/manage/account'
+    9 => '/admin/structure/menu/manage/...nt/add'
+    10 => '/admin/structure/menu/manage/lll'
+    11 => '/admin/structure/menu/manage/lll/add'
+    12 => '/admin/structure/menu/manage/...delete'
+    13 => '/admin/structure/menu/manage/kkk'
+    14 => '/admin/structure/menu/manage/kkk/add'
+    15 => '/admin/structure/menu/manage/...delete'
+    16 => '/admin/structure/menu/manage/jjj'
+    17 => '/admin/structure/menu/manage/jjj/add'
+    18 => '/admin/structure/menu/manage/...delete'
+    19 => '/admin/structure/menu/manage/iii'
+    20 => '/admin/structure/menu/manage/iii/add'
+    21 => '/admin/structure/menu/manage/...delete'
+    22 => '/admin/structure/menu/manage/hhh'
+    23 => '/admin/structure/menu/manage/hhh/add'
+    24 => '/admin/structure/menu/manage/...delete'

so, it seems the patch isn't a complete solution, although it does fix the order of the extra links, the original links are still first.

I think because DefaultMenuLinkTreeManipulators::generateIndexAndSort uses weight as well as label:

$new_tree[(50000 + $instance->getWeight()) . ' ' . $instance->getTitle() . ' ' . $instance->getPluginId()] = $tree[$key];

when I test on my other local site (8.7.1), it's doing what I expect.

I'm still investigating why.

I think I've figured out the problem.

This loads the menu ids keyed by id.
$menu_ids = $this->entityTypeManager->getStorage('menu')->getQuery()->pager(self::MAX_BUNDLE_NUMBER)->execute();

I was trying to add
->sort('label')

but that didn't work. I guess because they're config entities? It didn't throw an error, but it didn't give accurate results.

changing it to

->sort('title')

seemed to help, but it didn't work on Drupal 8.8. It was throwing an error. Plus, I don't think that will work when the menu is translated. I think this might still need some work display translated titles sorted correctly. It turns out Menu::sort is actually a method used on that page! It's actually ConfigEntityBase::sort().

I ended up fixing it by looking how ConfigEntityListBuilder::load sorts them, because /admin/structure/menu has them sorted correctly.

@@ -319,10 +320,10 @@ class ExtraLinks extends DeriverBase implements ContainerDeriverInterface {
       ] + $base_plugin_definition;
       // Adds links to /admin/structure/menu.
       // We do not display more than 10 different menus.
-      $menu_ids = $this->entityTypeManager->getStorage('menu')->getQuery()->pager(self::MAX_BUNDLE_NUMBER)->execute();
-      $menus = $this->entityTypeManager->getStorage('menu')->loadMultiple($menu_ids);
+      $menus = $this->entityTypeManager->getStorage('menu')->loadMultiple();
+      uasort($menus, [Menu::class, 'sort']);
+      $menus = array_slice($menus, 0, self::MAX_BUNDLE_NUMBER);
       if (count($menus) == self::MAX_BUNDLE_NUMBER) {
-        $menus = array_slice($menus, 0, self::MAX_BUNDLE_NUMBER);
         $links['entity.menu.collection'] = [
             'title' => $this->t('All menus'),
             'route_name' => 'entity.menu.collection',

Unfortunately, this means we need to load all of the entities to get this accurate. Perhaps we can come up with a different way later, but at least this works.

Oops. I got tripped up by testbot prefacing the urls with '/subdirectory/' again.

Renaming the test so as not to conflict with other issue I'm working on, 3063356.

I tried to test this with translation. But as far as I can tell, menus don't translate, they have a set language, and all menus in all languages display.

Is there any possibility not to use new way of sorting? Of course beside reverting changes from commit?

@adriancid Exactly the same it was before (if there was any at all). Can upgrade to 2.x branch only with condition: sequences in all menus remains the same.