Patch to add support for hook_breadcrumb to permit a module to have better control over breadcrumb generation.

Simple example: Suppose a user wants to replace "Home" with "example.com" for all breadcrumbs, including the menu based ones. A module could implement this hook to achieve that.

CommentFileSizeAuthor
#19 bc-weight-73834-5.patch1.5 KBpwolanin
hook_breadcrumb.patch751 bytes_craig
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

_craig’s picture

Status: Active » Needs review
drewish’s picture

subscribing. this would cut down on a lot of problems the image_gallery has with path auto and other breadcrumb modifying modules.

_craig’s picture

The intent of this patch is to permit a single breadcrumb-focused module (such as taxonomy_breadcrumb, custom_breadcrumb, or some yet unwritten breadcrumb-focused module) to have better control over all breadcrumbs. Right now that isn't possible with the core drupal_set_breadcrumb function where it's basically a free-for-all. Multiple modules can alter breadcrumbs using drupal_set_breadcrumb but currently only the "last" call wins. Since modules can call drupal_set_breadcrumb in any number of places this makes it difficult for module developers to sort out interactions with other modules.

This patch doesn't address how normal (i.e. not breadcrumb-focused) modules would fight it out, but would permit a single module to have a final say over breadcrumb generation. If "normal" modules started using this proposed hook (instead of calling drupal_set_breadcrumb), it wouldn't solve this problem. With this patch, only a single hook_breadcrumb would get called (controlled by the module weight).

Perhaps this patch could be expanded to assist with the fight over drupal_set_breadcrumb, but that wasn't really my original intent.

Full Discloser: I'm the author and maintainer of the Taxonomy Breadcrumb module.

thehong’s picture

+1 for this idea.

:-)

drewish’s picture

_craig, this would solve my problems i could just tell users to re-weight their modules to get them in the order that they're expecting. i just don't want to have to modify image_gallery to check if any other bread crumb modules are enabled disable it's own.

catch’s picture

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

Didn't make it in, bumping. Still applies.

birdmanx35’s picture

Still applies with offset.

RobLoach’s picture

Status: Needs review » Needs work

This sounds like a very handy feature.... I would like to see more done with hook_breadcrumb though. Maybe an $op saying what we're currently doing with it (view, set, etc)? Having more arguments passed in about the context of which it is being called would prove helpful.

webchick’s picture

Does it make sense for this to be a hook? Isn't it more a case of one module saying, "Do breadcrumbs this way!" rather than several modules saying "Do this." "Do this also." "Also do this..."

RobLoach’s picture

I find sometimes that some of the modules that affect the breadcrumb conflict with each other. In one of my projects, I was using a Panel that displayed a number of different Views and mini-panels. All of these panes, views, and nodes conflicted with the breadcrumb, resulting in something that definitely didn't make sense as its default. A solution to this is beyond me.

beginner’s picture

I've just posted about this on the dev list.

beginner’s picture

The following post explains clearly what the current problem is, and provides an example:
http://lists.drupal.org/pipermail/development/2008-June/030337.html

A related feature request is #73834: Allow multiple breadcrumbs on a page.

hook_breadcrumb() is definitely the way to go. It can be expanded to accommodate for more use cases.

Now, we have two ways to go about it:

1) the minimalist way, which would be to get this very simple patch up to RTBC status. Only this would already save a lot of headaches.
Follow up issues can later build up on this with more features.

2) The comprehensive way, which would add $op, $path, $args, etc, and make multiple breadcrumbs possible (with one default breadcrumb).

It really boils down to which way is more likely to be accepted by the core commiters. Can someone get them to weigh in on this issue, so that we can proceed with patch building and testing, knowing we have their blessing (i.e. knowing we won't be wasting our time)?

webchick’s picture

There is only one core committer currently for D7. Three guesses who it is. ;)

As mentioned on the devel list, I'd be curious to know how far the new D6 theming system can help on this issue.

beginner’s picture

I don't have direct communication channels with any of my three guesses... ;]

We probably can use the theming API as a roundabout way to override the breadcrumbs, but it can only be a temporary solution until core is fixed: we are talking about the content about the breadcrumbs, here, not how they are themed. Content and Display are supposed to be separate, so using the theming system to alter the content of the breadcrumbs does not correspond to the guidelines.

beginner’s picture

Title: New hook, hook_breadcrumb » fix breadcrumbs arbitration bug with hook_breadcrumb
Category: feature » bug

Actually, I thought about it a bit more and I agree with pwolanin (who proposes another patch there): this is a bug.

