Add configuration options for dynamic block titles

I have added a patch that lets you pick one of the following options to display the block title:

- Use the given block title as is
- Use the menu title as label
- Use the active trail's parent menu link title as label
- Use the active trail's root menu link title as label

The previous patch doesn't account for menu blocks without items and breaks.

Here's a new patch.

Same patch as #3, but moved a line as it conflicts with #2811337: 2+ level menu block not limited to active parent.

Thanks for the patch @syndz. The patch seems to working fine on my installation. I guess we only need to add an option to make the block title a link. At this moment, it is just a label correct?

@vollepeer That is correct. This patch does not include transforming the title into a link. I figured that'd be another issue.

Patched not working?
https://www.drupal.org/files/issues/menu_block-label_configuration-28096...
https://www.drupal.org/files/issues/menu_block-label_configuration-28096...

I tried both patches above and receive this both times. Thanks for any assistance.

missing header for unified diff at line 8 of patch
can't find file to patch at input line 8
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: menu_block.module
|IDEA additional info:
|Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
|<+>UTF-8
|===================================================================
|--- menu_block.module (revision c6195b4829e3687ccd7e635d6ab2ee15ac7ec9fd)
|+++ menu_block.module (revision )
--------------------------
File to patch:

@nelslynn Try: patch -p0 < menu_block-label_configuration-2809699-4-d8.patch

@syndz: Thanks for the patch, so far it works great for me!

I'm glad it works for you all!

I've refactored the patch a bit and have added a new option:
- Active item's title as block title.

The rest of the functionality remains the same.

Perhaps someone is able to review the patch so we can get this into the next release?

#9 Works great for me -- thanks!

#9 works as advertised, thank you for the patch.

Previous patch didn't apply cleanly during make.
This one is good to go.

I've tested the patch, and it works as expected. However, I do not like the architecture this patch introduces.

1. There is no interface that defines the public method MenuBlock::blockLabel

2. The patch introduces 6 methods to the MenuBlock class for providing menu titles. IMHO these methods are candidates to be re-used by other code. Further they represent a specific and focused sub-task of the module. I propose to place these methods in a separate MenuTitle service within the Menu Block module.

I've introduced a service, interfaces and separated responsibilities.

The changes should be compatible with the previous patch.

Added an extra option: "Initial menu item's title" which would be the menu item based on the active trail as configured in "Initial menu level".

The patch is based on #14.

The patch in #14 and #15 introduces Exceptions, but these aren't correctly caught. I'm working on a fix.

Here's the promised patch.

The previous patch contained code of another unrelated issue.

Here's a re-roll of the patch in #18.

The configuration option is lost in #19. Working on a fix.

Re-added the configuration option.

Fixed working of Initial menu item by extending the root item class.

Have tried out patch in #22 and it works for me.
Thanks

patch #22 worked fine.
@Syndz, Thanks.

#22 working

I can also confirm #22 works. Gonna change this to RTBC.

I just ran into an issue with #22. It assumes the only menu items we would want are created manually and have a UUID in the plugin name. Some have a plugin ID like `standard.front_page`. This was causing errors on my project's homepage. I've updated the patch to lookup the link title using Drupal's `MenuLinkManagerInterface` rather than the `EntityInterface`.

I had noticed that a menu item created by views rather than manual was not showing, where manual ones worked fine (so i changed mine) might this be the same problem?

Yes, I just looked at what a plugin ID is for a view link. Specifically, I ran into a couple issues with this method:

  protected function getMenuLinkTitleByPluginId($pluginId) {
    list($entityType, $uuid) = explode(':', $pluginId);

    if ($menuLink = $this->entityRepository->loadEntityByUuid($entityType, $uuid)) {
      return $menuLink->label();
    }

    return '';
  }

In my front_page example, there was no colon to explode the ID on and so the call to list() was throwing an exception. Views IDs are a little different, in that they're structured like views_view:views.test.page_1. So while you won't get an exception with list(), it's still expecting the second value from explode() to be a UUID which is probably why your links weren't working. I just tested this functionality with Views created menu items, and it still works as expected for me.

Sorry @jnettik, #27 didn't work for me. I have all manually created menu items. In this case all the menu block UI is missing:
https://www.evernote.com/l/ABMjlqVanY5GeJQnKHTY4-AKK-9SUxbgK0sB/image.png

#22 does seem to work fine for me.

I just spun up a fresh install of Drupal 8.3.3, Menu Block dev, and the patch in #27 on simplytest.me and the patch worked without issue for me. Can anyone else reproduce this?

Weird. I though it could be a conflict with auto_entitylabel. It is and it isn't. (disabling that module I get back the menu_block fields, but not the field for which title to use; reenabling the module, still the same functionality). This is with drupal 8.3.1.

Ah ha. I did have the wrong version of the module. (from meat-space conversation) drush was pulling down 8.x-1.3-dev. So, #27 seems to work fine with the real dev version of this module. (had to install with composer).

from a first bird eye check, both #22 and #27 patches:

• show active trail for the "Initial menu item's title" option instead of the real menu parent item, if it's the intended behaviour I wonder why it is called "Initial .." instead of "Active trail's ...",
• on a multi language site, the last 4 options don't honor the active language item translation.

last of all: thank you for the good coding.

The patch from #27 works for me as well, but I see a couple places that @tourtools' comment about missing translations is correct (and some other minor style issues).
e.g. menu_block/src/Plugin/Block/MenuBlock.php line 59 '#title' => 'Use the following as title',

It does apply nicely and is functional.

Updated patch to fix missing translation, and a few other code style errors.

I was able to apply the patch in #36 without issue.

I've ran the interfaces through translation, some pieces get translated and others do not. I'm not sure if that's anything with the code, since the strings are now translatable as of #36. Leaving as needs review, as I'm not sure I follow the first item of feedback in #34 to say it's been addressed.

@jnettik can you point out the elements that aren't getting translation support you expect? I only went through the code effected by #27, so its possible they are from other parts of the module (and probably should get their own issue). Of course it's also possible I just plain missed something.

In #34, the feedback was "on a multi language site, the last 4 options don't honor the active language item translation". I can confirm that several of those options aren't translated in the dropdown despite them being wrapped in $this->t(). I also noticed several parts of the interface that Menu Block isn't responsible for also not translated. #34 made me wonder if more was needed from the patch to handle translation. If that's not the case, then we should be good!

Anyone can explain what's the order for patches and on stable or dev version please ?

Thanks a lot

The current patch to try is the one from #36. It was developed against the development head at the time, and likely requires at least dev to apply. I haven't had to re-apply it recently, so it would be good to know if it still applies cleanly.

Ok thanks, but why when i try to applied the patch, it creates 3 files (MenuBlockNoActiveTrailFoundException.php...) in vendor\zendframework\zend-feed\src\Exception ?
They should be in the module directory right ?

Thanks for help

@devil2005: How are you applying the patch? I've been using Composer and haven't had any issues with the patch applying correctly.

I think that the easiest way to do it is writing menu-main.html.twig with your own theme suggestion and add {{ item.title }} in this file and pass extra argument to the macro :) Works well

Example: menu twig from bootstrap 3.

