Page title is always included in the breadcrumb for node not processed by menutrails

I can confirm that this bug exists and is correctly fixed by the patch.

As described, currently, menutrails currently sets a breadcrumb for all node pages. This is problematic both because it may wipe out other custom breadcrumb setting and because it is buggy, including the current node's title in the breadcrumb.

The proposed fix looks correct. There doesn't appear to be a valid use case for setting breadcrumbs on nodes not handled by menutrails, so returning FALSE in these cases is correct.

I've made two very minor tweaks, to punctuation in a comment and wording in a form element description.

Could you try this patch instead?

#460476: Enabling "set breadcrumbs" breaks breadcrumb on pages outside of Menu Trails Menu has been marked as duplicate of this issue.

The patch in #2 works for node pages, but the breadcrumb in my views page is still messed up: the title of the view is included as a link in the breadcrumb. Not sure if that should be handled in a separate issue.

Patch in #2 including SQL cleanup indeed works as expected.

Moved Views issue to #767558: Remove views breadcrumb handling from Menutrails.

See also #560482: Consider merging with menu_breadcrumb. If we accept that we shouldn't be trying to implement full handling of all breadcrumb setting for menu items, and instead leave that task to Menu breadcrumb, then this is ready to go in.

Revised this some more, fixed a couple of PHP notices, and added comments.

However, although I think this patch "should work", I'm not 100% comfortable with just committing this patch. This actually cries for unit tests, as there are gazillions of different configurations, conditions, and scenarios.

Fixed comments.

Yeah, it's a bit hard to decide the best path here. There's a lot of cleanup that's needed, and some of it is related to this issue, though maybe not directly required to fix the reported bug.

The bigger issue is, should we be changing breadcrumbs in any case other than "this is a node managed by menutrails"?

If we're not ready to decide on that, we could just fix the issue without refactoring by simply not adding the current page to the breadcrumb. Patch attached. Should fix the reported bug for both nodes and views, but doesn't touch either the larger question or the need for cleanup.

Including the duplicate issue, we have 3 patches with 3 entirely different fixes for the same bug. That very likely means that at least 2 of them are breaking the module's functionality... isn't it always that way?

Yeah ;)

Here's an attempt at a brief recap.

Problem:

Breadcrumbs generated for a menu item include the menu item itself. This is by design for the original purpose of menutrails: generating a node whose type or taxonomy term has a designated parent menu item. In this original use case, we indeed want that immediate parent to appear in the breadcrumb. Example: node type 'story' is assigned a parent menu item of "Stories", which is a Views page display with the path "stories". Breadcrumb is, correctly, Home » Stories.

But in two cases - (a) nodes not managed by menutrails and (b) views page displays - we're now also generating breadcrumbs, based not on a designated parent menu item but on the menu item for the node or view itself. In these cases, the last breadcrumb link is the menu item itself, and so is superfluous. Example: for the above Views page display, the breadcrumb will also be Home » Stories.

Solution:

There are two potential solutions.

The first - represented by the first patch in this issue and also the patches in #460476: Enabling "set breadcrumbs" breaks breadcrumb on pages outside of Menu Trails Menu and #767558: Remove views breadcrumb handling from Menutrails - is to say, let's stop setting menu trails when the current node or view page has a menu item. Problem solved--the breadcrumb isn't broken because we no longer set it.

The second solution is, let's keep setting these breadcrumbs, but fix the error of including the current page in the breadcrumb, so the breadcrumb we set isn't broken. This is what's done in the patch in #8.

Which is better? In the long term, I think the first solution is. Setting breadcrumbs based on the current path having a menu item is complex and is exactly what the menu_breadcrumb module does. There's a lot that would need to be added to menutrails before it could fully fill this niche--e.g., it would need to support many other object types like panels pages that can also have menu items.

But in the short term the patch in #8 is probably a relatively harmless fix. If we're not going to roll back the views page display breadcrumb setting, the patch in #8 is needed.

Proposed path forward: test the patch in #8. If confirmed, apply, maybe adding a comment that the if may be removed if we roll back node and view page display menu item setting. We get an interim fix. Then set this back to needs work and consider removing node menu item setting, leaving the views equivalent to #767558: Remove views breadcrumb handling from Menutrails.

sun, I'll look for you on IRC when I get a chance to work this through if you have time.

Thanks for that extensive summary. I'd already deinstalled Menu Trails when I found this issue which enabled me to work further.

I had consistency problems between
- pages which are menu items
- pages with content types whose parent menu is set by menutrails -- and even those content types had inconsistent behaviour among themselves. (Depending on whether that parent menu was a view or not, I guess?)

Patch #8 fixed them all. (And I understand the patch; it does stuff as advertised
So I'm setting RTBC again, for patch in #8, on the basis that
- this is waiting a while already
- the module with patch #8 is IMHO definitely better than without.

Can be set back to 'needs work' after applying #8

#8 is good for views, pages withouth menutrails handling, and submenu pages. solved my problems

Patch #8 is awesome! Works for me as well.

Thanks for reporting, reviewing, and testing! Committed to 6.x.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.