- Patch #631550 by sun: fixed stale and improper logic for...

- Patch #631550 by sun: fixed stale and improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB. Added lots of code comments, and added tests.

includes/menu.inc

@@ -162,7 +162,7 @@
  * parent item. An example is the path "node/52/edit", which performs the
  * "edit" task on "node/52".
  */
-define('MENU_LOCAL_TASK', MENU_IS_LOCAL_TASK);
+define('MENU_LOCAL_TASK', MENU_IS_LOCAL_TASK | MENU_VISIBLE_IN_BREADCRUMB);
 
 /**
  * Menu type -- The "default" local task, which is initially active.
@@ -170,7 +170,7 @@
  * Every set of local tasks should provide one "default" task, that links to the
  * same path as its parent when clicked.
  */
-define('MENU_DEFAULT_LOCAL_TASK', MENU_IS_LOCAL_TASK | MENU_LINKS_TO_PARENT);
+define('MENU_DEFAULT_LOCAL_TASK', MENU_IS_LOCAL_TASK | MENU_LINKS_TO_PARENT | MENU_VISIBLE_IN_BREADCRUMB);
 
 /**
  * Menu type -- An action specific to the parent, usually rendered as a link.
@@ -178,7 +178,7 @@
  * Local actions are menu items that describe actions on the parent item such
  * as adding a new user, taxonomy term, etc.
  */
