Enabled subtree links of a disabled parent link are added to the menu tree at the wrong depth.

We've just run into a similar issue: menu items and their children being attached to the entirely wrong parent seemingly for no reason, even when all items were active

It ultimately stems from the list of links returned from DB query in MenuTreeStorage::loadLinks() not being ordered properly, despite the function correctly setting the SQL orders (p1 ASC, p2 ASC, etc.) before querying and the data in the DB being correct. It seems to make no sense. It's mostly ordering it correctly, but a few odd items are being moved out of order somehow

    for ($i = 1; $i <= $this->maxDepth(); $i++) {
      $query->orderBy('p' . $i, 'ASC');
    }
    ...
    $links = $this->safeExecuteSelect($query)->fetchAllAssoc('id', \PDO::FETCH_ASSOC);
    // links are already out of order

Perhaps there's a bug in the SQL fetch code that leads to the row order being wrecked when converted to an associative array, or perhaps there's some custom hook that runs after queries

This code triggers the issue:

$parameters = new MenuTreeParameters();
$parameters->onlyEnabledLinks();

This doesn't:

$parameters = new MenuTreeParameters();

It seems this also doesn't tho 🙃:

$parameters = new MenuTreeParameters();
$parameters->onlyEnabledLinks();
$parameters->addCondition('expanded', 1);
$parameters->setActiveTrail([...]);
$parameters->setMaxDepth(3);

We're working around the issue by not using onlyEnabledLinks() and instead manually filtering out disabled links after loading the full tree

I also ran into this issue while having the patch from comment #24 applied. Seems like it doesn't fix all the edge cases.
E.g:
Having the following menu tree

- item 1
-- item 1a
-- item 2a
--- item 3a
- item 2 (disabled)
-- item 2a
- item 3
-- item 3a
--- item 3b
- item 4

Expecting:

- item 1
-- item 1a
-- item 2a
--- item 3a
- item 3
-- item 3a
--- item 3b
- item 4

Getting:

- item 1
-- item 1a
-- item 2a
--- item 3a
- item 3
-- item 3a
--- item 3b
-- item 4

Because there are some menu items that are active on the second level with the parent disabled, it is skipped and goes to the next one without returning the current subtree. It goes to the next link and add it to the latest sub-tree in use, instead of starting a new one. When skipping a link we should also check if we should actually return the current sub-tree because the parents changed.
Added a patch to fix it.

@ciprian.stavovei we don't want to mix MRs and patches if we can help it, previously this appeared to be an MR, though the MR needs to be updated 11.x

moving to NW for that.

I tried patch #34. In my case it doesn't fully work.

My menu tree:
...
- item 4
-- item 41
--- item 411
--- item 412
--- ...

-- item 42 (disabled)
--- item 421
--- ...

-- item 43
--- item 431
--- item 432
--- ...

Expected:
...
- item 4
-- item 41
--- item 411
--- item 412
--- ...

-- item 43
--- item 431
--- item 432
--- ...

Got:
- item 4
-- item 43
--- item 431
--- item 432
--- ...

Tried to debug, but without success.
Something wrong in this part:

if (!empty($tree_parent['depth'])
  && $tree_link_definition['depth'] > $tree_parent['depth']
  && $tree_link_definition['parent'] !== $tree_parent['id']
) {
  return $tree;
}

My workaround: don't use OnlyEnabledLinks() and filter the disabled items in the recursion loop.