#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.
Comment | File | Size | Author |
---|---|---|---|
#20 | 932134.patch | 2.11 KB | catch |
#19 | 932134.patch | 2.11 KB | catch |
#16 | 932124.patch | 2.11 KB | catch |
#7 | 932134.patch | 2.15 KB | catch |
#4 | 932134.patch | 2.1 KB | catch |
Comments
Comment #1
sunNote that stale menu callback links disappeared for me after applying the patch in #550254: Menu links are sometimes not properly re-parented
Comment #2
catchHere's a patch for the update handler. No test yet, hoping to get to that this evening though.
Comment #3
catchPatch 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...
Comment #4
catchSee 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.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk for this as a temporary solution. We should fix the underlying bug, because those should have been purged automatically by menu_rebuild().
Comment #6
chx CreditAttribution: chx commentedMissed 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}.
Comment #7
catchUpdate via chx, running tests locally now but they are taking /ages/.
Comment #8
catchPassed locally.
Comment #9
sunHm. 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:
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.
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedWooo.
admin_menu
just got bumped on my list of modules not to install on any site :)Comment #11
catchAlso 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.
Comment #12
catchOk 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.
Comment #13
sun@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.
Comment #14
juan_g CreditAttribution: juan_g commentedcatch wrote:
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.
Comment #15
catchhead2head 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.
Comment #16
catchNow 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.
Comment #19
catchtypo.
Comment #20
catchsorry, impatience.
Comment #21
ksenzeeYes, 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.
Comment #22
webchickAwesome. Committed to HEAD. Thanks!