#907690: Breadcrumbs don't work for dynamic paths & local tasks #2 introduced a fundamental change for MENU_CALLBACK.

Whereas in Drupal 6, and Drupal 7 prior to commit, router items defined as MENU_CALLBACK would create an entry in the {menu_links} table, now they don't.

When generating a breadcrumb, menu_set_active_trail() and the call to menu_tree_page_data() will generate a tree including all existing links where type = 0 (alongside actual candidates for the breadcrumb).

On an upgraded Drupal 6 site with all core modules enabled, this is 18 links compared to 4 with a clean HEAD install (and one of those four links is for filter/tips which looks to have the wrong menu definition) - each of which has to be fully built and access checked.

On ex2 which has some contrib modules and custom modules defining menu callbacks, the number is 44.

This means that on every single page on a site, access callbacks are run for paths such as 'batch', 'user/autocomplete' and 'rss.xml' despite their type being 0 in the database and them having zero chance of ever appearing in a breadcrumb.

To fix this, we'll need the following:

1. An update handler to remove all links defined as MENU_CALLBACK where customized = 0 from the {menu_links} table.
2. An addition to the upgrade path tests to ensure that a callback which exists in both D6 and D7 won't appear in {menu_links} after upgrade, user/autocomplete exists in both versions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Note that stale menu callback links disappeared for me after applying the patch in #550254: Menu links are sometimes not properly re-parented

catch’s picture

Status: Active » Needs review
FileSize
986 bytes

Here's a patch for the update handler. No test yet, hoping to get to that this evening though.

catch’s picture

FileSize
1.87 KB

Patch with the test. Took a while to get the test to pass (and same issue with the actual upgrade), without a menu_rebuild() before running the query, not exactly clear why yet but uploading progress since this is the last non-rtbc beta blocker...

catch’s picture

FileSize
2.1 KB

See comment.

While this fixes the immediate problem #550254: Menu links are sometimes not properly re-parented (or if there's a limited subset of that which would have caught this in menu_rebuild()) would be a good followup, since this is quite fragile now.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Ok for this as a temporary solution. We should fix the underlying bug, because those should have been purged automatically by menu_rebuild().

chx’s picture

Status: Reviewed & tested by the community » Needs work

Missed module = 'system' from your query. To quote http://api.drupal.org/api/function/_menu_link_build/6 we set this as 'system', so that we can be sure to distinguish all the menu links generated automatically from entries in {menu_router}.

catch’s picture

Status: Needs work » Needs review
FileSize
2.15 KB

Update via chx, running tests locally now but they are taking /ages/.

catch’s picture

Passed locally.

sun’s picture

Hm. There's actually a simpler way, which I had to add to admin_menu's settings page in HEAD, so as to be able to work on various menu system issues for D7:

  // Delete all auto-generated menu links derived from menu router items.
  db_delete('menu_links')
    ->condition('module', 'system')
    ->execute();
  // Rebuild menu links from current menu router items.
  menu_rebuild();

Could additionally be limited to 'customized' => 0. But the effective result is that all menu links that are derived from router items are getting rebuilt. The menu_build() happens elsewhere after the update, of course.

Damien Tournoud’s picture

Wooo. admin_menu just got bumped on my list of modules not to install on any site :)

catch’s picture

Also we don't want to call API functions during the upgrade path, and a menu_rebuild() called before this update runs would actually stop it from working too.

edit: I see sun wanted to rely on the menu_rebuild() after the update, but still don't see a reason to do that beyond this patch.

catch’s picture

Issue tags: -beta blocker

Ok I'm removing the beta blocker tag from this because whenever we either fix the underlying bug or add this patch, all those links will get cleared up.

However having them is a real performance hit, and could also have strange side effects (which is how I originally found this on ex2 when a non-standard access callback was getting fired on completely unrelated pages), so either this or the menu link reparenting issue or something else that deals with this needs to block a release. We can also add an interim head2head update cleaning out the bad links until it's properly fixed.

sun’s picture

@Damien: Don't worry... it's a separate form button and form submit handler that's only available with the optional admin_devel add-on module.

@catch: Of course, the menu_rebuild() doesn't belong into the update function -- that's automatically done via drupal_flush_all_caches() after all updates ran.

juan_g’s picture

catch wrote:

Ok I'm removing the beta blocker tag from this because whenever we either fix the underlying bug or add this patch, all those links will get cleared up.

Is it all right to go beta with this critical upgrade path / API change issue pending?

If so, then it's quite possible that beta is here just after fixing #895014: All fields of a node type are lost on module disable, unless something more emerges. ;)

Good work, everyone.

catch’s picture

head2head issue is here - that needs a slightly different update run anyway, since MENU_CALLBACK ends up as 0 in the db for D7, but was 4 in D6 (nothing in menu_router for 4 in D7 so it's harmless for this update to run for head2head sites or post-beta. #933068: 907690 - head2head upgrade for stale menu_links entries.

catch’s picture

FileSize
2.11 KB

Now we're supporting updates from both 6 and 7, we need to account for both potential values for 'type' in menu_router, just changed the query to IN(0, 4) instead of = 4.

Status: Needs review » Needs work

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

catch’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

typo.

catch’s picture

FileSize
2.11 KB

sorry, impatience.

ksenzee’s picture

Status: Needs review » Reviewed & tested by the community

Yes, let's go ahead with this, since #550254: Menu links are sometimes not properly re-parented isn't a release blocker. Convenient that the two menu_router types don't overlap with anything else in either version.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome. Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -D7 upgrade path

Automatically closed -- issue fixed for 2 weeks with no activity.