Core is inconsistent when handling two menu items with the same path. It says one thing in the UI and does another in reality.

There is a good use-case for having two menus point to the same path : having a menu and one of it's children point to the same page, because you want the main section to have a default sub-section.

Ideally menu_link_get_preferred() should not just sort on the mlid, but also on the depth on the item so that sub-menu items get preferred over their parents. This would fix this issue in http://drupal.org/node/1783700

Marked #1472354: Node menu link is inconsistent with active trail when multiple items point to the same node as a duplicate (although that issue was actually older, the patch here was posted first and is already against Drupal 8 whereas that one wasn't).

Drupal 7 patch to the same problem

#1: menu-handle_duplicates-1649062-01.patch queued for re-testing.

Would there be an argument for exposing the sorting technique with a hook? That way modules and themes can modify the sorting as they require.

Any news on that ?
I noticed that the last valid patch wasn't working anymore. So i RE-wrap it. I had to change it a bit but the idea is the same.
Tested on Drupal 8. I'm actually more interested in a D7 patch but i guess we have to validate this one first.

menu-handle_duplicates-1649062-10.patch queued for re-testing

I am having a similar issue, however, the interesting thing is that we've built a custom module that uses node grants to allow admins to limit *view* access on a per-node basis. Note this is all on D7.

The client is now building a menu which looks like this:

Node/1 (only anon. users)
-- FAQ (everyone has access)

Node/2 (only verif. users)
-- FAQ (everyone has access)

Now when I'm not logged in, and click on the FAQ link, things work as expected.

HOWEVER when I AM logged in, and click on FAQ, my entire menu will disappear! This is because of the fact I have *no* access on Node/1, but the FAQ link beneath it, is being triggered regardless of that fact. Because I have no access to node/1, that link and the entire tree beneath it are hidden from the users' view.

This is a patch for D7, rolled from #10 above. Worked for me.

https://drupal.org/files/issues/menu-handle_duplicates-1649062-13.patch

The patch for D7 in comment #13 above seems to be missing. :-( I wrote another that will hopefully stick around for those that need it. (It is based off of the patch in #10, but written for D7).

I think the simple test failed my patch because it is written for D7 and this issue technically marked for D8. I do think the patch is still valid though. Is there something I should have done differently when posting this backport?

If the bug exists in D8, it needs to be fixed there first. If the issue is set to 8.x, a 7.x patch will always fail to apply.

Lets postpone for 8 on #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, since that may fix the problem in 8 anyhow.

Makes sense, thanks! Question... The backport I wrote is helpful for those on D7 so should I update this issue to D7 and then rerun the simple test? (So others can try it out and report issues they find) Or, do I create a new issue and attach the patch there? Or do I just leave it alone and use my own patch internally for now? :-) Sorry I'm not sure of the correct procedure.

The logic in your patch is assuming the link with the greater mlid wins. Couldn't that be flawed?

To answer your question, I would create a new ticket under D7 core and add this patch. OR create a patch based on D8 and resubmit.

I've run into a very similar issue when developing a site for a client, and have noticed several issues targeted at Drupal core that have the same general idea:

• #1808280: Many menu items for one node results in incorrect menu trail
• #1875824: Active trail should favor "non-customized" menu items
• #306412: Get the deepest menu entry when more than one entry points to the same content

There are probably more, but that's as far as I have looked.

I realize we have already decided to postpone this issue in the hopes that it will be fixed in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, but I need a Drupal 7 solution for this issue pretty quickly.

I have created (and attached) two patches. One of the patches is for Drupal 8, and the other is for Drupal 7 (of course, the Drupal 7 issue will probably fail testing because the issue is assigned to Drupal 8).

