Class active-trail not added to li element when linking to front page

The following workaround code was adapted from #64. It corrects the problem of flagging all routes of the same name by also comparing the route parameters.

/**
 * Prepares variables for block__system_menu_block templates.
 *
 * Work around an issue where front page menu links are not set in the
 * active trail.
 *
 * Default template: block.html.twig.
 *
 * @see https://www.drupal.org/project/drupal/issues/1578832
 */
function my_theme_preprocess_block__system_menu_block(&$variables) {
  if (!\Drupal::service('path.matcher')->isFrontPage()) {
    return;
  }

  $homepage_path = \Drupal::config('system.site')->get('page.front');
  $homepage_url = \Drupal::service('path.validator')->getUrlIfValid($homepage_path);
  if (!$homepage_url instanceof Url) {
    return;
  }

  foreach ($variables['content']['#items'] as $key => $item) {
    /** @var \Drupal\Core\Url $url */
    $url = $item['url'];
    if (!($url instanceof Url) || !$url->isRouted()) {
      continue;
    }

    $is_front = $url->getRouteName() == '<front>';
    $is_configured_route = $url->getRouteName() == $homepage_url->getRouteName() &&
      $url->getRouteParameters() == $homepage_url->getRouteParameters();
    if ($is_front || $is_configured_route) {
      $variables['content']['#items'][$key]['in_active_trail'] = TRUE;
    }
  }
}

dcam → changed the visibility of the branch 11.x to hidden.

I disagree with the approach in the previous MR that made MenuTreeStorage::loadByRoute() load front page routes. It violates the single responsibility principle and will result in unexpected behavior for calling code.

My own approach is to add a check for the front page to the menu.active_trail service's getActiveLink() function. That function is responsible for determining whether a menu is in the trail or not. <front> is effectively an alias for the front page route. So if anything needs to compensate for these non-standard <front> links, that seems like the most appropriate function to do it.

That said, I was a little confused at first because the docblock for MenuActiveTrailInterface::getActiveLink() is misleading:

  /**
   * Fetches a menu link which matches the route name, parameters and menu name.
   *
   * @param string|null $menu_name
   *   (optional) The menu within which to find the active link. If omitted, all
   *   menus will be searched.
   *
   * @return \Drupal\Core\Menu\MenuLinkInterface|null
   *   The menu link for the given route name, parameters and menu, or NULL if
   *   there is no matching menu link or the current user cannot access the
   *   current page (i.e. we have a 403 response).
   */

There is no "given route" because you don't pass a route to the function. Instead, it gets the current route from the current_route_match service. There's some code smell going on here. I assert that the docblock in the interface is incorrect and needs to be changed. I updated it to say that it looks for a menu link for the currently active route, although even that can't be guaranteed by the interface. There's an argument that the docblock change should be a separate issue, but since we're altering the behavior of this function to look for "alias" links that should be active for the current route, then I argue that it's an appropriate change for this issue.

The lack of route injection for the function also made the test harder to write. I tried to make a kernel test, but eventually punted on that and made a functional test that checks for Olivero's active trail class. I don't like it, but I couldn't figure out how to do it any other way. If someone has suggestions, please let me know.

dcam → changed the visibility of the branch 1578832-tree-storage-check-front to hidden.

oily → changed the visibility of the branch 1578832-tree-storage-check-front to hidden.

