Add options to system menu block so primary and secondary menus can be blocks rather than variables

+1 for adding this functionality to Drupal core! Linking Jacine's post to this issue as well:

http://drupal.org/node/489130

"From a theming perspective, it's incredibly hard to work with menu output. When trying to do drop-down menus, or anything the requires running your menus through menu_tree() you are left with zero context. This can be incredibly frustrating and time consuming when you don't know how to get around it.

In my experience, 9 times out of 10, using the menu_block resolves this, because it adds descriptive classes and allows enough options (like starting level, depth, etc) so that you get what you want the first time around and don't have to screw around with the theme functions.

This has been a pain point for themers for a while, and menu_block solves it, so I'm hoping it will be considered for core."

subscribing

DamZ quickly mentioned to me yesterday in the bar, and it does seem reasonable. Hitting the virtual subscribe button.

This doesn't necessarily have to wait on the Menu Blocks contrib module though, as moving the functionality into core would mean moving parts of it in rather then the whole thing (would quite possibly need a re-write anyway). I'm moving this back to active ;-) .

#620618: Optimize menu tree building and use it for toolbar contains a minimally-but-not-public-API-changing patch that accomplishes the menu system functionality.

We can keep this issue to work on the UI feature (menu.module). That, however, is probably a D8 task. (Although I'd really love to see it in D7 already.)

If this doesn't make it to Drupal 8, I am going to cry my eyes out.

+1 to what Jacine said. Also: subscribing.

Anyone willing to take this on? Pretty please?

This is going to require a new UI I assume. dtx = "drupal themer experience"

Subbing.

Better title.

See also #430886: Make all blocks fieldable entities.

D7 api updating for this module should be a part of gsoc 2012. I will post this as gsoc 2012 idea.

I bumped #503810: Convert primary and secondary menus to blocks which seems a prerequisite for this issue. Correct?

Probably not, because they should become menu blocks -- so we need this first.

FYI: It's possible that #1869476-19: Convert global menus (primary links, secondary links) into blocks might duplicate this issue.

This issue is still on my Top 10 list for D8. Progress was only disrupted, because the required code here would have had to duplicate the new block plugins with an one-off implementation.

@JohnAlbin: Any chance we can make this happen for D8? :)

It doesn't entirely duplicate this issue; it does, however, reduce this issue to "add a couple more options to that block", which would be a win all-around.

Would be so nice to make the site builder task of building out navigation a bit more obvious!

I started work on this in #1869476: Convert global menus (primary links, secondary links) into blocks, but I'm going to try to move the patching work here.

I'm trying to duplicate the approach we used in #1053648: Convert site elements (site name, slogan, site logo) into blocks, which was to first create the ability to add a new type of block, then use that new type of block to replace the corresponding variables in the page template. Breaking it up should help make this more manageable.

In order to replace the primary and secondary menus in the page templates, what we really need is the ability to add a menu with just one specific level. The top level for primary links and the next one down for secondary links.

The good thing is that core already has the ability to add a block for any of the defined menus. So what this patch does is to tweak SystemMenuBlock to add configuration options so that when you add any of the defined menus, you can select the starting level and how many levels down you want to go.

The default is to start at the top level and have unlimited levels, so that placing a menu by default still shows the full tree.

However, for primary links you could select just the top level and allow a depth of one, which would be the equivalent of the primary_links variables. Secondary links would be the second level and a depth of one.

I used the port of menu_block to d8 that was being worked on as a basis for the configuration options. That's here: https://github.com/larowlan/menu_block/

Unfortunately this isn't working quite right. You can select the starting level and the depth, but it doesn't actually affect anything. Of course that's less than useful. Hopefully somebody can point me in the direction of what I'm doing wrong here.

If we get this working, get test coverage, etc., then we could get this in, and then have follow-up issues to add more of menu_block's features in.

It looks like the parameters you're passing to menu_build_tree() aren't quite right:

- they should not have an initial #
- I think it looks like you need to specify 'active_trail'

I'm not sure how you get the thing you need to pass to active_trail though. I'm wondering whether you should be using menu_tree_all_data() instead, but I haven't time to delve further into that.

Another thing though: shouldn't there be a new block type for this too?

Looks like this is going to get disrupted by #2207893: Convert menu tree building to a service..

@joachim: Yes, once that's in, that will definitely require a number of changes. I don't know how long it will take to get that committed, though. Hopefully soon?

Hoping we can still move this patch forward in the meantime and settle any conceptual issues about what configuration options we should put in at first, for example. I think min_level and max_level are the minimum for what we need.

I think some of the logic that's needed is in https://api.drupal.org/api/drupal/includes!menu.inc/function/menu_naviga... -- but that returns actual links, rather than a menu tree. I'm not sure it does max-level though.

So I think a good plan of action would be to base code on the logic in menu_navigation_links(), but which instead returns a tree structure.

Of course, once the blocks work, then the current menu links in https://api.drupal.org/api/drupal/includes!theme.inc/function/template_p... should be replaced with menu blocks, and menu_navigation_links() can be deprecated.

Here's a patch that moves things forwards a little -- I've fixed the starting level.

The depth I am less sure of -- what would be the expected output for different depths? I am just getting one level of links in my admin menu block regardless!

I played around with this some more and swapped out some of the logic used in the menu_block module. I think it's just a variation of what's in #27. This does seem to work as far as I understand this.

The maximum depth thing is kind of weird. As far as I can tell, even if you set this to display a maximum of five levels, you'd still only see one level down by default. If we had the expanded option, that might have an effect, but without that, it's a bit irrelevant right now. Maybe I'm understanding this wrong though.

We'll have to rebuild this anyhow once #2207893: Convert menu tree building to a service. gets in, but hopefully this moves us forward.

I do think the way this works is probably the minimum we'd need to replace the primary_links and secondary_links.

There are definitely more features in menu_block. I think the plan was to get in these key features in first, then add the other ones in later if we're able to do so.

Changed a default. May or may not help fix some of the test errors.

This adds in the option to force the menu all children as expanded. I was hoping this would make the depth setting actually show something. The expanded setting works, but I don't think that's resulting in the depth being limited as expected.

Here's how the depth setting is currently working.

As an example, I created a menu four levels deep and a block that starts at the second level, but only has a depth of one.

If I am on the page at the top of that active trail, it shows the second level link, but not the third level link.

If I am on the page at the second level in that active trail, it shows the second level link (the current page) and the the third level child link.

If I go down to that third level page in the active trail, it still shows the second level link (the parent page) and the third level link (the current page), but not the fourth level link that's a child.

