#550254 by Damien Tournoud, sun: Fixed some causes of menu links are sometimes not properly re-parented.

2 files changed, 293 insertions, 36 deletions

diff --git a/includes/menu.inc b/includes/menu.inc
index b72e3ff90..6536c484d 100644
--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -2907,42 +2907,8 @@ function menu_link_save(&$item) {
     }
   }
 
-  // If we have a parent link ID, we use it to inherit 'menu_name' and 'depth'.
-  if (isset($item['plid'])) {
-    if ($item['plid']) {
-      $parent = db_query("SELECT * FROM {menu_links} WHERE mlid = :mlid", array(':mlid' => $item['plid']))->fetchAssoc();
-    }
-    // If the parent link ID is zero, then this link lives at the top-level.
-    else {
-      $parent = FALSE;
-    }
-  }
-  // Otherwise, try to find a valid parent link for this link.
-  else {
-    $query = db_select('menu_links');
-    // Only links derived from router items should have module == 'system', and
-    // we want to find the parent even if it's in a different menu.
-    if ($item['module'] == 'system') {
-      $query->condition('module', 'system');
-    }
-    // We always respect the link's 'menu_name'; inheritance for router items is
-    // ensured in _menu_router_build().
-    $query->condition('menu_name', $item['menu_name']);
-
-    // Find the parent - it must be unique.
-    $parent_path = $item['link_path'];
-    do {
-      $parent = FALSE;
-      $parent_path = substr($parent_path, 0, strrpos($parent_path, '/'));
-      $new_query = clone $query;
-      $new_query->condition('link_path', $parent_path);
-      // Only valid if we get a unique result.
-      if ($new_query->countQuery()->execute()->fetchField() == 1) {
-        $parent = $new_query->fields('menu_links')->execute()->fetchAssoc();
-      }
-    } while ($parent === FALSE && $parent_path);
-  }
-  // If a parent link was found, assign it and derive its menu.
+  // Try to find a parent link. If found, assign it and derive its menu.
+  $parent = _menu_link_find_parent($item);
   if (!empty($parent['mlid'])) {
     $item['plid'] = $parent['mlid'];
     $item['menu_name'] = $parent['menu_name'];
@@ -3064,6 +3030,78 @@ function menu_link_save(&$item) {
 }
 
 /**
+ * Find a possible parent for a given menu link.
+ *
+ * Because the parent of a given link might not exist anymore in the database,
+ * we apply a set of heuristics to determine a proper parent:
+ *
+ *  - use the passed parent link if specified and existing.
+ *  - else, use the first existing link down the previous link hierarchy
+ *  - else, for system menu links (derived from hook_menu()), reparent
+ *    based on the path hierarchy.
+ *
+ * @param $menu_link
+ *   A menu link.
+ * @return
+ *   A menu link structure of the possible parent or FALSE if no valid parent
+ *   has been found.
+ */
+function _menu_link_find_parent($menu_link) {
+  $parent = FALSE;
+
+  // This item is explicitely top-level, skip the rest of the parenting.
+  if (isset($menu_link['plid']) && empty($menu_link['plid'])) {
+    return $parent;
+  }
+
+  // If we have a parent link ID, try to use that.
+  $candidates = array();
+  if (isset($menu_link['plid'])) {
+    $candidates[] = $menu_link['plid'];
+  }
+
+  // Else, if we have a link hierarchy try to find a valid parent in there.
+  if (!empty($menu_link['depth']) && $menu_link['depth'] > 1) {
+    for ($depth = $menu_link['depth'] - 1; $depth >= 1; $depth--) {
+      $candidates[] = $menu_link['p' . $depth];
+    }
+  }
+
+  foreach ($candidates as $mlid) {
+    $parent = db_query("SELECT * FROM {menu_links} WHERE mlid = :mlid", array(':mlid' => $mlid))->fetchAssoc();
+    if ($parent) {
+      return $parent;
+    }
+  }
+
+  // If everything else failed, try to derive the parent from the path
+  // hierarchy. This only makes sense for links derived from menu router
+  // items (ie. from hook_menu()).
+  if ($menu_link['module'] == 'system') {
+    $query = db_select('menu_links');
+    $query->condition('module', 'system');
+    // We always respect the link's 'menu_name'; inheritance for router items is
+    // ensured in _menu_router_build().
+    $query->condition('menu_name', $menu_link['menu_name']);
+
+    // Find the parent - it must be unique.
+    $parent_path = $menu_link['link_path'];
+    do {
+      $parent = FALSE;
+      $parent_path = substr($parent_path, 0, strrpos($parent_path, '/'));
+      $new_query = clone $query;
+      $new_query->condition('link_path', $parent_path);
+      // Only valid if we get a unique result.
+      if ($new_query->countQuery()->execute()->fetchField() == 1) {
+        $parent = $new_query->fields('menu_links')->execute()->fetchAssoc();
+      }
+    } while ($parent === FALSE && $parent_path);
+  }
+
+  return $parent;
+}
+
+/**
  * Helper function to clear the page and block caches at most twice per page load.
  */
 function _menu_clear_page_cache() {
diff --git a/modules/simpletest/tests/menu.test b/modules/simpletest/tests/menu.test
index 087293670..773282ed9 100644
--- a/modules/simpletest/tests/menu.test
+++ b/modules/simpletest/tests/menu.test
@@ -467,6 +467,225 @@ class MenuRouterTestCase extends DrupalWebTestCase {
 }
 
 /**
+ * Tests for menu links.
+ */
+class MenuLinksUnitTestCase extends DrupalWebTestCase {
+  // Use the lightweight testing profile for this test.
+  protected $profile = 'testing';
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Menu links',
+      'description' => 'Test handling of menu links hierarchies.',
+      'group' => 'Menu',
+    );
+  }
+
+  /**
+   * Create a simple hierarchy of links.
+   */
+  function createLinkHierarchy($module = 'menu_test') {
+    // First remove all the menu links.
+    db_truncate('menu_links')->execute();
+
+    // Then create a simple link hierarchy:
+    // - $parent
+    //   - $child-1
+    //      - $child-1-1
+    //      - $child-1-2
+    //   - $child-2
+    $base_options = array(
+      'link_title' => 'Menu link test',
+      'module' => $module,
+      'menu_name' => 'menu_test',
+    );
+
+    $links['parent'] = $base_options + array(
+      'link_path' => 'menu-test/parent',
+    );
+    menu_link_save($links['parent']);
+
+    $links['child-1'] = $base_options + array(
+      'link_path' => 'menu-test/parent/child-1',
+      'plid' => $links['parent']['mlid'],
+    );
+    menu_link_save($links['child-1']);
+
+    $links['child-1-1'] = $base_options + array(
+      'link_path' => 'menu-test/parent/child-1/child-1-1',
+      'plid' => $links['child-1']['mlid'],
+    );
+    menu_link_save($links['child-1-1']);
+
+    $links['child-1-2'] = $base_options + array(
+      'link_path' => 'menu-test/parent/child-1/child-1-2',
+      'plid' => $links['child-1']['mlid'],
+    );
+    menu_link_save($links['child-1-2']);
+
+    $links['child-2'] = $base_options + array(
+      'link_path' => 'menu-test/parent/child-2',
+      'plid' => $links['parent']['mlid'],
+    );
+    menu_link_save($links['child-2']);
+
+    return $links;
+  }
+
+  /**
+   * Assert that at set of links is properly parented.
+   */
+  function assertMenuLinkParents($links, $expected_hierarchy) {
+    foreach ($expected_hierarchy as $child => $parent) {
+      $mlid = $links[$child]['mlid'];
+      $plid = $parent ? $links[$parent]['mlid'] : 0;
+
+      $menu_link = menu_link_load($mlid);
+      menu_link_save($menu_link);
+      $this->assertEqual($menu_link['plid'], $plid, t('Menu link %mlid has parent of %plid, expected %expected_plid.', array('%mlid' => $mlid, '%plid' => $menu_link['plid'], '%expected_plid' => $plid)));
+    }
+  }
+
+  /**
+   * Test automatic reparenting of menu links.
+   */
+  function testMenuLinkReparenting($module = 'menu_test') {
+    // Check the initial hierarchy.
+    $links = $this->createLinkHierarchy($module);
+
+    $expected_hierarchy = array(
+      'parent' => FALSE,
+      'child-1' => 'parent',
+      'child-1-1' => 'child-1',
+      'child-1-2' => 'child-1',
+      'child-2' => 'parent',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+
+    // Start over, and move child-1 under child-2, and check that all the
+    // childs of child-1 have been moved too.
+    $links = $this->createLinkHierarchy($module);
+    $links['child-1']['plid'] = $links['child-2']['mlid'];
+    menu_link_save($links['child-1']);
+
+    $expected_hierarchy = array(
+      'parent' => FALSE,
+      'child-1' => 'child-2',
+      'child-1-1' => 'child-1',
+      'child-1-2' => 'child-1',
+      'child-2' => 'parent',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+
+    // Start over, and delete child-1, and check that the children of child-1
+    // have been reassigned to the parent. menu_link_delete() will cowardly
+    // refuse to delete a menu link defined by the system module, so skip the
+    // test in that case.
+    if ($module != 'system') {
+      $links = $this->createLinkHierarchy($module);
+      menu_link_delete($links['child-1']['mlid']);
+
+      $expected_hierarchy = array(
+        'parent' => FALSE,
+        'child-1-1' => 'parent',
+        'child-1-2' => 'parent',
+        'child-2' => 'parent',
+      );
+      $this->assertMenuLinkParents($links, $expected_hierarchy);
+    }
+
+    // Start over, forcefully delete child-1 from the database, simulating a
+    // database crash. Check that the children of child-1 have been reassigned
+    // to the parent, going up on the old path hierarchy stored in each of the
+    // links.
+    $links = $this->createLinkHierarchy($module);
+    // Don't do that at home.
+    db_delete('menu_links')
+      ->condition('mlid', $links['child-1']['mlid'])
+      ->execute();
+
+    $expected_hierarchy = array(
+      'parent' => FALSE,
+      'child-1-1' => 'parent',
+      'child-1-2' => 'parent',
+      'child-2' => 'parent',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+
+    // Start over, forcefully delete the parent from the database, simulating a
+    // database crash. Check that the children of parent are now top-level.
+    $links = $this->createLinkHierarchy($module);
+    // Don't do that at home.
+    db_delete('menu_links')
+      ->condition('mlid', $links['parent']['mlid'])
+      ->execute();
+
+    $expected_hierarchy = array(
+      'child-1-1' => 'child-1',
+      'child-1-2' => 'child-1',
+      'child-2' => FALSE,
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+  }
+
+  /**
+   * Test automatic reparenting of menu links derived from menu routers.
+   */
+  function testMenuLinkRouterReparenting() {
+    // Run all the standard parenting tests on menu links derived from
+    // menu routers.
+    $this->testMenuLinkReparenting('system');
+
+    // Additionnaly, test reparenting based on path.
+    $links = $this->createLinkHierarchy('system');
+
+    // Move child-1-2 has a child of child-2, making the link hierarchy
+    // inconsistent with the path hierarchy.
+    $links['child-1-2']['plid'] = $links['child-2']['mlid'];
+    menu_link_save($links['child-1-2']);
+
+    // Check the new hierarchy.
+    $expected_hierarchy = array(
+      'parent' => FALSE,
+      'child-1' => 'parent',
+      'child-1-1' => 'child-1',
+      'child-2' => 'parent',
+      'child-1-2' => 'child-2',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+
+    // Now delete 'parent' directly from the database, simulating a database
+    // crash. 'child-1' and 'child-2' should get moved to the
+    // top-level.
+    // Don't do that at home.
+    db_delete('menu_links')
+      ->condition('mlid', $links['parent']['mlid'])
+      ->execute();
+    $expected_hierarchy = array(
+      'child-1' => FALSE,
+      'child-1-1' => 'child-1',
+      'child-2' => FALSE,
+      'child-1-2' => 'child-2',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+
+    // Now delete 'child-2' directly from the database, simulating a database
+    // crash. 'child-1-2' will get reparented under 'child-1' based on its
+    // path.
+    // Don't do that at home.
+    db_delete('menu_links')
+      ->condition('mlid', $links['child-2']['mlid'])
+      ->execute();
+    $expected_hierarchy = array(
+      'child-1' => FALSE,
+      'child-1-1' => 'child-1',
+      'child-1-2' => 'child-1',
+    );
+    $this->assertMenuLinkParents($links, $expected_hierarchy);
+  }
+}
+
+/**
  * Tests rebuilding the menu by setting 'menu_rebuild_needed.'
  */
 class MenuRebuildTestCase extends DrupalWebTestCase {