Menu rebuild breaks link hierarchy

I found that issue is here in rebuild(array $definitions) method :

if ($definitions) {
  foreach ($definitions as $id => $link) {
    // Flag this link as discovered, i.e. saved via rebuild().
    $link['discovered'] = 1;
    if (!empty($link['parent'])) {
      $children[$link['parent']][$id] = $id;
    }
    else {
      // A top level link - we need them to root our tree.
      $top_links[$id] = $id;
      $link['parent'] = '';
    }
    $links[$id] = $link;
  }
}
foreach ($top_links as $id) {
  $this->saveRecursive($id, $children, $links);
}
// Handle any children we didn't find starting from top-level links.
foreach ($children as $orphan_links) {
  foreach ($orphan_links as $id) {
    // Force it to the top level.
    $links[$id]['parent'] = '';
    $this->saveRecursive($id, $children, $links);
  }
}

Yeah the issue was on both the places. Created a patch hope this will work. :)

Applied patch and manually tested against a menu with a hierarchy 2, 3, and 4 levels deep. It works perfectly on my end. Nice work!

Creating a test case.

Written a test case "MenuParentChildRelationshipTest".

Uploaded test only to check fail test.

Oops!! Sorry.

Uploading the patch with short array syntax and comments fixing.

Reason to remove the following code :
$links[$id]['parent'] = '';

is that because it was setting all children's parent to blank string. The condition to unset parent is already checked in "saveRecursive" method.

I applied this patch and worked fine. Actually this is created using

git format-patch master --stdout > patchname.patch

Command. Which creates the patch like this.

Please use git diff, format-patch is much harder to parse for a human.

Also, please revert the unnecessary array() to [] changes. We all prefer [] but that's out of scope for this issue.

Updated the patch with git diff command.

Used Menu::create method instead of entity_create.

As a web test, this test takes 18 seconds. Rewritten to be a kernel test, it takes 2 seconds.
This was what I was trying to describe on IRC, sorry for just jumping in and doing it.

I know that we have an issue for that already ... and another one I can't find at the moment ... #2477337: Saving a menu item with internal path instead of entity reference loses hierarchy

Note: We need a better issue title here, its not helpful at all

1. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -798,7 +796,7 @@ public function getExpanded($menu_name, array $parents) {
   -    if (!empty($links[$id]['parent']) && empty($links[$links[$id]['parent']])) {
   +    if (!empty($links[$id]['parent']) && empty($children[$links[$id]['parent']])) {
   

   That totally makes sense.

2. +++ b/core/modules/system/src/Tests/Menu/MenuParentChildRelationshipTest.php
   @@ -0,0 +1,94 @@
   +  public function testParentChildRelationshipOnMenuRebuild() {
   

   Do you mind moving this directly to MenuTreeSTorageTest? It is already a kernel Test ....

3. +++ b/core/modules/system/src/Tests/Menu/MenuParentChildRelationshipTest.php
   @@ -0,0 +1,94 @@
   +    $this->menuLinkManager->deleteLinksInMenu('menu_test');
   ...
   +    $node = Node::create(['type' => 'page', 'title' => 'Test Title']);
   +    $node->save();
   ...
   +    $child = $base_options + [
   +      'link' => [['uri' => 'internal:/admin/content/files']],
   +      'parent' => $links['parent'],
   

   seems not really needed here ...

4. +++ b/core/modules/system/src/Tests/Menu/MenuParentChildRelationshipTest.php
   @@ -0,0 +1,94 @@
   +      'link' => [['uri' => 'entity:node/' . $node->id()]],
   

   it would be great if we could just use test entity types here?

Moved testParentChildRelationshipOnMenuRebuild() to MenuTreeSTorageTest.

Please let me know why following code is not needed? Without adding a child how we will test parent child relationship?

$child = $base_options + [
      'link' => [['uri' => 'internal:/admin/content/files']],
      'parent' => $links['parent'],

I was more talking about the need to create a node.

Removed extra line of code.

Adding tag to explain what is going on.

Isn't that issue pretty much the same as #2468713: Internal/custom menu links force-reset to the top of their menus on cache rebuild ?
Breaking link hierarchy is the same problem on the other issue so moving this to critical as well.

#2468713: Internal/custom menu links force-reset to the top of their menus on cache rebuild has changed some of the same lines - so this will need a reroll and confirmation that it is still an issue.

I confirms that this is still and issue. I will update the patch.

Just checked. Patch applied cleanly. I believe there is no need to update the patch. Moving this again to needs review.

This is a test only patch, I'm curious whether this still fails after the other issue.

If this doesn't fail, we are safe and can mark the issue as fixed.

Why not add this test anyway.

1. +++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
   @@ -462,4 +486,38 @@ protected function assertMenuLink($id, array $expected_properties, array $parent
   +   * Tests creating links with the path created using views and test
   +   * relationship on rebuild.
   

   Should be online.

2. +++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
   @@ -462,4 +486,38 @@ protected function assertMenuLink($id, array $expected_properties, array $parent
   +  public function testParentChildRelationshipOnMenuRebuild() {
   

   After rebuild sounds much better.

Why not add this test anyway.

True, would be nice to not rely on nodes but rather on entity_test entities for example though.

testParentChildRelationshipOnMenuRebuild
It would be great to somehow communicate in the title that this is about parent entries being something else than static menu links.

Reupload the patch, might have been a random failure.

This test is really basically the same as the new test in #2468713: Internal/custom menu links force-reset to the top of their menus on cache rebuild IMHO we could mark it as duplicate.

What about adding the contributors of this issue onto #2477337: Saving a menu item with internal path instead of entity reference loses hierarchy, that would be nice!

I've added @sumitmadan and @tim.plunkett to the other commit message - if they comment on that issue I'll update the commit credit on issue too.