Add the option in system menu block to "Expand all items in this tree"

NW for tests.

Do we need an upgrade path to update the config of existing blocks? I think yes.

Do we need an upgrade path to update the config of existing blocks? I think yes.

Do we need an upgrade path to update the config of existing blocks? I think yes.

Yes, we should set the default value to FALSE for all those blocks.

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

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

We are missing to change \Drupal\system\Plugin\Block\SystemMenuBlock::defaultConfiguration

If I understand you correct this should already be possible if you flag every menu item which has children as 'expanded'.

I also ran into this problem (documented here) and the children got only rendered if they were part of the active trail. But my main nav. works as hover-menu and needs to get rendered always with all children (only first two levels). I also opened an issue which I'm not sure if it is exactly the same. What do you think?

In general it would definitely be more intuitive and usable if you could define this on the block instead of the menu item which makes the menu itself more flexible. Because there could be several menu block instances of the same menu with different settings, e.g. one expanded and one only toplevel items.

I see, if this would be possible and the block configuration includes those menu settings (I remember menu_block module back in D7 had those abilities) I could also solve my issue posted.

Nice work @Sam152

Yes, we should set the default value to FALSE for all those blocks.

We need to update the schema of all existing SystemMenuBlock instances as well.
And

We are missing to change \Drupal\system\Plugin\Block\SystemMenuBlock::defaultConfiguration

this is still not addressed.

Does this really need a database update? Won't the default value be enough in this context?

Yes, whenever we add a new key to schema we have to update the existing active config schema so that everything remains in sync.

fixing tests.

N/W for update test.

Added empty update hook.

adding removed tag(accidently removed)

Setting to N/W for update & update path test.

unassiging from sam as he's not working anymore on it.
Waiting for jibran to provide any example for update hook to update only those menu block plugins.

we have now upgrade path and upgrade path test.
Please also give credit to jibran for reviewing and helping in pushing this issue.

correct interdiff

Drafted CR b/c we are adding new option, CR needs updation https://www.drupal.org/node/2669550

Looks perfect.

Looks random indeed.

+++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
@@ -0,0 +1,57 @@
+    // Check that system_menu_block does not have settings expand_all_items.
+    $blocks = \Drupal::entityTypeManager()->getStorage('block')->loadMultiple();
+    /** @var $blocks \Drupal\block\BlockInterface[] */
+    foreach ($blocks as $block) {
+      if (strpos($block->getPluginId(), 'system_menu_block:') !== false) {
+        $this->assertFalse(array_key_exists('expand_all_items', $block->get('settings')), 'Expand all items settings does not exist');
+      }
+    }
+

Not sure if this is really relevant to check

#39, RT
#40,
Don't wanna do the check for all blocks, only need to take care of plugin system_menu_block
That's check was for taking care of that.

Edit : That's check was needed for checking that 8.0.x does not have the expand_all_items key in schema.

Not setting back to RTBC per #35, leaving for swentel, may be he has more thoughts

Its working but it was hard to understand the issue/feature.

Finally I figure it out and trying to explain with screenshots below to help others to understand.

Added screenshot to Issue Description

Back to RTBC as per #35

1. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
   @@ -138,6 +138,16 @@ public function addExpandedParents(array $parents) {
   +  public function emptyExpandedParents() {
   +    $this->expandedParents = array();
   +    return $this;
   +  }
   
   +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -151,6 +159,9 @@ public function build() {
   +    if (!empty($this->configuration['expand_all_items'])) {
   +      $parameters->emptyExpandedParents();
   +    }
   

   I'm afraid this is entirely the wrong way to do this.

   \Drupal\system\Plugin\Block\SystemMenuBlock::build() generates a MenuTreeParameters object by doing:

   $parameters = $this->menuTree->getCurrentRouteMenuTreeParameters($menu_name);
   

   If we look at that method, we see this:

         // We want links in the active trail to be expanded.
         ->addExpandedParents($active_trail)
         // We marked the links in the active trail to be expanded, but we also
         // want their descendants that have the "expanded" flag enabled to be
         // expanded.
         ->addExpandedParents($this->treeStorage->getExpanded($menu_name, $active_trail));
   

   So this patch first lets \Drupal\Core\Menu\MenuLinkTree::getCurrentRouteMenuTreeParameters() spend a lot of time retrieving menu items marked as "expanded", by querying the DB. And then this patch overwrites all that work by calling the new (poorly named) emptyExpandedParents() method.

   That's just backwards.

   This should either improve \Drupal\Core\Menu\MenuLinkTree::getCurrentRouteMenuTreeParameters() itself or improve SystemMenuBlock's usage of that. We don't need emptyExpandedParents() at all.

2. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -150,6 +150,28 @@ function testMenu() {
   +  public function testExpandAllItems() {
   

   This is an integration test. We also need a unit test for this in MenuLinkTreeTest.

This should address the issues in #45

- If block is configured to display an expanded menu, it creates the MenuTreeParameters itself with only the active trail, and not ExpandedParents. If not expanded, parameter creation is delegated to MenuLinkTree.
- A unit test is added to ensure that the full menu is displayed at any level, when both the active trail is empty or is set.

I mistakenly made the last patch against 8.1.x. Here's a re-roll against 8.2.x.

Just some few nitpicks, the patch itself really a) Makes sense, there are IMHO quite some usecases for it and b) functionality wise, its looking great.

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -107,6 +108,13 @@ public function blockForm($form, FormStateInterface $form_state) {
   +      '#description' => $this->t('Expand all items of this menu tree to show all items including the active trail.'),
   

   Is it just me or is this description not really helpful? I think the title itself tells you enough

2. +++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,57 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\system\Tests\Update\MenuBlockPostUpdateTest.
   + */
   

   Nitpick, this is no longer needed

3. +++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,57 @@
   +/**
   + * Tests hook_post_update().
   + *
   ...
   +  public function testPostUpdateMenuBlockFields() {
   

   I think it would be helpful to post to the actual instance: system_post_update_add_expand_all_items_key_in_system_menu_bloc

4. +++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,57 @@
   +  protected function setDatabaseDumpFiles() {
   +    $this->databaseDumpFiles = [
   +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.bare.standard.php.gz',
   +    ];
   +  }
   

   I just quickly ensured that standard profile actually ships with blocks by default:

     'data' => 'a:12:{s:4:"uuid";s:36:"56c57064-ab55-4618-b004-4f886c7a1535";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:3:{s:6:"config";a:1:{i:0;s:19:"system.menu.account";}s:6:"module";a:1:{i:0;s:6:"system";}s:5:"theme";a:1:{i:0;s:6:"bartik";}}s:2:"id";s:19:"bartik_account_menu";s:5:"theme";s:6:"bartik";s:6:"region";s:14:"secondary_menu";s:6:"weight";i:0;s:8:"provider";N;s:6:"plugin";s:25:"system_menu_block:account";s:8:"settings";a:7:{s:2:"id";s:25:"system_menu_block:account";s:5:"label";s:17:"User account menu";s:8:"provider";s:6:"system";s:13:"label_display";s:1:"0";s:5:"cache";a:1:{s:7:"max_age";i:-1;}s:5:"level";i:1;s:5:"depth";i:1;}s:10:"visibility";a:0:{}}',
     'data' => 'a:12:{s:4:"uuid";s:36:"cec8b255-8844-438e-8317-2346e2d423db";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:3:{s:6:"config";a:1:{i:0;s:18:"system.menu.footer";}s:6:"module";a:1:{i:0;s:6:"system";}s:5:"theme";a:1:{i:0;s:6:"bartik";}}s:2:"id";s:13:"bartik_footer";s:5:"theme";s:6:"bartik";s:6:"region";s:12:"footer_fifth";s:6:"weight";i:0;s:8:"provider";N;s:6:"plugin";s:24:"system_menu_block:footer";s:8:"settings";a:7:{s:2:"id";s:24:"system_menu_block:footer";s:5:"label";s:11:"Footer menu";s:8:"provider";s:6:"system";s:13:"label_display";s:1:"0";s:5:"cache";a:1:{s:7:"max_age";i:-1;}s:5:"level";i:1;s:5:"depth";i:0;}s:10:"visibility";a:0:{}}',
     'data' => 'a:12:{s:4:"uuid";s:36:"fef5d925-68fa-47da-8f07-7a32cf588d14";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:3:{s:6:"config";a:1:{i:0;s:16:"system.menu.main";}s:6:"module";a:1:{i:0;s:6:"system";}s:5:"theme";a:1:{i:0;s:6:"bartik";}}s:2:"id";s:16:"bartik_main_menu";s:5:"theme";s:6:"bartik";s:6:"region";s:12:"primary_menu";s:6:"weight";i:0;s:8:"provider";N;s:6:"plugin";s:22:"system_menu_block:main";s:8:"settings";a:7:{s:2:"id";s:22:"system_menu_block:main";s:5:"label";s:15:"Main navigation";s:8:"provider";s:6:"system";s:13:"label_display";s:1:"0";s:5:"cache";a:1:{s:7:"max_age";i:-1;}s:5:"level";i:1;s:5:"depth";i:1;}s:10:"visibility";a:0:{}}',
     'data' => 'a:12:{s:4:"uuid";s:36:"c2bf0078-b5b6-4715-b431-4938186343f8";s:8:"langcode";s:2:"en";s:6:"status";b:1;s:12:"dependencies";a:3:{s:6:"config";a:1:{i:0;s:17:"system.menu.tools";}s:6:"module";a:1:{i:0;s:6:"system";}s:5:"theme";a:1:{i:0;s:6:"bartik";}}s:2:"id";s:12:"bartik_tools";s:5:"theme";s:6:"bartik";s:6:"region";s:13:"sidebar_first";s:6:"weight";i:0;s:8:"provider";N;s:6:"plugin";s:23:"system_menu_block:tools";s:8:"settings";a:7:{s:2:"id";s:23:"system_menu_block:tools";s:5:"label";s:5:"Tools";s:8:"provider";s:6:"system";s:13:"label_display";s:7:"visible";s:5:"cache";a:1:{s:7:"max_age";i:-1;}s:5:"level";i:1;s:5:"depth";i:0;}s:10:"visibility";a:0:{}}',
   

5. +++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,57 @@
   +      if (strpos($block->getPluginId(), 'system_menu_block:') !== false) {
   ...
   +      if (strpos($block->getPluginId(), 'system_menu_block:') !== false) {
   
   +++ b/core/modules/system/system.post_update.php
   @@ -41,3 +41,28 @@ function system_post_update_recalculate_configuration_entity_dependencies(&$sand
   +    if (strpos($block->getPluginId(), 'system_menu_block:') !== false) {
   

   nitpick: let's use uppercase FALSE

6. +++ b/core/modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,57 @@
   +        $this->assertTRUE(array_key_exists('expand_all_items', $block->get('settings')), 'Expand all items settings exist');
   

   the actual function is assertTrue ... PHP is simply caseInSenSItive. Should we check that the value is FALSE by default, as well?

7. +++ b/core/modules/system/system.post_update.php
   @@ -41,3 +41,28 @@ function system_post_update_recalculate_configuration_entity_dependencies(&$sand
   +/**
   + * @addtogroup updates-8.0.x-to-8.1.x
   + * @{
   ...
   +/**
   + * @} End of "addtogroup updates-8.0.x-to-8.1.x".
   + */
   

   Note: We are updating from 8.1 to 8.2 actually

8. +++ b/core/modules/system/tests/src/Kernel/Block/SystemMenuBlockTest.php
   @@ -286,6 +286,82 @@ public function testConfigLevelDepth() {
   +  /**
   +   * Tests the config expanded option.
   +   */
   +  public function testConfigExpanded() {
   

   <3 I love this kernel test!

9. +++ b/core/modules/system/tests/src/Kernel/Block/SystemMenuBlockTest.php
   @@ -286,6 +286,82 @@ public function testConfigLevelDepth() {
   +      $this->assertIdentical($expectations[$id], $this->convertBuiltMenuToIdTree($items), format_string('Menu block %id with no active trail renders the expected tree.', ['%id' => $id]));
   ...
   +      $this->assertIdentical($expectations[$id], $this->convertBuiltMenuToIdTree($items), format_string('Menu block %id with an active trail renders the expected tree.', ['%id' => $id]));
   

   Let's use sprintf or just "Menu block $id". Format_string is deprecated and shouldn't be used in tests

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -107,6 +108,13 @@ public function blockForm($form, FormStateInterface $form_state) {
+      '#description' => $this->t('Expand all items of this menu tree to show all items including the active trail.'),

yep, makes no sense and active trail confusing newcomers

I removed the description on the 'Expand All Items' checkbox, and also moved it above the 'Menu Levels' fieldset so that it appears right after the 'Display title' checkbox.

Sorry for being a litte bit annoying :) What do you think about my new suggestions for the label?

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -74,38 +75,44 @@ public static function create(ContainerInterface $container, array $configuration
   +      '#title' => $this->t('Expand all items in this tree'),
   

   This is the first occurrence of the word "tree" on this page, so I'm wondering whether "Expand all menu items" would be easier to understand

2. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -74,38 +75,44 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $form['menu_levels'] = [
          '#type' => 'details',
          '#title' => $this->t('Menu levels'),
          // Open if not set to defaults.
          '#open' => $defaults['level'] !== $config['level'] || $defaults['depth'] !== $config['depth'],
          '#process' => [[get_class(), 'processMenuLevelParents']],
   -    );
   +    ];
    
        $options = range(0, $this->menuTree->maxDepth());
        unset($options[0]);
    
   -    $form['menu_levels']['level'] = array(
   +    $form['menu_levels']['level'] = [
          '#type' => 'select',
          '#title' => $this->t('Initial menu level'),
          '#default_value' => $config['level'],
          '#options' => $options,
          '#description' => $this->t('The menu will only be visible if the menu item for the current page is at or below the selected starting level. Select level 1 to always keep this menu visible.'),
          '#required' => TRUE,
   -    );
   +    ];
    
        $options[0] = $this->t('Unlimited');
    
   -    $form['menu_levels']['depth'] = array(
   +    $form['menu_levels']['depth'] = [
          '#type' => 'select',
          '#title' => $this->t('Maximum number of menu levels to display'),
          '#default_value' => $config['depth'],
          '#options' => $options,
          '#description' => $this->t('The maximum number of menu levels to show, starting from the initial menu level. For example: with an initial level 2 and a maximum number of 3, menu levels 2, 3 and 4 can be displayed.'),
          '#required' => TRUE,
   -    );
   +    ];
   

   Let's skip this CS only changes. They can be fixed as part of #2571965: [meta] Fix PHP coding standards in core

3. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -133,7 +141,16 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +      $active_trail = $this->menuActiveTrail->getActiveTrailIds($menu_name);
   +
   +      $parameters = new MenuTreeParameters();
   +      $parameters->setActiveTrail($active_trail);
   

   Small tip: Always try to define variables in the smallest distance where they are needed. In this case it would be a little bit more readable to first define $parameters and then $active_trail

Makes sense to me.

Updated label and remove code style changes.

@gapple
Can you post interdiffs so people can follow up what you did?

Thanks a lot for addressing those points!

Moving back to RTBC since the re-test was successful.

uh, not sure what happened there. Test page still shows passes...

Why are we adding this functionality to the menu block and not to the menu itself?

I guess the answer to my question in #64 might be that menu blocks already allow configuring the menu levels in a way that the menu system itself doesn't (which as far as I can see only allows specifying that individual links be expanded or not), so it's not introducing a new pattern to configure the display of the menu on its block instance only.

Note that the screenshots have a redundant description that I was going to NW the patch for, but that is actually removed already in the current patch.

That said, though, I think this should go inside the "Menu levels" fieldset maybe?

@xjm There is an option for that actually. But maybe you might have two instances of that menu in a block where one is expanded, the other is not.

I'd also recommend updated screenshots, since the current screenshots don't reflect the current patch. That way it's easier to ask for others to review the proposed change. Thanks!

@xjm There is an option for that actually. But maybe you might have two instances of that menu in a block where one is expanded, the other is not.

Where is the option? I wasn't able to find it except on individual menu items. I was worried about things becoming confusing and people not seeing the results of their attempted changes, etc., but if the menu depth configuration is only on the block (with core anyway) then I convinced myself it makes sense for this option to be on the block as well.

@xjm yes, you're right, it's on the individual item only, sorry for any confusion :)

Is it safe to use #56 for now because the latest failed (Drupal 8.2.2):

patching file modules/system/src/Tests/Update/MenuBlockPostUpdateTest.php

patching file modules/system/system.post_update.php

Hunk #1 FAILED at 57.

1 out of 1 hunk FAILED -- saving rejects to file modules/system/system.post_update.php.rej


patching file modules/system/tests/src/Kernel/Block/SystemMenuBlockTest.php

patching file profiles/minimal/config/install/block.block.stark_admin.yml

patching file profiles/minimal/config/install/block.block.stark_tools.yml

patching file profiles/standard/config/install/block.block.bartik_account_menu.yml

patching file profiles/standard/config/install/block.block.bartik_footer.yml

patching file profiles/standard/config/install/block.block.bartik_main_menu.yml

patching file profiles/standard/config/install/block.block.bartik_tools.yml

   Could not apply patch! Skipping.

Hi,

I have correct the patch in the #56 to work with 8.1 (just the path of SystemMenuBlockTest.php file).

in attachment.

The failed test from #73 shouldn't have affected the issue status

Updated summary

I'm wondering whether we should get the UX team involved. Is the label for itself self descriptive enough?

From the issue summary:

This isn't ideal for things like primary navigation which require subtrees to be shown regardless of which item the user is viewing.

Is this for use cases like having all items available for making (mega menu) dropdowns in a main nav? So it's for "things like *some* primary navigation systems like mega menu dropdowns which require etc…" ? Would be useful to clarify the “Why” for this change a bit more.

Initially I had the same question as xjm in #64 about where this feature lives but that's ok (in a consistency sense, the current situation does not seem ideal but out of scope here)

As for the label, would it help to mention “always”?

“Always expand all menu items”

Is this for use cases like having all items available for making (mega menu) dropdowns in a main nav? So it's for "things like *some* primary navigation systems like mega menu dropdowns which require etc…" ? Would be useful to clarify the “Why” for this change a bit more.

When talking about possible use cases, one should note, that the patch adds an option to always expand all menu items per menu block, not per menu. So you can combine this with the existing options for initial menu level and maximum levels to display. This greatly increases the usefulness of this option. To give you a rough idea, we're currently using the patch for the following use cases on several different sites:

• Dropdowns (not necessarily mega dropdowns)
• Footer navigation (if the first two levels should be visible at all times without the need for editors to check the expand all option for each parent item)
• Sitemaps (all or the first x levels of primary navigation should be shown, regardless of menu item settings)
• Sidebar navigation, if several levels should be visible at all times (compared to the other cases, this seems to be somewhat rare)

You could possibly come up with more use cases, if you put some thought into it.

Initially I had the same question as xjm in #64 about where this feature lives

If this setting was a per menu setting instead of a per menu block setting, a lot of our use cases wouldn't work anymore and the feature would be much less useful, if at all. We really need the flexibility to show the same menu with menu item settings honoured in one place and menu item settings overridden by this option in other places on the same site. But that's just us. Can't speak for the rest of the community of course ;).

For those who need this on 8.2.x I backported the patch which was not applying anymore.

Yes, this would be a menu block setting, not a menu setting.

Re-rolling the patch for 8.4.x

reverting also #2856625: Remove unused menu active trail service from SystemMenuBlock

We use this feature for the main navigation because we have a second level navigation block in the sidebar. And because in the sidebar we only want to have the menu tree of the active menu path we need to mark the menu items "not expand all". As such this should be a block configuration and not a menu configuration. Or at least be "overridable" on a per block basis.

The current custom workaround we use is to create a new "expanded_menu" block plugin.

I think adding a second location to configure whether menu items are expanded or not is only leading to confusion. We have an option to set a single menu item as expanded on the edit page for that item, and we need to keep that option anyway.
At the moment it is unclear how this tick-box for the instance of a menu relates to the general menu item settings.
And how does the user know that are two places for setting whether a menu item is expanded or not?

There are also cases where on one level the menu items should be expanded, but on the next one they shouldn't. Or where on one level some items are extended and others are not. This tick-box however seems to be an all-or-nothing solution.

There is another issue that's trying to reduce the amount of clicking needed to configure the display of a menu by adding a tick-box for Expanded on the Edit page of the menu instead of only on the menu item page. That will already improve the usability, without producing a potentially confusing duplication of configuration. #2802837: Improve discoverability of menu "expanded" option

This patch is compatible with the 8.3.0 release and can be used with Composer. It is a reroll of the patch in #77, but this last works on the 8.3x-dev version but not with the current release.

Hey showing the whole tree regardless of active path is an important feature for any type of front-end menu manipulation.

The Menu items actually are one correct place to set this preference, but the output menu should have the option to override that preference down to the depth limit configured.

If folks can't wait for this to get into core, than the contrib menu_block module (https://www.drupal.org/project/menu_block) provides this in drupal 8. Still, this should definitely be a core option.

I'm not quite sure what the state of this feature request is, but if it's to be taken up then UI text need to be changed:

• on the form for each menu item is a tick box whether to display this item or not: That needs an explanation that this setting can be overridden when the menu is placed as a block, because otherwise sitebuilders and themers will get completely confused.
• it's additional functionality that need to be added to the Uses section of the hook_help text

.

@ifrik Adding explanation seems sensible. Just a side-note: This feature is not about whether or not a menu item is active/visible. That setting remains unaffected.

Here's an example of why the proposed feature is very useful, as it allows configurations like this setup:

1. Top 1
   • Sub 1
   • Sub 2
2. Top 2
   • Sub 3
3. Top 3
   • Sub 4
   • Sub 5

Goals:

1. The main menu shall display the top two levels for the page's main navigation
2. The main menu shall follow the active trail for secondary navigation, starting at the top level as well.

To achieve the first using the default behavior, we must mark all "Top" items as expanded. If we visit the page of "Top 1", the secondary navigation block will display all 5 "Sub" items, since their parents are all expanded. With the proposed patch, the menu items no longer need to be expanded, and instead the primary block can be set to "Expand all". The secondary block will then only display the three "Top" items and "Sub 1" + "Sub 2".

I'm just worried that users are confused if they don't know that there are two different places to set something and wonder why this one menu item is expanded even though they clearly said it shouldn't....
So we need a note on the edit form of the individual menu item, that the "Expanded" option can be overwritten as a whole when a menu is placed as a block.
And a second note on the block form, that this overwrites the individual menu item configuration.
And a new Use in the Help text: Expanding all menu items in a block.

Setting this to "needs work" to add the UI texts

Re-rolled the patch in #85 so it applies to the latest version of 8.4.x.

This is broken in 8.5.x since #2856625: Remove unused menu active trail service from SystemMenuBlock https://www.drupal.org/node/2912736

This persists in drupal-8.5.0-rc1.

What files should I patch?

I've do the changes manually in the database:

SELECT * FROM dbdrupal.menu_tree WHERE menu_name='main';
UPDATE dbdrupal.menu_tree SET expanded='1' WHERE menu_name='main';

This persists in drupal-8.5.0-rc1.

What files should I patch?
I've do the changes manually in the database:

SELECT * FROM dbdrupal.menu_tree WHERE menu_name='main';
UPDATE dbdrupal.menu_tree SET expanded='1' WHERE menu_name='main';

Attached patch for 8.5 to bring back MenuActiveTrail.

This probably needs to be rethought if we don't want MenuActiveTrail in system blocks?

Added some tests for #101.

Maybe the file name has to change from core-84x-... to core-85x- or core-86x-... :)

@dubcanada did you address and fix the problem commented in #98?

Re-rolled the patch against the current version of the 8.5.x branch. In https://cgit.drupalcode.org/drupal/commit/core/modules/menu_ui/src/Tests... the tests were (re)moved so the patch from #101 does not apply anymore in 8.5.4.

Please note:

• I've just removed the parts of the patch from #101 that did not apply anymore.
• New tests might need to be added.
• If #101 did not address the issue mentioned in #98 this patch doesn't do that too.

Because of the above I'll leave the issue status to NW.

Quick review:

1. +++ b/core/modules/system/system.post_update.php
   @@ -178,3 +178,18 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +  $blocks = $block_storage->loadMultiple();
   +  /** @var \Drupal\block\BlockInterface[] $blocks */
   +  foreach ($blocks as $block) {
   

   nit: the type hint should be above the variable assignment.

2. +++ b/core/profiles/standard/config/install/block.block.bartik_tools.yml
   --- a/core/tests/fixtures/config_install/multilingual.tar.gz
   +++ b/core/tests/fixtures/config_install/multilingual.tar.gz
   

   Why is this changed?

+1 for this feature. Passes manual testing.

Screenshot of menu with sublinks without the box checked:

Screenshot of menu with the box checked:

Screenshot of the UI with the checkbox:

+++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
@@ -130,7 +130,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      '#description' => $this->t('If selected and this menu link has children, the menu will always appear expanded. This option may be overridden for the entire menu tree when placing a menu block.'),

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -348,7 +348,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDescription(t('If selected and this menu link has children, the menu will always appear expanded. This option may be overridden for the entire menu tree when placing a menu block.'))

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -98,6 +111,13 @@ public function blockForm($form, FormStateInterface $form_state) {
+      '#title' => $this->t('Expand all menu items'),
...
+      '#description' => $this->t('Override the option found on each menu link used for expanding children and instead display the whole menu tree as expanded.'),

These titles/descriptions make sense to me and describe the functionality and options provided well.

Overall thorough patch and testing, double checked that all the existing block config was also updated too :)

1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -864,6 +864,41 @@ public function testMenuParentsJsAccess() {
   +    $this->assertSession()->linkExists($parent->getTitle());
   +    $this->assertSession()->linkExists($child_1->getTitle());
   +    $this->assertSession()->linkExists($child_2->getTitle());
   

   It would be good to test that the children aren't shown if the expand_all_items is then set to FALSE.

2. +++ b/core/modules/system/system.post_update.php
   @@ -178,3 +178,18 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +  $block_storage = \Drupal::entityTypeManager()->getStorage('block');
   +  /** @var \Drupal\block\BlockInterface[] $blocks */
   +  $blocks = $block_storage->loadMultiple();
   +  foreach ($blocks as $block) {
   +    if (strpos($block->getPluginId(), 'system_menu_block:') === 0) {
   +      $block->set('settings.expand_all_items', FALSE);
   +      $block->save();
   +    }
   +  }
   

   This should use the \Drupal\Core\Config\Entity\ConfigEntityUpdater for batched updates. Some sites have a lot of blocks.

Nice, feedback from #125 is fixed, back to RTBC!

I manually tested the initial level selection in conjunction with the new setting and everything worked as expected.

Issue credit:

• I've credited @bircher because the reroll appeared to be moving the patch forward on to the latest dev branch. @esolitos and @gmario don't get credit because they were adding patches that backport to previous versions and that tends to make issues very confusing.
• @dawehner, @jibran, @xjm, @ifrik, @swentel, @kim.pepper, @Wim Leers, @4aficiona2, @yoroy, @alexpott, @ckaotik, @andypost and @FeyP all made constructive code review or UI review comments that have helped land the patch.

Committed f335587 and pushed to 8.7.x. Thanks!

1. +++ b/core/modules/system/system.post_update.php
   @@ -178,3 +178,16 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'block', function ($block) {
   +    if (strpos($block->getPluginId(), 'system_menu_block:') === 0) {
   +      $block->set('settings.expand_all_items', FALSE);
   +      return TRUE;
   +    }
   +    return FALSE;
   +  });
   

   This can be even simpler!

2. +++ b/core/modules/system/tests/src/Functional/Update/MenuBlockPostUpdateTest.php
   @@ -0,0 +1,38 @@
   +    $this->assertArrayHasKey('expand_all_items', Block::load('bartik_account_menu')->get('settings'));
   

   We should also be asserting on the value.

diff --git a/core/modules/system/system.post_update.php b/core/modules/system/system.post_update.php
index 8e73763c57..98ad684dec 100644
--- a/core/modules/system/system.post_update.php
+++ b/core/modules/system/system.post_update.php
@@ -184,10 +184,6 @@ function system_post_update_states_clear_cache() {
  */
 function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sandbox = NULL) {
   \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'block', function ($block) {
-    if (strpos($block->getPluginId(), 'system_menu_block:') === 0) {
-      $block->set('settings.expand_all_items', FALSE);
-      return TRUE;
-    }
-    return FALSE;
+    return strpos($block->getPluginId(), 'system_menu_block:') === 0;
   });
 }
diff --git a/core/modules/system/tests/src/Functional/Update/MenuBlockPostUpdateTest.php b/core/modules/system/tests/src/Functional/Update/MenuBlockPostUpdateTest.php
index c3447420bf..77ce66dcf4 100644
--- a/core/modules/system/tests/src/Functional/Update/MenuBlockPostUpdateTest.php
+++ b/core/modules/system/tests/src/Functional/Update/MenuBlockPostUpdateTest.php
@@ -32,7 +32,9 @@ protected function setDatabaseDumpFiles() {
   public function testPostUpdateMenuBlockFields() {
     $this->assertArrayNotHasKey('expand_all_items', Block::load('bartik_account_menu')->get('settings'));
     $this->runUpdates();
-    $this->assertArrayHasKey('expand_all_items', Block::load('bartik_account_menu')->get('settings'));
+    $settings = Block::load('bartik_account_menu')->get('settings');
+    $this->assertArrayHasKey('expand_all_items', $settings);
+    $this->assertFalse($settings['expand_all_items']);
   }
 
 }

Fixed on commit. I ran the MenuBlockPostUpdateTest locally first.

#94 was addressed in #113. Adjusting issue summary to reflect that.

Great to see this is now done! :)

Oops forgot to add my changes that I outlined in #128. Pushed a quick follow-up.

I probably made a mistake, but menu_block recent change changed the Dependency Injection and constructor ended in there, this change broke the constructor. Is there a best practice to avoid accidentally doing this? Guessing "composition" ...

Or should I make a follow-up to make this argument optional with fallback to \Drupal::service()?

#2968049: Dependency Injection issue

Created a follow-up after discussing with @Berdir, @andypost and @gapple on slack. #3018863: Make SystemMenuBlock's new constructor argument dependency optional

This issue broke the upgrade test for DER 1.x to 2.x https://www.drupal.org/pift-ci-job/1147416.

I'm marking it NW so someone can confirm and then we can create the follow-up and mark this fixed.

Created #3020718: system_post_update_add_expand_all_items_key_in_system_menu_block fails if blocks module is disabled because attached patch fixed the DER upgrade test fail.