Everything seems to work fine within Drupal core, and there is no conflict within core modules as far as setting breadcrumbs is concerned.
However, the whole purpose of core is to allow contrib modules to do their stuff. Drupal core uses the hook_*() system to arbitrate the conflicting demands of contrib modules.

However, in this case core completely fails to perform its arbitration duty. As noted in some of the concrete examples above, contrib modules have no clear way to properly estimate when to call drupal_set_breadcrumbs(). It can't be too early, nor too late. Drupal should not allow a lot of guess work to come into play.

The patch offered at the begginning of this thread (hook_breadcrumb) is perfectly right to fix this bug. It does not introduce new feature straight away, so it can be applied in D7 and promptly backported to D6. On its own, this patch does not modify in any way the behavior of core, but it provides a way for contrib module maintainers to fix bugs that are littering contrib modules.

After the simpler hook_breadcrumbs has been backported, we can then use the other thread mentioned above to enhance the feature set of hook_breadcrumbs() for D7.

beginner’s picture

Status: Needs work » Needs review

The original patch still applies and allows maintainers to fix breadcrumbs related bugs.

webchick’s picture

I don't have direct communication channels with any of my three guesses... ;]

And those of us who do generally want to keep them, so we don't use these channels to draw attention to random issues in the queue. :P "patch (reviewed & tested by the community)" is the best way to attract a maintainer's attention.

We probably can use the theming API as a roundabout way to override the breadcrumbs, but it can only be a temporary solution until core is fixed: we are talking about the content about the breadcrumbs, here, not how they are themed.

Fair enough.

The patch offered at the begginning of this thread (hook_breadcrumb) is perfectly right to fix this bug. It does not introduce new feature straight away, so it can be applied in D7 and promptly backported to D6.

No... It's an API change, so wouldn't be backported. But you could use the patch on your local install if needed.

After the simpler hook_breadcrumbs has been backported, we can then use the other thread mentioned above to enhance the feature set of hook_breadcrumbs() for D7.

No... that's not how it works. When you make API changes to core, you fix it properly; you don't put in a stop-gap solution.

