Problem/Motivation

When visiting a page which belongs to a child menu tree, menu block displays other trees on the same level, from different parents, when the parents are set to expanded and the starting level is greater than 1.

E.g. using the following structure:

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

Expected behavior:

When the user sets the menu block to level 3, they expect the subtrees under either Event 1, or Event 2 to show, e.g.

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

Observed Behavior:

Instead, when visiting either "About Event" page, the menu block displays items from Event 1 and Event 2 subtree:

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

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

Proposed resolution

  • If menu block level is greater than 1, show only the menu items along the active menu trail, regardless of "expanded" settings on other menu trails.
  • For menu items with starting level > 1, remove the ability to show a fully-expanded menu tree

Remaining tasks

Improve the UI text which explains what the "Expanded" option means on the menu block configuration.
For menu items with level > 1, "Expanded" means "show all children of this menu item, only if this menu item or one of its children is in the active menu trail"

API changes

N/A

Files: 
CommentFileSizeAuthor
#95 drupal-n2631468-95.patch2.83 KBDamienMcKenna
#93 drupal-n2631468-93.patch129.63 KBDamienMcKenna
#93 drupal-n2631468-93-TEST-ONLY.patch1.41 KBDamienMcKenna
#84 menu_block_follow_active-2631468-84.patch2.83 KBaaronbauman
#75 menu_block_follow_active-2631468-75.patch1.89 KBacrosman
#64 menu_block_follow_active-2631468.patch1.01 KBkiwad
#45 menu_block_follow_active-2631468-45.patch5.33 KBsnufkin
#41 menu_block_follow_active-2631468-41.patch2.74 KBsnufkin
#40 menu_block_follow_active-2631468-40.patch2.79 KBsnufkin
#34 menu_block_follow_active-2631468-29.patch2.73 KBsnufkin
#29 menu_block_follow_active-2631468-29.patch2.72 KBesolitos
#10 drupal_core_menu_block-follow_active_trail-2631468-7.patch2.02 KBsnufkin
#7 drupal_core_menu_block-follow_active_trail-2631468-7.patch2.02 KBTom Robert
#5 2631468-5.patch2.08 KBacke

Comments

kiwad created an issue. See original summary.

akeemw’s picture

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);
      } 
    }
kiwad’s picture

Status: Active » Needs review
dawehner’s picture

Status: Needs review » Active

Needs review means there is some form of patch already.

acke’s picture

Status: Active » Needs review
FileSize
2.08 KB

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.

Status: Needs review » Needs work

The last submitted patch, 5: 2631468-5.patch, failed testing.

Tom Robert’s picture

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
Tom Robert’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: drupal_core_menu_block-follow_active_trail-2631468-7.patch, failed testing.

snufkin’s picture

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

johnmcc’s picture

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.

snufkin’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 10: drupal_core_menu_block-follow_active_trail-2631468-7.patch, failed testing.

johnmcc’s picture

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

Yes, I have.

snufkin’s picture

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.

johnmcc’s picture

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

johnmcc’s picture

Sorry - better formatted patch.

snufkin’s picture

+++ 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.

johnmcc’s picture

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

snufkin’s picture

Status: Needs work » Needs review

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

The last submitted patch, 17: 2631468-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2631468-19.patch, failed testing.

The last submitted patch, 16: 2631468-16.patch, failed testing.

johnmcc’s picture

Let's try it without the whitespace this time.

johnmcc’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: 2631468-24.patch, failed testing.

snufkin’s picture

Status: Needs work » Needs review
FileSize
2.2 KB

Lets try it this way :)

Status: Needs review » Needs work

The last submitted patch, 27: menu_block_level_vs-2631468-27.patch, failed testing.

esolitos’s picture

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.

lokapujya’s picture

+++ 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?

Status: Needs review » Needs work

The last submitted patch, 29: menu_block_follow_active-2631468-29.patch, failed testing.

snufkin’s picture

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.

lokapujya’s picture

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.

snufkin’s picture

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

snufkin’s picture

Title: Menu block level VS Expanded menu items » Menu subtrees in menu blocks show all subitems regardless of the active menu item

Changing title to summarise the issue a bit better.