{#
/**
 * @file
 * Default theme implementation to display a menu.
 *
 * Available variables:
 * - menu_name: The machine name of the menu.
 * - items: A nested list of menu items. Each menu item contains:
 *   - attributes: HTML attributes for the menu item.
 *   - below: The menu item child items.
 *   - title: The menu link title.
 *   - url: The menu link url, instance of \Drupal\Core\Url
 *   - localized_options: Menu link localized options.
 *
 * @ingroup templates
 */
#}
{% import _self as menus %}

{% set title = 'Menu name' %}

{#
  We call a macro which calls itself to render the full tree.
  @see http://twig.sensiolabs.org/doc/tags/macro.html
#}
{{ menus.menu_links(items, attributes, 0, title) }}

{% macro menu_links(items, attributes, menu_level, title) %}
  {% import _self as menus %}
  {% if items %}
    <h2 class="menu-title"> {{ title }} </h2>
    {% if menu_level == 0 %}
      <ul{{ attributes.addClass('menu', 'nav') }}>
    {% else %}
      <ul{{ attributes.addClass('dropdown-menu') }}>
    {% endif %}
    {% for item in items %}
      {%
        set item_classes = [
          item.is_expanded? 'expanded',
          item.is_expanded and menu_level == 0 ? 'dropdown',
          item.in_active_trail ? 'active',
        ]
      %}
      {% if menu_level == 0 and item.is_expanded %}
        <li{{ item.attributes.addClass(item_classes) }}>
        <a href="{{ item.url }}" class="dropdown-toggle" data-target="#" data-toggle="dropdown">{{ item.title }} <span class="caret"></span></a>
      {% else %}
        <li{{ item.attributes.addClass(item_classes) }}>
        {{ link(item.title, item.url) }}
      {% endif %}
      {% if item.below %}
        {{ menus.menu_links(item.below, attributes.removeClass('nav'), menu_level + 1, item.title) }}
      {% endif %}
      </li>
    {% endfor %}
    </ul>
  {% endif %}
{% endmacro %}

#36 worked for me - would like to try the approach of @lamp5 in #44 as sounds better than patching but would need a bit more detail or an example ideally.
Also would need it to only apply to the menu block being used for 2nd level not the top level one.. (both seem to use that twig file)

Both solutions seem fine I think. This used to be a feature of Menu Block, and works well for site builders or in circumstances where you might not have access to the code base. The Twig approach sounds like a good solution as well, especially until this feature gets committed. They're not mutually exclusive really.

Updating the status of this to RTBC.

I was applying the patch wrong,
seems like #36 works!

+1 #36

Nice work.

#36 works for me too ! Thanks !

It would still add an item to: Fixed parent item title

The patch in #36 fixed a coding standards problem that was not related to this issue, but conflicted with other patches to the menu_block module.

This patch removes the coding standards fix (it should be fixed directly in the module and not by patch).

Attaching the inter-diff for 51 vs 36. Still applies so I think RTBC is still correct.

Is there any room in this issue for adding a linking ability to the block title? If not, should another issue be created with this patch as the basis for it?

@johnny5th: Given that this patch is RTBC and has lots of people following it and interested, I'd strongly vote for a separate issue if you want to add more features. Let's get this in ASAP, as-is, and then we can build more on top of it.

And yes, +1 to RTBC. I'm using patch #51 and it's working fine. Seems highly over-engineered to me (I can't imagine anyone trying to swap out classes for the behavior this is adding, nor why we need 4 new distinct kinds of exceptions that could be thrown) but I guess that's the D8 Way(tm)...

Thanks,
-Derek

I've added a separate issue and patch based on this one at #2950943: Add links to menu parent block title.
Thanks for all of y'alls work on this!

Update
Never mind. The current dev version of Panels now does a call to menu_block_block_view_alter. So this is indeed working with panels, if you have the current dev version.

--------------------------------------------------------

I am having trouble getting this to work with menu blocks displayed via non-standard ways. I'm using patch at #51.

Is hook_block_view_alter the best way to install the desired title?

I am finding that menu_block_block_view_alter is not called for blocks that are added to the page via Panelizer (which I believe is using ctools) and via the block_field module (which I use inside of Paragraph types). Are these modules rendering the block incorrectly or are they just using an alternate method? They seem to use the label as is from the block configuration. I don't pretend to understand why they don't go through hook_block_view_alter.

Menu blocks that I add to the page via the standard block layout are correctly going through hook_block_view_alter.

I can alter the displayed label in a hook_preprocess_block or if I add a public function for getConfiguration in the MenuBlock class.

Any thoughts on this?

Maybe it is by design but it doesn't work the way I thought it would.
I have 2 menu block in my footer and if I'm not on an active trail of any of them, there is no title displayed.
And when I am on an active trail, both menu titles are the root menu of the current active trail.

Example:
- Foo 1
- Bar 1
- Bar 2

- Foo 2
- Bar 3
- Bar 4

If I'm on page Bar 2, both titles displayed are Foo 1 and if i'm on the frontpage, both titles are empty.
I'm using 8.x-1.4 and I applied patch #51.
Is it normal behaviour and I should look into some other solution or I'm doing something wrong?

Thank you.

@Syndz, @acrosman @Eyal Shalev, or @jnettik Can you someone explain the benefits of this architecture over #12. I don't want to speak for @Sutharsan but he is away for a couple of weeks so may not be able to answer. My gut feeling is this is overly complicated for the task so I'm asking for a "Cost–benefit analysis".

None of the current suggested patches work when placing a menu block using layout builder. It might be a layout builder issue, but the hook menu_block_block_view_alter is not being called when rendering the block in a layout builder configured entity.

@pookmish I had a quick chat with @tim.plunkett on Slack and yes layout builder will not without implementing a layout builder event \Drupal\layout_builder\LayoutBuilderEvents::SECTION_COMPONENT_BUILD_RENDER_ARRAY.

He also mentioned

#12 correctly does `$block->getBaseId() === 'menu_block'` and not `$block instanceof MenuBlock`

Also, considering decorating SystemMenuBlock to make this easier.

Thanks @joelpittet for checking on that and the quick response

I can't speak much to the cost-benefit analysis, but heres a new patch that takes the patch from #51 and adds that event subscriber.

Founds like 51 + another event subscribing might make the patch more effective but doesn't answer main concern that the current patch is overly complicated. From looking back over this issue it seems the extra complexity was added without much explanation. I was actually looking at this earlier today since I needed it for a project for the first time in a while. I can try to take a look at a re-roll of #12 with the event subscriber from #61 in the next couple days if someone else doesn't get there first.

How about this attached patch? It uses the idea from #12. Its even more simple than #12 because instead of using the block_view_alter hook and the event subscriber, it overrides the core getConfiguration() method and sets the appropriate title at that time. This seems to work with cacheing enabled.

I was working on getting the two to blend together and was struggling to get the event subscriber to install on a site with just the standard profile enabled. This new version installs cleanly, but there are still issues.

I created a few pages and tossed them on the main navigation:
--Home
---a
----a.1
----a.2
---b
----b.1
----b.2

Place a new block for a Main Navigation into the main content region. The block is set with default Menu Levels (1 & Infinite) and Advanced options to expand all, any fixed parent item I tried, and "Active Trail's Parent Title". It appears to work as expected on a.1, a.2, b.1, and b.2. But on pages a or b the site displays and error and logs:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "standard.front_page" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 133 of /var/www/vhosts/d8/pub/core/lib/Drupal/Core/Entity/EntityTypeManager.php).

#63 is looking more along the lines I feel comfortable with committing. Thanks @pookmish I'll review a bit more thoroughly tomorrow and give the others a chance to chime in. Thanks for giving it a test @acrosman.

Putting this to needs review for #63

@joelpittet Some call it over-architecture, some will call it proper architecture. I like the way the #51 code is build. It is more than I envisioned, but is it properly crafted. Except for the hook_block_view_alter(), which is a kind-of a magic way to set the title (let the MenuBlock class take care of its own tile). But I also understand your sentiment of over-architecture; is this level of abstraction really needed?

Though I still think it would be good to separate the logic out that is responsible for making the block title, I also want to progress this issue. It is now 1.5 years old, and still not finished. Any of the patches that functionally does the job is good to go. As long as the keys that define the types of block title remains unchanged, the code can be refactored in a follow-up issue.

The #63 patch is for me the most acceptable one to move forward. I do have some comments on the implementation, but I leave those for next comments.

1. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -54,6 +87,24 @@ class MenuBlock extends SystemMenuBlock {
   +      '#title' => 'Use the following as title',
   

   Use less words for a field label.

2. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -54,6 +87,24 @@ class MenuBlock extends SystemMenuBlock {
   +        'active_item' => $this->t('Active item\'s title'),
   +        'parent' => $this->t('Active trail\'s parent title'),
   +        'root' => $this->t('Active trail\'s root title')
   

   Don't use "\'" in a translatable string.

3. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -54,6 +87,24 @@ class MenuBlock extends SystemMenuBlock {
   +        ]
   +      ]
   +    ];
   

   (Nested) array items should be followed by a comma.

4. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -212,6 +264,7 @@ class MenuBlock extends SystemMenuBlock {
   +      'label_type' => 'block',
   

   Use a constant, not a (magic) string.

5. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +279,127 @@ class MenuBlock extends SystemMenuBlock {
   +    return Menu::load($this->getDerivativeId())->label();
   

   Use Dependency injection.

6. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +279,127 @@ class MenuBlock extends SystemMenuBlock {
   +   * @param string $pluginId
   +   *   Menu Item ID.
   +   *
   

   Considering the description, "$pluginId" is not the right name for this variable.

7. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +279,127 @@ class MenuBlock extends SystemMenuBlock {
   +    $parentIds = \Drupal::entityQuery($entityType)
   

   Use Dependency injection.

8. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +279,127 @@ class MenuBlock extends SystemMenuBlock {
   +      $parent = MenuLinkContent::load($parentId);
   

   Use Dependency injection.

9. +++ b/menu_block.module
   @@ -100,3 +103,17 @@ function menu_block_theme_suggestions_menu(array $variables) {
   +function menu_block_block_view_alter(array &$build, BlockPluginInterface $block) {
   +  if ($block instanceof MenuBlock) {
   +    /** @var \Drupal\menu_block\Factory\MenuBlockTitleFactoryInterface $menuBlockTitleFactory */
   

   IMHO the MenuBlock class should take care of its own block title. The alter hook is Ok if it is the only way, but for this case I don't see the need.

@Sutharsan thanks for the review. Honestly I didn't even complete a full review as such from the patch i used to make the one prior to this.

Attached I've corrected the standards issue @Sutharsan pointed out. And I implemented the constants which i do agree makes for a cleaner class and consistency.

I just applied the patch in #69 to the same site I was using in #64 and it runs more smoothly but still has a problem, and I ran down the details a bit more.

The previous patch through an error because it could not find the plugin for the menu item, the current patch silently fails to set the menu block's title. The problem appears to be related to the fact that getLinkTitleFromLink assumes the link_id is in the form: [entity_type]:[entity_uuid], which is fine for the vast majority of use cases. But when the menu item is injected by a module's (or profile's) links.menu.yml the ID is in the form [module].[menu_item_id]. When the default front page view would be the route that provides the menu the ID comes in as standard.front_page, the attempt to use , and returns an empty string instead of the menu link title. This is probably also a problem for any statically defined route that doesn't have an entity behind it.

 private function getLinkTitleFromLink($link_id) {
    list($entity_type, $entity_uuid) = explode(':', $link_id);

   try {
      $entity_query = $this->entityTypeManager->getStorage($entity_type)
       ->getQuery('AND');
    } (\Exception $e) {
      return '';
    }

The problem becomes more pronounced if you use other menus installed by other parts of the standard profile. If you put the user account menu into a menu block and use any of the values from the menu as the title you never get a title displayed.

@pookmish, Do provide an interdiff if you make a new patch (except re-rolls). That helps reviewers to understand the changes made.

@acrosman good find. I modified the getLinkTitleFromLink() to look within the menu tree to find the appropriate link element. i've tested and this works when using a view as a menu item and I assume it should work in other contexts.

@sutharsan sorry for not including an indertiff. I'll begin to do so.

This patch improves on #69 by fixing the link item title. It also provides a fallback to the block label in case no link is found.

I've not tested the patch, only checked interdiff. No architectural review, only reviewed details.

1. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -352,10 +354,9 @@ class MenuBlock extends SystemMenuBlock {
   -
   ...
   -    }
   +    };
    
   

   ';' is not needed.

2. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -415,34 +416,35 @@ class MenuBlock extends SystemMenuBlock {
       * @return string
       *   The given item title or empty string.
   

   Return value can now also be null.

3. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -415,34 +416,35 @@ class MenuBlock extends SystemMenuBlock {
   +    if($link = $this->findLinkInTree($menu, $link_id)){
   

   Please use a tool to check your patch for coding standards (e.g. https://www.drupal.org/project/coder).

4. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -415,34 +416,35 @@ class MenuBlock extends SystemMenuBlock {
   +      return $link->link->getTitle();
   

   Does this return the translated value (if applicable)?

5. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -415,34 +416,35 @@ class MenuBlock extends SystemMenuBlock {
   +   * @param array $menu_tree
   +   *   Array of menu link id's and its subtree.
   

   This description confuses me. 'Array' is plural, "its' is singular.

6. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -415,34 +416,35 @@ class MenuBlock extends SystemMenuBlock {
   +   * @return \Drupal\Core\Menu\MenuLinkTreeElement|bool
   +   */
   +  protected function findLinkInTree(array $menu_tree, $link_id){
   

   Returning null will have the same effect. No need to return two different types of data.
   Also, missing description.

Review of #69 patch

1. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +22,56 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   +   * Constant definitions for block title replacement.
   +   */
   +  const LABEL_BLOCK = 'block';
   

   'title' is not consitent with the constant names.
   'replacement' describes the usage, not the content. The usage can change is therefore not good to be used in the description.

2. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -18,6 +22,56 @@ use Drupal\system\Plugin\Block\SystemMenuBlock;
   +  /**
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   +   */
   +  protected $entityTypeManager;
   

   Missing description. See coding standards.

3. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +301,148 @@ class MenuBlock extends SystemMenuBlock {
   +   *   The new label.
   

   'new' is not relevant here.

4. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +301,148 @@ class MenuBlock extends SystemMenuBlock {
   +  public function blockLabel() {
   +    switch ($this->configuration['label_type']) {
   

   Use a verb (get) in the method name to make it more descriptive.

5. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +301,148 @@ class MenuBlock extends SystemMenuBlock {
   +   *   Current item's parent title.
   

   The method description already describes what will be returned. If you duplicate this in the return value description, you increase the maintenance cost and/or increase the risk that the two descriptions deviate. Keep it concise: "The menu item title."

6. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -226,4 +301,148 @@ class MenuBlock extends SystemMenuBlock {
   +   *   The root menu item title.
   

   See above.

I think if we can confirm its accurately works as designed, a cleanup with an improved documents/comments is a trivial task. I'm well aware of coder module but at this time functionality is more important IMO.

Re-rolling patch to hopefully allow it to patch along side https://www.drupal.org/project/menu_block/issues/2756675#comment-12601859

@Sutharsan your question about the translation: Yes I installed and configured the menu items translation and the code return the translated string. I have also changed comments as you suggested. I hope you have a chance to test the functionality of the patch.

This patch has been working for me for a while now. I've noticed though, but can't find a pattern, that the block title itself gets saved into the configuration of the block. 2-3 of my menu blocks get named "Home" over time.

I can confirm that #77 also works good for us. Anything left for RTBC?

I can confirm that #77 worked for me too. I applied it to the 8.x-1.5 version.

#77 works for me.

Would be even better if it also linked to the parent item; hopefully https://www.drupal.org/project/menu_block/issues/2950943 will work too.

Added "Fixed parent item" option.
(see interdiff)

It's very useful when need to render fixed part of menu tree and block title should be equal to parent menu item.
I know, when we have static parent, than we have static block title, but we need to cover followup issue for example #2950943: Add links to menu parent block title.

I can confirm #82 works for the "Fixed parent item" option, which I needed (as well). Thanks for sharing, @5n00py.

Been running #82 in production on several sites for a few months now.

I've been following this issue for a while, and using various patches without problem.

However, the issue title wasn't matching the scope of the patch. I think this is a bit closer to what we're doing.
Adding (most of) a proper summary, too.

Meanwhile, I also tried to clean up some of the PHPDoc comments to be more accurate, complete, and to more closely follow core conventions (e.g. "Gets ..." as the initial action verb, not "Get", etc).

Fixed a few other minor coding standards things (not doing assignments inside if(), ordering use statements alphabetically, etc).

My only remaining concern is that the UI is a bit confusing with the "Fixed parent item" option added in #82, its interaction with the menu level setting, the placement of the settings (e.g. it doesn't seem like good UI practice to have 2 settings that intimately impact each other living in totally separate fieldsets), the order of all the options in the drop-down (which seems somewhat arbitrary right now), the specific wording of the labels and descriptions, etc. So I think this whole thing could use a bit more UI attention.

All that said, it's certainly very close to RTBC. ;)

Thanks!
-Derek

Applies perfectly for me (along with #2950943-14: Add links to menu parent block title)!!

I agree with your UI concerns; mulling it over...

It wouldn't address everything, but as a start: what if there were help text with the "Display title" (label_display) checkbox? I know it's a core block setting, so maybe it's not good to add help text like that -- I'm not sure the best practices. But, if it's not Bad, there could be some help text like,
Menu blocks: If "Display title" is enabled, there are additional block title settings available in the "Advanced options" fieldset, with the "Use as title" field.

Okay, after months of using this patch, it was starting to really piss me off because of how it directly manipulates the block's configuration. This results in the actual block config (on disk) getting out of sync. This is the problem mentioned by @johnny5th in #78. For example, every time I try to export my system's config, I get something like this:

--- a/config/block.block.sub_navigation.yml
+++ b/config/block.block.sub_navigation.yml
@@ -17,7 +17,7 @@ provider: null
 plugin: 'menu_block:main'
 settings:
   id: 'menu_block:main'
-  label: 'Sub menu navigation'
+  label: '<a href="/">Home</a>'
   provider: menu_block
   label_display: visible
   follow: false

This is because of the way this code works:

  public function getConfiguration() {
    $label = $this->getBlockLabel() ?: $this->label();
    $this->setConfigurationValue('label', $label);
    return $this->configuration;
  }

That's really *not* what we want to be doing. ;) Instead, we want to set the block title dynamically, in a way that doesn't touch the actual block config. So, I lifted some of my code from #2950943-14: Add links to menu parent block title, made it not dependent on if the block title is a link or not, and set the block's title in such a way that it should work fine, without touching the actual block config (and generating the evil config sync divergence above).

I'm basically only testing this with the label_type: root configuration, so other reviews and testing would be great. Also would be nice to test on some other themes, since it's sort of dependent on what your block twig templates are doing. See code comments for me.

Of course, it'd also be good to have some tests for all this, but menu_block doesn't currently define *any* tests so I don't think it's worth holding up this patch any longer for that. Could be added in a follow-up.

RTBC?

Thanks!
-Derek

Ugh, that's only "failing" because of #3086171: Add @file doc block to menu_block.module. Back to needs review...

Just tested #87. Really great and important feature for this module with lots of flexible options. Thank you very much @dww for these improvements. Before we used #77 and in Drupal 7 we loved these options already.

I couldn't apply patch #87 with composer. It would be nice to have an update on this issue.

@OlgaS: re: #91 -- perhaps you're using the last official release, instead of -dev? #87 won't apply to 8.x-1.5. You need to be on the 8.x-1.x branch (8.x-1.x-dev). If you do that, you're fine:

Here are the relevant parts of my composer.json:

  ...
  "require": {
    ...
    "drupal/menu_block": "1.x-dev",
    ...
  }
  ...
  "extra": {
    "patches": {
      ...
      "drupal/menu_block": {
        "[ #2809699-87] Add configuration options for dynamic block titles" : "https://www.drupal.org/files/issues/2019-10-06/2809699-87.menu_block-label_configuration.patch",
        "[ #2950943-15] Add links to menu parent block title" : "https://www.drupal.org/files/issues/2019-10-06/2950943-15.menu_block-title_as_link.patch"
      },
      ...
    }
  }
  ...

Here's what composer does with them:

% rm -rf modules/contrib/menu_block
% composer install
...
  - Installing drupal/menu_block (dev-1.x 6a2e5a8): Cloning 6a2e5a8e83 from cache
  - Applying patches for drupal/menu_block
    https://www.drupal.org/files/issues/2019-10-06/2809699-87.menu_block-label_configuration.patch ([ #2809699-87] Add configuration options for dynamic block titles)
    https://www.drupal.org/files/issues/2019-10-06/2950943-15.menu_block-title_as_link.patch ([ #2950943-15] Add links to menu parent block title)
...
% cat modules/contrib/menu_block/PATCHES.txt
This file was automatically generated by Composer Patches (https://github.com/cweagans/composer-patches)
Patches applied to this directory:

[ #2809699-87] Add configuration options for dynamic block titles
Source: https://www.drupal.org/files/issues/2019-10-06/2809699-87.menu_block-label_configuration.patch


[ #2950943-15] Add links to menu parent block title
Source: https://www.drupal.org/files/issues/2019-10-06/2950943-15.menu_block-title_as_link.patch

So the update we need here is someone who's not me to carefully review #87 and set the status to "RTBC" (if there are no further problems) or "Needs work" if anything isn't working as expected.

Thanks!
-Derek

p.s. Hah, d.o's [#xxx] filter is running inside code blocks. Added spaces so the output isn't so weird.

Thank you so much Derek - works great with "drupal/menu_block": "1.x-dev"!

#87 works great for me, I've been using label_type: parent ("Active trail's parent title"). I did a lot of checking, everything was the same (in a good way) before/after switching from #85 to #87. I shouldn't get any change in the block config YML, right? (I don't see a reason for it to be different, but, just wanted to check.)

A note for other folks: If you're using #2950943: Add links to menu parent block title, too, like I am, make sure to get the reroll in #15 over there ;-)

..........
@dww what would you think about tweaking the options list to indicate which option is "drupal default," the way it's explained in the issue summary? -- I made a version that does that, but my plan was just to help get this to RTBC, sooo I'll just post the interdiff below -- if you think it's a good idea, I'll upload the actual files, and if not, we can be "done" I think?

diff -u b/src/Plugin/Block/MenuBlock.php b/src/Plugin/Block/MenuBlock.php
--- b/src/Plugin/Block/MenuBlock.php
+++ b/src/Plugin/Block/MenuBlock.php
@@ -116,7 +116,7 @@
       '#title' => $this->t('Use as title'),
       '#description' => $this->t('Replace the block title with an item from the menu.'),
       '#options' => [
-        self::LABEL_BLOCK => $this->t('Block title'),
+        self::LABEL_BLOCK => $this->t('Block title (Drupal default)'),
         self::LABEL_MENU => $this->t('Menu title'),
         self::LABEL_FIXED_PARENT => $this->t("Fixed parent item's title"),
         self::LABEL_ACTIVE_ITEM => $this->t("Active item's title"),

..........
(I've been thinking about UI things generally, I do think the menu block configuration form is already confusing, but again, don't want to slow things down. I mentioned this back in #86 -- just as a start: what if there were help text with the "Display title" (label_display) checkbox? I know it's a core block setting, so maybe it's not good to add help text like that -- I'm not sure the best practices. But, if it's not Bad, there could be some help text like, Menu blocks: If "Display title" is enabled, there are additional block title settings available in the "Advanced options" fieldset, with the "Use as title" field.

Sorry sorry for the extra noise -- a tweak to my "note for other folks" --

If you're using #2950943: Add links to menu parent block title, too, like I am, make sure to get the reroll in #15 over there -- AND make sure the patch from #2950943 comes after the patch from this thread (#2809699).

I can confirm that this works without any issues using the dev release (8.x-1.x-dev) and applying the patches needed according to the composer example that Derek supplied in #92!

The only thing I found was that I needed to recreate my block after this was patched in order to have the ability to make these changes.

I was testing the patch from #87 and ran into a problem with the block title (label). It is (no longer) taking into account whether the title (label) should be displayed or not.

This is happening here:

$label = $this->getBlockLabel() ?: $this->label();
// Set the block's #title (label) to the dynamic value.
$build['#title'] = [
  '#markup' => $label,
];

...

// Set the generated label into the configuration array so it is
// propagated to the theme preprocessor and template(s) as needed.
$build['#menu_block_configuration']['label'] = $label;

...


/**
 * Implements hook_preprocess_hook() for "block".
 *
 * Set the block label with the #menu_block_configuration label if it exists.
 */
function menu_block_preprocess_block(&$variables) {
  if (isset($variables['content']['#menu_block_configuration']['label'])) {
    $config_label = $variables['content']['#menu_block_configuration']['label'];
    // Some block twig templates (especially classy + bartik from core) use
    // `{{ configuration.label }}` to print the label. Others just use
    // `{{ label }}`. Therefore, we have to set both template variables for
    // this to work consistently.
    $variables['configuration']['label'] = $variables['label'] = $config_label;
  }
}

You will now always see the title even if:
- You deselect the "Display title" option in the block configuration
- You dynamically instantiate a block with configuration that includes hiding the label (label display: FALSE/0 see code snippet below)

This code snippet worked before:

/** @var \Drupal\Core\Block\BlockPluginInterface $main_menu_block */
$main_menu_block = $this->blockManager->createInstance('menu_block:main', [
  'label_display' => FALSE,
]);

After applying the patch in #87 it suddenly shows the main menu title.

So, the label display option should be taken into account here. I suppose the preprocess function should be changed to adhere to the label_display the same way the block module is doing this:

$variables['label'] = !empty($variables['configuration']['label_display']) ? $variables['configuration']['label'] : '';

Updated patch from #87 to take into account that the label can be hidden by configuration.

Juste tested #99, works fine!

But i found one thing: "Expand all menu items" (Regular settings) / "Expand all menu links" (Advanced Options).

This is kinda confusing to me. First i assumed this is just an duplicate (mainly because the german translation is wrong - i will check this later), but of course the behavoir is different. So we may should clearify this difference in the description?

But i'm still not completely sure what the difference is. The advanced setting expands the whole menu, every single link is printed out, no matter what the active trail is or what depth it has (except you set a fixed parent item or something..).
The regular setting just overrides the "Show as expanded" option just for the CURRENT / ACTIVE menu item and expands the first submenu? Right?

Is anyone using this with the patch in https://www.drupal.org/project/menu_block/issues/3089134 for allowing one block for multiple menus?

I got errors when i tried them together.

EDIT - adding info on errors

Message TypeError: Argument 5 passed to Drupal\menu_block\Plugin\Block\MenuBlock::__construct() must implement interface Drupal\Core\Menu\MenuActiveTrailInterface, none given, called in [...]/modules/contrib/menu_block/src/Plugin/Block/MultipleMenuBlock.php on line 48 in Drupal\menu_block\Plugin\Block\MenuBlock->__construct() (line 72 of [...]/modules/contrib/menu_block/src/Plugin/Block/MenuBlock.php)

I'm missing another option for block title - "starting level's parent" or $menu_root.

Photos (root level)
- Flora
–- Vegetables
--- Carrot
--- Onion
-- Fruits
--- Apple
--- Banana
- Fauna
Some other root level item
...

With starting level = 3 (fixed), expanded = true, it would be nice to have title Flora for either Vegetables or Onion (any level 3 or 4 items)
Menu root is one of level 2 items (Flora or Fauna).

FLORA
------------
Vegetables
- Carrot
- Onion
Fruits
- Apple
- Banana
-------------

Something like "starting level -1 in active trail" would be really useful.
In my case Vegetables and Fruits are "nolink" menu items used for grouping, so I can't even see Flora as a block title, only Vegetables/Fruits (parent) or Photos (root), apart from other obvious options.

@alisonjo315 re: #94:

@dww what would you think about tweaking the options list to indicate which option is "drupal default," the way it's explained in the issue summary?

Whoops, sorry I missed this question! Uhh, I'm not sure site builders will necessarily know what "Drupal default" means as UI text. In general, the less words in the UI, the better. I'm inclined to leave it as-is.

Similarly, re: injecting help text, less is more. ;) I think there are some UI issues still in here, but I'm not sure adding more text will help them. :/ I'd be in favor of opening a follow-up issue to address the UI more holistically.

@michaelablackham re: #97:

The only thing I found was that I needed to recreate my block after this was patched in order to have the ability to make these changes.

Hrm, that's concerning. Maybe we need a hook_update_N() or hook_post_update() to do something to make existing menu_block blocks see the new settings. Haven't had that problem myself (since I've been using this patch from the beginning of building the sites where I need it), so I haven't experimented with this. Maybe an update function is worth adding if someone feels inspired. Otherwise, I'd be okay with mentioning it in the release notes. ;)

@Steven Buteneers re: #98 Good catch! Interdiff in #99 looks fine. Thanks for fixing that!

@thomas.frobieter re: #100 - yup. That's confusing. However, that's 100% outside the scope of this patch. This feature doesn't touch those settings at all, and you'll see the same behavior with and without this patch. Therefore, please see if anything at https://www.drupal.org/project/issues/menu_block?text=expand is already talking about this, and if not, you could open a separate UI bug report about that.

@gsquirrel re: #101 - nope, never tried #3089134: Ability to display "the menu selected by the page". Not surprised there's a conflict. Whatever issue lands 2nd will have to deal with it, so adding it as a related issue. But I hope this one goes in first. ;)

@scoff re: #102: Thanks for the suggestion. However, this issue has dragged on for way too long as it is. Adding yet more features to it is only going to further delay ever getting this into the module. Would you be willing to open a follow-up child issue for this feature request?

All that said, I'm hereby declaring menu_block-label_configuration-2809699-99.patch from #99 RTBC. The code is solid, fix is legit, and I just tested it manually. Works as advertised. It's a very small but important change from the previously RTBC patch. Let's get this in...

Thanks!
-Derek

+1 for RTBC, patch is applying on 1.x-dev and is working.

So I'm an idiot. I posted a new issue, but it really belongs here.

So I'm testing things out with D9, and I'm getting the error #101 while applying the patch in #99. The System Menu Block seems to have a deprecation that may need to be updated in D9.

Sorry for double posting.

Can we get this code in for 8.7.x and 8.8.x support now, and then worry about D9 for the rest of this module in another issue?

To make it work in 8.8.x and D9,

It's missing one parameter in the construct call of MenuBlock and it's required to pass one more parameter (active_trail).

It should look like,
parent::__construct($configuration, $plugin_id, $plugin_definition, $menu_tree, $active_trail);
instead of
parent::__construct($configuration, $plugin_id, $plugin_definition, $menu_tree);

at line no 75 of src/Plugin/Block/MenuBlock.php

Updated patch from #99 to add missing parameter it construct call of MenuBlock class.

Looking more closely at the changes in #108, I think the whole thing is a bit off. SystemMenuBlock.php already has a protected $menuActiveTrail;. So it's unnecessary for MenuBlock to have that at all. We only need to accept it in the constructor and create() methods, pass it on to the parent class, and we can use it directly without having it ourselves.

Tests are still failing locally, but here's a cleanup of the menuActiveTrail stuff. Interdiff is totally confused, so here's a raw diff of this patch vs. #108.

Thanks, tests! ;) They caught a real bug with this patch. We forgot to update the config schema for the new block setting.

Sort of annoying that the testbot didn't flag that directly. Instead, I had to look at the output of the failed tests and found this:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.main with the following errors: block.block.main:settings.label_type missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 95 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

But once I saw that, I knew what the problem was, and it's an easy fix. See attached. Test bot should be happy with this, now...

I'd love to set this back to RTBC, but that's not exactly cool. ;) Anyone else willing to re-review/test this?

Also, anyone feel inspired to update MenuBlockTest (now in the 8.x-1.x branch thanks to #3038396: Add automated tests) to verify this new feature works as designed?

Thanks!
-Derek

Just to make sure we don't introduce a bug for D9, they may have changed the constructor on us... there are a couple ways to either decorate, extend without having to worry about the constructor in the parent class, but I don't have the examples off hand for both but I mentioned in the comments on this blog article
https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

This issue is mentioning this potential issue:
#3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor

Thanks, @joelpittet for sharing the article. A nice pattern. I am going to refactor this.

Re: #113: Just commented at #3115436. That's indeed solved by the fix I posted here at #110. The existing MenuBlock code was written for much older core. SystemMenuBlock now needs the active trail service itself. It has a BC layer in the constructor to set it for you if you don't pass it explicitly, but this BC layer is deprecated in 8.8.x and removed in 9.0.x. That's what I'm fixing with #110. If you'd rather, I could pull that out from here as a separate (small) patch for #3115436 if you'd rather fix that in isolation for better scope management. Just let me know.

Cheers,
-Derek

p.s. @jungle - please don't. ;) If we're going to completely refactor the class, let's do it at #3115436 not here.

Hello Darek,

After looking into this long issue I tried to apply your patch from #111 but it didn't apply:

error: patch failed: config/schema/menu_block.schema.yml:8
error: config/schema/menu_block.schema.yml: patch does not apply
error: patch failed: src/Plugin/Block/MenuBlock.php:3
error: src/Plugin/Block/MenuBlock.php: patch does not apply

I got the same result trying to apply #108.

I want to give it a review test as you mentioned to move it into RTBC but couldn't complete that.

Regards
Essam

@3ssom re: #116: You must start with the latest from the 8.x-1.x branch (like the version of this issue says). The patch doesn't apply against 8.x-1.5. You must either use the 8.x-1.x-dev snapshot, or better yet, a Git checkout.

Thanks,
-Derek

#111 works perfectly for me, on 1.5 *with* the following other patches applied:
#2756675: Add option to make the starting level follow the active menu item
#3022011: Implement config schema for block.settings.menu_block.*.{follow, follow_parent}
#2950943: Add links to menu parent block title

(in other words, my version isn't totally identical to -dev, but it seems I have the patches you'd need to have in order to apply #111)

-------
@dww delayed response to #103, re: UI/help text -- I see your point, thanks for the follow-up!)

(ack, sorry for the extra comment)

I also tested applying it on -dev and it applied cleanly for me (via composer).

One caveat: I've had this patch on my site since #85, so I can't speak to any issues folks may have had re: using the latest version of the patch on existing menu blocks.

Hello Darek,

My bad, I blame the composer :D

Anyway I've cloned the 8.x-1.x and applied the #111 patch, applied with no problems!

I've also tried that new functionality (Use as title) on a menu block, working fine with no error showing in the dblog.

I'd consider this as RTBC as alisonjo315 changed the status.

Great work!

Regards
Essam

When there are no menu links to render the parent title won't show as the block title. Instead the menu title is shown. This happens since there is no $variables['content']['#menu_block_configuration']['label'] in this specific case.
This has been adjusted in this patch.

Thanks for all the work on this module,
Bram

@Bram Linssen re: #121: Thanks for improving this.

However, if you upload a new patch to an RTBC issue, you should:
A) Include an interdiff from the previously RTBC patch to make it easier for everyone to see what you changed.
B) Set the status back to "Needs review" so other folks can verify your changes before the maintainer tries to commit it.

Cheers,
-Derek

p.s. I'm tempted to mark this issue postponed on #3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor since that's a much smaller scope, touches some of the same code, and prevents menu_block from working on D9, etc. There's some basically out-of-scope changes in the patches here, and it'd be cleaner to fix those items first, separately, then come back and add the new feature on top of a more solid foundation.

Hi! I tried the patch in #111, and it worked as expected. I patched the latest Dev release of the module. Works great. SO far no errors.

I did find that my use case does not seem to be addressed. I'm wondering if there's some other way to achieve what I need.

I'd like to show a designated Active Trail Head for the label -- and have that label change based on the Trail head at a certain level.

So let's say I have menu item 3, and it has children 3.a,3.b,3.c. If I'm on 3.a,3.b, or 3.c, I want my block heading to be Item 3, and I want it to STAY item 3, even if I go down another level to 3.a.i or 3.a.ii.

Additionally, if I switch to Item 2's children (2.a,2.b,2.c) OR Item 2's Grandchildren (2.a.i, 2.b.i. 2.c.i), I want to the label for the Block to become Item 2, and stay Item 2, even if I traverse deeper down the trail of Item 2.

• Item 1
• Item 2
  • 2.a
    • 2.a.i
  • 2.b
    • 2.b.i
  • 2.c
    • 2.c.i
• Item 3
  • 3.a
    • 3.a.i
    • 3.a.ii
    • 3.a.iii
  • 3.b
  • 3.c

Finally -- I'd LOVE an option to be able to make my Block heading a link... but that's probably something to ask in a different Feature Request.
UPDATE: I've successully added the patch from this issue to enable "make block label link": https://www.drupal.org/project/menu_block/issues/2950943
on TOP OF patch #111 from this thread to the current Dev release -- and all are working great in a production site environment.

Hi @nkraft, I have the exact same use case (Fixed Parent item AND make it linked to menu item).

I'm following this issue and https://www.drupal.org/project/menu_block/issues/2950943 in the hopes that one or both can achieve our scenario.

Here's a screen shot of the Menu Block config from D7 with options selected to achieve the desired result.

NOTE: I'm using the Fixed Parent config option in D8, but Menu Block doesn't currently display that menu item text nor is it linkable to the menu item.

I'm keeping my fingers and toes crossed for us!

Indeed, if you apply the latest patch in #2950943: Add links to menu parent block title with the latest in here, it works fine. For example, the sidebar at:

https://www.breema.com/about-breema/new-to-breema

is the site I built where I needed both of these features, and why I contributed to a working combination of them.

Meanwhile, if anyone wants to get to #3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor before I have a chance, that'd probably help land everything.

Thanks,
-Derek

Here's a re-roll on top of #3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor. This continues the pattern introduced there and avoids the constructor API change problems. Due to reroll and conflicts, interdiff can't hang. Here's a raw diff of the patch files from this vs. #121.

Meanwhile, marking this issue postponed on landing the fix from #3115436. The attached patch won't apply until that lands, so there's no sense feeding it to the test bot.

Thanks,
-Derek

p.s. Also attaching an interdiff of #111 vs. #121. Agree that's a useful fix. Thanks, @Bram Linssen!

Looking more closely at #121 and the interdiff, the comments aren't complete, aren't wrapped to 80 chars, and are slightly incorrect. This fixes all of that.

Assuming #3115436-10: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor lands (instead of patch 5 over there), here's a new version that takes advantage of method chaining.

Oh right, code standards wants a blank line before @return

Sorry, #130 is a bogus patch. This is what I meant. ;)

Thanks for the update. Patch from #131 didn't apply cleaning for me:

[mac:kristen:menu_block8]$ patch -p1 < 2809699-131.patch 
patching file config/schema/menu_block.schema.yml
patching file menu_block.module
patching file src/Plugin/Block/MenuBlock.php
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 30.
Hunk #3 FAILED at 58.
Hunk #4 succeeded at 47 with fuzz 1 (offset -31 lines).
Hunk #5 succeeded at 95 (offset -29 lines).
Hunk #6 succeeded at 190 (offset -29 lines).
Hunk #7 succeeded at 300 (offset -29 lines).
Hunk #8 succeeded at 346 (offset -29 lines).
Hunk #9 succeeded at 360 (offset -29 lines).
3 out of 9 hunks FAILED -- saving rejects to file src/Plugin/Block/MenuBlock.php.rej

@dww Gave me the patch dependency in Slack which was successful. Thanks.

[mac:kristen:menu_block8]$ patch -p1 < 3115436-11.patch
patching file src/Plugin/Block/MenuBlock.php
[mac:kristen:menu_block8]$ patch -p1 < 2809699-131.patch
patching file config/schema/menu_block.schema.yml
patching file menu_block.module
patching file src/Plugin/Block/MenuBlock.php
[mac:kristen:menu_block8]$ patch -p1 < 2950943-25.patch
patching file config/schema/menu_block.schema.yml
patching file src/Plugin/Block/MenuBlock.php

#3115436: Use proper dependency injection and protect menu_block from changes to the SystemMenuBlock constructor is now in the 8.x-1.x branch of Git, so #130 should apply cleanly now. Queued that for testing.

Fixes the unused use's from #131. Thanks, bot!

Considering this a feature request that I could see having some interesting issues, I'd like to see a few tests here to cover the options to prove they are working as expected.

#135 confirmed what I just experienced... it doesn't apply anymore! Needs a reroll.

In the meantime I used
"drupal/menu_block": "1.x-dev#95dbaf6",

with patch #99
to keep this working like before.

Thank you for your time and work, @dww!

Here's a re-roll of #135

I'll work on some tests

Same patch from previous, with test coverage for each option.

Great work, Aaron! I just had a first look and tested the patch in our environments, it works perfectly like expected. Thank you for your time and work! RTBC +1 - let's get further feedback!

Great start on the tests, thanks! Removing the tag. A few concerns/quibbles:

1. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +   * @dataProvider testMenuBlockTitltOptionsDataProvider
   

   I'm not a huge fan of @dataProvider for functional tests. This is quite slow, since we completely reinstall all of Drupal for each config option. We could have this method as a protected helper method, and have a single public test method that loops over the array from the existing provider. There's no chance these cases are going to somehow conflict or cause weirdness if everything runs in the original Drupal install.

   However, at #3113992: The 'Update' page has no idea that some updates are incompatible we recently had this debate, and they decided it was okay even in Functional, and that mostly it's to be avoided in FunctionalJavascript. So we could keep this, but I would be curious how much slower this is. ;)

2. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +  public function testMenuBlockTitleOptions($option, $title, $test_link = NULL) {
   

   We should have @param docs for each of these, regardless of if we keep the dataProvider or this becomes a helper method.

3. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +    $this->assertSession()->elementTextContains('css', '#block-main-menu', $title);
   

   Can this be more specific and target the block's h2, not just the entire block containing that text? If we're visiting the child-1-1 page by default, we're going to see all the names in the block most of the time, so the tests aren't actually giving us much confidence that the feature is working as intended.

4. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +      [
   +        'option' => MenuBlock::LABEL_FIXED_PARENT,
   +        'title' => 'child-1 menu item'
   +      ],
   

   What happens with LABEL_FIXED_PARENT at .../parent itself? Can we add a test case for that, too?

5. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +        'option' => MenuBlock::LABEL_PARENT,
   +        'title' => 'child-1 menu item',
   +        'test_link' => 'menu-block-test/hierarchy/parent/child-1/child-1-2',
   +      ],
   

   What happens with LABEL_PARENT if you visit .../parent itself? Would like to see that added here.

6. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,78 @@ class MenuBlockTest extends BrowserTestBase {
   +      [
   +        'option' => MenuBlock::LABEL_ROOT,
   +        'title' => 'parent menu item'
   +      ],
   +      [
   +        'option' => MenuBlock::LABEL_ROOT,
   +        'title' => 'parent menu item',
   +        'test_link' => 'menu-block-test/hierarchy/parent/child-1/child-1-1',
   +      ],
   

   Aren't these duplicate since that's the default test link above?

   Maybe this wants to be child-1-2?

Thanks again!
-Derek

Thanks for all this feedback.

I'm not a huge fan of @dataProvider for functional tests. This is quite slow, since we completely reinstall all of Drupal for each config option. We could have this method as a protected helper method, and have a single public test method that loops over the array from the existing provider. There's no chance these cases are going to somehow conflict or cause weirdness if everything runs in the original Drupal install.

Wow, I had no idea. That explains why these tests were taking so long locally!
I've replaced this with a protected helper method, and eliminated method arguments entirely.

Can this be more specific and target the block's h2, not just the entire block containing that text? If we're visiting the child-1-1 page by default, we're going to see all the names in the block most of the time, so the tests aren't actually giving us much confidence that the feature is working as intended.

The selector block-main-menu is the id of the h2. It's redundant, but I added the h2 to make this clearer.

The other feedback is incorporated as well - please give it a review.

Excellent, thanks!

A few final concerns before I'd RTBC (although I've contributed so much already, I probably shouldn't be the one to RTBC this):

1. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -26,6 +28,23 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
   +  const LABEL_FIXED_PARENT = 'fixed_parent';
   
   @@ -89,6 +122,26 @@ class MenuBlock extends SystemMenuBlock {
   +        self::LABEL_FIXED_PARENT => $this->t("Fixed parent item's title"),
   

   Given the test assertions and results, seems this is more accurately just "LABEL_FIXED", no? Has nothing to do with parents at all. You just select a fixed item as the block title. Value as just 'fixed'? Do I misunderstand? I'm not using this option in my sites, so I'm just now paying closer attention to it.

2. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -324,4 +387,165 @@ class MenuBlock extends SystemMenuBlock {
   +  public function getBlockLabel() {
   

   Does this need to be public?

3. +++ b/src/Plugin/Block/MenuBlock.php
   @@ -324,4 +387,165 @@ class MenuBlock extends SystemMenuBlock {
   +      case self::LABEL_FIXED_PARENT:
   +        return $this->getFixedParentItemTitle();
   ...
   +  /**
   +   * Gets the title of a fixed parent item.
   +   *
   +   * @return string|null
   +   *   Title of the configured (fixed) parent item, or NULL if there is none.
   +   */
   +  protected function getFixedParentItemTitle() {
   

   More to rename?

4. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +    $options = $this->getTestMenuBlockTitltOptions();
   

   Typo in the name of this method.

   However, at this point, maybe we don't need/want a whole separate method. We could just have the $options array defined inline right here. Although that's a huge array, so maybe it's better to leave them separate?

5. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +      $test_link = empty($option['test_link']) ? 'menu-block-test/hierarchy/parent/child-1/child-1-1' : $option['test_link'];
   

   Do we actually need empty() here, or can we use NULL coalesce?

   $test_link = $option['test_link'] ?? 'menu-block-test/hierarchy/parent/child-1/child-1-1';
   

6. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +  protected function getTestMenuBlockTitltOptions() {
   

   If we keep it...
   A) Needs rename.

   B) We should document the @returns array[] and explain the expected keys in the sub arrays.

7. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +      [
   +        'option' => MenuBlock::LABEL_BLOCK,
   +        'title' => 'Block title',
   +      ],
   +      [
   +        'option' => MenuBlock::LABEL_MENU,
   +        'title' => 'Main navigation',
   +      ],
   

   Core providers, when they're this long, tend to use throw-away but helpfully self-documenting array keys for each of these. Helps make sense of why all the different options are in the array, and also helps ensure uniqueness. ;) Perhaps we don't care here. /shrug

Only NW for the typo in the method name, everything else is just for consideration or polish.

Thanks!
-Derek

p.s. a few others:

1. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +    $options = $this->getTestMenuBlockTitltOptions();
   +    foreach ($options as $option) {
   

   Also, if we keep this a separate method, we don't need the $options variable:

   foreach ($this->getTestMenuBlockTitleOptions() as $option) {
   

2. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,90 @@ class MenuBlockTest extends BrowserTestBase {
   +      [
   +        'option' => MenuBlock::LABEL_PARENT,
   +        'title' => 'child-1 menu item',
   +      ],
   +      [
   +        'option' => MenuBlock::LABEL_PARENT,
   +        'title' => 'parent menu item',
   +        'test_link' => 'menu-block-test/hierarchy/parent/child-1',
   +      ],
   +      [
   +        'option' => MenuBlock::LABEL_PARENT,
   +        'title' => 'child-1 menu item',
   +        'test_link' => 'menu-block-test/hierarchy/parent/child-1/child-1-2',
   +      ],
   +      [
   +        'option' => MenuBlock::LABEL_PARENT,
   +        'title' => 'parent menu item',
   +        'test_link' => 'menu-block-test/hierarchy/parent',
   +      ],
   

   Might make more sense to order these 4 cases from /parent to parent/child-1/child-1-2 and leave the "empty" one last?

Do we actually need empty() here, or can we use NULL coalesce?

Using coalesce will throw a notice if the array index doesn't exist right? Is this a problem for test bot?

+ public function getBlockLabel() {

Does this need to be public?

In fact, now that you mention it, I think this actually just override label().
Overriding label() breaks everything.
I think it should stay public though, because this is the only way callers would be able to get the user-facing block label.

Rerolled per feedback.

+++ b/tests/src/Functional/MenuBlockTest.php
@@ -458,4 +459,86 @@ class MenuBlockTest extends BrowserTestBase {
+      $test_link = empty($option['test_link']) ? 'menu-block-test/hierarchy/parent/child-1/child-1-1' : $option['test_link'];

Could be $test_link = $option['test_link'] ?? 'menu-block-test/hierarchy/parent/child-1/child-1-1';

Using coalesce will throw a notice if the array index doesn't exist right? Is this a problem for test bot?

Tested on my local, no problem.

Back to "Needs Work" because this tripped a11y tests in my profile.

I can't tell from the conversation above when or why menu_block_preprocess_block was added, but we should not be setting label to empty string. As in core, label_display should be used to determine label visibility in the twig template. CSS should be used to control label visibility, because label is important for screen readers.

Is there any objection to removing menu_block_preprocess_block, which seems tangential to the purpose of this patch in the first place?

--- a/menu_block.module
+++ b/menu_block.module
@@ -105,3 +105,25 @@ function menu_block_theme_suggestions_menu(array $variables) {
 
   return $suggestions;
 }
+
+/**
+ * Implements hook_preprocess_hook() for "block".
+ *
+ * Set the block label with the #menu_block_configuration label if it exists.
+ */
+function menu_block_preprocess_block(&$variables) {
+  if (isset($variables['content']['#menu_block_configuration']['label'])) {
+    $config_label = $variables['content']['#menu_block_configuration']['label'];
+    // Some block twig templates (especially classy + bartik from core) use
+    // `{{ configuration.label }}` to print the label. Others just use
+    // `{{ label }}`. Therefore, we have to set both template variables for
+    // this to work consistently.
+    $variables['label'] = !empty($variables['configuration']['label_display']) ? $config_label : '';
+  }
+  // Even without $variables['content']['#menu_block_configuration']['label'] we
+  // need to change the label. For example, this happens when there are no menu
+  // items to render. But we still need to set the block label.
+  if (isset($variables['label'])) {
+    $variables['configuration']['label'] = $variables['label'];
+  }
+}

PS. here is the citation from block--system-menu-block.html.twig

* Headings should be used on navigation menus that consistently appear on
* multiple pages. When this menu block's label is configured to not be
* displayed, it is automatically made invisible using the 'visually-hidden' CSS
* class, which still keeps it visible for screen-readers and assistive
* technology. Headings allow screen-reader and keyboard only users to navigate
* to or skip the links.
* See http://juicystudio.com/article/screen-readers-display-none.php and
* http://www.w3.org/TR/WCAG-TECHS/H42.html for more information.

@AaronBauman re: #149 + #150:

I added menu_block_preprocess_block() in #87. I think we still need that. It's not tangential to this patch, it allows the patch to set the active label without changing the block's label configuration.

However, @Steven Buteneers in #98 + #99 added the part where we set the label to '' in some cases. That's probably the part that needs to be reworked to address the problems you discovered at #149.

So, basically, we need a solution that satisfies both #98 and #149.

Thanks!
-Derek

OK, great, thanks for untangling the thread.
Again, based on the documention in block--system-menu-block.html.twig:

* - label: The configured label of the block if visible.
* - configuration: A list of the block's configuration values.
* - label: The configured label for the block.

I think the solution is along these lines:

function menu_block_preprocess_block(&$variables) {
  if (isset($variables['content']['#menu_block_configuration']['label'])) {
    $config_label = $variables['content']['#menu_block_configuration']['label'];
    // Label is set empty string unless configured to be visible.
    $variables['label'] = !empty($variables['configuration']['label_display']) ? $config_label : '';
    // Configuration label is set regardless.
    $variables['configuration']['label'] = $config_label;
  }
}

I'm shooting in the dark here, because I can't re-create the issue from #98 at all.

@Steven Buteneers are you around to re-test?

@AaronBauman re: #152: Mostly looks great, thanks for resolving the untangling. ;)

A few remaining concerns:

1. +++ b/menu_block.module
   @@ -105,3 +105,18 @@ function menu_block_theme_suggestions_menu(array $variables) {
   +    $variables['label'] = !empty($variables['configuration']['label_display']) ? $config_label : '';
   

   Should this include @see pointing to the block.module example this is lifted from?

2. +++ b/menu_block.module
   @@ -105,3 +105,18 @@ function menu_block_theme_suggestions_menu(array $variables) {
   +    // Configuration label is set regardless.
   

   Shouldn't this comment explain why we need to do this? None of us are going to remember all the details we just learned in a few months. ;)

3. +++ b/tests/src/Functional/MenuBlockTest.php
   @@ -458,4 +459,86 @@ class MenuBlockTest extends BrowserTestBase {
   +      'settings[label_display]' => TRUE,
   

   Given all the confusion over this, let's add some test cases that twiddle this value, too. And even if it's FALSE, we should assert that the invisible accessible label is still in the markup.

Thanks again!
-Derek

I'm working on #153, I'll post a new patch in a bit...

This fixes all of #153.

I also realized that since we're optimizing this test to not use a @dataProvider, we need to use the custom assertion messages to provide context. Otherwise, if something goes wrong, there's no way to know which iteration we were on that failed. See #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for more. ;)

So now, if you artificially break something, phpunit will print out something like this:

There was 1 failure:

1) Drupal\Tests\menu_block\Functional\MenuBlockTest::testMenuBlockTitleOptions
Test case 'menu root 2' should have the right title.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'parent menu itemz'
+'parent menu item'

Otherwise, you'd just see:

There was 1 failure:

1) Drupal\Tests\menu_block\Functional\MenuBlockTest::testMenuBlockTitleOptions
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'parent menu itemz'
+'parent menu item'

and you couldn't tell which setting was broken...

I think the comments are reasonably clear now, but if anyone wants to further flesh them out, please do. ;)

Almost there! I don't feel qualified to RTBC this issue -- anyone else want the honors?

Thanks!
-Derek

Re: #155:

+++ b/menu_block.module
@@ -105,3 +105,23 @@ function menu_block_theme_suggestions_menu(array $variables) {
+    // as hidden text for assistive technologies (for templates that handle this).

Whoops, this line is longer than 80 chars...

Attached patch fixes that.

Looks there's no reason to introduce new public method setEntityTypeManager - property could be assigned directly in constructor

@andypost re: #157: See https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

We don't want to have a constructor, so we're using setter injection via create(). Since create() is public static, it can't access protected members, hence we need a setter.

Oh, slick! Upon further investigation, and thanks to @alexpott's comment at https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl... pointing to https://3v4l.org/KSYat I was wrong in #158. Because of some magic about static factory methods I don't fully understand, we don't need the public setter, and can access protected methods directly from create(). Much cleaner. Yay! Thanks, @andypost for raising it. ;)

Related: opened #3133102: Remove unneeded public MenuBlock::setMenuParentFormSelector() method