Active trail should favor "non-customized" menu items [#1875824]

When multiple menu items point to the same page, the active trail is often not the intended one. This happens a lot. It is very difficult that a breadcrumb and a URL are singular but a menu item can be multiple.

The trouble comes in menu_link_get_preferred. Various solutions have been presented such as preferring the item that is most deeply nested #306412: Get the deepest menu entry when more than one entry points to the same content or by having multiple active trails #609542: Active trail isn't set on all menu items pointing to the current path.

One simple improvement I think we should make is to, for a node, favor the per-node menu settings over the 'customized' menu items made via menu administration. Because only one menu item can be made on a node form, treating this item as canonical relative to active trail (like we treat its path alias setting there as canonical) makes a lot of sense and is easy for editors to understand.

What we currently have is no real ordering of which menu item to prefer in an active path, so adding a bit of ordering that makes sense to users seems like a big step forward. And also a one line patch. Creds to Timmy P.

Comment	File	Size	Author
#13	menu-active-trail-consistency-option1-1875824-13.patch	1.15 KB	khaled.zaidan
#13	8.2.x: PHP 5.5 & MySQL 5.5 21,175 pass, 23 fail
#10	menu-active-trail-consistency-option2-1875824-10.patch	989 bytes	khaled.zaidan
#10	8.2.x: PHP 5.5 & MySQL 5.5 21,178 pass
#10	menu-active-trail-consistency-option1-1875824-10.patch	1.15 KB	khaled.zaidan
#10	8.2.x: PHP 5.5 & MySQL 5.5 21,163 pass, 23 fail
#3	core-1875824-3-d7.patch	535 bytes	barancekk
#3
	menu-order-customized.patch	555 bytes	Jody Lynn

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

28 December 2012 at 21:36

Status:

Needs review

» Needs work

The last submitted patch, menu-order-customized.patch, failed testing.

Comment #2

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 28 December 2012 at 22:39

Status:

Needs work

» Needs review

menu-order-customized.patch queued for re-testing.

Comment #3

barancekk CreditAttribution: barancekk commented 16 May 2013 at 19:52

File	Size
core-1875824-3-d7.patch	535 bytes

I edited the patch to make it work on 7.22 version of drupal. Attaching in this comment.

Thank you

Comment #4

16 May 2013 at 19:54

Status:

Needs review

» Needs work

The last submitted patch, core-1875824-3-d7.patch, failed testing.

Comment #5

stefan.r CreditAttribution: stefan.r commented 8 January 2015 at 13:01

Issue summary:	View changes
Status:	Needs work	» Closed (duplicate)

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

Comment #6

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 10 January 2015 at 23:15

Status:

Closed (duplicate)

» Needs work

Can you explain how this is a duplicate? That's a 7.x issue.

Comment #7

stefan.r CreditAttribution: stefan.r commented 11 January 2015 at 00:55

Sorry, I had missed that this was a D8 issue with the D7 patch and all, the other issue contained a patch which did essentially the same.

How would this be implemented in D8? "customized" seems to have been replaced by MenuLinkInterface::isResettable(), but that doesn't mean the same thing as "customized" any more.

Comment #8

11 January 2015 at 00:55

Version:

8.0.x-dev

» 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #9

11 January 2015 at 00:55

Version:

8.1.x-dev

» 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #10

khaled.zaidan CreditAttribution: khaled.zaidan at Reading Room commented 2 December 2016 at 15:36

File	Size
menu-active-trail-consistency-option1-1875824-10.patch	1.15 KB
8.2.x: PHP 5.5 & MySQL 5.5 21,163 pass, 23 fail
menu-active-trail-consistency-option2-1875824-10.patch	989 bytes
8.2.x: PHP 5.5 & MySQL 5.5 21,178 pass

Looking at this issue in D8.

I've found a possible fix attached in the patch (actually 2 potential fixes, hence the 2 patches).

The issue happens when the MenuActiveTrail service decides what the active trail is. This happens in Drupal\Core\Menu\MenuActiveTrail::getActiveLink(). We can see in that function, loadLinksByRoute() returns all the links the match the current route, and then simply the first one of the links is chosen as the active trail.

if ($route_name) {
  $route_parameters = $this->routeMatch->getRawParameters()->all();

  // Load links matching this route.
  $links = $this->menuLinkManager->loadLinksByRoute($route_name, $route_parameters, $menu_name);
  // Select the first matching link.
  if ($links) {
    $found = reset($links);
  }
}
return $found;

So we can either:
1) Instead of simply picking the first link here, we do something smarter to pick the one that appears in the node form (I'll get to how in a below).
Or
2) Make loadLinksByRoute() give preference to the link that appears on the node form.

So I looked at how the link that appears on the node form is chosen. And found that it's chosen in menu_ui_get_menu_link_defaults($node), which sorts links by mlid (or id in the menu_link_content table).

So we can mimic that behaviour by either choosing the link with lowest mlid in Drupal\Core\Menu\MenuActiveTrail::getActiveLink() or by sorting ascendingly by mlid in loadLinksByRoute().

I looked through the code for uses of loadLinksByRoute(), and didn't find any functionality (in core at least) using this other than the active trail. And there doesn't seem to be any reason for the specific order links currently come back in. So I see no harm in changing it, and for the sake of consistency across any potential related functionalities, I think going with this option (option 2) makes more sense.

Anyway, I hope someone could have a look at these patches, and hopefully we see this issue fixed soon.

PS. Attached patches are against 8.2.x-dev.

Comment #11

gmclelland CreditAttribution: gmclelland commented 2 December 2016 at 15:39

Status:

Needs work

» Needs review

Patched attached. Setting to NR

Comment #12

2 December 2016 at 16:22

The last submitted patch, 10: menu-active-trail-consistency-option1-1875824-10.patch, failed testing.

Comment #13

khaled.zaidan CreditAttribution: khaled.zaidan at Reading Room commented 12 December 2016 at 16:34

File	Size
menu-active-trail-consistency-option1-1875824-13.patch	1.15 KB
8.2.x: PHP 5.5 & MySQL 5.5 21,175 pass, 23 fail

Oh, I'm feeling silly now, I had a bit of bad code in the patch for option 1 here:

$link_mlid = $link_mlid;

Attached is a modified version.

12 December 2016 at 17:16

Version:

8.6.x-dev

» 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #20

12 December 2016 at 17:16

Version:

8.8.x-dev

» 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Comment #21

12 December 2016 at 17:16

Version:

8.9.x-dev

» 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

12 December 2016 at 17:16

Version:

9.5.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.