mohit_aghera’s picture

Status: Needs work » Needs review

Changing status to trigger test bot.

Status: Needs review » Needs work

The last submitted patch, 34: menu_block_follow_active-2631468-29.patch, failed testing.

esolitos’s picture

Assigned: Unassigned » esolitos

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

lokapujya’s picture

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

snufkin’s picture

Status: Needs work » Needs review
FileSize
2.79 KB

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.

snufkin’s picture

Argh, changed the wrong element.

The last submitted patch, 40: menu_block_follow_active-2631468-40.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 41: menu_block_follow_active-2631468-41.patch, failed testing.

lokapujya’s picture

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.

snufkin’s picture

Status: Needs work » Needs review
FileSize
5.33 KB

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.

esolitos’s picture

Assigned: esolitos » Unassigned
Status: Needs review » Reviewed & tested by the community

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! :)

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update, +Needs usability review
+++ 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?

snufkin’s picture

Issue summary: View changes

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.

esolitos’s picture

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?

catch’s picture

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.

akeemw’s picture

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.

Cottser’s picture

Version: 8.0.x-dev » 8.1.x-dev

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 :)

snufkin’s picture

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?

mitchalbert’s picture

patch #45 works for me and fixes my issue

catch’s picture

Status: Needs review » Needs work

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.

snufkin’s picture

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?

mtalt’s picture

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

yvesvanlaer’s picture

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

acrosman’s picture

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.

esolitos’s picture

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.

yoroy’s picture

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.

DamienMcKenna’s picture

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

kiwad’s picture

Version: 8.1.x-dev » 8.2.x-dev
kiwad’s picture

Proposition of a re-rolled patch from #45 keeping only the basic fix and leaving all the new "Follow" configuration

kiwad’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: menu_block_follow_active-2631468.patch, failed testing.

kiwad’s picture

The patch fails because it introduces a regression (the $tree generated is not the same depth) but that is kind of normal since we are fixing the behaviour. Don't really know how to have a patch that will pass the test without introducing a regression. The "follow" previously added was defaulted to false, so it wasn't introducing a regression, but seems to be consensus that we'd better fix the bug instead of introducing a configuration.

What's the policy for core patch introducing a regression ?

DamienMcKenna’s picture

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.

acrosman’s picture

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.

awolfey’s picture

#64 is working for me. Thanks.

Adrian83’s picture

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

kiwad’s picture

Added an issue for the test that need's to be re-written :

#2745003: Rewrite test ConfigLevelDepth

kiwad’s picture

DamienMcKenna’s picture

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?

acrosman’s picture

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).

acrosman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 75: menu_block_follow_active-2631468-75.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

esolitos’s picture

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

aaronbauman’s picture

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.

aaronbauman’s picture

Status: Needs work » Needs review
aaronbauman’s picture

Status: Needs review » Needs work

test from #77 still failing

acrosman’s picture

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

aaronbauman’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
2.83 KB

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.

aaronbauman’s picture

Issue summary: View changes
ironsizide’s picture

Status: Needs review » Reviewed & tested by the community

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.

acrosman’s picture

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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

xjm’s picture

+++ 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.

aaronbauman’s picture

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.

acrosman’s picture

@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?

aaronbauman’s picture

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

DamienMcKenna’s picture

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)

Status: Needs review » Needs work

The last submitted patch, 93: drupal-n2631468-93.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

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

The last submitted patch, 93: drupal-n2631468-93-TEST-ONLY.patch, failed testing.

DamienMcKenna’s picture

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

mikewink’s picture

The patch from #95 works for me very well in Drupal 8.2.4. I have not tested the test part of the patch though.

MrPeanut’s picture

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!

AndersNielsen’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

StevenGFX’s picture

Issue tags: +FLDC17

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
    
StevenGFX’s picture

Applied patch in #95. Menu displays correctly.

-- Event 1
--- About Event 1
--- Subscribe to Event 1
Ky1eT’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs work

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.

DamienMcKenna’s picture

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.

aaronbauman’s picture

Assigned: Unassigned » aaronbauman

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.

aaronbauman’s picture

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.

DamienMcKenna’s picture

@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.

eigentor’s picture

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