Hello,

Since some projects, I heard some of my colleagues complaining about the difficulty to alter links displayed in the breadcrumb.

It comes from the fact the breadcrumb is built through the "menu_get_active_breadcrumb()" function that retrieves the breadcrumb and returns directly them as hyperlinks in the array that will be consumed as variable in the "theme_breadcrumb()" call of "template_process_page()".

In order to add an attribute a breadcrumb item, that obliges us either to parsing the string variable in order to inject it in the HTML or to regenerate each breadcrumb item in order to add it before render them as link.

In order to avoid this kind cumbersome process, IMO, "menu_get_active_breadcrumb()" should returns an array of keyed arrays. Each keyed array would contain data allowing creating the hyperlink displayed in the breadcrumb and could be "preprocessed" by any modules or themes if necessary.

It is what I propose in the attached patch.

In it, I also keep the old implementation in order to ensure the backward compatibility.

CommentFileSizeAuthor
#11 change_breadcrumb_strings_into_array-2863108-11.patch6.94 KBjyraya
#7 change_breadcrumb_strings_into_array-2863108-7.patch6.04 KBjyraya
#2 change_breadcrumb_strings_into_array-2863108-1.patch5.85 KBjyraya

Comments

jyraya created an issue. See original summary.

jyraya’s picture

jyraya commented
Issue summary: View changes
StatusFileSize
new change_breadcrumb_strings_into_array-2863108-1.patch5.85 KB
jyraya’s picture

jyraya commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: change_breadcrumb_strings_into_array-2863108-1.patch, failed testing.

jyraya’s picture

jyraya commented

Ok. I see the problem I will try to fix ASAP.

jyraya’s picture

jyraya commented
Assigned: Unassigned » jyraya
jyraya’s picture

jyraya commented
StatusFileSize
new change_breadcrumb_strings_into_array-2863108-7.patch6.04 KB

Ok.

I change the backward compatibility strategy in the patch. All manual and automated tests are green.

Let's try this new patch.

jyraya’s picture

jyraya commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: change_breadcrumb_strings_into_array-2863108-7.patch, failed testing.

pol’s picture

pol
Location Brussels
commented

Hi,

Very good initiative, this is what I do in a very similar way in Atomium, without keeping the backward compatibility.

One thing to say, once this is done, you could move the generation of the breadcrumb from the process to the preprocess then.

Good luck!

jyraya’s picture

jyraya commented
Status: Needs work » Needs review
StatusFileSize
new change_breadcrumb_strings_into_array-2863108-11.patch6.94 KB

Let's try again . I replace the overriding of the theme_breadcrump in garland by a hook_preprocess_breadcrumb().

That let me think about the backward compatibility for others custom or contrib themes that override the "themeBreadcrumb" function. Someone knows how to deal with this kind of situation?
Should we:

  • Communicate that they have to adapt their implementation like I did for the "Garland" theme?
  • Put something to insure that their code still work?

Any opinions are welcomed!

jyraya’s picture

jyraya commented

Ok. tests failing with PHP 7 but nothing related to the patch.

Concerning the your proposal Pol, I need to investigate this part because this comment in the template_process_page explains why it is in the process instead of the preprocess:

    // Build the breadcrumb last, so as to increase the chance of being able to
    // re-use the cache of an already rendered menu containing the active link
    // for the current page.
    // @see menu_tree_page_data()
jyraya’s picture

jyraya commented
Assigned: jyraya » Unassigned
pol’s picture

pol
Location Brussels
commented
Status: Needs review » Reviewed & tested by the community

+1 for using this patch.

I'm already doing something similar in the Atomium theme.

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +Drupal 7.60 target
David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Drupal 7.60 target
  1. I looked this over and I'm concerned about backwards compatibility, not just of theme_breadcrumb() implementations but also for code that calls drupal_get_breadcrumb(). We are changing the data structures here that all that code would expect to receive.
  2. I'm also not sure how much of this is really necessary - doesn't hook_menu_breadcrumb_alter() already let you alter these links before they are generated? It lets you alter anything returned by menu_get_active_breadcrumb() (the use case mentioned in the issue summary above). I suppose it doesn't let you alter anything set by drupal_set_breadcrumb(), so if that's the goal (to allow callers of drupal_set_breadcrumb() to pass-in non-rendered links and then provide an opportunity for those to be altered) I think we could consider a patch that just handled that, which should be possible to do in a backwards-compatible way. 
  3.      'breadcrumb' => array(
-      'variables' => array('breadcrumb' => NULL),
+      'variables' => array('breadcrumb' => NULL, 'separator' => ' » '),

    This "separator" stuff is interesting but I don't see what it has to do with the rest of the patch. 

  4. -    $output .= '<div class="breadcrumb">' . implode(' » ', $breadcrumb) . '</div>';
...
+        $items[] = filter_xss($breadcrumb_item);

    Why is a new filter_xss() call being introduced here?

  5. Does any of this need to be added to Drupal 8 also?

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.