It looks like the following issues are needing to be solved:

  1. The ability to consistently predict when a breadcrumb is going to be set so that something (like the theme) doesn't come along later and wipe out my changes.
  2. The ability for my module to completely override the breadcrumb behaviour in Drupal core, and any other module, and be the "one true way" of establishing breadcrumb dominance. ;)
  3. The ability for my module modify the breadcrumb coming from Drupal, for example to add the current page (unlinked) to end of all breadcrumbs.
  4. The ability to add several breadcrumbs to the page, for example like Amazon.com has on product listings where you can browse by manufacturer, product category, etc. (this is http://drupal.org/node/73834)

The patch in #0 is nice because it re-uses an existing construct (module weights) to ascertain which module 'dominates' the others. It does not allow me to modify the breadcrumb from my module, without handling the entire thing myself.

Also, right now, if I var_dump($breadcrumb) in drupal_set_breadcrumb() I get:

array
  0 => string '<a href="/6x/">Home</a>' (length=23)
  1 => string '<a href="/6x/admin">Administer</a>' (length=34)
  2 => string '<a href="/6x/admin/content" title="Manage your site&#039;s content.">Content management</a>' (length=91)

This already-rendered breadcrumb does no one any good (unless they want to get into regex... ugh) and isn't nearly enough information for a module to make an intelligent decision about what it ought to do with the breadcrumb at that stage. At the very least, it seems like we need a structured array kind of like:

$breadcrumb = array(
  'main' => array(
    array(
      'path' => '',
      'title' => t('Home'),
      'module' => 'node',
      'weight' => -10,
    ),
    array(
      'path' => 'admin',
      'title' => t('Administer'),
      'module' => 'system',
      'weight' => 1,
    ),
    array(
      'path' => 'admin/content',
      'title' => t('Content'),
      'module' => 'system',
      'weight' => 2,
    ),
  ),
);

Now, my module has a lot more that it could do here. I could unset($breadcrumb); and regenerate the whole thing myself. I could simply stick my additional breadcrumb in between Home and Admin, and re-weight the stack. I could check to see if problem_breadcrumb module has set a breadcrumb, and if so, act accordingly. I could stick an additional breadcrumb item at the end. I could even add in an entirely new breadcrumb other than 'main' (such as 'taxonomy') and allow the theme system to print that somewhere else in the page.

An approach more on this order that lets modules intelligently decide when they ought to cut in and take control would get my +1. I think the initial patch is a good starter, but not flexible enough for the varied needs of contrib.

moshe weitzman’s picture

Status: Needs review » Needs work

Whatever we do here applies equally well to drupal_set_title() as well as drupal_set_breadcrumb(). The issue is is that sometimes during page rendering it is convenient to set the title because of some data you have calculated. This will have to be redone hook_breadcrumb(). One nice solution to that inelegance in storing your calculated bits like breadcrumb in context.module

This patch *always* uses the same module for breadcrumb definition which doesn't look right to me. Different pages could have different modules in charge.

pwolanin’s picture

Title: fix breadcrumbs arbitration bug » fix breadcrumbs arbitration bug with hook_breadcrumb
Status: Needs review » Needs work
FileSize
1.5 KB

here's a minimal patch that might actually be suitable for 6.x as a bug fix.

This is a minimal change that just allows a developer to be sure that their BC will have priority over one being set (for example) by a core module.

Basically - if the breadcrumb is set in the page callback, I think there is pretty much no way to override it unless you hack the theme layer. That's what I'm identifying as the bug. The change proposed should be a backward-compatible and handles arbitration by adding a weight parameter (we could call it $priority or something else if you like).

I also am not sure what's above is the right approach for enhancing this - we already are keeping the active trail as somewhat structured data. See function http://api.drupal.org/api/function/menu_set_active_trail/7 I'd rather see some tweaks to the system in D7 to make an _alter hook for this or some such if there is such a need. The place for such an _alter function might be in: http://api.drupal.org/api/function/menu_get_active_breadcrumb/7 before the active trail is rendered into the BC using l().

pwolanin’s picture

Status: Needs work » Needs review
pwolanin’s picture

Title: fix breadcrumbs arbitration bug with hook_breadcrumb » fix breadcrumbs arbitration bug
moshe weitzman’s picture

Title: fix breadcrumbs arbitration bug with hook_breadcrumb » fix breadcrumbs arbitration bug
Status: Needs work » Needs review

There is such an easy workaround for this that I don't see the point in changing D6 at this time. The objection folks have mentioned is knee jerk IMO. I have heard - "but thats the theme layer." We need a terminology upgrade here. I propose to call the templates and theme functions the "theme layer". The preprocess stage is not part of that layer. We are still manipulating structured data there, not big HTML strings. Modules are full fledged citizens in this layer as they can hook into preprocess as easily as themes or theme engines. changing breadcrumb during prepreprocess is a minor drupal sin at best.

I like pwolanin's ideas about fixing this for D7.

pwolanin’s picture

ah, good point - I don't have all the aspects of the new system in my mental model thoroughly. Since any pre-process hooks will come after the page callback in D6, that's a fine time to tweak the active trail and be pretty confident that you will have the last say. Unfortunately, looks like you'd have to waste a few cycles, since you can't run before template_preprocess_page:

see: http://drupal.org/node/223430

# moduleName_preprocess
- Do not confuse this with the preprocessor before it. This allows modules that did not originally implement the hook to influence the variables. This would run for all hooks.
# moduleName_preprocess_hook
- Same idea as the previous preprocessor but for specific hooks.

moshe weitzman’s picture

yeah, it is a bit inconvenient that you can't elegantly run before template_preprocess_page(). we should figure out how best to address it. i wanted to do this already when i needed to call drupal_add_js($data, 'setting') after the whole page was created.

JohnAlbin’s picture

Status: Needs review » Needs work

In drupal_get_breadcrumb():

if ($breadcrumb === FALSE) should be if (empty($breadcrumb)) since $breadcrumb could be an empty array and doesn't look like it will ever be boolean.

Also, in drupal_set_breadcrumb():

Doesn't $stored_breadcrumb[$weight] = $breadcrumb; mean that every module that calls it with the same weight will still clobber each other? So that data will still be lost and not available in the preprocess layer.

Why not just let the module's weight determine the breadcrumb weight and store the breadcrumbs using $stored_breadcrumb[] = $breadcrumb;; the default can be to grab the last breadcrumb in the array. And then the preprocess layer can still do what it wants with the raw data.

Sborsody’s picture

subscribe

Sborsody’s picture

Realized today that overriding theme_breadcrumb() in a theme's template.php is the last chance I have to modify the breadcrumb and set it to what I want despite what some module tries to force (*cough*Taxonomy*cough*).

Oddly enough, it turned out that despite the drupal_set_breadcrumb() calls by various modules, menu_get_active_breadcrumb() still returns the one I need from menu_breadcrumb by the time mytheme_breadcrumb() gets executed. (6.17)

sun’s picture

Status: Needs work » Closed (duplicate)

D7 provides hook_menu_breadcrumb_alter() as well as hook_menu_local_tasks_alter() already, so this is obsolete.