Menu subtrees in menu blocks show all subitems regardless of the active menu item

We were able to reproduce this issue while working on a new Drupal 8 project as well. It seems like the menu block is not taking into account the active menu trail when the starting level configured is higher than 1.

We added the code below to SystemMenuBlock.php right below the conditional on $depth in the "build" method and it seems to handle the cases as expected. However, we are uncertain whether or not this is appropriate spot for this fix or if there is a differently layer that this should addressed in so any and all feedback is appreciated.

    // If the configured starting level is at least one level higher than the base
    // this is a sub menu and should only show links that are children of the 
    // currently active top level parent.
    if($level > 1) {
      $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
      // Reverse the array of active links so we can retrieve the top level 
      // active parent at the index of 1.
      $active_trail_reversed = array_reverse($active_trail);
      $sub_menu_active_root = array_values($active_trail_reversed)[1]; 
      
      // Both Min Depth and Max Depth are set 1 level less since we 
      // are starting below the natural root.
      if (!empty($sub_menu_active_root)) {
        $parameters->setRoot($sub_menu_active_root)
          ->setMinDepth($level - 1)
          ->setMaxDepth(min($level + $depth - 1, $this->menuTree->maxDepth()) - 1);
      }
      
      // If there is only a single active trail item (the root menu itself) then
      // clear it and set the expanded parent to be the root of the current page.
      // Preventing other sub menu blocks from displaying all of their child links.
      if(count($parameters->activeTrail) < 2) {
        $parameters->expandedParents = array($sub_menu_active_root);
      } 
    }

Needs review means there is some form of patch already.

We discovered that if we marked "Show as expanded", the menu is displayed at sublevels where it should not appear. The code above fix the main level. Our patch also fix sublevels.

Added some cleaning and configuration options.

I'm not quite sure about the hiding part of it is needed. I left it out so it reverts to the default behavior.

Made a mistake in the patch

count($parameters->activeTrail) > $level needs to be count($parameters->activeTrail) >= $level

Updated the patch. with >=. The patch seems to solve the issue for us.

I had a problem after applying the patch in #7 (with the correction). Nodes that didn't exist within the menu structure had menu blocks that once again showed every sub-menu item.

I'm not sure if this is a configuration issue, or something else.

Have you configured the menu block to "follow" the trail?

The menu block still shows the all child items when there is no active menu item used though, we may want to look at that scenario as well.

Have you configured the menu block to "follow" the trail?

Yes, I have.

I have the same, although not with nodes, but with other entity landing pages.

If we change the code to have an exception handling when the active trail count is higher or equal to the level, we could hide the menu entirely, e.g. by

    if ($level > 1 && $this->configuration['follow']) {
      if (count($parameters->activeTrail) >= $level) {
        $menu_root = array_values($parameters->activeTrail)[count($parameters->activeTrail) - $level];
        $parameters->setRoot($menu_root)
          ->setMinDepth(1);
      }
      else {
        return;
      }
    }

then I get the expected behaviour on my local test. Could you check? I'm not sure if this is the right way of doing it, feels dirty to just return in there.

Thanks, @snufkin, that does seem to work for me. Here's a patch for further review.

Sorry - better formatted patch.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -152,6 +160,15 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa
+      } else {

Sorry to be nitpicking, but the else should go on a new line according to Drupal CS.

Thanks, all feedback is very welcome! It's been a while since I did this. Hope this one's better.

Setting the issue to needs review to trigger the testing bot. The updated patch works well for us.

Let's try it without the whitespace this time.

Lets try it this way :)