I think that makes sense: that is starting at the second level, it shows a depth of one, although only if on that second level.

If I check expanded, then if I am on any page in that four-level deep active trail, it shows all four levels in the block: it ignores the depth setting. That seems incorrect.

menu_block.module has some utility functions that I'm not using right now, so maybe setting those up is the key to getting this to work correctly. I'm not sure if those utility functions should go within the SystemMenuBlock plugin, or if they're of enough use to put into menu.inc (or into the menu tree building service, once that patch goes in).

Feedback welcome.

This patch needs to be rewritten. SystemMenuBlock.php has changed a lot.

Yes, on my list.

Larowlan had done a conversion of the Menu Block module to d8 that I was using as a model. I need to take a look at that again, and then see how that matches up with the conversion of the menu tree building to a service.

There are a number of helper functions that could make some of this work easier. The tricky part is figuring out where to put them if they don't already exist. If they may be of general use, maybe in the new service?

I'll try to get this back on my radar.

Larowlan had done a conversion of the Menu Block module to d8

It was a team effort with Kim.pepper and jibran

#1869476: Convert global menus (primary links, secondary links) into blocks is currently a critical task postponed on this issue. It might not stay that way (see that issue's recent comments), but while it is, raising this one accordingly.

Tagging beta target to match #1869476-142: Convert global menus (primary links, secondary links) into blocks.

I'm going to try to start tackling this later this week.

#40: lovely! I'll provide reviews :)

Please clear the steps to fix the issue

This might also be considered postponed on: #2256497: [meta] Menu Links - New Plan for the Homestretch

if there is nothing active happening since we will be changing how menus are fetched. In particular, could use feedback on method changes that would facilitate this change.

> use feedback on method changes that would facilitate this change

When I last tried working on the patch for this, I really struggled with finding the API functions to get the data I needed.

Basically, menu block needs to be able to get:

* 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

So, the patch as written now would provide #1 (top level) and #3 (subtree)

I think #2 might be handled as well - basically it's the subtree containing that it's parent with a relative max depth of 1. You probably actually do want this, since if the parent is not accessible or hidden, you won't show that level.

So would be fair to postpone so that you'd have those API changes?

FYI github.com/previousnext/menu_block is a recent 8,x port

Was out of town for a funeral last week. I double-triple promise I'll look at this again, possibly as soon as tonight. The trouble I ran into before was figuring out where to put some of the functions necessary to make things work. Now that menu tree is a service, maybe some of those functions go over there?

Thanks for the port, larowlan. Now that you have John Albin on your team, maybe the two of you can unlock the secrets of how to make this work in core!

Anyhow, I'll take another stab at it. And of course if anybody else wants to give it a go, more power to you.

I think the performance part of this is going to be handled by #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

There's no reason this can't go in as an API addition at any point including a minor release after 8.0.x (except for possibly the theme variables for primary/secondary links) - so downgrading to major.

#48: correct

I think this is also a feature request again then?

Quick note that I don't think this is quite a feature request. My understanding is that one of the reasons this is important is because it allows us to use blocks to create primary and secondary menus, rather than hard coding those as variables that are output in templates, and that this would improve caching and thus performance. I might be off base on that, but I do think that's the Origin Story for this issue.

So...

I finally found a few hours to work on this. I took a look at the work in the previous patch, and basically recreated the changes rather than doing a reroll, because there had been so many changes with the menu tree functions moving around. The bulk of this work is based on a port of the menu_block module to Drupal 8: https://github.com/larowlan/menu_block.

The bad news is that I am probably Doing This Wrong. The ported module has an elegant set of classes and interfaces and services, and I basically stuffed the necessary functions into the MenuTree class and MenuTree class interface. Maybe there's a better way to do this, but I guess putting these functions there makes them available to others if they'd find them useful.

The good news is that this works! At least from the quick testing I did.

On the block layout menu, you can go to add a block for any of the menus, and then you have the ability to select:

1) The starting menu level for the block
2) The maximum menu level depth for the block
3) Whether or not to auto-expend the children of the menu

My understanding is that these are the three key features in order to replicate the primary menu and secondary menu blocks properly. I think there's one other block in the footer of Bartik that needs to be created with this too, but I'd have to look back to see what that is, and if this will fit the bill.

This doesn't encompass all of the features of menu block, just these initial three. In theory we could probably add in more, but this is a nice start.

A few of the features not ported:

• Title link: This lets you make the title of a block a link to that block's root menu item. I've found this pretty handy when constructing menu blocks.
• Follow: I've always found this sort of confusing. This lets you specify the starting level in relation to the active menu item, but only if the level you specify is deeper than the level of the active item. So if you wanted a menu block that always showed the children of the active menu item if it is three levels down, this could be useful for that.
• Follow parent: If you are doing follow, this lets you specify if the starting level is based on the active menu item or the active menu item's children. Again, I don't totally get why you'd need this, but I'm sure it is useful for some folks.
• Sort: If you want the active menu item to always appear on the top of the list of menu items at that level, this is kind of handy. Particularly if you have a very long, complex menu tree.
• Fixed parent item: If you want to set up a menu block that only shows children of a specific item in a menu that is not at the root level, then this feature is very handy.

For now I stuck to the three features that had been agreed upon as important. Now that I'm familiar with what's going on, it probably wouldn't be too difficult to add in some of the others.

I'd very much appreciate some feedback on this. I'm sure there are probably better ways to do some of the things I tried here, and it would be great to get feedback on that. Should some of these functions be in new or different classes? Should some of the work being done in the build function of the block be abstracted out to another class somewhere else? Do we need to add some of these functions to the menu tree building service? Is the way this is done going to work okay with the cacheing system?

I'm glad to help muddle my way through any suggested changes.

I've not looked at the code, but I've tried the patch out and managed to reproduce the primary and secondary blocks. Works great!

My test scenario is this:

- create three nodes and put them in the top level of the main navigation menu: alpha, beta, gamma
- create two more nodes, alpha-1 beneath alpha, and beta-1 beneath beta.
- create two blocks for the main navigation menu:
-- primary level, depth of 1
-- secondary level, depth of 1

On node alpha, the secondary block should show alpha-1. On node beta, it should show beta-1. And on gamma it should be empty.

