Menu item "active" class is not correctly added when using a view as the frontpage

Great catch!

So you changed it from

IF path
ELSE IF front

to:

IF path
IF front

I'd suggest changing it to

IF front
ELSE IF path

That's a bit more straightforward IMO.

@penyaskito: perhaps a caching problem? Note that active links are handled in JS for authenticated users, and in PHP for anonymous. Perhaps you're testing a different scenario, by accident?

Confirmed the bug exists. If front is the last link on the page, you could jump over all the other links without parsing them. The tests will need to cover these cases. I think we either need to search for each string(current patch and front) separately, or go in order of which one is found first. But, then we have to make sure the active-link class doesn't get added twice.

this should make the separate loops patch easier to look at.

Why did you roll #8? IMHO #4 is sufficient, and it's much easier to review. Protection to prevent the class from being added twice was already added in #2412241: Active menu links with identical paths get a duplicate "active" class for every active menu link.

It's possible #4 will work. Next step is to create tests.

But, it seems like that with the current logic before patch #4 and maybe still with patch #4 doesn't fix all cases.

If a front link is parsed and the front link is the last link on the page, then code will skip over parsing all current path links.

Or if a current path link is the last, it could skip over the front links.

The patches in #8 are 2 different approaches meant to get all front links and all current path links.

I've rerolled #4 and added some tests.

Awesome! Thank you so much for writing those tests :)

I'm only reordering/clarifying things a bit in test part, because it's easier to do than explain.

Test review (all addressed in this reroll)

1. +++ b/core/modules/system/tests/src/Unit/Controller/SystemControllerTest.php
   @@ -305,6 +305,26 @@ public function providerTestSetLinkActiveClass() {
   +    // Create some paths.
   

   s/paths/links/

2. +++ b/core/modules/system/tests/src/Unit/Controller/SystemControllerTest.php
   @@ -305,6 +305,26 @@ public function providerTestSetLinkActiveClass() {
   +    $front_path = '<a data-drupal-link-system-path="&lt;front&gt;">Front</a>';
   +    $current_path = '<a data-drupal-link-system-path="">Current Path</a>';
   +    $front_path_active = '<a data-drupal-link-system-path="&lt;front&gt;" class="active">Front</a>';
   +    $current_path_active = '<a data-drupal-link-system-path="" class="active">Current Path</a>';
   

   The "path" part of these variable names should become "link".

3. +++ b/core/modules/system/tests/src/Unit/Controller/SystemControllerTest.php
   @@ -305,6 +305,26 @@ public function providerTestSetLinkActiveClass() {
   +    // Test case to verify that the 'active' class gets set when current_path is first.
   ...
   +    // Test case to verify that the 'active' class gets set when front is first.
   

   These comments are confusing:
   - These are both test cases for the front page, so they can be grouped together
   - "current_path" reminds readers of the <current> route, which is not at all related
   - The use of '' as the front page's path actually further makes one suspect this is related to <current>, which it's not. Let's use a better front page path, just like in the other tests.

Code review

+++ b/core/modules/system/src/Controller/SystemController.php
@@ -366,11 +366,13 @@ public static function setLinkActiveClass(array $element, array $context) {
+        $type_match = 'front';
...
+        $type_match = 'path';

These have been removed in another patch, this is just wrongly merged.

But, worse than that: it doesn't look like this makes the test pass! (EDIT: as testbot now also says, I'd tested it locally first.) Neither #1 nor #4 make the tests pass. So perhaps we do want to go with something like the first option in #8 after all. But then we need to understand *why* such drastic changes are necessary. What exactly is wrong with the current logic?

The current logic is wrong because the while loop finds an occurrence of an active link for the "current path" or the "front" page. But, the if statement inside the loop processes the current path (when the front may have actually occurred first.) Then the offset is updated to check the remaining html only from that point on, (thus, skipping over the rest of the links.)

So adding the #8-parse-first-found solution, and a comment change to the tests.

So each iteration of the loop should parse the next link (whether it's exact path or special case front), as opposed to the next "exact path" link.

Ahh! I understand now :) Thanks for the clarification, the code now makes total sense. Thank you! (And sorry for my misunderstanding earlier!)

Sounds like a bug to me. This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed efc0e2e and pushed to 8.0.x. Thanks!