Better breadcrumbs for callbacks, dynamic paths, tabs

Oh I meant to include some example URLs that are fixed with this change. Compare the breadcrumb before and after applying this patch. On some forms the breadcrumb is entirely empty (only including the "Home" link), and on others it's just missing a few items in the middle of the path.

node/[nid]/delete
admin/build/node-type/[node-type]/delete (now admin/build/types/[node-type]/delete)

admin/build/menu/item/[mlid]/edit (now admin/build/menus/[menu-name]/[mlid])
admin/build/menu/item/[mlid]/delete (now admin/build/menus/[menu-name]/[mlid]/delete)
admin/build/menu/item/[mlid]/reset (now admin/build/menus/[menu-name]/[mlid]/reset)

I'm sure there are more paths than this fixed, especially within contrib modules that have deep configuration such as ImageCache, Flag, and Organic Groups.

We changed the menu paths for damn good reason - otherwise you hsve to blacklist all possible core and contrib paths from being menu names.

As far as I can tell, we changed the menu paths because of a limitation in the menu system that made breadcrumbs not work once underneath a MENU_LOCAL_TASK. I'm not sure what you mean by "blacklist all possible core and contrib paths from being menu names". This patch would drastically *reduce* the number of menu entries. Right now CCK for example loops through ALL the fields for ALL types and manually makes an entry for each one. The new approach would replace these entries with a much simplified, single menu handlers: admin/build/content/types/%node_type/fields/%field.

I'm working on a reroll since we're going to run into this breadcrumb/path problem all over again in the ImageCache UI.

This patch ties _menu_link_translate to arg().

_menu_link_translate() is oddly central to the loading of menu items and checking access control. _menu_link_translate() actually sets the $item['access'] flag, so unless we move the swapping of % wildcards up higher in the callback chain, it seems like the most appropriate place to put such logic. The use of arg() is only applicable when the item being checked is a page that the user is actively visiting, so it seems like this should have minimal side-effect on any other functionality.

Here's a patch that should pass tests, since (surpise!) SimpleTest was correct in that I had broken something in the previous patch. I still think the test is slightly flawed, but it adequately caught the problem caused by the changes. This patch also makes some code cleanup to be more DNTNG-friendly.

I'm still looking into Damien's suggestion in #6, since I'm not really sure how that to_arg() magic works.

I've marked this patch "needs review" so that testing bot can have a crack at it.

The patch above totally breaks book.module breadcrumbs it seems. Back to the code...

New version, even less code than before. More docs on the menu_tree_data() function. This patch should work with book.module in addition to supporting the dynamic paths such as admin/build/types/%/delete. Let's see what testing bot thinks of this one.

This version also (hopefully) addresses the problem of _menu_link_translate being tied to arg(). Now an "active_map" property is passed down in menu items that are in the active trail.

Updating for a missed "node-type" URL that caused the above test to fail.

I've split off the menu-customize/node-type re-naming to #508570: Restore URL consistency for node types and menus, so we can reconsider that particular change without affecting the menu.inc changes done here. I'll re-roll with the applicable changes only, though it won't correct items like "menu-customize" from being dropped from the breadcrumb.

Here's a patch for testing which implements the excellent suggestion from chx to use $item['original_path'] rather than arg(). It strips out all the menu path changes (meaning items are still dropped from the breadcrumb when editing content types or menus), but maintains an increased functionality when working with items that are underneath dynamic paths.

Hmm, I think I got a test bot with notices enabled or something. Anyway, here's another version that correctly uses db_or() to match DBTNG best practices.

Notice cleanup.

Arg, okay now fixes all the notices hopefully.

After some investigation, it seems that having the $item['original_path'] available also makes breadcrumbs within tabs a piece of cake. This patch abstracts this % to value replacement, since now it looks like we're doing this exact task in 4 different places in menu.inc.

Now with tests for breadcrumbs. I'm moving this to a bug report since breadcrumbs clearly are very broken, we just didn't have any tests previously to prove so. There's no way we can ship another version of Drupal with breadcrumbs in their current state, plus this issue is blocking #371374: Add ImageCache UI Core.

The API change to _menu_tree_data() is mostly because I couldn't figure out for the life of me how the current function works (the whole, "we're one loop behind, pop off and return the previous..."), and I needed to make the change there to include hidden items in the tree. Since I needed to rewrite the function, I figured it would be a good opportunity to correct the arguments that are passed into it.

