Posted by glu2006 on November 10, 2009 at 10:31am
| Project: | Category |
| Version: | 6.x-2.0-rc1 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Category Breadcrumb Exclude |
Issue Summary
We found that Category Breadcrumb Exclude variable doesn't work correctly. We added some content types there and we still saw them added into breadcrumb trail. This little patch fixes this issue. Please review it and consider to include into release.
Thank you.
| Attachment | Size |
|---|---|
| category_breadcrumbs.patch | 1.04 KB |
Comments
#1
Going to check later today (or maybe tomorrow). On first glance, I'm unsure what's the point of just moving the condition somewhere else, but maybe I'm missing something. Proper review coming soon :-)
#2
Ah, yes... Now I see, that this is about the additional "add node title to breadcrumb" feature, that wasn't suppressed by the exclude setting. To be honest, this was initially somehow by design (I'm the author of category_breadcrumb initial code), as I wanted to have consistent format of breadcrumbs on all pages, even the excluded types, so I really did want to add title even to these. But such a setup is quite problematic anyway, as there is a bunch of other pages (non-node) not processed by category_breadcrumb. Breadcrumb consistency on these is left to theme layer already (and my theme have the needed workaround already, making the "feature" unneeded even on my site). Really, when it comes to page titles and breadcrumbs, Drupal is somehow unsolvable :-/
So, in the end I agree with this patch, because the "feature" intended here doesn't really work anyway, and an Exclude setting is quite probably expected to stop really all activity in the specified cases. That said, the patch is correct, works as desired. I tested it (and installed on my site like all the other tested Category patches) and found no problem.
Attaching the same patch rerolled from Category package root directory, per Drupal common practice. No new changes in the code.
#3
Thanks. Committed to HEAD.
#4
Automatically closed -- issue fixed for 2 weeks with no activity.