[regression] missing menu active trail in Drupal 9.5.9

Providing a patch to revert 🐛 copyRawVariables should support default route parameters Fixed here as a short term workaround.

We have two options here

\Drupal\Core\Menu\MenuActiveTrail::getActiveLink or \Drupal\Core\Menu\MenuTreeStorage::loadByRoute can filter out default params.

Which would likely require using \Drupal\Core\Routing\RouteProvider::getRouteByName to load the route to get the defaults.

Or alternatively \Drupal\Core\Menu\MenuTreeStorage::preSave could add in defaults prior to save

I'm thinking \Drupal\Core\Menu\MenuTreeStorage::loadByRoute is probably the best place to do it

Thoughts?

Changing title per Special titles → .

Here's an initial attempt at the \Drupal\Core\Menu\MenuTreeStorage::loadByRoute handling recommended in #3.

I fiddled with tests in core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php but had trouble getting the right pieces together. Figure we can leave tests until we are more sure about approach.

The last submitted patch, 5: 3359511-5.patch, failed testing. View results →

Thanks a lot wells. The issue is also related to Menu block module because it also does not work if the Initial visibility level is set to higher than 1.

Patch works fine and solved my issue.

Thanks for testing, @roman.haluska. Be aware that some tests are still failing with the patch so you may face other regression. I haven't had the time yet to take a look at the fails.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -111,15 +119,18 @@ class MenuTreeStorage implements MenuTreeStorageInterface {
-  public function __construct(Connection $connection, CacheBackendInterface $menu_cache_backend, CacheTagsInvalidatorInterface $cache_tags_invalidator, $table, array $options = []) {
+  public function __construct(Connection $connection, CacheBackendInterface $menu_cache_backend, CacheTagsInvalidatorInterface $cache_tags_invalidator, RouteProviderInterface $route_provider, $table, array $options = []) {
     $this->connection = $connection;
     $this->menuCacheBackend = $menu_cache_backend;
     $this->cacheTagsInvalidator = $cache_tags_invalidator;
+    $this->routeProvider = $route_provider;

We will need a BC dance here, to check if the fourth argument is a string etc, someone could be sub-classing this

I'll work on this and the fails later in the week

Any chance you're using taxonomy_menu here?

The fails indicate this breaks functionality for menu links created via the core UI, but the fix is needed (in my testing) for taxonomy_menu

I think the issue exists there, as it is hard-coding the route params to taxonomy_term only, which if you have views enabled breaks routing for views

Edit, it looks like #3358402-9: [regression] route defaults are now automatically route parameters → has steps to test this with just core, I'll make a core test for that

Not using taxonomy_menu and yeah those core repro steps are in this issue description as well.

Unfortunately, at this stage, understanding many elements of this bug are beyond my capabilities, but I can confirm #3277784 caused a number of sections, on a site I manage, fail to render after upgrading to 9.5.9. Issue summary above seems to align with the problem I've encountered. Applying the reversion patch in #2 fixes all issues and I'll be using that so I can take the site to 9.5.9 for now.

Using patch #5 the missing sections of the site are rendering again, but the patch creates new problems with the site breadcrumbs. The site has both current_page_crumb and menu_breadcrumb installed and with patch #5 applied (no issues with #2 reversion applied) I'm getting duplication on the last breadcrumb entry and only on term pages. Sorry, I have not been able to isolate why.

Given upgrading from 9.5.8 (or prior) to 9.5.9 can result in parts of a page no longer rendering (causing a blocker to upgrade for some) is Normal Priority flag sufficient for this issue?

Using patch #5 the missing sections of the site are rendering again, but the patch creates new problems with the site breadcrumbs. The site has both current_page_crumb and menu_breadcrumb installed and with patch #5 applied (no issues with #2 reversion applied) I'm getting duplication on the last breadcrumb entry and only on term pages. Sorry, I have not been able to isolate why.

Thanks for sharing. This at least confirms my assumption that my attempted fix there has some issue. I'm at least going to remove that patch from display on this issue for now.

@wells, I wrote a test for this using your steps to reproduce, but it consistently passes.

So I tried the steps manually, and I can't reproduce, here's an example from Umami showing the menu link as active.

I'm using taxonomy menu and the issue comes from when you use that with views to generate the taxonomy page.

This approach should work for both taxonomy_menu and the expected behaviour in core for views.

I don't think it would be acceptable to put this into core until we can get steps to reproduce with just core.

Here's the patch to go with the interdiff above

1. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -655,17 +674,35 @@ public function loadByProperties(array $properties) {
   -    $query->condition('route_param_key', $param_key);
   +    $or = $query->orConditionGroup();
   +    $or->condition('route_param_key', [$param_key, $param_key_without_defaults], 'IN');
   +    $query->condition($or);
   

   ah I started out with an OR and ended up with an IN, this can be simplified

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -673,6 +710,7 @@ public function loadByRoute($route_name, array $route_parameters = [], $menu_nam
   +    $query->orderBy('route_param_key');
   

   In case there are multiple matches, go with the shortest one

3. +++ b/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php
   @@ -218,6 +239,14 @@ public function testBreadCrumbs() {
   +    $edit = [
   +      'title[0][value]' => 'Sub views menu',
   +      'link[0][uri]' => '/test-menu-link',
   +      'menu_parent' => "$menu:{$link->getPluginId()}",
   +    ];
   +    $this->drupalGet("admin/structure/menu/manage/{$menu}/add");
   +    $this->submitForm($edit, 'Save');
   

   This is the test case @wells mentioned in the other issue, this test passes and so does my manual testing

The last submitted patch, 17: 3359511-16.patch, failed testing. View results →

@larowlan I'm a little unclear on where/how I should be testing this. My test steps are from the 9.x branch and I can still reproduce this issue with the latest commits there. I'm not sure if this regression affects 10.x (and tangentially I'm unclear on what the 11.x branch is for).

Should I be testing from the 11.x branch? Would this not be addressed in 9.x with the upcoming EOL?

@wells 11.x is a bit like 'main' or 'master' in other branches. We've added it for now instead of a 'main' branch because there's a few things on d.o that don't support 'main' and expect versions to be named a certain way. We will eventually move to 'main' when those things are resolved.

We've made this change so that people don't have to constantly change the target of merge requests.

In terms of where you should be testing, the issue that caused the regression here went into all branches so I'd expect it exists in all three branches.

I was testing with 11.x, but I can repeat my testing on 9.5 in case it is unique to that version

Hope that helps

@larowlan thanks -- appreciate the explanations!

I'll try to get a test in with 11.x as well. Is it relevant that your new test failed (#19)?

I tested this in 9.5 and still could not replicate what you were seeing:

* Install Umami
* Add a new view, with a menu link
* Browse to that page, see the active menu trail on that link

The fail is odd

Expected breadcrumb Home on http://php-apache-jenkins-drupal-patches-185585/subdirectory/test-menu-link but found Home

🤔

The test passes locally, so I'll need to dig into it a bit, could be down to things running in a subdir on the testbot

phpunit -c app/core/ app/core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php --filter=testBreadcrumbs
⏳️ Bootstrapped tests in 0 seconds.
🐘 PHP Version 8.1.16.
💧 Drupal Version 11.0-dev.
🗄️  Database engine mysql.
PHPUnit 9.6.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\system\Functional\Menu\BreadcrumbTest
.                                                                   1 / 1 (100%)

Time: 00:26.320, Memory: 10.00 MB

OK (1 test, 145 assertions)

@larowlan could it be relevant I'm using the standard profile and not Umami?

I'll try Umami next...

I can reproduce using just the Umami demo profile and it's default menu items.

Here is a menu item with no active trail from the current 9.5.x HEAD:

And here is the same environment with 🐛 copyRawVariables should support default route parameters Fixed changes reverted:

Ah so I was looking at the styling, which both had the underline, I wasn't looking for the class.

I can take it from here - thanks

trying to fix the test first

These should be red/green

Fixed coding standards issues

Hiding old patches

The last submitted patch, 30: 3359511-fail-30.patch, failed testing. View results →

Also was able to verify on Umami install.

Patch #30 resolves the issue for me.

The last submitted patch, 30: 3359511-pass-30.patch, failed testing. View results →

Oh damn I thought this already got committed, hah.

These test fails looked valid. Anyone on this issue know what they stem from?

Hello, I think I'm facing a problem of this issue. But the patch of #30 not work for me.
So I tried to read records of menu_tree.

Data in 9.5.8 is below:

MySQL [drupal]> SELECT menu_name, route_name, route_param_key FROM menu_tree WHERE menu_name = 'breadcrumbs' LIMIT 6;
+-------------+--------------------------------+-------------------+
| menu_name   | route_name                     | route_param_key   |
+-------------+--------------------------------+-------------------+
| breadcrumbs | view.news_and_topics.page_2    | arg_0             |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=145 |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=143 |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=149 |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=147 |
| breadcrumbs | view.news_and_topics.page_2    | arg_0             |
+-------------+--------------------------------+-------------------+

Data in 9.5.9 with patch #30, after drush cache:rebuild is below:

MySQL [drupal]> SELECT menu_name, route_name, route_param_key FROM menu_tree WHERE menu_name = 'breadcrumbs' LIMIT 6;
+-------------+--------------------------------+-----------------------------------------------------------+
| menu_name   | route_name                     | route_param_key                                           |
+-------------+--------------------------------+-----------------------------------------------------------+
| breadcrumbs | view.news_and_topics.page_2    | arg_0&view_id=news_and_topics&display_id=page_2           |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=145&display_id=page_1&view_id=taxonomy_term |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=143&display_id=page_1&view_id=taxonomy_term |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=149&display_id=page_1&view_id=taxonomy_term |
| breadcrumbs | entity.taxonomy_term.canonical | taxonomy_term=147&display_id=page_1&view_id=taxonomy_term |
| breadcrumbs | view.news_and_topics.page_2    | arg_0&view_id=news_and_topics&display_id=page_2           |
+-------------+--------------------------------+-----------------------------------------------------------+
6 rows in set (0.001 sec)

I'm using this "breadcrumbs" menu with menu_breadcrumb module and breacrumbs of my site by the module are currently broken, because SQL like `WHERE route_param_key = 'taxnomy_term_145'` in the module not working properly by this problem.

I think this expanding `route_param_key` is from #3277784 🐛 copyRawVariables should support default route parameters Fixed and it may be fixed #2 patch, but I did not try yet.
I'm afraid of applying #2 patch causing any other problems or conflict with internal data structure of future version of Drupal.

Could anybody give me some advice?

because SQL like `WHERE route_param_key = 'taxnomy_term_145'` in the module

I'm sorry. The sql is executed in MenuTreeStorage::loadByRoute() called by menu_breadcrumbs through MenuLinkManager::loadLinksByRoute().

Needed to update new code in workspaces that sub-classes the menu tree storage service

Verified on Umami again and still seeing the fix.

Ran into this today with some view pages. I'm running Drupal 10.1 so 10.x is affected by this as well. Patch #38 wouldn't apply to 10.1 for me, but #30 did apply and fixed the issue.

In my case, finally I got this workaround.
Note that this includes a menu name for menu_breadcrumbs in my site, so you can not use this directly.

--- core/lib/Drupal/Core/Menu/MenuTreeStorage.php.orig	2023-07-13 16:19:51.661163432 +0900
+++ core/lib/Drupal/Core/Menu/MenuTreeStorage.php	2023-07-13 16:21:07.000000000 +0900
@@ -665,7 +665,17 @@
     $query = $this->connection->select($this->table, NULL, $this->options);
     $query->fields($this->table, $this->definitionFields());
     $query->condition('route_name', $route_name);
+    if ($route_name == 'entity.taxonomy_term.canonical' && $menu_name == 'MENU_FOR_menu_breadcrumbs') {
+      $q_group = $query->orConditionGroup()
+        ->condition('route_param_key', $param_key)
+        ->condition('route_param_key', "{$param_key}\\&%", 'LIKE');
+
+      $query->condition($q_group);
+    } else {
     $query->condition('route_param_key', $param_key);
+    }
     if ($menu_name) {
       $query->condition('menu_name', $menu_name);
     }

I was also hit by this as follows. I'm adding this comment here so that this kind of use case can be taken into account and as a courtesy to other community members who might have similar use case. I'm on 10.1 so I did not test patch #38 because as per #40, it won't apply to 10.1. I volunteer to test a new patch against 10.1 if there will be a patch that applies to 10.1

Context and use case
I have a custom module which creates competition menu links dynamically with plugin derivatives. "Competition" is a node content type specific to my site but the point here is that I'm dynamically adding competitions of the ongoing season to the menu.

My class Plugin/Menu/CompetitionMenuLink extends MenuLinkDefault and then the derivative class Plugin/Derivative/CompetitionMenuLink extends DeriverBase.

My symptom was as follows:

• I have a page view called "Finnish Championships", which is route view.competitions_current.page_1. I have this page view in my menu.
• My custom module is expected to generate the menu links as children of this page view
• The dynamically generated menu links were not generated as expected

Debugging observations

• I used menuLinkManager->loadLinksByRoute to search the parent menu link for the menu links to be created.
• I observed that menuLinkManager->loadLinksByRoute($route) did not return the parent menu link anymore like it did earlier.
• Based on the comments above, I checked menu_tree table in the database and observed that route parameters view_id and display_id had been added to route_param_key and route_params.
• Modifying the code so that I pass the route AND these route parameters to menuLinkManager->loadLinksByRoute resolved the issue for me.

Cheers,
Markus

+    // Also query without any default route parameters as they may not be
+    // present in the computed route_param_key.
+    $route_parameters_without_defaults = $route_parameters;
+    try {
+      $route = $this->routeProvider->getRouteByName($route_name);
+      foreach (array_keys($route_parameters_without_defaults) as $param) {
+        if ($route->hasDefault($param)) {
+          unset($route_parameters_without_defaults[$param]);
+        }
+      }
+    }

Shouldn't this be only removing route keys which MATCH the default? Otherwise, if you pass in a route key, which HAS a default, but is not the default, this will then match routes in the table which are for the default value, not the given value? Perhaps I'm misunderstanding the logic but it seems this might be only meant to be removing if the value is default, not if it has a default.

The last submitted patch, 38: 3359511-38.patch, failed testing. View results →

Expanding upon feedback from comment #40 above, I am experiencing this with Views pages that are listed as sub-menu items below top-level items in the main menu. In Twig menu--main.html.twig, menu.items[n].in_active_trail is not being activated for the parent menu item. Other sub-menu items that are not view pages do have the in_active_trail value set for the parent. I'm running Drupal 9.5.10 and the patch in #38 does not apply. On the other hand the patch in #30 does apply, but doesn't fix the issue. My workaround for now is to 1) convert the Views Page display to a Block display, 2) create a node with the desired path alias from the Page display, and 3) drop the View block onto the page — either via Layout Builder for the node or via Block regions in the theme with appropriate block visibility rule to restrict it to the desired path. Not an ideal solution, but it works.

Was also hit by this but my case is like described in #11 🐛 [regression] missing menu active trail in Drupal 9.5.9 Needs work and #16 🐛 [regression] missing menu active trail in Drupal 9.5.9 Needs work . I have taxonomy_menu and my term page is generated from a view. So if I understand correctly in this case the issue is in Taxonomy Menu (#3376390) 🐛 Only the first level is shown in the menu Active and not this core issue?

larowlan → credited stefan.butura → .

Crediting stefan.butura from the duplicate 🐛 Active trail not calculated correctly for Views menu items Closed: duplicate

Fully agree with #45. Have the same issue with active trail for menu items that duplicated multiple times ( they all are enabled ), some of them have 1 level in menu tree, some 2 ( all menu items located in `main` menu ).

Because of that, on node page for this menu item - I don't have second main menu that configured to show 2 level or greater.

Is there any quick solution for this (without deleting duplicates)? I've tried #30 on Drupal 9.5.1 but it's not work for me (patch applied but nothing changed).

@ever-c0de Thanks for validating my bug from #45 in #49! What I haven't been able to identify is whether this part of the bug existed before the Drupal 9.5.9 update or not, which might help identify whether this is part of the regression, or a long standing bug that deserves a separate issue.

I had do to the same thing as in comments from #42. Specifically, add in the view_id and display_id parameters.

Before:

$links = $this->menuLinkManager->loadLinksByRoute('view.courses.page_3', [], 'main');

After:

 $links = $this->menuLinkManager->loadLinksByRoute('view.courses.page_3', [
                                'view_id' => 'courses',
                                'display_id' => 'page_3',
                            ], 'main');

Concerning menu_breadcrumb → , I posted a patch 🐛 Wrong breadcrumb due to route alterations RTBC which does the trick for me.

I will provide a small update regarding #49 🐛 [regression] missing menu active trail in Drupal 9.5.9 Needs work . I fixed my own issue with active trail by decorating menu.active_trail service /Drupal/Core/Menu/MenuActiveTrail.php and changed getActiveLink method to have last link from $links variable.

This how it's looks:

      /*
       * Select the last matching link. I've done this because of duplicated
       * menu items.
       * On some block-menus we have set second depth of menu items.
       * If we have two identical menu items in one menu - it will not appear
       * because first link is selected (with lower(0) depth).
       */
      if ($links) {
        $found = end($links);
      }
    }
    return $found;

Basically, i've just changed $found = reset($links); from parent to $found = end($links); in my case. So, we receiving sorted by depth, weight and ID $links array from $this->menuLinkManager->loadLinksByRoute and in case we will have more than one link - the link with more depth will be active.

Patch helps me on 10.1.5 where in_active_trail was not getting called in my twig template.
Thanks!

I have same or similar problem when adding a view to the menu via the Views UI where in_active_trail does not appear to be showing true.

Adding via Menu UI works fine.

Steps to reproduce:

1. Set up a view
2. Add it to the main menu via the Views UI
3. On front end active class should be added to a href and list item
4. When walking through the code, notice how in_active_trail is set to false even it is currently in_active_trail

Screenshots here https://www.drupal.org/project/drupal/issues/3404791 🐛 Views not adding in_active_trail to menu item when adding via Views UI Closed: duplicate

For what it's worth, the patch in #38 applies with a few small offsets to Drupal 10.1.7 and fixes the problem of active-trail not being applied to menus for View pages.

#38 works on 10.2.2 with a taxonomy menu for the first set of terms, but does not work for nested terms.

Would be great to have this bug fix in next minor or patched version of Drupal.

#38 works for me.

@2dareis2do the patch here doesn't apply and there is no MR, so what is RTBC?

@catch sorry my bad. I thought I had the patch applied locally but I don't think I do.

#38 sounds like it should work though if testing with 10.1

@catch, I am not sure why the latest patch is not applying.

Works fine for me locally.

Created merge requests for 11.x and 10.2.x from patch at #38.

Setting back to RTBC since there were no changes and the CI tests passing.

I'm not sure what the process is for removing BC changes (e.g. the route provider check in MenuTreeStorage and test) but that check is still in the 11.x MR.

📌 Restrict access to empty top level administration pages for overview controller Fixed adds an @todo item to remove some fallback code once this issue is committed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

switching back to RTBC as this patch has been confirmed as working. Problem with CI?

As #72 and the issue commited first 📌 Restrict access to empty top level administration pages for overview controller Fixed

@andypost, please can you explain why you have changed this to needs work?

If I understand correctly, that change is dependent on this one being committed!

Sorry in advance for the "noise" but can someone give a a summary of where things stand with this REGRESSION? I feel like it is not getting the proper attention from core maintainers that it should? Or am I wrong in that impression.

I see two MRs. One is marked for Drupal 10 and the other, I assume, is for Drupal 11? I suppose work has stopped completely on 9.5.x even though this regression was introduced in 9.5.9?

Also, I think there is still problem with menu items that reference a view that has a contextual filter. I recall others discussion that, but I don't know if it was in this issue or another.

@JonMcL, thanks for asking about this issue. There are thousands of open core bugs, and thousands of potential core contributors -- including you. 🙂 We fix thousands of issues a year, but there are still many more out them. What gets fixed and when depends on contributors stepping up to complete the work needed to fi the issue.

Drupal 9 is end of life and insecure, so it does not receive any further commits. You should upgrade to Drupal 10 as soon as possible to avoid security vulnerabilities with your site.

All merge requests should be created against 11.x (which is our main development branch). We should close other merge requests that don't target 11.x. If and when this issue has a fix that is ready for commit, it will be committed to 11.x first, and then backported to the actively supported branches of core according to our backport policy. In the case of this particular issue, the fix appears to be complex and to require internal API changes, so it might be committed to minors only (which would mean it would be available in 11.1.0 and 10.4.0 if we manage to fix this in the next five months).

Some sites use composer-patches to run interim/partial fixes until a proper fix is available in a stable release.

Thanks!

Can anyone explain why this has been set to needs work.

Patch here works for me without issue.

Can anyone explain why this has been set to needs work?

It is needs work for comment #72.

The full #72 comment is:

📌 Restrict access to empty top level administration pages for overview controller Fixed adds an @todo item to remove some fallback code once this issue is committed.

I’m reading this as a task to be completed after “this issue is committed” (emphasis mine). How can the status of this issue be “needs work” when that work is meant to be done after the issue is committed? What actually needs to be done here?

Ah ok looking at the actual commit from 📌 Restrict access to empty top level administration pages for overview controller Fixed it seems the work to be done here is to update the patch here to also remove this if block: https://git.drupalcode.org/project/drupal/-/commit/02ca3176ead562c9e6175...

Sorry, the wording of #72 was very confusing.

Also proactively tagging for IS update, since we're missing a proposed resolution. Would be good to update the IS to the latest state before setting to needs review again as well, otherwise the issue will surely fail the review for that.

Might just be tired but I'm failing to understand what is going on/what the request is for the @todo referencing this issue in MenuAccessTest::testSystemAdminMenuBlockAccessCheck(), could someone take a look?

@quietone, @wells I was also confused by #72 but removed the fallback code and rebased both MRs.

Cool. Doesn't apply to 10.3 now. Love these MR patches.