Hide menu block on pages not included in menu

I have tested and it works well. I think we can mark as RTBC. Thank you.

This could use a test to make sure it's doing what it says on the tin, also I'd love if the checkbox label didn't have a markup in it (strong tags), and finally but most important, I'd really like an issue summary update with a scenario where this is useful as I can't picture it well enough with the description

It works well for me too with last dev version.

Thanks @MattDanger.

The patch wasn't applying to the latest 8.x-1.x-dev version of the module for me. I've re-rolled the patch to work with that version. I've also attempted to add a test for the hide flag, but I'm not getting any of the tests to work for me locally, so I expect the test to fail. Any help on getting the test to work correctly is welcome.

After some test, maybe this patch only works for the whole main menu.

If I select a menu item in "Fixed parent item", the block is also displayed in the pages of the other "Parent Item" of the main menu even if they are not part of the parent item selected in "Fixed parent item".

Do I have your use case correct?

Menu A
+- Parent Item 1
  +- Child Item a
  +- Child Item b
  +- Child Item c
+- Parent Item 2
  +- Child Item d
  +- Child Item e
  +- Child Item f

Block configuration
Fixed parent item = Parent Item 1
Hide on pages not in menu = true

The page linked to Child Item e is loaded.
Since Child Item e is not part of the menu tree with the root of Parent Item 1 and the 'Hide on pages not in menu' checkbox is true, the menu should not display.

If this use case is accounted for, I think modifying the checkbox label and description language from 'menu' to 'menu tree' to makes the functionality clearer.

Added a bunch of tests for this new feature, but not sure I totally understand what we're trying to do with this or how the effects impact existing features.

Sniff fixes.
Whitespace changes only.

Fixing derp syntax issue from #13

Looks like this patch is breaking lots of things and needs a re-think

I wanted to propose an updated description/use-case in response to Joel's request in #5. If this is accurate, it might also help with the thinking around testing. All the tests so far rely only on the mocked menu structure — to test this option, I think we'll need to create an actual node that's not included in the menu hierarchy to verify the menu block does not display.

Feedback is welcome!

Proposed Description
Don't render the menu block if the current page is not included somewhere in the full menu tree. This is regardless of any other options set to affect which links to render (fixed parent, starting depth, limit, follow, etc.). With this option enabled, you could further limit the block display with a visibility condition as needed.

Use case:
When building a menu for a site section or small collection of pages that are all linked in a given menu, this feature allows you to simply specify "don't show the menu block anywhere except on any page that belongs to this section" — i.e., any page linked somewhere (anywhere) in the menu.

In this case, 'hide on inactive' becomes a simpler, more deterministic alternative to the typical approach of limiting block visibility based on path — e.g. set a visibility condition to display the block only on pages in /my-section, /my-section/*. Paths can change, or be set incorrectly, and cause the menu block to be shown/hidden unexpectedly. On large sites with many sections, and/or different content types with various methods of path generation, setting visibility conditions based on paths can become somewhat fragile and difficult to maintain.

Looks this feature will not work as expected with menu links that use the <front> route. This is due to a long-standing issue with the core menu system #1578832: [already commited but followup for CR?] <front> menu links are missing active trail classes. Comment #75 provides a good summary of the root cause:

Which menu items get flagged as active comes from Drupal's menu.active_trail service. That service uses the current page's route match to get the page's route name and parameters, and invokes the aforementioned function [MenuTreeStorage::loadByRoute] to get the active link, and then it gets that links parents to build the rest of the trail.

The current page's route match will never, ever be the <front> route, it will always be what the actual route object is for that page in the system.

Basically, <front> is never included in the active menu trail. The 'hide on inactive' feature (as currently implemented) relies on the active trail to decide on whether to continue to build the block, or just return early (lines 237-240):

// Check if the active trail is empty.
if ($config['hide_on_nonactive'] && count($parameters->activeTrail) <= 1 ) {
  return [];
}

So, if you have a menu with a link to <front>, 'hide on inactive' enabled, and you're viewing the front page, the menu block will not display even though the front page is included in the menu. This explains why test assertions like the following are failing:

// Check that hide-on-active did not break other settings.
$this->drupalGet('<front>');
$this->assertSession()->pageTextContains('parent menu item');

If all this is correct, three options come to mind:

1. Leave the implementation as-is, and note in the feature description that it doesn't work for <front> menu links. Re-work tests in light of this.
2. Find an alternative way to implement the feature.
3. Postpone pending resolution of #1578832: [already commited but followup for CR?] <front> menu links are missing active trail classes.

The first one seems like a non-starter.

There might be cases where the current page is not in the menu and there's still an active trail (e.g., when using the Menu Trail by Path module).
Attached patch calls $this->menuActiveTrail->getActiveLink() instead of checking the length of the trail.

Not sure if this also improves the situation with links to the <front> route mentioned in #19.

I manually tested #20 — the $this->menuActiveTrail->getActiveLink() approach works great AND it resolves the <front> route issue from #19. Thanks @mrshowerman!

I can't figure out why the tests are failing, though. Most of the failures are in tests not related to this feature, and I can't see anything that would cause them to fail. As I said, works fine in manual testing.

Attaching patch that has this new approach without the changes made in 8, 11, 13 and 14. If the 'old' tests pass with the new feature code in place, I'll take another pass at some tests for this feature.

(No diff, as interdiff would not play ball.)

So much for that theory ¯\_(ツ)_/¯

FWIW, here's my pass at a patch that includes new tests for this feature that I think are more consistent with the intended functionality, and should work (but won't) 😀.

The existing tests were failing because 'hide_on_nonactive' setting was not in the schema.

Attached patch corrects that, makes a couple minor code improvements, and adds tests that accurately reflect (IMHO) the intended use of this feature.

Tests are passing, so setting to NR.

I also took a crack at updating the issue summary as requested in #5.

Removing needs tests and issue summary update tags.

#28 works nicely in our environment, the hide_on_nonactive config key is now stored correctly as true instead of 1.

+1 for #28

Thanks all for your patience, I have committed this to the dev release for inclusion in the upcoming release.