For my patches, I decided to sort menu links based on their customized status (so that non-customized links will appear first, based on #1875824: Active trail should favor "non-customized" menu items) and based on depth (so that the deepest, and therefore most specific, links will appear first, based on #306412: Get the deepest menu entry when more than one entry points to the same content).

I agree with graysadler in that sorting based on the MLID isn't really the best approach. The database already sorts on the primary key, so we're not really changing anything. We need some concrete fields to sort on so that we get the same results every time the function is called. If we sort based on primary keys, we run into the issue we're having now where the sorting depends on the order in which the menu links were created.

For the Drupal 7 patch, all I did was modify the PDO query that was being called, adding the sort options.

The Drupal 8 patch was a little more complicated, because they had already swapped the query for entity_load_multiple_by_properties. I looked at the source of that function and decided to modify its calls to the Entity Controller for the menu links. Because I don't have a local environment for Drupal 8 setup and I was primarily concerned with creating a patch for Drupal 7, I have not tested the Drupal 8 patch.

Because I have proposed a new solution in patches, I have marked the issue as "Needs Review" again. Maybe the Drupal 8 solution will be obsolete in the future, but as of right now this is still a problem in Drupal 8. It is certainly still a problem in Drupal 7.

I'm also upping the priority of the issue to Major, as this is a huge oversight in the menu system and produces unexpected results most of the time. This issue most likely affects several third-party modules that rely on the menu system for proper use (Menu Block, for example).

On a side note, perhaps the Backport Policy (or maybe the issue queue) is a little flawed in that we want to have accurate tests for the different versions of Drupal the patches are geared toward (in reference to my Drupal 7 patch inevitably failing). Maybe a flag in the name of the file or something to determine which version of the project to test the patch against? Just something to think about.

The code is very different after #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module and I'm not sure that I see how this bug report will be relevant. Among other things, the breadcrumb is based purley on path, not the menu link (even now in 8.x).

You should probably just make this a 7.x bug assuming we get the 8.x change in #2256521 commited.

Updating the issue to be Drupal 7 specific, and hiding some other patches that no longer apply or contain the approach that uses menu link IDs for sorting.

I guess this is one of those exceptions to the backport policy? Or should we reference this issue as a Drupal 7 backport for one of the features introduced in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module?

I'll re-test my patch for Drupal 7 above so that it's marked as passing. Should we mark the issues I mentioned above as duplicates of this one?

I've tried sammarks15's patch #21 for D7 and as far as I can tell it seems to work as I would expect. The whole menu system/breadcrumb trail can easily get confusing so it's a tricky thing to test, but from what I can see it is doing the correct thing.

I would like a little clarification/confirmation as to what is considered a "customized" menu item so I can be sure my test is accurate. To me, any menu selection made on the Node Edit page should be the ruling parent menu item. Any menu items you've entered manually via the "Structure -> Menus -> Add link" type functionality would be secondary as they are normally cross references or shortcuts to the real node. From what I understand (based on the other issues linked) is a "customized" menu item is that 2nd option, the manually entered option. Is that correct?

I am planning to continue to test patch #21 in my dev install instead of the patch I wrote in #14 as it seems more predictable. (Sorry for re-queuing my patch, I was actually just curious if it would pass now)

According to this Drupal 8 page (I imagine menu_link_schema is very similar for Drupal 7, but this was the first result), the customized flag on a menu link behaves exactly as you have described. My understanding of it is that any menu links created in-code or by a module will be marked as not customized, and any menu links created through the administration interface will.

This also applies to overriding menu links created in code. So for example, if there's a menu link defined by a module and you change the title of it, I believe it is marked as customized. That's just me thinking off the top of my head, I don't have the time to setup a sandbox instance and confirm that for myself at the moment.

Which is why we should probably update the administration pages to use the same logic as menu_link_get_preferred. The first step was to update menu_link_get_preferred to actually have some logic (before, there wasn't any sorting so it was sorting based on MLID, which is bad). The second step is to update the administration side to use the same sorting.

Sorting based on MLID isn't a very good idea because the sorting of the menu links will be dependent on the order in which the menu links were created. If we sort based on concrete values (like customized status and depth), the links will be in the same order no matter the order they were created in. It results in a much more predictable result from a programming standpoint.

I agree with your statement about the order of the customized status, but isn't that just something where we need to set a standard and go with it? Do we introduce an alter hook to change the results of menu_link_get_preferred? I'm not exactly sure what kind of performance impact introducing another hook has.

I understand your point about changing the behavior of existing Drupal 7 sites, and did not think about that when creating my patch.

I agree, sorting on the MLID isn't the best possible way to do it, but I imagine we would be doing more damage by changing something as significant as how the menus are ordered this late in the development of D7. Perhaps this is something we should think about for Drupal 8?

I still say having a hook in menu_link_get_preferred would be a good idea so that we can set the standard (sorting by MLID), and then let sites change that standard where they need to. For example, sorting by MLID isn't working in the site I'm currently working on, so I would need to implement the hook to alter the behavior a little. Again, I see no harm in adding a hook, but am unsure about the performance drawbacks of it.

At this point, would it be better to update the administration interface code to match menu_link_get_preferred, or update menu_link_get_preferred to match the administration code? My fear with updating the administration code is that there are potentially several places to think about (the node edit screen is probably not the only page where you have an option like that, and then we have to think about third-party modules as well). And my fear with updating menu_link_get_preferred is exactly what you said: we would be altering a front-facing component of Core that might change the way some menus are rendered on production sites.

Not sure if this is worth mentioning here or not, but I've found that this module:
https://www.drupal.org/project/menu_breadcrumb is helpful in terms of nodes being in different menus and showing different parents in the breadcrumb. (If anyone reading this thread is interested in that part of things)

In terms of this patch, based on the recent discussions on this post, I ~think~ I'm going to be using #14 for my D7 site (unless there is a better mlid sorting patch for d7 I missed?). I like how #21 behaves, it's what I would expect, but I can understand the argument from both sides and ultimately I guess repeating the sorting by mlid, while not perfect, is at least mimicking existing "bad" behavior in a consistent way.

With my move to Drupal one of the biggest struggles has been the fact that Drupal (aside from menus and taxonomy) really doesn't have any hierarchy. There is no real concept of a node having only 1 real, true, parent that is standardized. It gives me issues from menus, breadcrumbs, to URL patterns. The custom CMS I came from thrived on hierarchy and it's a big switch for me.

I am thankful to see people like you guys on this post discussing and working out the issues together though. It's appreciated!

I still say having a hook in menu_link_get_preferred would be a good idea so that we can set the standard (sorting by MLID), and then let sites change that standard where they need to.

Instead of a hook, #1854134: Add a query tag to the query in menu_link_get_preferred() to allow modules to alter the query might achieve the same goal?

So should we mark this one as a duplicate of #1854134: Add a query tag to the query in menu_link_get_preferred() to allow modules to alter the query? I agree, adding a query tag would be a good solution.

Closed #1808280: Many menu items for one node results in incorrect menu trail and #2205503: Breadcrumbs are not shown correctly when following the menu trail. as duplicates.

#1854134: Add a query tag to the query in menu_link_get_preferred() to allow modules to alter the query has gone into core, so now if you want to do something like #21, you can probably just do this:

MODULE_query_preferred_menu_links_alter(QueryAlterableInterface $query) {
  $query->orderBy('customized', 'DESC');
  $query->orderBy('depth', 'ASC');
}

Marking this as closed, as the query tag introduced by #1854134: Add a query tag to the query in menu_link_get_preferred() to allow modules to alter the query allows us to fix this issue (refer to the example above for how to do it).

I've added a module to use the query alter like in #38, but the code block never seems to run as it's wrapped in this IF statement:

if (!isset($preferred_links[$path])) {

Also, is using the customized or depth attributes really that reliable? Ideally i want the trail to be built using the menu item i've created from the node itself. Any other menu items created manually at structure -> menu can be ignored when creating the trail.

In reference to #29 and those who want the FIRST menu link for a page created to be the preferred path I am using:

function hook_query_preferred_menu_links_alter($query) {
  $query->orderBy('ml.mlid', 'DESC');
}

This patch doesn't seem to help when your original menu link is the parent item and the duplicate menu link is a child item, like this:

About Us (node/19) <-- Original/first menu item
-- Overview (node/19) <-- Duplicate
-- Contact (node/20)

Even with the hook_alter and order change to the SQL query, the 'Overview' link is being active-trailed instead of the original 'About Us' link when I view other children like the 'Contact Us' page.

sammarks15 I could kiss you for #21. I am the developer and content manager for a regional healthcare system. The menu tree is ridiculous for all of our service lines, patient info, provider info, recruitment, HR, events, etc. Lots of helper links. Having to make all of the menu items in reverse order was just crazy and not working at all. Want to add another helper link to another menu? Start all over. Gah! #21 is like magic to me. Set the preferred menu in the edit form. Add the helper links manually in the menu admin. Makes sense for our use case for sure. Such a huge help. Buy yourself a beer and send me the bill! Seriously thank you. You've saved me hours and days of work. Thank you.

Shouldn't we reopen this, since the solution proposed is not really sufficient?

The solution is: "there's a tag on the query so do what you want"...
Shouldn't there be a solution which fits all cases? The proposed altering orders by weight, but that won't work all the time. Setting your tree weights correctly to make this work can get really complicated.
I would suggest something more configurable for a suite builder, in other words some UI. I don't know what the "ideal“ solution should be, but perhaps a checkbox to indicate which menu item is the actual preferred item would suffice. I could write a contrib module to add that checkbox of course, but it's the core that should support multiple items, since it also allows them.

So please, maintainers of the Drupal core: reopen this issue, and thus the discussion.

I agree with #45 that this should be re-opened. I could have two items that point to the same node and where both are "customized" and have the same depth. Then, sorting by mlid may not always work consistently. In one set of duplicate links, mlid ASC might produce the right results while in another set, mlid DESC might instead produce the right results. It just depends on the order in which menu links were created in both cases. And both situations can exist on a site.

Something should be made policy that matches with the UI.

Does anyone know if this patch is available for D8? I was really hoping this was fixed somehow but no it just reared it's ugly head yet again. Thanks!