@mdrummond, the menu performance/caching part of this is going to be addressed instead by #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees which is why we downgraded it and why it's mostly a feature at this point. :) As far as I understand, in any case; not playing issue status wars until someone else weighs in. ;)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
@@ -300,6 +300,35 @@ public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail
+      foreach ($tree_with_trail AS $key => &$value) {

As -> as

I agree, stuffing it into the MenuTree service seems like overloading. Perhaps we could add a menu block service too, and then inject both the tree and block services, or the menu-block service could depend on the menu-tree service and then you only inject the menu-block service into the block. The render-tree call on the menu-block service would proxy to the menu-tree service.

Also, needs some tests, which unfortunately there are none of in menu_block. Might be worth discussing with @kim.pepper because I think he'd been looking at tests here github.com/previousnext/menu_block.

I guess the question is are the methods added to the menu tree class useful beyond menu blocks. I would guess yes, but I could be wrong on that.
Thought of mentioning tests: yes, we'd need to figure out what aspects to test, then write those tests.

1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
   @@ -616,4 +645,115 @@ protected function menuLinkTranslate(&$item) {
   +  /**
   +   * Title extracted from most recent manipulation.
   +   *
   +   * @var string
   +   */
   +  public static $title;
   

   why public? and why declared between methods?

2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTree.php
   @@ -616,4 +645,115 @@ protected function menuLinkTranslate(&$item) {
   +  public static function frontPage() {
   ...
   +  public static function pruneTree(&$tree, $level, $parent_item = FALSE) {
   ...
   +  public function trimActivePath(&$tree) {
   ...
   +  public static function depthTrim(&$tree, $depth_limit) {
   

   why everything is static but trimActivePath() is not? I would say: remove all statics, i dont see the need

3. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuTreeInterface.php
   @@ -142,6 +142,19 @@ public function buildPageData($menu_name, $max_depth = NULL, $only_active_trail
   +   * The data returned by buildPageData() has link['in_active_trail'] set
   +   * to TRUE for each menu item in the active trail. The data returned from
   +   * buildAllData() does not contain the active trail indicators. This is
   +   * a helper function that adds it back in.
   ...
   +  public function addActivePath(&$tree);
   

   Why not have this in buildAllData() then, if we are moving this functionality in MenuTree

ParisLiakos:

I was just porting over the way these methods and variables were declared in the d8 menu_block module update. It's very possible that the public/protected/status on these aren't ideal. I'm trying to help, but I'm not an expert on these things and could use the feedback.

As for putting the active path into build all data, I don't think that we should assume that everybody will want the active path all the time in build all data. If you were using build all data to show the entire tree of a menu, for example, the active path might not be relevant there.

not playing issue status wars until someone else weighs in

I discussed this with xjm, and we agreed it's correctly categorized as a task, since it's a blocker for #1869476: Convert global menus (primary links, secondary links) into blocks.

(commenting here after mdrummond mentioned this issue in IRC)

I would like to mention the idea of "menu formatters".
These formatters would be plugins and could render a menu as an ul-li list, a plain list of links without ul-li, a top dropdown with suckerfish, a sidebar menu, etc. some of these would affect the html, others only the css/javascript, or maybe some classes.

There is a D7 module called Menupoly where this idea is implemented and called "MenuTheme" instead of "menu formatter". For one project I even made a menu theme / menu formatter that would render links as entity teasers (for whatever entity is linked in the menu link).

Another idea from this module is that of the "menu source". So you could use a taxonomy term tree instead of menu links to render a tree.
Not sure if we really need that here, though. I actually never used this option myself.

I think it would be quite useful if core would provide an API for menu formatters, so people don't have to go crazy with theme functions.
Not sure if this is beyond the scope of this issue, though..

The goal we're trying to achieve right now is to be able to replace the hard-coded primary and secondary menus with blocks, and this issue introduces the features that would allow us to do that.

I think the features you described for menu formatting sound interesting, and developing an API for that could be useful, but that would probably be appropriate for another issue.

It looks like the MenuTree class is being moved around in #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module. Postponing this until that gets settled a little more. Maybe I can catch up with effulgentsia or pwolanin on IRC to figure out the best way to approach this once the dust settles.

#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees will likely land before #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module, and it already paves the way for this issue and #1869476: Convert global menus (primary links, secondary links) into blocks. :) It already cleans up the primary and secondary menus and makes them render cacheable. I've already got a few blockers/child issues committed, we're making fast progress.

The menu conversion part 4 is almost in. Once it is, I should be able to draft a new version of this patch that uses the new menu system to make this work. I walked through the new system with pwolanin on IRC a couple weeks ago, and it looked like it would work.

probably doesn't' need to be postponed any longer?

Yup. I'll try to get a new patch up in the next few days. Yay!

@mdrummond: ping me if you have any questions. As I said earlier, I specifically kept this use case in mind while working on the refactored rendering of menu trees, so it should have become much, much easier to support! :)

Thanks Wim. Will do. Yes, it looked much easier when I looked through the new methods a couple weeks ago!

Can we agree that the functionality of core menu block will be as suggested in #44

* 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

What about specifying which menu is to be used? Or is that assumed?

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.

Great. I just wanted to confirm the scope of this issue.

@mdrummond let me know if you need any help.

Okay, so here's a patch that takes advantage of the new menu system.

I didn't do a re-roll, because things are just too different. So I built this up based on the previous patch.

One of the big things to note is that I took off the "expanded" option. What that did was to auto-expand all of the menu items while preserving the active trail indicators on the menu tree. There doesn't seem to be an easy way for me to make that work with the new menu system, and frankly, I'm not sure it's necessary.

Our main goal is to be able to re-implement the primary and secondary menu variables as blocks, and to do that we needed to be able to have a couple other features in menu blocks. We needed to be able to: 1) select the starting level for the menu, and 2) show only one level of that menu. The good news is that you can do exactly that with this patch.

I had added in the support for the expanded option, because previously that seemed to be necessary to do what we needed to do. Now, this seems to work without that, so we can drop that from this patch.

I also want to clarify that the way we're implementing this is to add the level and depth selection to the existing menu blocks on the block layout page. On the block layout page, you can already select a block for any given menu: now when you do so, you can also select the starting level and depth. This is different than the menu block module, which had an option to create a specific menu block, where you selected the menu. Since we already have a way to select a menu on the block layout page, that form field seems unnecessary.

Also, to clarify, we're providing an option to choose how many levels of a menu to display, rather than the lowest level of a menu to display. That's more relevant for our goal of replacing the primary and secondary menu variables.

One thing I could use some help figuring out: the new menu block form fields I added are showing up at the very bottom, which is pretty ugly and confusing. It would be nice to move that up towards the top. I assume there's some weight option I need to add in to do that?

We'll also need test coverage for these new options. Suggestions on specific things we'd like to test here?

I think we should use MenuLinkTreeInterface::maxDepth() for max depth and level.

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -28,6 +30,13 @@
      /**
   +   * Stores the configuration factory.
   +   *
   +   * @var \Drupal\Core\Config\ConfigFactoryInterface;
   +   */
   +  protected $configFactory;
   +
   +  /**
       * The menu link tree service.
   
   @@ -50,13 +59,16 @@ class SystemMenuBlock extends BlockBase implements ContainerFactoryPluginInterfa
       *   The plugin_id for the plugin instance.
       * @param array $plugin_definition
       *   The plugin implementation definition.
   +   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   +   *   The factory for configuration objects.
       * @param \Drupal\Core\Menu\MenuLinkTreeInterface $menu_tree
   ...
       */
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $menu_active_trail) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, MenuLinkTreeInterface $menu_tree, MenuActiveTrailInterface $menu_active_trail) {
   ...
        $this->menuTree = $menu_tree;
   
   @@ -69,6 +81,7 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $container->get('config.factory'),
   

   These changes seems not necessary at all.

2. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -77,10 +90,90 @@ public static function create(ContainerInterface $container, array $configuratio
   +         '1' => $this->t('1st level (primary)'),
   +         '2' => $this->t('2nd level (secondary)'),
   +         '3' => $this->t('3rd level (tertiary)'),
   +         '4' => $this->t('4th level'),
   +         '5' => $this->t('5th level'),
   +         '6' => $this->t('6th level'),
   +         '7' => $this->t('7th level'),
   +         '8' => $this->t('8th level'),
   +         '9' => $this->t('9th level'),
   ...
   +     $form['depth'] = array(
   +       '#type' => 'select',
   +       '#title' => $this->t('Depth'),
   +       '#default_value' => $config['depth'],
   +       '#options' => array(
   +         '1'  => '1',
   +         '2'  => '2',
   +         '3'  => '3',
   +         '4'  => '4',
   +         '5'  => '5',
   +         '6'  => '6',
   +         '7'  => '7',
   +         '8'  => '8',
   +         '9'  => '9',
   +         '0'  => $this->t('Unlimited'),
   +       ),
   

   Maybe it would be great to explain that this is depth is relative to get given level above. Can we make the keys actual numbers? Maybe we should actually start with "0" and don't apply the "-1" below.

3. I think we should use MenuLinkTreeInterface::maxDepth() for max depth and level.

Good point, we should leverage this information somehow in the UI indeed.

One of the big things to note is that I took off the "expanded" option.

We hit massive performance issues with the expanded option on D7 sites.

Can we get some screenshots of the UI changes please?

    $max_depth = ($depth == 0) ? NULL : min($level + $depth - 1, $this->menuTree->maxDepth());

This line is effectively MenuLinkTreeInterface::maxDepth() since $this->menuTree is an injected instance of MenuLinkTreeInterface.

If you want us to make use of that in the form field itself, that's another matter. I was trying to figure out of this would even actually work. At the point where I'm calling maxDepth, it's before $this->menuTree->load(), and I couldn't tell if it was necessary for that to happen before maxDepth() provided the correct depth level.

As for item 1 in #75, my understanding is that is needed in order to store the config options for the block. I had to do something similar for the system branding block.

For item 2, suggestions are welcome. The wording I used was: Specify the maximum number of levels of the menu tree to display.

If one is suggested, it only display one level of the menu tree: the level that is specified with the level selector. For two levels, it would be the level selected plus the next level down.

If we want to limit the starting level and the max number of levels to display based on that particular tree's depth, I guess we'd need to do a menuTree->Load in the blockForm?

It seems like that would get pretty complex though, as the total depth is depending on the starting level and the number of levels that will be displayed. So unless some Ajax is involved, it will be almost impossible to properly limit the number of levels to display if the starting level is anything but 1.

As for item 1 in #75, my understanding is that is needed in order to store the config options for the block. I had to do something similar for the system branding block.

The system branding block uses it for
$site_config = $this->configFactory->get('system.site');
the config itself though is controlled by block plugins, so that they can be used both by panels and block module.

I am fine with getting it in as simple as it is currently. We can always iterate later, if needed, or contrib can.

Ah, yes. I get it now. Okay, I can pull those lines out about the configFactory..

One of the big things to note is that I took off the "expanded" option. What that did was to auto-expand all of the menu items while preserving the active trail indicators on the menu tree. There doesn't seem to be an easy way for me to make that work with the new menu system, and frankly, I'm not sure it's necessary.

Actually, there is an easy way. You can look at menu_navigation_links() in HEAD, which currently also does this. It's the third manipulator there:

  $manipulators = array(
    array('callable' => 'menu.default_tree_manipulators:checkAccess'),
    array('callable' => 'menu.default_tree_manipulators:generateIndexAndSort'),
    array('callable' => 'menu.default_tree_manipulators:extractSubtreeOfActiveTrail', 'args' => array($level)),
  );

However… I personally agree that this is unnecessarily complex logic. pwolanin also agreed with that. As does kim.pepper in #77 AFAICT. So I think it's fine to stop supporting that. It's ridiculously difficult to understand in the first place (*so* difficult that I think many would perceive its correct behavior as "buggy" and "strange" or even "utterly broken"). It does much more than just support "expanded" links (like the manipulator's name indicates: it also "follows" the active trail).

I'm only calling this out because we need to make that decision consciously.

I think the code looks wonderfully simple overall :)

One thing I could use some help figuring out: the new menu block form fields I added are showing up at the very bottom, which is pretty ugly and confusing. It would be nice to move that up towards the top. I assume there's some weight option I need to add in to do that?

Yep, you can just add a negative weight on the form elements to move them up the order.

Working well with manual testing. Attached some screenshots.

I pulled out the configFactory lines that were not needed, and I also changed the label and description for the depth form field. Hopefully a little more clear now what that does.

Fun fact I discovered after 90 minutes of banging my head against the Form API: it seems pretty much impossible to move block form fields around within a block form. I thought it would make sense to move the selectors for level and depth between the block label display checkbox and the caching fieldset. Not so easy as it turns out. You can set the weight of level and depth to -1, and they will appear above every other block form field. If you set the weight to 0, they appear after every other block form field. If you try to manually move the block label fields by setting their weight to -5, -100, or -5000, they stay right where they are thank you very much, even after clearing cache.

I looked at all the other standard blocks and all of their custom fields appear between the visibility tabs and the region selector, so I guess that's just the way these things go.

I think the next thing we'll need is tests. Suggestions on what specifically to test? Or if somebody wants to write tests, you are more than welcome to do so.

Fun fact I discovered after 90 minutes of banging my head against the Form API: it seems pretty much impossible to move block form fields around within a block form

Don't even try. The API is designed that the form is in the form it is. This also don't belongs into the issue of this patch tbh.
Consistency is king here, if you ask me.

Well tests should show that a certain block starts from a specific level and just wents on until a specified depth. Do you need some help writing the test? Just ask, if you need to know anything.

I also remember some quirkiness with block configuration forms, but like dawehner says: changing that is very much out of scope for this issue.

This reroll should fix the schema failures. I also put the defaultConfiguration() method back in the original location, that makes the patch simpler to review.

Same schema failures. Odd.

I may have done something wrong. I only looked at block.settings.system_branding_block in system.schema.yml, and created the analogous version. I expected it to be as simple as that, perhaps it's not — but I probably just made a stupid mistake!

1. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -375,6 +375,17 @@ block.settings.system_branding_block:
   +  label: 'Branding block'
   

   Menu block.

2. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -77,10 +78,71 @@ public static function create(ContainerInterface $container, array $configuratio
   +        '1' => $this->t('1st level (primary)'),
   +        '2' => $this->t('2nd level (secondary)'),
   +        '3' => $this->t('3rd level (tertiary)'),
   +        '4' => $this->t('4th level'),
   +        '5' => $this->t('5th level'),
   +        '6' => $this->t('6th level'),
   +        '7' => $this->t('7th level'),
   +        '8' => $this->t('8th level'),
   +        '9' => $this->t('9th level'),
   ...
   +        '1'  => '1',
   +        '2'  => '2',
   +        '3'  => '3',
   +        '4'  => '4',
   +        '5'  => '5',
   +        '6'  => '6',
   +        '7'  => '7',
   +        '8'  => '8',
   +        '9'  => '9',
   +        '0'  => $this->t('Unlimited'),
   

   These keys are string.

3. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -101,7 +163,11 @@ public function defaultConfiguration() {
   +      'level' => 1,
   +      'depth' => 0,
   

   And these are int.

4. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -375,6 +375,17 @@ block.settings.system_branding_block:
   +      type: integer
   ...
   +      type: integer
   

   so is it int or string?

And this is the config inspector error.
Configuration validation

array (
  'block.block.bartik_tools:settings.level' => 'Missing schema.',
  'block.block.bartik_tools:settings.depth' => 'Missing schema.',
)

Schema is perfect I think it has something to do with type: block_settings so I'd suggest to ask schema guru :)

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -77,10 +78,71 @@ public static function create(ContainerInterface $container, array $configuratio
+    $max_depth = ($depth == 0) ? NULL : min($level + $depth - 1, $this->menuTree->maxDepth());

=== not ==

Made some updates that will hopefully help this to work right.

I'm storing the level and depth keys as strings, because when I tried to make them ints, there were errors. But then converting those keys to ints for when they're actually used. Maybe there's a better way to set the array keys to avoid the conversion issue. But hopefully this should avoid the errors. Feedback welcome!

Botched the patch. Will fix and re-upload.

Hopefully this will actually have bytes. Usually useful.

Also, I think I got the level and depth keys working using int, which avoids the need for intval when using the keys.

Moving Things in The Wrong Direction Since 2013™

Hopefully this will fix all the block related fails. Schema is still incorrect

Or this.

Clarifying the scope of the issue.

Updated patch includes schema fixes for the tools and admin menus in Stark.

Oh, also minor thing, the issue causing the schema fails is probably a beta blocker, and Alex Pott will be adding an issue about that tomorrow. The issue is related to plugin block derivatives that define custom configuration options in their schema.

Missing the patch?

Yes, missing the patch :(

Created to #2317865: Config schema definitions for plugins aren't applied to their derivatives to resolve the config schema issues discovered by this patch

Yes, fun fact, it does usually help when you upload the patch.

I think all that's missing is the test coverage? I'd recommend to copy MenuLinkTreeTest::testCreateLinksInMenu(), that has a good example of how you can create a menu tree for testing purposes.

Thank you @mdrummond for fixing stark menu blocks.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -77,10 +78,71 @@ public static function create(ContainerInterface $container, array $configuratio
+    $level = (int) $config['level'];
+    $depth = (int) $config['depth'];

There is another issue with config, these values are saved as string in config files :( I was unable to find the issue perhaps it is related to schema. I have no idea.

#110: see #103 and #106 — it's indeed a problem with config schemas that's causing this.

This is the issue that will fix the schema issues (hopefully): #2317865: Config schema definitions for plugins aren't applied to their derivatives

Ok thanks for the explanation here is a combine patch which contain #107 and #2317865: Config schema definitions for plugins aren't applied to their derivatives

sorry for the cross post.

Back to NW for #108

#2317865: Config schema definitions for plugins aren't applied to their derivatives is in, so hopefully this will work now.

.

Re-roll. This removes the changes to TypedConfigManager added in #113.

Of course, this requires changes to the schema. Anyone here speak yaml schema?

I'll take care of making changes based on the change notice: https://www.drupal.org/node/2319739

Okay, this is my understanding of how the schema should change. We'll see what happens! If this works, we'll still need tests.

Discussed some minor cleanup issues in IRC with @chx.

The schema looks like its the cause of the test fails:

block.block.bartik_footer:settings.visibility: Missing schema.

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -375,6 +375,17 @@ block.settings.system_branding_block:
+block.settings.system_menu_block:*:
+  type: mapping

This should be type: block_settings in order to inherit those generic settings, like visibility, per #125.

Ah! Thanks so much. I'll do another round tonight.

The example in the change notice (https://www.drupal.org/node/2319739) is why I changed from block_settings to mapping. We might want to update that.

If this works, then tests.

Thought this might nudge Testbot. But maybe not.

We have a need tests tag here.

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -77,10 +78,63 @@ public static function create(ContainerInterface $container, array $configuratio
+    $options = array(0 => $this->t('Unlimited'));
+    $options += range(0, 9);

I think we need to switch the order.

I'll take a look.

Nearly there... I spent an hour or so on this, but couldn't get the access checker to allow the right routes to be returned.

+++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
@@ -17,49 +21,148 @@
+    //   - 7
+    // - 6

According to code it is 6,7 and 7 has no parent.

Restores the calculateDependencies() test, and adds some default permissions to the routes. Still need to investigate why the returned tree is only showing external links.

OK. I think I worked out how to mock out a route and allow it to be accessed.

I had a dream about this last night (sad I know), and this really needs to be a web test, so we can test that only a sub-menu is rendered on lower level pages.

You know an issue is a good one when you start getting dream ideas on fixes for the patch. :/

Thanks for all your work on this Kim.

It's late here, so perhaps I'm mistaken, but…

[…] so we can test that only a sub-menu is rendered on lower level pages.

Is this really supposed to happen? Didn't we in #73, #77 and #82 say that we didn't want to keep the "expanded" option behavior? The logic to do that in primary/secondary menu links in HEAD is provided by:

    array('callable' => 'menu.default_tree_manipulators:extractSubtreeOfActiveTrail', 'args' => array($level)),

SystemMenuBlock doesn't apply that tree manipulator, and #73 intentionally didn't copy that.

I think there may be some misunderstanding about what we've started calling the "expanded" option here? I think there's two parts:

1. MenuLinkTree::getCurrentRouteMenuTreeParameters() (which SystemMenuBlock uses), already determines which links have the expanded flag enabled, and ensures they're expanded.
2. DefaultMenuLinkTreeManipulators::extractSubtreeOfActiveTrail() ensures that we only show the subtree relative to the current active menu trail

I think point 1 can actually "really" be considered the "expanded" option. I thought point 2 was explicitly omitted (with which I would agree, and as I'm pretty sure would pwolanin).
If we omit point 2, then there is no "only rendering of a subtree on lower level pages". And hence no test coverage is needed for it either.

(Sorry for the lengthy message, but I think we should clear this up.)

Only having written all this, I realize that maybe only a sub-menu is rendered may also mean that submenus along the active trail are getting expanded… in which case you don't have to write a web test, you can just mock the active trail by using a mocked MenuActiveTrail::getActiveTrailIds() :)

Wim,

I should have made myself more clear. I'm simply trying to test when you choose Level => Secondary.

According 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.'"

I have no idea how to mock out MenuActiveTrail::getActiveTrailIds() in a KernelTestBase test. :-)

Here's a re-roll.

According 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.'"

But that's what I'm saying in #143: this conditional rendering is what DefaultMenuLinkTreeManipulators::extractSubtreeOfActiveTrail() provides.

So, to get a complete sense of how the current patch behaves in comparison with the current hardcoded primary/secondary menu links, I did some testing.

I configured the primary and secondary menu links to use the same menu over at admin/structure/menu/settings, then I created two nodes, and then created this menu tree:

Next, I created two instances of the "Main navigation" menu block. The first is configured to show only the first level, the second is configured to only show the second level. I've named these blocks "first level" and "second level", respectively.
You'll want to compare the primary (top left) and secondary (top right) links with those in the "first level" and "second level" blocks.

It turns out that by default, the behavior matches:

Front page:

Node one:

But, if we want a link to always be visible, even if the path doesn't match, we have to check its "Show as expanded" checkbox. Then the behavior changes:

Front page:

Node one:

The question is: is this the behavior we want? I think it is, because it's conceptually simple to understand: rendering a subset (starting at a configured level and ending at a configured level) of a menu works in exactly the same way as rendering the entire menu, but instead of rendering all levels, it's merely rendering the requested level only:

I'm working on test coverage, including the different active trails I asked about. Will unassign as soon as I post that.

In this reroll:

1. using $this->menuTree->maxDepth() to determine which options to provide for the level and depth settings in the UI
2. slightly simplified the changes in SystemMenuBlock::build() to make it easier to understand for future readers of the code
3. full test coverage

See interdiff-cleanup.txt for the first two points, interdiff-tests.txt for the third.

Thanks Wim. Nice work on test coverage!

+++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
@@ -17,32 +24,126 @@
+  public function atestSystemMenuBlockConfigDependencies() {

I think this was unintentional.

Fixes #148

Oops, yes, that was accidental :)

The issue summary is also still stating: Replace the hard-coded primary and secondary menus with blocks — but I think the plan is to tackle that in #1869476: Convert global menus (primary links, secondary links) into blocks? If so, I think the issue summary should be updated.

Also tagging with "Performance" because this is a soft-blocker for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

Overall, I think this patch is ready now. Let's do a final round of reviews.

1. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -50,16 +151,134 @@ public function testSystemMenuBlockConfigDependencies() {
   -      'entity' => array(
   -        'system.menu.' . $menu->id()
   -      ),
   -      'module' => array(
   -        'system'
   -      ),
   -      'theme' => array(
   -        'stark'
   -      ),
   +      'entity' => array('system.menu.' . $this->menu->id()),
   +      'module' => array('system'),
   +      'theme' => array('stark'),
   

   It seems like these whitespace changes are rather unnecessary?

2. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -50,16 +151,134 @@ public function testSystemMenuBlockConfigDependencies() {
   +    // Scenario 1: test all block instances when there's an active trail.
   

   s/scenario 1/scenario 2/

3. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -50,16 +151,134 @@ public function testSystemMenuBlockConfigDependencies() {
   +    // Helper function to "ASCIIfy" a tree returned by MenuLinkTree::build().
   +    $asciify = function(array $build, $indentation = 0) use (&$asciify) {
   

   I went with this approach because it makes it easy to see what's going on in the tests: you have a textual representation of the tree, which is very easy to understand.

   However, now that I see this test (that I've written myself), perhaps all that <<< EOF syntax isn't that great. Perhaps this helper function should not return a string representation, but an array representation? Relatively few people know about the <<<EOF string notation in PHP, so I suspect having the expectations defined as arrays will be easier to understand for many.

   If you guys think this is fine, then I'm okay to leave it as is.

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -77,9 +78,67 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $level_count = count($options);
   +    for ($i = $this->menuTree->maxDepth() + 1; $i <= $level_count; $i++) {
   +      unset($options[$i]);
   +    }
   

   Did we considered to just use array_slice?

2. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -77,9 +78,67 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $parameters->setMaxDepth(($depth === 0) ? NULL : min($level + $depth - 1, $this->menuTree->maxDepth()));
   +
   

   It would be great to explain this a bit better.

3. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -17,32 +24,126 @@
   +    $routes = new RouteCollection();
   +    $requirements = array('_access' => 'TRUE');
   +    $options = array('_access_checks' => array('access_check.default'));
   +    $routes->add('example1', new Route('/example1', array(), $requirements, $options));
   +    $routes->add('example2', new Route('/example2', array(), $requirements, $options));
   +    $routes->add('example3', new Route('/example3', array(), $requirements, $options));
   +    $routes->add('example4', new Route('/example4', array(), $requirements, $options));
   +    $routes->add('example5', new Route('/example5', array(), $requirements, $options));
   +    $routes->add('example6', new Route('/example6', array(), $requirements, $options));
   +    $routes->add('example7', new Route('/example7', array(), $requirements, $options));
   +    $routes->add('example8', new Route('/example8', array(), $requirements, $options));
   +
   +    $mock_route_provider = new MockRouteProvider($routes);
   +    $this->container->set('router.route_provider', $mock_route_provider);
   

   nice!

4. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -50,16 +151,134 @@ public function testSystemMenuBlockConfigDependencies() {
   +  public function testConfigLevelDepth() {
   

   yeah in general being more direct than indirect in the test is an advantage. otherwise people will have a hard time to understand this patch in the future.

5. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -50,16 +151,134 @@ public function testSystemMenuBlockConfigDependencies() {
   +    $asciify = function(array $build, $indentation = 0) use (&$asciify) {
   ...
   +    $place_block = function ($level, $depth) {
   

   This feels like overuse of anonymous functions.

#151.1: Haha :P I *knew* I had forgotten about one of the zillion array_* functions :D

#151.4: Can you clarify what you mean?

Addressed everything in #150 and #151.

1. +++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
   @@ -137,7 +134,13 @@ public function build() {
   +    // When the depth is configured to zero, there is no depth limit. When depth
   +    // is non-zero, it indicates the number of levels that must be displayed.
   +    // Hence this is a relative depth that we must convert to an actual
   +    // (absolute) depth, that may never exceed the maximum depth.
   +    if ($depth > 0) {
   

   Thank you!

2. +++ b/core/modules/system/src/Tests/Block/SystemMenuBlockTest.php
   @@ -203,82 +192,100 @@ public function testConfigLevelDepth() {
   +    $active_trail_expectations['all'] = [
   +      'test.example1' => [],
   +      'test.example2' => [
   +        'test.example3' => [
   +          'test.example4' => [],
   +        ]
   +      ],
   +      'test.example5' => [
   +        'test.example7' => [],
   +      ],
   +      'test.example6' => [],
   

   And suddenly our code looks a little bit like python

Nice work Wim. Thanks for the reviews Daniel!

And suddenly our code looks a little bit like python

:)

Updated the issue summary to help core committers assess this issue.

#2323721: [sechole] Link field item and menu link information leakage caused the exception. Rerolled to pass tests again.

Really sorry to do this, but from the screenshot it seems to me that no one from the UX team has reviewed this patch. :) And because changing these labels (if necessary) would also change the names of various internal keys, we need to do this prior to committing or we could end up with a schema update on our hands since we're (hopefully) so close to beta.

Here's an up-to-date screenshot, which has slightly different labels/wording. This shows up in the modal whenever you place a block under the "Menu" category at admin/structure/block.

I don't have a degree in HCI, but here's my take.

I feel like this level of configurability in the UI is a *super* edge case, and as a result not the kind of thing core would normally provide. Seems like we could replicate primary / secondary links behavior with just:

Starting level: Primary / Secondary / possibly Tertiary, but that's stretching it.
Description: Primary will display top-level links and always be visible. Secondary will display child links only when the parent menu link is active.

Depth: Show only top level / Show all levels below

and if you need uber-fancy configuration options like the ability to take the "quaternary" nav to exactly the 8th level, you can use a contrib UI for that (though by all means, keep API support for it).

I think it's really important to note that adding these menu block options was a goal prior to the desire to use this for primary and secondary menu blocks. If you look at John Albin's initial issue summary, he mentions a few other use cases where it would be useful to be able to select a specific starting level for a menu and then show a specific number of menu levels

It turns out that having these two options is exactly what we would need in order to get rid of the primary and secondary menu variables that are built into core right now, and doing so has a big performance improvement.

Adding these two options has a nice extra ability of allowing a site builder to build much more robust navigation systems. Having a menu block that can display two or three levels of menus is great, because that allows you to set up drop down menus or mega menus for example. Or more complex off-canvas navigation for mobile.

Simply because core is only using the top level and the next level down for its purposes of replacing primary and secondary menus shouldn't mean that we arbitrarily place limits that make these options only relevant for that specific purpose.

Doing so means we would have to significantly rework this patch and its associated tests for the express purpose of giving people fewer options for how to build a navigation system with core's menu blocks. That seems backwards.

If we think that most people will want to create a menu block that displays the entire menu tree, then I suppose we could put these two options in a collapsed fieldset with a label like "Limit menu levels displayed." I wouldn't fight making that change.

What this boils down to is that adding these two options allows us to solve a significant performance problem, but it also has the benefit of providing extra options that the 178,000 people who downloaded the menu block module seem to want.

To quote Jacine from comment #7 on this issue, "If this doesn't make it to Drupal 8, I am going to cry my eyes out."

Are we arguing over how many options appear in the drop-downs? Or are there other UX issues?

If it's just the number of options, then limiting them to an arbitrary number, a) makes the code more complex b) puts arbitrary limits on something based on our assumptions of how it would be used.

Kim

I agree with your points, Kim. Let's try to get yoroy or some of the other UX team members to take a look at this. I'd like to preserve the options we're providing people. If we can document the use cases for providing the options we're providing, that might help.

Is there any way we can put out a UX signal to get some eyes on this?

So what about 1,2,3 and unlimited?

I do not see how it makes it more usable for a select field to have options ranging from 1 to 3 and then unlimited rather than 1 to 8 or 9 and then unlimited. Chopping off those last options doesn't make it easier for people to understand that menus have different levels.

It's just an arbitrary limit that reduces the number of use cases that this can be useful for, and forces those use cases that might need that option to either have a custom module developed to remove that arbitrary limit or use a contributed modules like menu block to remove that limit.

I don't see an arbitrary limit greatly enhancing the user experience of those who don't need those extra options, but I do see an arbitrary limit greatly harming the user experience of those who would use those extra options.

As Kim said, these arbitrary limits would also make the back end code more complex.

I'm a front end person more than I'm a back end person. I greatly care about usability and user experience and making things easy for people. I do a lot of site building tasks, and I make use of the menu block module regularly in Drupal 8.

Providing clear options for people is really important for making things easy to use. Artificially limiting what people can sometimes add more frustration rather than less.

I'm going to make a revision to tweak some of the labeling to help make things more clear.

So as it happens, it's very good this was knocked back from RTBC. An array_slice function was resetting the array keys for the menu level form selector, which meant that when the default level was set to 1, the second level was shown as the default (even though the actual menus in the block defaulted to the first level). I added preserve_keys TRUE to array_slice, and it's working again. Yay! We may want a test to catch that in the future.

I made a number of changes to try to improve the UX.

The menu level selection is now in a details section with the label of "Limit menu levels displayed" this is collapsed by default. I also tweaked the labels and descriptions for the level and depth fields to clarify what each of them does. Finally, I got rid of primary, secondary and tertiary on the menu levels. I really don't know think labeling levels 1-3 with those additional words helps to clarify what these fields do. These options are available for every menu block. Is it helpful to know that you're selecting the primary level of the tools menu? Probably not. Saying that you're starting with level 1 is probably just as easy, if not easier to understand.

Feedback welcome. I've included a few screenshots for your viewing pleasure.

Trying to add in the schema change for moving the menu level settings into a details section. Hopefully this does the trick.

I'm apparently misunderstanding how to do schemas for settings in a detail element. If somebody could point me in the right direction, it would be most appreciated.

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -355,6 +355,21 @@ block.settings.system_branding_block:
+block.settings.system_menu_block:*:
+  type: block_settings
+  label: 'Menu block'
+  mapping:
+    menu_levels:
+      type: mapping
+      label: 'Limit menu levels displayed'
+      mapping:
+        level:
+          type: integer
+          label: 'Starting level'
+        depth:
+          type: integer
+          label: 'Maximum number of levels'
+

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -77,9 +78,75 @@ public static function create(ContainerInterface $container, array $configuratio
+    $config = $this->configuration;
...
+    $form['menu_levels']['level'] = array(
...
+      '#default_value' => $config['level'],
...
+    $form['menu_levels']['depth'] = array(
...
+      '#default_value' => $config['depth'],

Well, the config schema needs to document the data structure of the config, not the form. The data structure of the config was kept flat with no menu_levels container (see quoted code), so it needs to keep being flat in the config schema. OR the saving of the config data needs to happen to under a menu_levels keys, then access needs to happen from under that key in which case, the schema would be correct.

dawehner and timplunkett walked me through this on IRC. They suggested doing an override of the blockSubmit form: previously the settings weren't being saved. That's working now. I'm not entirely clear if this will fix the schema issue, but the changes I made in 170 were a misstep, as menu_levels isn't involved in the config, as Gabor pointed out above.

We'll see if this goes green.

Just a note that the interdiff in 173 is with the patch in 167, not 170, since 170 was the wrong direction.

Instead of doing the blockSubmit() override, I changed that to using #parents, to ensure the desired name attribute is generated, which removes the need for transforming form values upon submit.

This fixed the test failure also.

Finally, I made sure that #open (top open the <details>) is set automatically if the block's menu levels settings are customized.

Instead of doing the blockSubmit() override, I changed that to using #parents, to ensure the desired name attribute is generated, which removes the need for transforming form values upon submit.

Can't we just use a #tree => FALSE as well?

+++ b/core/modules/system/src/Plugin/Block/SystemMenuBlock.php
@@ -111,6 +114,7 @@ public function blockForm($form, FormStateInterface $form_state) {
+      '#parents' => ['settings', 'level'],

@@ -123,6 +127,7 @@ public function blockForm($form, FormStateInterface $form_state) {
+      '#parents' => ['settings', 'depth'],

This introduces a hidden dependency on block entities, which are part of block.module, not core like block plugins are.

The 'settings' key is not part of the API, and page_manager (for example) does not use it.

Can't we just use a #tree => FALSE as well?

That's what I thought, but apparently not. You cannot set #tree = FALSE on level 2 and then set #tree = TRUE on level 3 and get the expected results (which would be #parents = ['level 1', 'level 3']), sadly :(

This introduces a hidden dependency on block entities, which are part of block.module, not core like block plugins are.

How does this introduce a hidden dependency on block entities?

The 'settings' key is not part of the API, […]

This is true.

Back to NR to see if anybody knows how to use #tree instead of #parents to get this to work.

1. Regarding #tree: #759222: #tree does not default to TRUE, fails to meet developer expectations

   #tree => FALSE cannot be used here, because it resets all #parents, correct.

   #parents may be used, but if so, it must respect the #parents of the calling code / form structure.

2. The form structure of a plugin should have no knowledge of any kind of "outer form". A plugin cannot assume where exactly it is located in a form. Any assumptions about that would break the encapsulation of the plugin.

   I was under the assumption that a plugin-defined "sub-form" would already be processed in a way that only exposes the plugin's values to the plugin's validateForm() + submitForm() methods to guarantee the encapsulation regardless of how and where a plugin is used… but it looks like that hasn't been implemented yet.

   Related: #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects

This comment is probably not helpful for resolving the immediate problem at hand, but nevertheless I hope the pointers are helpful.

How does this introduce a hidden dependency on block entities?

It assumes how the block configuration form is structured, while page manager might have a different one.

This sounds like a pretty good plan! Happy to see this patch ongoing. From a UI perspective its sad, we only have dropdowns to our disposal. The ideal UI here would be to show some tree like UI with "dummy" or real taxonomies and allow to manipulate what levels you wish to show (using toggles or so), this would be the level of UX Drupal should pursue.

When I come off my cloud, were I dream Drupal one day is :) I suggest doing the following:

• The fieldset label should capture the "functionality" not activity. So I would go for "Menu levels". I would avoid displayed, as that kinda conflicts with "visibility" above.
• Each label should clearly describe the functionality. For example:
• Starting menu level - could be more actionable "Start at menu level". With the options just being 1, 2, 3, 4, 5 etc.
• Maximum number of menu levels - could be "Menu levels to display"
• Each description should be concise and add clarity
• "Blocks that start with the 1st level will always be visible. 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." - I have no idea what this is trying to explain. I don't understand how blocks are now part of this, I thought this was about menu and the whole trail active menu stuff, is just flabbergasting.
• "Specify the maximum number of levels of the menu to display, beginning with the starting menu level selected above." - the beginning of this description should be the label. The other part is unclear to me, when I select 1; does it or does it not show the 1st level?

Fixing the labeling and description should remedy some of the UX, but as Angie clearly points out it is a bit of an odd one. Understandably that we want this, but hard to fix within our current UI patterns. I don't think changing the number of options necessarily makes it more clear, its mostly the whole levels concept that is hard for people to parse - because we are not a sitemap like CMS.

I love the little gem "Select the region where this block should be displayed." below it. Such a useless description, if the label isn't clear - we should always fix the label, not add a description.