-define('MENU_LOCAL_ACTION', MENU_IS_LOCAL_TASK | MENU_IS_LOCAL_ACTION);
+define('MENU_LOCAL_ACTION', MENU_IS_LOCAL_TASK | MENU_IS_LOCAL_ACTION | MENU_VISIBLE_IN_BREADCRUMB);
 
 /**
  * @} End of "Menu item types".
@@ -2318,12 +2318,14 @@ function menu_get_router() {
  * Builds a link from a router item.
  */
 function _menu_link_build($item) {
-  if ($item['type'] == MENU_CALLBACK) {
-    $item['hidden'] = -1;
-  }
-  elseif ($item['type'] == MENU_SUGGESTED_ITEM) {
+  // Suggested items are disabled by default.
+  if ($item['type'] == MENU_SUGGESTED_ITEM) {
     $item['hidden'] = 1;
   }
+  // Hide all items that are not visible in the tree.
+  elseif (!($item['type'] & MENU_VISIBLE_IN_TREE)) {
+    $item['hidden'] = -1;
+  }
   // Note, we set this as 'system', so that we can be sure to distinguish all
   // the menu links generated automatically from entries in {menu_router}.
   $item['module'] = 'system';
@@ -2374,8 +2376,9 @@ function _menu_navigation_links_rebuild($menu) {
           $item['plid'] = $existing_item['plid'];
         }
         else {
-          // If it moved, put it at the top level in the new menu.
-          $item['plid'] = 0;
+          // It moved to a new menu. Let menu_link_save() try to find a new
+          // parent based on the path.
+          unset($item['plid']);
         }
         $item['has_children'] = $existing_item['has_children'];
         $item['updated'] = $existing_item['updated'];
@@ -2574,7 +2577,6 @@ function _menu_delete_item($item, $force = FALSE) {
  *   saved.
  */
 function menu_link_save(&$item) {
-
   drupal_alter('menu_link', $item);
 
   // This is the easiest way to handle the unique internal path '<front>',
@@ -2600,15 +2602,17 @@ 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 {
-      // Don't bother with the query - mlid can never equal zero..
       $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
@@ -2616,10 +2620,9 @@ function menu_link_save(&$item) {
     if ($item['module'] == 'system') {
       $query->condition('module', 'system');
     }
-    else {
-      // If not derived from a router item, we respect the specified menu name.
-      $query->condition('menu_name', $item['menu_name']);
-    }
+    // 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'];
@@ -2634,18 +2637,16 @@ function menu_link_save(&$item) {
       }
     } while ($parent === FALSE && $parent_path);
   }
-  if ($parent !== FALSE) {
+  // If a parent link was found, assign it and derive its menu.
+  if (!empty($parent['mlid'])) {
+    $item['plid'] = $parent['mlid'];
     $item['menu_name'] = $parent['menu_name'];
   }
-  $menu_name = $item['menu_name'];
-  // Menu callbacks need to be in the links table for breadcrumbs, but can't
-  // be parents if they are generated directly from a router item.
-  if (empty($parent['mlid']) || $parent['hidden'] < 0) {
-    $item['plid'] =  0;
-  }
+  // If no corresponding parent link was found, move the link to the top-level.
   else {
-    $item['plid'] = $parent['mlid'];
+    $item['plid'] = 0;
   }
+  $menu_name = $item['menu_name'];
 
   if (!$existing_item) {
     $item['mlid'] = db_insert('menu_links')
@@ -2667,15 +2668,17 @@ function menu_link_save(&$item) {
       ->execute();
   }
 
-  if (!$item['plid']) {
+  // Directly fill parents for top-level links.
+  if ($item['plid'] == 0) {
     $item['p1'] = $item['mlid'];
     for ($i = 2; $i <= MENU_MAX_DEPTH; $i++) {
       $item["p$i"] = 0;
     }
     $item['depth'] = 1;
   }
+  // Otherwise, ensure that this link's depth is not beyond the maximum depth
+  // and fill parents based on the parent link.
   else {
-    // Cannot add beyond the maximum depth.
     if ($item['has_children'] && $existing_item) {
       $limit = MENU_MAX_DEPTH - menu_link_children_relative_depth($existing_item) - 1;
     }
@@ -3083,8 +3086,24 @@ function _menu_router_build($callbacks) {
       $parent_path = implode('/', array_slice($item['_parts'], 0, $i));
       if (isset($menu[$parent_path])) {
 
-        $parent = $menu[$parent_path];
-
+        $parent = &$menu[$parent_path];
+
+        // If we have no menu name, try to inherit it from parent items.
+        if (!isset($item['menu_name'])) {
+          // If the parent item of this item does not define a menu name (and no
+          // previous iteration assigned one already), try to find the menu name
+          // of the parent item in the currently stored menu links.
+          if (!isset($parent['menu_name'])) {
+            $menu_name = db_query("SELECT menu_name FROM {menu_links} WHERE router_path = :router_path", array(':router_path' => $parent_path))->fetchField();
+            if ($menu_name) {
+              $parent['menu_name'] = $menu_name;
+            }
+          }
+          // If the parent item defines a menu name, inherit it.
+          if (!empty($parent['menu_name'])) {
+            $item['menu_name'] = $parent['menu_name'];
+          }
+        }
         if (!isset($item['tab_parent'])) {
           // Parent stores the parent of the path.
           $item['tab_parent'] = $parent_path;

modules/menu/menu.module

@@ -399,15 +399,23 @@ function _menu_parents_recurse($tree, $menu_name, $indent, &$options, $exclude,
 }
 
 /**
- * Reset a system-defined menu item.
+ * Reset a system-defined menu link.
  */
-function menu_reset_item($item) {
-  $new_item = _menu_link_build(menu_get_item($item['router_path']));
+function menu_reset_item($link) {
+  // To reset the link to its original values, we need to retrieve its
+  // definition from hook_menu(). Otherwise, for example, the link's menu would
+  // not be reset, because properties like the original 'menu_name' are not
+  // stored anywhere else. Since resetting a link happens rarely and this is a
+  // one-time operation, retrieving the full menu router does no harm.
+  $menu = menu_get_router();
+  $router_item = $menu[$link['router_path']];
+  $new_link = _menu_link_build($router_item);
+  // Merge existing menu link's ID and 'has_children' property.
   foreach (array('mlid', 'has_children') as $key) {
-    $new_item[$key] = $item[$key];
+    $new_link[$key] = $link[$key];
   }
-  menu_link_save($new_item);
-  return $new_item;
+  menu_link_save($new_link);
+  return $new_link;
 }
 
 /**

modules/menu/menu.test

@@ -252,10 +252,9 @@ class MenuTestCase extends DrupalWebTestCase {
     );
 
     // Add menu link.
-    $this->drupalPost("admin/structure/menu/manage/$menu_name/add", $edit, t('Save'));
+    $this->drupalPost(NULL, $edit, t('Save'));
     $this->assertResponse(200);
     // Unlike most other modules, there is no confirmation message displayed.
-
     $this->assertText($title, 'Menu link was added');
 
     // Retrieve menu link.
@@ -267,7 +266,10 @@ class MenuTestCase extends DrupalWebTestCase {
     // We know link1 is at the top level, so $item1['deptj'] == 1 and $item1['plid'] == 0.
     // We know that the parent of link2 is link1, so $item2['plid'] == $item1['mlid'].
     // Both menu links were created in the navigation menu.
-    $this->assertTrue($item['menu_name'] == $menu_name && $item['plid'] == $plid && $item['link_path'] == $link && $item['link_title'] == $title, 'Menu link has correct data');
+    $this->assertEqual($item['menu_name'], $menu_name);
+    $this->assertEqual($item['plid'], $plid);
+    $this->assertEqual($item['link_path'], $link);
+    $this->assertEqual($item['link_title'], $title);
     if ($plid == 0) {
       $this->assertTrue($item['depth'] == 1 && !$item['has_children'] && $item['p1'] == $item['mlid'] && $item['p2'] == 0, 'Menu link has correct data');
     }
@@ -370,9 +372,6 @@ class MenuTestCase extends DrupalWebTestCase {
     // Verify menu link.
     $this->drupalGet('');
     $this->assertNoText($title, 'Menu link was reset');
-
-    // Verify menu link.
-    $this->drupalGet('');
     $this->assertText($old_title, 'Menu link was reset');
   }
 

modules/simpletest/tests/menu.test

@@ -6,11 +6,11 @@
  * Provides SimpleTests for menu.inc.
  */
 
-class MenuIncTestCase extends DrupalWebTestCase {
+class MenuRouterTestCase extends DrupalWebTestCase {
   public static function getInfo() {
     return array(
-      'name' => 'Hook menu tests',
-      'description' => 'Test menu hook functionality.',
+      'name' => 'Menu router',
+      'description' => 'Tests menu router and hook_menu() functionality.',
       'group' => 'Menu',
     );
   }
@@ -197,9 +197,9 @@ class MenuIncTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Tests for menu hiearchy.
+   * Tests for menu hierarchy.
    */
-  function testMenuHiearchy() {
+  function testMenuHierarchy() {
     $parent_link = db_query('SELECT * FROM {menu_links} WHERE link_path = :link_path', array(':link_path' => 'menu-test/hierarchy/parent'))->fetchAssoc();
     $child_link = db_query('SELECT * FROM {menu_links} WHERE link_path = :link_path', array(':link_path' => 'menu-test/hierarchy/parent/child'))->fetchAssoc();
     $unattached_child_link = db_query('SELECT * FROM {menu_links} WHERE link_path = :link_path', array(':link_path' => 'menu-test/hierarchy/parent/child2/child'))->fetchAssoc();
@@ -208,6 +208,95 @@ class MenuIncTestCase extends DrupalWebTestCase {
     $this->assertEqual($unattached_child_link['plid'], $parent_link['mlid'], t('The parent of a non-directly attached child is correct.'));
   }
 
+  /**
+   * Tests menu link depth and parents of local tasks and menu callbacks.
+   */
+  function testMenuHidden() {
+    // Verify links for one dynamic argument.
+    $links = db_select('menu_links', 'ml')
+      ->fields('ml')
+      ->condition('ml.router_path', 'menu-test/hidden/menu%', 'LIKE')
+      ->orderBy('ml.router_path')
+      ->execute()
+      ->fetchAllAssoc('router_path', PDO::FETCH_ASSOC);
+
+    $parent = $links['menu-test/hidden/menu'];
+    $depth = $parent['depth'] + 1;
+    $plid = $parent['mlid'];
+
+    $link = $links['menu-test/hidden/menu/list'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/add'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/settings'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/manage/%'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $parent = $links['menu-test/hidden/menu/manage/%'];
+    $depth = $parent['depth'] + 1;
+    $plid = $parent['mlid'];
+
+    $link = $links['menu-test/hidden/menu/manage/%/list'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/manage/%/add'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/manage/%/edit'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/menu/manage/%/delete'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    // Verify links for two dynamic arguments.
+    $links = db_select('menu_links', 'ml')
+      ->fields('ml')
+      ->condition('ml.router_path', 'menu-test/hidden/block%', 'LIKE')
+      ->orderBy('ml.router_path')
+      ->execute()
+      ->fetchAllAssoc('router_path', PDO::FETCH_ASSOC);
+
+    $parent = $links['menu-test/hidden/block'];
+    $depth = $parent['depth'] + 1;
+    $plid = $parent['mlid'];
+
+    $link = $links['menu-test/hidden/block/list'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/block/add'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/block/manage/%/%'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $parent = $links['menu-test/hidden/block/manage/%/%'];
+    $depth = $parent['depth'] + 1;
+    $plid = $parent['mlid'];
+
+    $link = $links['menu-test/hidden/block/manage/%/%/configure'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+
+    $link = $links['menu-test/hidden/block/manage/%/%/delete'];
+    $this->assertEqual($link['depth'], $depth, t('%path depth @link_depth is equal to @depth.', array('%path' => $link['router_path'], '@link_depth' => $link['depth'], '@depth' => $depth)));
+    $this->assertEqual($link['plid'], $plid, t('%path plid @link_plid is equal to @plid.', array('%path' => $link['router_path'], '@link_plid' => $link['plid'], '@plid' => $plid)));
+  }
+
   /**
    * Test menu_set_item().
    */

modules/simpletest/tests/menu_test.module

@@ -58,6 +58,112 @@ function menu_test_menu() {
     'page arguments' => array(TRUE),
     'access arguments' => array('access content'),
   );
+
+  // Hidden tests; base parents.
+  // Same structure as in Menu and Block modules. Since those structures can
+  // change, we need to simulate our own in here.
+  $items['menu-test'] = array(
+    'title' => 'Menu test root',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+  );
+  $items['menu-test/hidden'] = array(
+    'title' => 'Menu test parent',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+  );
+
+  // Hidden tests; one dynamic argument.
+  $items['menu-test/hidden/menu'] = array(
+    'title' => 'Menus',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+  );
+  $items['menu-test/hidden/menu/list'] = array(
+    'title' => 'List menus',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -10,
+  );
+  $items['menu-test/hidden/menu/add'] = array(
+    'title' => 'Add menu',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_ACTION,
+  );
+  $items['menu-test/hidden/menu/settings'] = array(
+    'title' => 'Settings',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_TASK,
+    'weight' => 5,
+  );
+  $items['menu-test/hidden/menu/manage/%menu'] = array(
+    'title' => 'Customize menu',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
+  $items['menu-test/hidden/menu/manage/%menu/list'] = array(
+    'title' => 'List links',
+    'weight' => -10,
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
+  );
+  $items['menu-test/hidden/menu/manage/%menu/add'] = array(
+    'title' => 'Add link',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_ACTION,
+  );
+  $items['menu-test/hidden/menu/manage/%menu/edit'] = array(
+    'title' => 'Edit menu',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_TASK,
+    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,
+  );
+  $items['menu-test/hidden/menu/manage/%menu/delete'] = array(
+    'title' => 'Delete menu',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
+
+  // Hidden tests; two dynamic arguments.
+  $items['menu-test/hidden/block'] = array(
+    'title' => 'Blocks',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+  );
+  $items['menu-test/hidden/block/list'] = array(
+    'title' => 'List',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'weight' => -10,
+  );
+  $items['menu-test/hidden/block/add'] = array(
+    'title' => 'Add block',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_ACTION,
+  );
+  $items['menu-test/hidden/block/manage/%/%'] = array(
+    'title' => 'Configure block',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+  );
+  $items['menu-test/hidden/block/manage/%/%/configure'] = array(
+    'title' => 'Configure block',
+    'type' => MENU_DEFAULT_LOCAL_TASK,
+    'context' => MENU_CONTEXT_INLINE,
+  );
+  $items['menu-test/hidden/block/manage/%/%/delete'] = array(
+    'title' => 'Delete block',
+    'page callback' => 'node_page_default',
+    'access arguments' => array('access content'),
+    'type' => MENU_LOCAL_TASK,
+    'context' => MENU_CONTEXT_NONE,
+  );
+
   return $items;
 }