@dcam There seems to be a test has got broken. Line 126 of SystemMenuBlockTest.php seems root of the problem?:

  // This creates a tree with the following structure:
    // - 1
    // - 2
    //   - 3
    //     - 4
    // - 5
    //   - 7
    // - 6
    // - 8
    // With link 6 being the only external link.
    $links = [
      1 => MenuLinkMock::create(['id' => 'test.example1', 'route_name' => 'example1', 'title' => 'foo', 'parent' => '', 'weight' => 0]),
      2 => MenuLinkMock::create(['id' => 'test.example2', 'route_name' => 'example2', 'title' => 'bar', 'parent' => '', 'route_parameters' => ['foo' => 'bar'], 'weight' => 1]),
      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => 'example3', 'title' => 'baz', 'parent' => 'test.example2', 'weight' => 2]),
      4 => MenuLinkMock::create(['id' => 'test.example4', 'route_name' => 'example4', 'title' => 'qux', 'parent' => 'test.example3', 'weight' => 3]),
      5 => MenuLinkMock::create(['id' => 'test.example5', 'route_name' => 'example5', 'title' => 'title5', 'parent' => '', 'expanded' => TRUE, 'weight' => 4]),
      6 => MenuLinkMock::create(['id' => 'test.example6', 'route_name' => '', 'url' => 'https://www.drupal.org/', 'title' => 'bar_bar', 'parent' => '', 'weight' => 5]),
      7 => MenuLinkMock::create(['id' => 'test.example7', 'route_name' => 'example7', 'title' => 'title7', 'parent' => 'test.example5', 'weight' => 6]),
      8 => MenuLinkMock::create(['id' => 'test.example8', 'route_name' => 'example8', 'title' => 'title8', 'parent' => '', 'weight' => 7]),
    ];

Adding Issue tags:

The Issue summary is for Drupal 7. Before and after screenshots for Drupal 11 are needed.

Have updated the Issue summary so it makes more sense in relation to Drupal 11.

There is more work to do. The screenshots need to be updated for Drupal 11 and any discrepancies with the existing D8 and D7 sections in the issue summary can be rectified.

@dcam There seems to be a test has got broken.

Yeah. I saw that. But it was well past quitting time for the day, so I haven't tried to debug it. That's why I didn't set the issue status to Needs Review. Just from glancing at the backtrace it looked like the change to the service either broke the system menu block or maybe just the test. I'm not sure which.

@dcam Great work. This is a long-standing bug!

SystemMenuBlockTest was failing because it mocks all the routes. But when the menu links were being built for the block they went through the path.matcher service which checked for the existence of the routes. Since they didn't actually exist it threw exceptions. I mocked the service to fix the test.

@dcam Great!

Issue summary update

Left some comments on MR

Thanks

A kernel test needs to be created to verify the fix.

I’ve contributed my two cents and implemented it as a unit test, as the necessary mocking was straightforward, just like in the existing MenuActiveTrailTest unit test.

I've tested the MenuActiveTrail::getActiveLink() method, which should return the first <front> route menu link when the current page is the front page and corresponds to a route without its "own" links. This scenario occurs in a default fresh Drupal installation, where the home page is the view.frontpage.page_1 route and the main navigation menu has only a "Home" link provided by the system core module for the route <front>.

Super close, just 1 more comment.

That was a hard one! :)

Thanks! just needs a rebase now and think it's good to move up

Oops! I'm sorry, I was so focused on the test pipeline passing that I overlooked the merge conflicts :)

Thanks for the review!

Thanks, believe this is good to go now.

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.

Updated the target branch. Restoring RTBC status.

@dcam just re-ran the one failing test

Pipeline green, test only test fails as it should.

Thanks @oily. I forgot to check on the test results after I rebased the MR to make certain it would still apply after all the recent commits to Core.

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.

I'm going to have to iteratively update the StandardPerformanceTest to account for the changes to the cache as a result of the bug fix. Because for some reason my local environment doesn't want to run FunctionalJavascript tests today so that I can do this in one commit.

I could help you out if you want. Let me know here or contact me in slack :)

All the tests are green again. I'm setting this back to Needs Review instead of RTBC only because this wasn't a simple rebase this time. I had to make adjustments to the expected values in the StandardPerformanceTest. They're probably innocuous, but someone should look them over.

The new numbers look reasonable to me

I rebased again due to a merge conflict caused by this morning's commits to Core. This was an easier rebase than the last one, so I'm leaving it at RTBC.

Marking as needs work, as this patch does not apply to 10.4.

That's normal @drupgirl, it has to apply to the last major development branch (11.x in this case)

You can make a backport and publish it here as a patch, for others.

Sorry for all the rebasing spam. I've been trying to keep this up to date despite all of the other recent commits. Practically every change to the performance test forces a rebase and update of this MR.

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.

I rebased the MR against 11.x again. Restoring RTBC status.

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.

Rebased again.

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.

Rebased. Again. Restoring RTBC.