Problem/Motivation

For anonymous users, links to the front page* get the is-active class on all pages given that there is a legitimately active link** that comes after them somewhere in the HTML response.

* More precisely: links that have the data-drupal-link-system-path attribute set to <front>. (Links to the front page can also have the actual front page path (e.g. node) in the attribute in which case this bug does not surface.)
** In fact it is sufficient if the data-drupal-link-system-path attribute matches the current path, even if, for example, the language of the query parameters of the link result in the link not being marked active in the end.

Steps to reproduce

  1. Enable the Compose tips link on /admin/structure/menu/manage/tools
  2. Make sure you are logged out
  3. Go to /filter/tips
  4. See that the Home link in the primary menu gets marked as active, where it should not.

A screenshot showing the previously described scenario.

Proposed resolution

The bug is caused by a slight logic error in ActiveLinkResponseFilter::setLinkActiveClass(). A check whether $is_front is TRUE is missing in one spot.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

The last submitted patch, 2: 2560987-4-active-link--tests-only.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php
    @@ -139,7 +139,7 @@ public static function setLinkActiveClass($html_markup, $current_path, $is_front
    -      $pos_front = strpos($html_markup, $search_key_front, $offset);
    +      $pos_front = $is_front ? strpos($html_markup, $search_key_front, $offset) : FALSE;
    

    It would be nice to document that a bit.

  2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ActiveLinkResponseFilterTest.php
    @@ -326,6 +326,42 @@ public function providerTestSetLinkActiveClass() {
    +    $data[] = [
    ...
    +    $data[] = [
    ...
    +    $data[] = [
    ...
    +    $data[] = [
    

    named tests would be nice

tstoeckler’s picture

Your wish is my command.... or something like that.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It was a small bug fix, but a big fix for the long term maintainability of this function.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice find and simple fix. Committed a53ec3d and pushed to 8.0.x. Thanks!

  • alexpott committed a53ec3d on 8.0.x
    Issue #2560987 by tstoeckler: Active class (almost) always added to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.