I have added the schema definition to the config, this should fix the errors in the previous patches (Based on #27).
We also need some tests for this new feature!

I would also propose to increase the priority to Major, since the menu block level handling is basically useless without this option. I can't think of more than 1 occasion where the current approach is the correct one.
Naturally feel free, to revert it back to Normal if you do not agree with this.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -152,6 +160,18 @@ public function build() {
+        return;

Didn't review the whole patch, but if you want to return nothing, shouldn't this return [] instead of NULL?

To my knowledge there is no difference between return NULL and return. And looking at code from core there are cases where the build function will not have a return, if certain conditionals are not triggered. No return value is basically the same as a return; or return NULL; call.

Since the @return type for the build method does state it is a renderable array, I would say that it should be an array regardless, however there are examples from core where this is not true, so for me the implementation in the patch seems to be acceptable.

True, the Core code is robust enough to handle it. However, you will not find any block() methods that explicitly return without returning an array; Instead you will find return []; or return array();. Plus on the page you linked to it reads, "it must then only return an empty array".

I tested the patch manually and it does give the right menu:

• About Event 1
• Subscribe to Event 1

Started looking at where the tests should be added. Not sure, but it might be possible to test this in one of the PHPUnit Tests in: \Drupal\Tests\Core\Menu\. It was pretty easy to set up the menu for manual testing, but not sure yet exactly how to set it up in a test.

Well let this not be the reason for this patch not moving forward. Replacing the return; with return array();.

Changing title to summarise the issue a bit better.

Changing status to trigger test bot.

I'll fix the bot errors tonight, i have seen what it is.

Any ideas about writing the tests? Maybe we can figure it out.

The test errors are due to the fact that we are providing an integer value in the default configuration for the follow item in the block, while the schema declares this as a boolean. Patch is now using a boolean in the default block configuration.

Argh, changed the wrong element.

Nice, down from 44 to 2 fails.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -168,6 +188,7 @@ public function defaultConfiguration() {
+      'follow' => FALSE,

I think this is the only change that was made. But if possible, please provide interdiff with future patches so we can see the changes.

The tests failed because the default configuration provided by the install profile was not updated for the latest block configuration schema. Updating patch to add the follow property to the blocks that triggered the test fails. For now all of these are set to false, we may want to think if we need to set any to true.

Sorry I got caught in the events, the patch provided by snufkin in #45 was exactly was I was going to upload right now, if it wasn't already there. :)
so for me is a RTBC! :)

+++ b/core/modules/system/config/schema/system.schema.yml
--- a/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -101,6 +101,13 @@ public function blockForm($form, FormStateInterface $form_state) {
 
+    $form['menu_levels']['follow'] = array(
+      '#type' => 'checkbox',
+      '#title' => $this->t('Make the starting level follow the active menu item.'),
+      '#default_value' => $config['follow'],
+      '#description' => $this->t('If the active menu item is deeper than the level specified above, the starting level will follow the active menu item. Otherwise, the starting level of the tree will remain fixed.')
+    );

The newly added text is very confusing - I barely understand it myself.

Also is it necessary to show this all the time, or could it depend on the value of the level/depth options?

Tagging needs issue summary update - I'd like to know what the use case is for not having this behaviour - i.e. why make it a configuration option vs. just changing the behaviour?

I've updated the summary a little bit. I have not stated whether or not this is a new feature (so we need the UI), or a bug (we don't need the UI) until it is decided in the comments.

I personally can't think of a use-case where we would want to show other subtrees from other parents, so I would be happy to reroll the patch without the UI changes.

I do agree that the use cases where one would want to show other subtrees from other parents are.... non existent?
I am totally for simply changingfixing the logic of the block, however changing this would possibly(?) break some sites, so maybe we should simply set it as enabled by default, leaving also the UI?

That feels like the kind of behaviour change we could reasonably do in a minor release (which to introduce the UI we'd have to make it a minor release anyway, so moving to 8.1.x).

If someone really is relying on the current behaviour, then we could treat that as a bug report/regression once it's reported and add the UI in response. If no-one is though, we save ourselves some logic and interface to maintain, as well as any confusion the additional option might cause for people seeing it.

I'm in agreement with others here that there may be no use cases for the current core behavior. Because of this I believe the default core behavior to me is a bug because right now on D8 (as far as I know) there is no way to show a contextual left hand navigation like we were able to do with Menu Block on D7.

The wording called out in #47 looks to be from the D7 menu_block contrib module, I can't see any mention of that so just making sure that is known. Not saying that makes it acceptable for core :)

I think we have a fair consensus that we should consider this as a bug, and if so, could we not fix it against 8.0.x? Or the reason to change the version against 8.1.x was to first fix it there and then backport?

patch #45 works for me and fixes my issue

Moving to needs work to remove the configuration option.

@snufkin this could potentially go into 8.0.x if it's ready for the next (last) patch release window for that version.

I have not been able to reproduce the issue on 8.0.5 and 8.0.x anymore, works as expected. Could someone else confirm?

I was still experiencing the issue in 8.0.5 and the patch in #45 fixed the issue. I think there is a need for another option to hide the sibling menu items when you drill down further into the menu. Given the following...

Parent1
- Child1.1
-- Grandchild 1.1.1
-- Grandchild 1.1.2
- Child1.2
-- Grandchild 1.2.1
-- Grandchild 1.2.2
- Child1.3
-- Grandchild 1.3.1
-- Grandchild 1.3.2

If I were on the Child 1.1 page, I may only want to display 1.1.1 and 1.1.2. I would want to have the option to hide 1.2 and 1.3

I confirm this patch works for one sub level.
It solved my issue at lease but I'm only using 2 levels.

I was still seeing problems in 8.0.5 before the patch, which did resolve the issues in my use case. While I understand the need for clear text, functional with slightly wonky interface is better than broken. I suggest this move forward and that a new issue be opened to address the field label.

I have tested on 8.0.5 and I can still reproduce if i'm not using the patch.

I believe that #57 should be addressed in another issue, although I believe in some occasions it might be useful, it's more an enhancements than a bug.

Expanding menu trees other than the one the current menu item is in is a bug, not a feature. So yes, lets remove this behaviour.

+1 for fixing the behaviour rather than adding more options that would still leave the broken default behaviour.

Could it be argued that the testConfigLevelDepth test was wrong in the first place?

The test's menu structure should be like this:

• test.example1
• test.example2
  • test.example3
    • test.example4
• test.example5
  • test.example7
• test.example6
• test.example8

Therefore the test should have checked that if the menu level was set to '2' that only test.example3 or test.example7 would show, whereas the bug indicates that both would be shown.

Back to writing tests.

For what it's worth, the patch in #64 does appear to work properly despite the test failures, so it does look like a problem with the tests requiring an update before this can go in.

#64 is working for me. Thanks.

This is still an issue on 8.2.0. Patch #64 worked for me.

Seeing as a rewritten test would highlight this bug, thus would need to have the bug fixed before the test could be committed, is there any value in having the separate issue for fixing the test?

I've updated the patch from #64 to include updates to the tests that review test.example7 from the expected results set. I'm not convinced that this will cover all use cases (e.g. when the block should show text.example7 instead of 3).

Both #75 and #45 patches apply to 8.3.x, however they don't seems to work as they are supposed to.

Can someone summarize the proposed behavior change / bug fix?

Here is my take, based on discussion and content of these patches:

• For menu items with starting level = 1, no change
• For menu items with starting level > 1, repurpose "show as expanded" checkbox to mean "show all children of this menu item, only if this menu item or one of its children is in the active menu trail"
• For menu items with starting level > 1, remove the ability to show a fully-expanded menu tree

Is all that correct?
Did I miss anything?

That much makes sense to me, but the patches in this thread have the following additional side effect which does not:

• For menu items with starting level > 1, do not show any menu when current active menu item level is equal to starting level.
  

edit: After re-saving the menu items, this issue went away, and most recent patch works as described above.

I'm not sure if this is the "not working" bit that esolitos is referring to.

test from #77 still failing

The summary in #80 is correct. What's really needed at this point is to get the test fixed so it can get committed.

OK, updating the issue summary clarified the reason why this test is failing.

For the $no_active_trail_expectations portion of this test, the active trail is not set.
ie. There's no request context, so no active trail is set.
Since we expect for blocks with level > 1, the menus do not appear at all, we need to adjust the test:

@@ -218,9 +218,7 @@ public function testConfigLevelDepth() {
       'test.example6' => [],
       'test.example8' => [],
     ];
-    $no_active_trail_expectations['level_2_only'] = [
-      'test.example7' => [],
-    ];
+    $no_active_trail_expectations['level_2_only'] = [];
     $no_active_trail_expectations['level_3_only'] = [];
     $no_active_trail_expectations['level_1_and_beyond'] = $no_active_trail_expectations['all'];
     $no_active_trail_expectations['level_2_and_beyond'] = $no_active_trail_expectations['level_2_only'];

Once that test was fixed, I moved on to $active_trail_expectations['level_2_and_beyond'] which was also failing.
2 changes to the previous patches addressed this failure:

1. Since "activeTrail" is in reverse order -- deeepest-child first with index 0, and the top-level parent has the greatest index -- we need to array_reverse before identifying the root:
   

   +        $menu_trail_ids = array_reverse(array_values($parameters->activeTrail));
   +        $menu_root = $menu_trail_ids[$level - 1];
   

2. Since changing the menuTreeParameter will change the starting level, we also have to adjust the max-depth (if it is set).
   

   +        if ($depth > 0) {
   +          $parameters->setMaxDepth(min($level - 1 + $depth - 1, $this->menuTree->maxDepth()));
   +        }
   

It feels like this is still missing test coverage, but I can't think of what.

I installed a fresh 8.3.x-dev site and applied the path. Added menus, behavior worked as expected. Ran unit tests and they passed locally. Looks good to me.

Applies cleanly to 8.2.4 as well. It would be great to get it applied to both.

Thanks for testing this patch! We still need to add automated test coverage for the bug, though.

+++ b/core/modules/system/tests/src/Kernel/Block/SystemMenuBlockTest.php
@@ -218,9 +218,7 @@ public function testConfigLevelDepth() {
-    $no_active_trail_expectations['level_2_only'] = [
-      'test.example7' => [],
-    ];
+    $no_active_trail_expectations['level_2_only'] = [];

@@ -266,7 +264,6 @@ public function testConfigLevelDepth() {
-      'test.example7' => [],

@@ -276,7 +273,6 @@ public function testConfigLevelDepth() {
-      'test.example7' => [],

Also, if we are changing the expectations here from the previous test coverage, we should confirm why it was this way in the first place. A git blame or git log -L on the removed lines in the test could help.

I included updated tests with the latest patch.
Are you saying those need to be pulled out and posted separately?

Also, if we are changing the expectations here from the previous test coverage, we should confirm why it was this way in the first place. A git blame or git log -L on the removed lines in the test could help.

Here's where these tests were introduced: #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables
Maybe someone can scan that thread and recall the conversation to describe the motivation?
Looking at the thread for 5 minutes, I don't see any mention about the specific lines in question.

@xjm for an explanation of the test issues see comment 68 above. It's been clear from the discussion on this thread that the tests were wrong since before D8 was launched. So the adjustments that aaronbauman made were in keeping with those I attempted a while back and others have been discussing for most of year. Is there something in particular you're looking for to make this fix acceptable?

Standalone test with line-by-line explanation posted to #2745003: Rewrite test ConfigLevelDepth, fails as expected, not sure what to do next.

I closed the other issue as it was completely redundant, here are the two files uploaded again so you can see how the old corrected test fails and then the patch with the fix. (no code changes, this is exactly the same as #84)

Whoops, accidentally included some local composer changes in drupal-n2631468-93.patch.

(just to be clear, #95 is identical to #84)

The patch from #95 applies to 8.3.x-dev but doesn't seem to be working as expected. I have my initial menu level set to 2. I've tried it with "Expand all menu links" checked and unchecked. In both cases, I still have the original issue.

Actually I seem to have another issue, so ignore this post!

Tested #95, fixed the issue for me. Thumbsup (credits #84 for the initial work too!)

I was able to reproduce on a fresh install of Drupal 8.2.6.

1. Created 7 nodes with menu links.
2. Created the following menu structure.
   

   - Home
   - Events
   -- Event 1
   --- About Event 1
   --- Subscribe to Event 1
   -- Event 2
   --- About Event 2
   --- Subscribe to Event 2
   

3. Set the Initial menu level in the menu block to 2
4. Maximum number of menu levels to display in the menu block set to Unlimited
5. Visited Events 1 page. The menu block displays items from Event 1 and Event 2 subtrees.
   Menu displayed:

   -- Event 1
   --- About Event 1
   --- Subscribe to Event 1
   --- Subscribe to Event 2
   --- About Event 2
   

Applied patch in #95. Menu displays correctly.

-- Event 1
--- About Event 1
--- Subscribe to Event 1

Thanks @StevenGFX for the manual testing, and @aaronbauman and @DamienMcKenna for investigating the needed test change.

My feedback in #89 is partly but not quite wholly addressed. It's clear from #68 and the test-only patch @DamienMcKenna provided in #93 that these changes are needed for compatibility with the new approach. However, it's still not clear why the test was asserting this apparently bugged behavior in the first place.

@aaronbauman traced it to #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables above. So all I think the last thing we need to do to cover my question in #89 is to have one other person go over that issue and confirm that it wasn't intentionally set that way. Someone might need to poke around a bit on the interdiffs and old patches on the issue to figure out exactly what the reasoning was for adding it (or if there was none and it dated to the first draft of the patch with no explanation, etc.). That should cover it I think.

Finally though, I think we still do also need new tests asserting the new behavior explicitly as per #88. #102 actually looks like a good scenario to translate into an additional automated test, perhaps.

Thanks everyone! I think this is close.

Thanks for the continued work, everyone, and to xjm for the direction. If nobody gets to it first, I'll try to look into this one during the week.

OK, I think I've untangled this.

Spoiler alert:
There were (at least) 2 different threads going in parallel which resulted in this behavior getting into core, along with tests to pass it, #1869476 and #474004 respectively.

I'm working on typing up a fully annotated timeline now.

The test change relates to what I'll call the "sibling-trail" bug, laid out in detail on this thread. (To recap the bug: for menu blocks with starting depth level 2+, it doesn't make sense to show expanded links outside of the active menu trail.)

Here's the history I pieced together of how this behavior came to be:

Roll back to the stone ages of Menu Block module - the inspiration for the "starting level" feature - and there has never been a "sibling-trail" bug. (JohnAlbin may wish to confirm or deny this statement.) Either because of the additional configuration options offered by Menu Block, or because of explicit accommodation, Menu Block's behavior works off the shelf as expected in this thread.

For Drupal core, roll back to January 2013, and in 1869476-19 -- converting menus to blocks -- Sun makes many of the points repeated in this thread and in 474004, especially (emphasis mine):

"Secondary links" support: Each instance supports a "follow" setting, which essentially means that the "Starting level" follows the currently active menu link. In essence, that's what resembles the current secondary links: If the current active link is "About" in menu 'main', then the secondary links show the children of "About" in the 'main' menu.

This position is echoed by kim.pepper in 474004-70:

Can we agree that the functionality of core menu block will be ...
* all the top-level items in a menu
* all the sibling items to the current location
* the tree to a certain depth from the current location

and Crell in 474004-71:

The block you configure defines which menu to pull data from. So when setting up a block you'd select:

1) The menu to pull from
2) The top level to start from
3) The bottom level to go to

(The block only shows up if the current page is within the range of points 2 and 3.) I believe that's what menu_block does today.

The meat of the sibling-trail bug is introduced around 474004-143:
- Wim Leers suggests that we don't want to support "the expanded option behavior", but doesn't mention how that will effect menu behavior outside of core's special primary/secondary menus.
Then, in 474004-144, kim.pepper attempts to clarify, and suggests adhering to the help text: "Blocks that start with the 2nd level or deeper will only be visible when the trail to the active menu passes through the block’s starting level.'"
Then in 474004-145, Wim Leers dismisses this concern, and advocates dropping support for this behavior, because core's primary/secondary menus won't be affected. Wim Leers provides screenshots to illustrate this position, but does not provide examples with enough depth to surface the sibling-trail bug.

Wim Leers also suggests twice in this thread that pwolanin would agree with this decision, but neither pwolanin nor crell re-join the discussion after #70.

Sun commented again in 474004-181, but does not weigh in on the sibling-trail bug.

Bojhan and yoroy weigh in with usability concerns in 474004-183 and 474004-188, but only from the perspective of admins - not from the perspective of the user-facing menu block or the sibling-trail bug.

For me this is the key takeaway:

It's apparent from the discussion in 474004 that the change arounded "expanded" behavior is discussed at some length, but not in relation to the sibling-trail bug. Further, the original issue description has not been updated with any illustration of the user-facing changes. The various descriptions in comments on the thread do not address the sibling-trail bug at all, even though the behavior is covered by its tests.

Probably useful at this point to have someone from #474004 weigh in on this assessment, because i'm way too far down the rabbit hole now.

@aaronbauman: Huge thanks for digging through all of that!

IMHO it seems like the original feature request was slightly derailed, that the tests were not specific enough to clarify the exact intent and then a very limited use case was considered for the functionality with other use cases (thus an aspect of the agreed functionality) dismissed.

Patch #95 worked for me, too. (8.2.4)

Patch #95 worked for me (and a colleague on a different instance) on Drupal 8.3.1 vanilla standard install, but there is a strange side-effect: on pages with no sub-menu, the region which the block is in is displayed, but empty (so the page.region is not empty, but the render() doesn't generate any HTML, thus the empty region).
A {{ dump(page.region) }} does indeed contain an array of the menu block, but the render array only contains #cache, #weight, #lazy_builder and #theme_wrappers keys. Hope that helps.

(Note: the "monstruosity" {% if page.region|render|trim is not empty %} did the trick...)

I'm sitting here with @lokapujya at Druplacon baltimore running 8.3.1 and it looks fixed to us on my machine without the patch

The "observed behavior" in the issue summary doesn't seem to be the case. What I see when on the "About Event 1" page is:
-- Event 1
--- About Event 1
--- Subscribe to Event 1
-- Event 2

After reading ALL the comments, see comment #80 for a description of the latest patches proposed changes. It proposes to repurpose "show as expanded". Why would we do that? If you don't want it to be expanded, then just don't check it off.

It also removes the ability to see all the items at the current level. What if you have a menu, and you DO want to see the all the items at the current level of the menu?

The "observed behavior" in the issue summary doesn't seem to be the case. What I see when on the "About Event 1" page is:
-- Event 1
--- About Event 1
--- Subscribe to Event 1
-- Event 2

Sounds like you don't have the "show as expanded" property set.

After reading ALL the comments, see comment #80 for a description of the latest patches proposed changes. It proposes to repurpose "show as expanded". Why would we do that? If you don't want it to be expanded, then just don't check it off.

There are 2 different use-cases identified for the "show as expanded" checkbox:
1. For an entire menu tree, always show all of the links for all branches
2. For a given menu sub-tree, always show all of the child links in the tree
The most recent patch supports these 2 use cases, while eliminating the behavior of showing sibling-level menu items from separate sub-trees for level 2+ menu items.

It also removes the ability to see all the items at the current level. What if you have a menu, and you DO want to see the all the items at the current level of the menu?

If you want to create a menu which shows all top-level siblings, that behavior is preserved. In that case, the "Expand" checkbox will continue to show all children of those branches as desired.

For level-2+ items, it's the general consensus on this and other threads that this use case simply doesn't make sense.

In any hierarchical structure, the top-level items provide integral context.
It's not a plausible use case that someone would want to see *all* level-2 items across *all* sub-trees, without the context of their top-level parents.

Please review the discussion and links to bug reports for users encountering this behavior.

I am in the sprint room and it's definitely not fixed on 8.4.x

re @simon in #111:
I can't reproduce this empty-block behavior locally.
Are you sure this behavior is a side effect of the patch?
Can you explain the steps to reproduce?

re bug raised in #111, I can reproduce, but it's not from this patch (I can reproduce without it as well). That was reported in #2642816: Menu block is never empty, region keeps showing up.

I'll list the reproduction steps here in case anyone wants to validate this, but I think it's a separate issue:

1. Install core with Standard profile
2. Create six basic pages arranged on the main navigation menu so that 1, 3, and 5 are on menu root, 2 and 4 are subs of 1 and 3 respectively (page 6 intentionally not on menu).
3. Create a menu block with Initial visibility level set to 2, and levels to display unlimited. Place this block in Featured Top (the only empty default region, and helpfully with a grey background.
4. View all 6 pages, and you will see the menu correctly when it should be there, and an empty grey bar on remaining pages.

Again, this happens with or without the patch from #95, which is the same as the patch from #84.

Since that is a separate issue, and this patch does not impact it for better or worse, I'm setting this issue back to RBTC.

Thanks for the feedback, @acrosman, and for pointing out the other issue.

@aaronbauman's description of how the bug was introduced is fantastic.

However I don't think that xjm's other point in #105 has been addressed:

Finally though, I think we still do also need new tests asserting the new behavior explicitly as per #88. #102 actually looks like a good scenario to translate into an additional automated test, perhaps.

So CNW for adding more specific test coverage for the behaviour we want.

Re-roll the #95 patch because it's failed to apply on 8.4.x branch.

However I don't think that xjm's other point in #105 has been addressed:

Finally though, I think we still do also need new tests asserting the new behavior explicitly as per #88. #102 actually looks like a good scenario to translate into an additional automated test, perhaps.

Right, thanks for calling out that detail. The existing test scenarios already include a minimal menu hierarchy to elicit this behavior, and the proposed change updates test assertions to provide explicit coverage.

To address #102 specifically, StevenGFX's scenario includes more menu items to produce a similar behavior.
But, we can already observe the behavior with the existing scenario.
Therefore, we don't need to change the existing scenario, we need only change our assertions.

Again, to summarize briefly:

Scenarios / Givens:

NB: This is the existing scenario; proposed change in this thread doesn't touch this at all.

• This menu tree:
  

      // - 1
      // - 2
      //   - 3
      //     - 4
      // - 5
      //   - 7
      // - 6
      // - 8
  

• A starting-level-2 menu block
• An active menu of item 3 OR no active menu item at all

Observed behavior:

• class SystemMenuBlock: The Menu Block displays item 7.
• class SystemMenuBlockTest: assert that item 7 should be displayed.

Expected behavior / proposed change:

• class SystemMenuBlock: The menu block does not display item 7
• class SystemMenuBlockTest: assert that item 7 should not be displayed

I think we are back to RTBC. As @aaronbauman points out the tests as written in the current patch cover the scenarios.

Hm OK the fails in #93 and explanation in #123 look reasonable to me.

Fixed this on commit:

FILE: ...tes/8.x/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 153 | ERROR | [x] Short array syntax must be used to define arrays
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed/pushed to 8.4.x, thanks!

Hurray! Thank you, everyone!

Wow! Glad to see that we made it by 8.4.x! :)
Good job everyone!

I'm thrilled to see this is finally in. Is it reasonable to get this into one of the 8.3 releases that may still come along before 8.4 is released?
At last check (the security update the other day) the patch applied to 8.3 as well.

Sorry, I was reading this thread, with no intention of commenting when Firefox hung up. When I restored my Drupal.org session this was posted. I see no way to delete it and suspect that deleting even inadvertent posts is not wanted. Sorry again.

Moving back to 8.4 and Bug just for record keeping. No worries @fkelly, the machines get the best of all of us from time to time.

I think that a patch to Superfish may be required if you want to fit this into 8.3.

https://www.drupal.org/node/2896583

before it can run with the updated SystemMenuBlock.php from the 8.4 commit. So, if this update is applied to 8.3 someone should look at coordinating that update with Superfish. (On my local test system I'm experimenting with Superfish as a means to get drop down hierarchical menus working and driving myself buggy with the way expanded menu items work (or don't)). I tried the 8.4. SystemMenuBlock.php on my 8.3 system and it caused the error reported in the link above. Restored the old SystemMenuBlock.php for now.