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

This issue also crops up with a slightly different setup:

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

Results in:

- item 1
-- item 1a
-- item 1b
-- item 2a (this is assigned the wrong parent!) 
- item 3

The patch in #2 caused my toolbar to become pretty much empty. The bunch of failed tests also indicates that the patch does not (really) fix the problem, unfortunately :(

Let's see how this patch fares with the tests. Not sure if it also fixes the example @TVoesenek provided, but I would think so.
We should probably add a test for this.

I've tried both patches myself and it didn't work correctly on all scenarios.
I tried then to read the code and this part, imho, is the problem:

if ($next && $next['depth'] > $depth) {

The assumption here is that if there is a next item and its depth is greather than current item, then it's a child of it. This assumption proved wrong when MenuLinkTree::load is invoked with menu tree parameter set to show only enabled links.

Here I provide a patch that address this wrong assumption (which worked in above scenarios and others)
if ($next && $next['depth'] > $depth && $next['parent'] === $tree_link_definition['id']) {

Further investigating it I realized that children of a disabled parent were rendering at the root with only the path above.

I expanded the above patch to avoid adding to root level items with a disabled parent.

I've also added a kernel test to validate the scenario.

I missed a set of parentheses in the big if.
Attached the updated one.

I've added a private method to better validate what goes inside the menu tree and what not.

We were also having the problem where enabled child menu items of disabled parent menu item were being displayed under the wrong parent.

We are having this issue in Drupal 8; we applied the patch in #15, cleared caches and this has fixed the problem for us.

Many thanks

The patch in #15 did suppress the disabled items and its children but it messed up the depth and parenting of other items in my menu.

Item A (enabled)
- Item A_a
-- Item A_a_1
--- Item A_a_1_i
--- Item A_a_1_ii
Item B (enabled)
- Item B_a
Item C (disabled)
- Item C_a (enabled)

ended up rendering as

Item A
Item B
- Item A_a
-- Item A_a_1
--- Item A_a_1_i
--- Item A_a_1_ii

children of Item B didn't get rendered at all

The patch at #15 also didn't work for me and removed other valid, enabled menu items. I'll have a look at the patch and see if I can get it working for my particular case.

Rerolled patch #14 with Drupal 9.5.x

Hi there, I am trying this issue as part of the contribution day Prague 2022. Thanks

I created a merge request with the latest patch #24 to get familiar with the new workflow.

I tested the latest patch release and I confirm it works as expected.

Upon further testing I found out a scenario which leads to wrong results when only enabled links are requested.

The query results returned by loadLinks method takes into account that only links with enabled set to 1 should be returned.

In a scenario with the following tree:

A
-- Aa (disabled)
--- Aaa (enabled)
-- Ab (enabled)

The Ab element will not be rendered under A, because the algorithm in place within the method treeDataRecursive takes for granted that the $next element is possibly the first subtree element, but there could be scenarios in which this won't be true with filtered out links.

So my guess is that the whole logic here isn't enough to cover all scenarios and we should also rethink a fast way to calculate the tree recursively and not having all items available.

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 are 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

I suspect this is the source of a few of the problems in this thread

  protected function loadLinks($menu_name, MenuTreeParameters $parameters) {
    ...
    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
    return $links;
  }

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.