Updated: Comment #20

Problem/Motivation

A programatically added menu item is not removed from the menu after the menu link code has been removed.

Steps to reproduce

  • Create a module with a single route and callback.
  • Add a menu link to the route using an implementation of hook_menu_link_defaults(). Add the menu link to the 'main' menu (for convenience).
  • Enable the module.
  • Check that the menu link appears in the main menu.
  • Remove the code from MODULE_menu_link_defaults().
  • Clear the cache
  • Observe that the menu link is stil in the main menu.

Proposed resolution

Remaining tasks

  • None (patch in #29 is ready for review)

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Caligan’s picture

Version: 7.0 » 7.x-dev
Status: Active » Needs review
FileSize
1.41 KB

Successfully replicated this with the provided steps. Menu items registered in the menu_links table that are later turned into MENU_CALLBACKs (which should not be in menu_links) aren't removed from the table by _menu_navigation_links_rebuild() because their paths are still valid.

This patch sieves for MENU_CALLBACK items and removes them from menu_links along with any stale entries.

ins0mn1ac22’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

patch looks good. nitpick:

+    ->condition(db_or()->condition('router_path', $paths, 'NOT IN')->condition('router_path',$paths_drop, 'IN'))

missing a space after 'router_path' in the second db_or() condition.

here are some hints to help you build a test:

// file modules/simpletest/tests/menu_test.module
/**
 * Implements hook_menu().
 */
function menu_test_menu() {
  // This item is menu-altered by another test module.
  $items['menu_alter_test'] = array(
    'title' => 'Test hook_menu_alter',
    'page callback' => 'node_save',
    'type' => MENU_NORMAL_ITEM,
  );


// file: modules/simpletest/tests/menu.test
class MenuRouterAlterTestCase extends DrupalWebTestCase {
  public static function getInfo() {
    return array(
      'name' => 'Menu alter',
      'description' => 'Tests menu alter functionality.',
      'group' => 'Menu',
    );
  } 
  
  public function setUp() {
    parent::setUp('menu_test');
  } 
  
  public function testMenuAlter() {
    // Check that our link from menu_test_menu() is registered.
    // Enable our menu alter test module, clear caches.
    module_enable('menu_alter_test');

    // Validate that our menu link is gone.
  }
}


// file: modules/simpletest/tests/menu_alter_test.module
/**     
 * Implements hook_menu_alter().
 */   
function menu_alter_test_menu_alter(&$items) {
  // Set an item to MENU_CALLBACK from MENU_NORMAL_ITEM.
  $items['menu_alter_test'] = MENU_CALLBACK;
}


// file: modules/simpletest/tests/menu_alter_test.info
name = "Hook menu alter tests"
description = "Support module for menu alter hook testing."
package = Testing
version = VERSION
core = 7.x
hidden = TRUE
dependencies[] = menu_test
Caligan’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.52 KB

Patch bundled with a test verifying that menu items changed to MENU_CALLBACK are removed from menu_links table (and so from visible menus).

sun’s picture

Title: Changing a Menu Types Doesn't Remove Items From menu_link table » Menu link is not removed after changing the menu router item's type
Version: 7.x-dev » 8.x-dev
sun’s picture

#1148940: Menu links not removed from menu has been marked as duplicate

Blackice2999’s picture

Status: Needs review » Reviewed & tested by the community

Hi,

works fine for me. It can be good tested with Views and also fixes: #1029022: Menu item created in a view is not removed from the menu when removed from the view

sun’s picture

Status: Reviewed & tested by the community » Needs review

This needs some more reviews, especially from menu system maintainers.

I don't have time myself right now, but will try to come back to this issue shortly.

yoroy’s picture

sun’s picture

+++ b/includes/menu.inc
@@ -2689,11 +2689,16 @@ function _menu_link_build($item) {
     if ($item['_visible']) {
...
     }
+    // Identify MENU_CALLBACK items; used to discard records in menu_links.
+    if ($item['type'] === MENU_CALLBACK) {
+      $menu_drop[$path] = $item;
+    }

Why isn't this simply an "else" ?

-2 days to next Drupal core point release.

hefox’s picture

Same question as sun; switching to other type of menu items other than normal item, like local tasks, will make them not-menu-links, so it'd only work for a subset of links it needs to.

Why keep track of both path and item if only using path? Seems like a waist of memory to keep track of extra information.

drzraf’s picture

the patch (#3) did not removed the menu entry created by a view (dup: #1418618: Views may leave stale menu entries)

hefox’s picture

Status: Needs review » Needs work
hefox’s picture

Status: Needs work » Needs review

Here's a slightly different take on it
Patch:

  • Instead of an or condition, changes the paths it checks against to be only visible paths, so the query instead of being "delete all non-customized menu links who's path doesn't exist anymore in menu router" (what core has) is "delete all non-customized menu entries that who's path isn't "visible" in the menu -- ie has a menu entry". This seems to work and makes logically sense to me.
  • Moves the test into menu_test where the other core menu tests exists
  • Changes the static function to be more usable if need to add more test on menu information changing.

Didn't really change the actual test other than the function call that sets the static.

hefox’s picture

Right.. the patch.

cweagans’s picture

Issue tags: +Needs backport to D7
ifernando’s picture

The patch menu_callback_drop-1079628-1.patch works for me.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, drupal_1079628_menu_type_change_13.patch, failed testing.

Sutharsan’s picture

Title: Menu link is not removed after changing the menu router item's type » Programatically added Menu link is not removed after removing the code.
Issue summary: View changes

Since the original report and patches the menu system got a full overhaul, but the problem still remains. Changing issue title and description to match the current situation.

joegraduate’s picture

Version: 8.0.x-dev » 7.x-dev

As far as I can tell, this is no longer a problem in Drupal 8.0.x. Just tested a clean install of Drupal 8.0.4 and this issue does not exist. This is still a problem in 7.x, however.

joegraduate’s picture

The attached patch is a backport of #14. This patch fixes the problem for me in the latest 7.x-dev version of Drupal core.

joegraduate’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: programatically_added-1079628-21-d7.patch, failed testing.

joegraduate’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

Updated patch with fixed tests.

joegraduate’s picture

joegraduate’s picture

Patch in #24 still applies cleanly to the latest 7.x-dev and 7.44.

joegraduate’s picture

Issue summary: View changes

Patch in #24 still applies cleanly to the latest 7.x-dev and 7.50. Please review!

Sutharsan’s picture

Status: Needs review » Needs work
@@ -2958,10 +2960,10 @@ function _menu_navigation_links_rebuild($menu) {
         ->execute();
     }
   }
-  // Find any item whose router path does not exist any more.
+  // Find any item whose router path does not exist or is no longer visible.
   $result = db_select('menu_links')
     ->fields('menu_links')
-    ->condition('router_path', $paths, 'NOT IN')
+    ->condition('router_path', $default_menu_link_paths, 'NOT IN')
     ->condition('external', 0)
     ->condition('updated', 0)
     ->condition('customized', 0)

If I interpret the code correctly the conditions are _and_-ed. The description says _or_. If I'm right one must be corrected. (but I have to be careful with negative logic)

joegraduate’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
211 bytes

Good catch, @Sutharsan. I've updated the confusing comment you identified to more accurately describe what is happening.

joegraduate’s picture

Issue summary: View changes
joegraduate’s picture

kopeboy’s picture

Guys, how to delete an orphaned menu item before this is commited!?

I have menu items on main menu that were created with Page Manager. The pages were deleted and now I can't change paths or delete them.
I even tried recreating pages with the same path, change menu item to tab and delete pages but tat doesn't work.
Now I have 3 menu items with same name, same link

Update: ok I could delete them by changing the name (the Menu link title) and then doing Reset > they immediately disappeared.

thomasmurphy’s picture

I had a lot of problem with the menu table in D7 on a very large website with a very large menu which had stopped working because it wasn't managed properly. There were some strange examples items appearing and disappearing, so I finally just connected to the database directly and cleaned it up there. What I did discover was that there were a lot of orphaned menu records which didn't appear in the UI because their parent no longer existed, as per this issue, but if a menu item with a matching name was created they appeared in the UI again. The next time I see anything that weird again I'm going straight to the database.