The "reset" check prevents the Menu module "reset" functionality from re-parenting a menu link to a hidden item that is not in the interface. Basically when ordering menu items through the interface, you have to assign parents that visibly exist in the menu (MENU_NORMAL_ITEM), but when building the links off of the menu router, Drupal allows any type of link to be a parent so that breadcrumbs can be built properly. The particular problem I had was with the "user/logout" and "user/login" links. Because the path at "user" is NOT a MENU_NORMAL_ITEM (it's a callback), if you used "Reset" on the link through the Menu UI, it would end up being parented to the MENU_CALLBACK. While this might actually be correct when it comes to the breadcrumb, it was a strange thing to have happen in the UI, since it would indent the "Logout" link since the "user" item was it's parent, but the "user" item was not visible. There might be a better way to solve this than special casing for this the menu.module UI, especially since the "reset" functionality is broken anyway (it will always shove all links into the "navigation" menu, regardless of where the link was originally defined).

The get_object_vars() call looks like it was added #320510: [DBTNG] revamp menu.inc with DBTNG syntax. I'm not sure if my changes make it unnecessary or not.

The hidden change is definitely a bug in the patch. I don't think a backport to D6 would be wise, since this fundamentally changes the way link structures are built, even if we didn't change the function signatures.

I'd like to remove the "reset" exception, but that would consequently break our (already not-quite functioning) reset test. We might need to fix that first, though fixing the way "reset" works could take a significant effort, since it really needs to be completely rethought. The current approach to reset doesn't work at all, other than to move the link to the navigation menu at the root level.

This patch removes the unnecessary ;hidden=TRUE

I'm giving up on this issue for a least a while. The feedback here is driving me insane.

Okay, per a classic webchick-calming down and some inner reflection, here's what I'd like to have happen on this issue.

I'm stuck from continuing further on this issue because I don't know how to fix things any further (within the scope of this issue). I set out to "fix the breadcrumbs" by solving the following:

- The breadcrumb should maintain a true "hierarchy" that is reflected by every link the user clicks into the site. Steps within the hierarchy are not skipped, nor does a step suddenly disappear when it was present on the previous page.

- The breadcrumb stays even if you are on a tab, or a sub-tab of a page. This applies not only to admin pages, but also to book nodes or other trees whose breadcrumb mirrors the URL path.

- The breadcrumb is present (and accurate) for all confirmation pages. It should contain all the pages that it took to get to the confirmation form.

So that's what I've set out to do, and it does seem to be working. I've added additional tests to make sure that all these cases work and hopefully don't become broken again.

Now we need to decide, what else needs to be done to fix breadcrumbs? There are other problems that we're running into, such as object_get_vars() call and the "reset" link functionality being fundamentally broken. However, I'd really, really just like to get breadcrumbs "fixed" and move on. These problems are not directly caused nor affected by this patch, and it seems they should be left to followups. If there's a *better* way of handling the "reset" problem, I'd like to know what that is (I've tried several already), but the current approach is the best I can think of.

I need to know if the fundamental approach I'm using is even close to correct. This patch is essentially allowing both dynamic paths such as "node/%/edit" and explicit paths such as "node/10" to reference each other as necessary to get a full parentage. Previously "node/%/edit" (which is not in the menu tree) didn't even try to check against an explicit path like "node/10". Now it tries to match on explicit paths first and the falling back to dynamic ones by adding an extra condition to the "IN ()" SQL statement we're using (it's already using an IN statement because we're also matching on ). In addition it includes MENU_CALLBACK items when building up parentage, so they are included in the breadcrumb. Other than those specific changes, the rest of the code is just being updated to make those two things possible.

this implementation of function menu_tree_data() will be dramatically less efficient, since it will repeatedly iterate over every link in the menu and essentially throws away the information we got by ordering by the p1...p9 columns.

To clarify, it seems that our current implementation uses foreach() over a database result object. Normally each call to foreach() would "rewind" the result set and start from the beginning, but our implementation does not do this for DB compatibility (according to Crell). So our rewind function does nothing at all (from includes/database/prefetch.inc):

  public function rewind() {
    // Nothing to do: our DatabaseStatement can't be rewound.
  }

So what we need to do to keep the same efficiency is put an array_pop/_shift() in our loop to keep this same efficiency, but be much more clear about what's actually happening here.