Convert global menus (primary links, secondary links) into blocks

+1 to this.
I see this implementation different. @EclipseGC proposed some time ago a discovery for menus like ctools does for now.
While I involved in #1814916: Convert menus into entities I'd like to mention about inconsistency in menus - we have 2 different kinds of menu menu_list_system_menus() and menu_get_menus()... suppose if we break BC then each menu-edit form should have a checkbox "Provide a block"

Blocks as plugins will already provide menus as blocks. Primary/Secondary menus are specially styled menus that are currently only usable at the page tpl level.

This patch is specifically breaking those elements out into reusable blocks, and providing bartik specific overrides (as bartik actually does do different styling for these).

Since 'primary' and 'secondary' links are expressions of 'main' and 'user' menu, aren't they already blocks? If bartik would have them assigned to regions during install, as other blocks are, the static code in page.tpl wouldn't be needed. And since bartik's block/region mapping is given to all new themes when enabled, these would also have them by default. I can see Seven's need for the static code in page.tpl but I really don't see the need in any other theme.
As to 'special' styling, all menus are styled to meet design needs for every theme, as bartik already does. It seems simpler to me to assign User and Main menu to regions in bartik during install than to add code to make something a block when it already has a block implementation.

Drupal has long had a primary and secondary menu theme element. This is not new, we are not proposing anything new, we are simply porting what already exists. Also Primary and Secondary are actually pointers to other menus that can be configured (including pointing both to the same menu allowing for secondary displaying sub menu items for a given parent. These are obviously similar to menu blocks, but not the same. Unification of some sort might ultimately be wise, but likely not at this time. We need to get the existing feature set ported, not debate how to rewrite the feature set.

Eclipse

Note:

- Primary + secondary links have lost their meaning over years.

- The way Bartik outputs them is more akin to modern themes.

- The actual output of those menu links is not special; they are menu link trees starting on a certain level and limited to 1 level.

- The actual difference to other menu links is a special context/data-source: Secondary links render the 2nd level of links of a menu tree, but only if the corresponding parent link of the 1st/parent level is active.

- That special behavior is the default, but it can be re-configured on the Menu settings page.

Please also note that one of my primary goals for D8 was #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, since Menu Block module replaces the entire primary/secondary links plus all menu blocks with configurable menu block instances, which are able to solve a dozen of different real world use-cases.

However, work on that issue has stalled, since it requires a notion of block instances, which is supposed to be introduced by Blocks & Layouts, and which I did not want to (temporarily) duplicate. But as soon as that is available, moving Menu Block's functionality into core and killing primary/secondary links altogether is what we actually want and need to do.

I'd suggest to do the following:

1) Change the default plugin implementation to a MenuLinksPlugin, which just renders a single menu with a depth limited to 1, and which gets the following plugin settings injected:

- menu_name to render
- the starting level to render (0 or 1)
- (id + class) HTML attributes to apply to the rendered output

2) Make what Bartik is doing (separate menus) the default, so it doesn't need to override it. Stark doesn't care, Seven doesn't output these links to begin with.

That will get us prepared for the further settings we want to add for menu block plugin instances and a good step closer to ditching the special concept of primary/secondary menu links ASAP.

Lastly, I also want to note that I'm a little scared that the existing patch here tries to implement plugins in a theme... First of all, I don't think we can expect this level of familiarity with PHP from themers, so that rings some alarm bells from my perspective. ;) I'm confident that we need different and more easy ways for themers to override things, which is probably a discussion topic for the plugin system of its own. Secondly, Drupal does not handle theme extensions in a reliable way currently, which essentially means that they can come and go at will. AFAIK, the plugin system needs reliable plugin implementations.

I wasn't saying eliminate primary and secondary links. I am saying that bartik and many other themes don't need them. Those who do can still add the static code, or whatever evolves in drupal 8, to their templates and have the special behavior of these menus. I just think that code is unnecessary and disturbing to newcomers in bartik, which is the 1st theme that newcomers look at. It is unnecessary to convert them to blocks, just make them available by default to themes, as there is no way for a new theme to assign a block to a region before it is first enabled.

Suppose if we make #1814916: Convert menus into entities in then menu module could introduce this kind of block instance plugins. The only question in context of menu entities - what module should hold the implementation of menu entities.
The menu module could just make UI to manage menus and their blocks

EDIT related #1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()

#503810: Convert primary and secondary menus to blocks is an antecedent of this issue and has some interesting reading. But it is not a duplicate. Shouldn't it be resolved 1st?

That one advocates getting rid of primary/secondary links. If that were so, then this would simply about assigning 'main' and 'user account' menus to bartik during install.

Read the patch, and I don't think the consensus there was as stated in #60.

That sounds very reasonable. But if the primary/secondary link code is not going to be removed does anything need to be done? I can use 'Main' and 'User Account' menu blocks in my themes now, but it would be easier if Bartik also used them so that the usage would be 'inherited'. I would think Stark(through system page.tpl) would still use primary/secondary links.

How about something along these lines? I did not comment/test anything here, this is just a gut reaction to the current state of things. As all theme components are moved to blocks the preheader region I added to bartik can be refolded back into header. the main_menu region I added could also likely be removed, but for the sake of clarity might be best if kept. All of these issues are fairly irrelevant to this specific issue but matter to the whole discussion. Also, when using the new block I added here, it becomes pretty obvious that we need to be pushing block title up to administration which might solve a few things elsewhere but is outside the scope of this issue.

Eclipse

I think that's the overall direction we want and need to go. :)

That said, I think it's too limited/focused. I think we should have marked this as a duplicate of #503810: Convert primary and secondary menus to blocks instead. We're repeating the entire discussion. :-/

In essence, the conclusion in the other issue(s) was long before that we need to get rid of all of this, entirely. Instead, we have a single MenuBlock plugin type, which comes with (almost all of) the settings that the contrib Menu Block module provides.

Reality is, that's what everyone actually needs. The entire progress on #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables was only halted, because it would have had to introduce a stop-gap block-instance-alike API just for menu blocks, and I didn't want to waste any time on duplicating that effort with a temporary API that gets deleted later on.

Therefore, if we really want to do ourselves a favor, then we skip all of the intermediate steps in between and directly hop onto the actual, final need of site builders and users, which means:

1. A single MenuBlock plugin type.
2. For each instance, you configure the menu_name to use. (not derivatives)
3. Each instance defines the minimum + maximum levels (depth) of links to build in the menu tree. ("Starting level" + "Maximum depth")
4. Each instance defines whether the min/max tree should be output expanded; i.e., ignoring the active trail of the current page, and expanding all links + sub-links in the visible levels.
5. "Secondary links" support: Each instance supports a "follow" setting, which essentially means that the "Starting level" follows the currently active menu link. In essence, that's what resembles the current secondary links: If the current active link is "About" in menu 'main', then the secondary links show the children of "About" in the 'main' menu.

@see menu_block.admin.inc

I'm fairly sure that we should be able to copy/paste a lot of menu_block's code. There have also been a range of menu link tree building functions in the meantime (most notably, menu_build_tree(), which allows for custom, parametrized link tree builds) that should make all of those configuration parameters a piece of cake to implement.

In summary: Instead of introducing a MenuNavigation block plugin type, in addition to the already existing menu blocks in HEAD, we should get rid of them altogether, and replace them with a single MenuBlock plugin type that all of us actually need and want. :)

I'm happy to help. :) And I hope that @JohnAlbin is available, too. ;)

So, the MenuNavigation block I just wrote in that patch does everything you asked for except for min/max depth. It just expects a single level. I think we might have to render it differently if we do the variable depth thing, but I understand why you want it. Otherwise, there's not really a need for the "follow" feature as the methodology for doing that doesn't actually have to know about another block on the page. If it can display items of the 2nd level for a menu (because the current page is part of that menu and has children or whatever) then it will.

I'm open to what you're discussing, however I think the patch I just provided is a pretty good one for one replacement of what's in core, it's small, and won't creep scope wise. If we fixed the tests that are breaking here and added a few new ones, it could probably be committed in the relatively near future, and we'd ALMOST have menu_block in core as well. Expanding on what that plugin does and removing the other plugins that provided similar functionality would be a good follow up IMO.

Eclipse

Hm. This should pass some more tests, or at least reduce the amount of exceptions...

But there's a critical problem:

Thousands of tests expect at least the "Account links" (account) menu to appear as secondary links, by default, in all themes, including Stark.

With this patch, that's no longer case, since 1) Block module is optional, and of course, 2) the block instance config to place the account menu doesn't exist.

Speaking, very unhelpfully, from a Testing system perspective, less baked-in assumptions and defaults are great ;) However, that doesn't really help ;)

FWIW, I pushed this as menu-links-1869476-sun branch into @EclipseGc's Blocks/Layout sandbox.

@Gábor: I'm not sure my point came across. ;)

The critical problem is that, as of now,

1. Block module is not a required module. (Perhaps it should be one?)
2. There is no default configuration that would place the secondary links as before.
3. Pretty much the entire core test suite relies on the secondary links to contain the account menu links, which is a unique indicator for whether a user is logged in or not ($this->assertText(t('My account'));).
4. Without the previously existing secondary links, there is no other indication in the HTML markup for pages being requested by authenticated users that would indicate whether the user is logged in or not.

Removing this dependency from tests is a mammoth undertaking, similar to the size of #1541298: Remove Node module dependency from Testing profile.

Adding Block module as required module (via .info), or adding Block module to the Testing profile, and providing default configuration via Block module or Testing profile, would be a stellar, ugly idea. ;)

Postponing on #1894692: Allow to identify whether a user is logged without presence of Account menu links as secondary links

@sun

ugh, that was a much clearer communication, and I didn't like anything you said (not your fault, just sucks that the tests are dependent on that). Understanding that this is postponed based upon your very relevant points here, is the basic functionality still an ok target for a first pass in your opinion?

Eclipse

Yes, I think it's definitely a step forward. I'd agree with trying this first, but my secret hope still is that, once this patch is green, that it should be a no-brainer to merge the new MenuNavigationBlock class into the existing MenuBlock (since those two would really be strange duplicates to each other), which in turn would bring us to ~50% of menu_block in core already. The other configuration settings can be added in a follow-up issue. Of course, the merging could also happen in a separate follow-up issue, but I'm allowed to have my secret hopes, right? :)

Will the tests still fail if Bartik is modified to use 'Main Menu' and 'User Account Menu' blocks instead of primary/secondary links? It would remove a lot of code from bartik. Is this feasible as a separate issue?

@dcrocks: Yeah, they will still fail. The issue is not Bartik, nor the Standard profile. Instead, the issue is with any test that is not using the two, which means >90% of all tests. ;) Meanwhile though, I've provided a patch for #1894692: Allow to identify whether a user is logged without presence of Account menu links as secondary links. With that in place, the total amount of failing tests should significantly decrease here.

So the focus is still on removing them altogether. My interest is just in removing Bartik's use of primary/secondary links. Thanx for the insight.

#22: menu.links_.22.patch queued for re-testing.

Hrm. Initial attempt to make the OpenID tests work, but this is non-trivial to resolve. Already spent a good chunk of time on those, still looking into it.

To get a better grasp of the new block plugins, I skipped over the broken OpenID tests for now, and instead, moved on to the UserAccountLinksTest, which apparently is a dedicated test class for the former secondary links functionality.

I made sure to document the melting of my brain in #1871696-123: Convert block instances to configuration entities to resolve architectural issues and following comments, in case anyone is interested. :)

Now, with that being grilled, this Hawaii Toast should at least show a green light for UserAccountLinksTest. ;)

Overall, I really like the direction this is going. Especially the changes to the template files :)

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/block/block/MenuNavigation.phpundefined
@@ -0,0 +1,120 @@
+  public function settings() {
...
+  public function blockForm($form, &$form_state) {
...
+  public function blockSubmit($form, &$form_state) {

Missing docblocks

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/block/block/MenuNavigation.phpundefined
@@ -0,0 +1,120 @@
+    $configuration = $this->getConfig();

You can just use $this->configuration here

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -636,13 +632,20 @@ protected function drupalLogin($account) {
+   * Asserts whether a given user account is logged in.
+   */
+  protected function assertUserIsLoggedIn($account) {

Missing @param

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.phpundefined
@@ -29,6 +29,15 @@ public static function getInfo() {
+    $this->block = $this->drupalPlaceBlock('menu_navigation', array('region' => 'account'), array(
+      'menu' => 'account',
+      'level' => 0,
+      'id' => 'secondary-menu',
+    ));

@@ -58,11 +67,16 @@ function testSecondaryMenu() {
+    $settings = $this->block->get('settings');
+    $this->assertNoRaw($settings['id']);
+    // The entire menu block should not appear either.
+    $this->assertNoRaw(check_plain($this->block->label()));

All of this block stuff looks good to me

+++ b/core/themes/bartik/template.phpundefined
@@ -151,3 +151,39 @@ function bartik_field__taxonomy_term_reference($variables) {
+ * Implement hook_block_view_alter().

Implements

+++ b/core/themes/bartik/template.phpundefined
@@ -151,3 +151,39 @@ function bartik_field__taxonomy_term_reference($variables) {
+function bartik_block_view_alter(&$build, $block) {

These should be typehinted, array and Drupal\block\Plugin\Core\Entity\Block respectively (with a use statement)

+++ b/core/themes/bartik/template.phpundefined
@@ -151,3 +151,39 @@ function bartik_field__taxonomy_term_reference($variables) {
+  if ($block->get('id') == 'bartik.primary_navigation') {

$block->id()

+++ b/core/themes/bartik/template.phpundefined
@@ -151,3 +151,39 @@ function bartik_field__taxonomy_term_reference($variables) {
+    $build['#block_config']['subject'] = '';
...
+    $build['#block_config']['subject'] = '';

Is this also for mimicking existing functionality? This makes me really uncomfortable, I could picture someone angrily filling in the block label over and over wondering why it doesn't take effect. Maybe we should form_alter out the label for these?

+++ b/core/themes/bartik/template.phpundefined
@@ -151,3 +151,39 @@ function bartik_field__taxonomy_term_reference($variables) {
+    $build['#prefix'] = '<div id="main-menu" class="navigation" role="navigation">';
...
+    $build['#suffix'] = '</div> <!-- /#main-menu -->';
...
+    $build['#prefix'] = '<div id="secondary-menu" class="navigation" role="navigation">';
...
+    $build['#suffix'] = '</div> <!-- /#secondary-menu -->';

Eww. Is this temporary? Just to mimic the current markup? If so, let's add a comment about that. And can prefix/suffix be moved to each other

Eww. Is this temporary? Just to mimic the current markup? If so, let's add a comment about that. And can prefix/suffix be moved to each other

I think the appropriate thing to do here is to provide a different theme wrapper, so yes, I'd consider them temporary (and the wrapper probably in a followup).

Eclipse

Hah, OK, nice, that explains the increase in the test failures:

The HTML ID secondary-menu is unique.
	Drupal\system\Tests\Ajax\MultiFormTest->testMultiForm():80
Initial page contains unique IDs
	Drupal\system\Tests\Ajax\MultiFormTest->testMultiForm():80

I guess I'll simply slap a drupal_html_id() onto the #attributes][id of the block in the next reroll, but I'm very certain that I actually opened a can of worms there :-D

After applying the latest patch, should I be able to see the primary links & secondary links blocks here:
admin/structure/block/list/block_plugin_ui%3Abartik/add

If so I must be doing something wrong.

It is there but as one block only. It is called 'Menu Navigation'. In this block you can configure the target region, source menu, and the number of levels to display. In the 'Main Navigation' and 'User Account menu' blocks you can only configure the target region. It appears the 'special' behaviors of primary/secondary' links are preserved. Perhaps there should be a better name, as this one doesn't stand out from 'Main Navigation'. The other 2 blocks are named after the menus they display. It seems this one should contain the word 'Menu', as it does dish up menus, and I know there is a concern about including the word 'links', perhaps just 'Menu Display/Print/??
It is interesting that on the Menu module settings page there is no longer any defaults set. Does this form fit any purpose anymore?

Actually I would vote for 'Display a Menu', since this block can display any menu available to the site.

Could the new regions added to Bartik have more generic names since I can punch whatever menus I like into those blocks? Maybe 'Top of Page Menu Bar' and 'Top of Content Menu Bar' or something similar?

This may not belong here but I notices this as I was playing with this. If you have a block currently assigned to a region but then go into Add Block and assign the same block to a new region, without changing its 'Title', it simply gets moved from the old region to the new one. Reasonable I guess, but if the intent was to ADD a block, not just MOVE it, then it may be a somewhat surprising result. I also noticed that if a block was once assigned to a region but is no longer assigned to any region, its config .yml file stile exists in sites/default/../active directory.

Would it be conceptually simpler to take the block you used for 'menu-navigation' and use it to build all the menus in drupal? Is there any feature of any of the core 4 menus that couldn't be built with this block? It would seem to add flexibility and, I think, be simpler conceptually to drupal developers.

I'm not sure I am being clear. I am trying to say that all the core menus would be assigned to a region through this block. Now, every menu, the core 4 or any new site menus, show up in the 'add blocks' table as unique blocks. Adopting this would mean only one block, menu-navigation, would show up in the 'add blocks' table, and it could be used to place any menu in any region. This can be done with the patch as it currently exists so it seems wasteful and confusing to have these other menu blocks also in the list. When done, what is the difference between using the 'add blocks' form to place the 'tools' block in a region and a 'menu-navigation' block that selects the tool menu?

Besides making the 'add blocks' list shorter and simpler, it would also mean that creating a menu wouldn't also create a block.

That's outside the scope of this issue, but likely the answer is yes. The bigger problem here is that you want a way to display them through different theme functions if you're going to do that, and getting a list of theme functions that support rendering menus isn't something drupal supports right now (and is something we could REALLY use and for more than just menus).

Eclipse

Hm. I think I disagree there. The Navigation menu (btw, it's called Tools now) on its own just represents the data source, a menu/links tree.

1) If you create a block that uses this data source, then you should be able to configure how the data source is handled and rendered for the particular block instance you have.

2) You can have multiple block instances for the same data source.

3) The underlying idea of #46 would rather translate into "a block instance of a block instance". That's a configuration and dependency territory that I don't think we want to get in.

4) Instead, it's much simpler to just be able to create multiple block instances for the same data source, but configurable them differently.

5) The actual output is (or should be) always the same: A HTML bullet item list (UL/LI). This is universally true. The only thing that makes any difference is the CSS class that is applied to the wrapping container: That's what gets you from "a list of links" to menu link trees to local tasks to local actions to contextual links to dropbutton links to entity action links to pagers to... etc.pp. — In turn, we're circling back into many related discussions: We need a Theme Component Library, and, as already mentioned here, elsewhere, and in #503810: Convert primary and secondary menus to blocks, we essentially need a way to "classify" blocks, so a theme is able to apply discrete styles for particular block, whereas the raw HTML markup does NOT change, only the CSS class of the block does.

I think what this is is a block instance of a data source of a particular type. If the HTML gave instance specific information as well as data source type I would probably have all I need for my CSS. Ie, instance is menu block called X in region A of data source type 'tool menu' I can write specific CSS as well as use generic menu type CSS. In any case if this patch goes through as it stands I do have the generic menu assignment block I want, plus a few data source specific blocks I will probably ignore.

Thanks for the clarification @dcrocks, I found it.

Should there be a checkbox in the UI to hide the block title using element-invisible? I think that's going to be a common request and users aren't going to want to dig through the CSS to remove them that way. Would be nice if it were just a simple option when adding/editing a block.

@mgifford: #1875260: Make the block title required and allow it to be hidden

Tagging.

This patch #1079010: The secondary links heading, "Secondary menu", is wrong is marked RTBC and was ready to go, but is waiting on this issue here as it would obviously over-ride it.

I haven't seen a lot of movement on this in the last week though, so what should I do about the patch that was ready to go?

1. Updated Standard menu navigation blocks for latest Block API changes.
2. Updated block_test_theme for new account and main_menu regions.
3. Fixed Ajax\MultiFormTest; menu navigation block HTML IDs must be unique.
4. Various clean-ups (#36).

Please note:

The bartik_block_view_alter() override generally looks bogus/needless to me. However, we're currently lacking a proper way for block plugins to control and adjust the 1) wrapping block markup (e.g., via buildWrapper()), as well as 2) .element-invisible for block instance subjects. These two details are discussed in at least two other issues already.

#54: menu.links_.54.patch queued for re-testing.

1. Fixed UserPasswordResetTest.
2. Fixed Views Wizard/MenuTest.
3. Fixed MenuRouterTest.
4. Fixed MenuTest.
5. Fixed OpenIDRegistrationTest.
6. Various clean-ups.

With #58, hopefully only the upgrade path test failure will remain. And, that one actually gets very very tricky:

1. Primary/secondary links were previously built-in and essentially provided by System module (which always exists and is always enabled).
2. The new menu blocks are provided by Menu module. Menu module may not be enabled.
3. Even if Menu module was enabled, the new blocks can only appear if Block module is enabled. Block module may not be enabled.
4. And if that wasn't enough already, a menu block can only be auto-generated during the upgrade path, if we know 1) for which theme(s) and 2) in which region of each theme.

As a result, the upgrade path would have to look like this:

1. The update is performed by System module (the former/original owner).
2. Block and Menu modules are enabled by the update.
3. For each theme that exists, we determine the first available region, and a block configuration is written (as raw config, without going through any Menu, Block, Entity, or Plugin system APIs).

The only alternative to that would be to skip the upgrade path and require all sites to (re)configure/place their primary/secondary links blocks. After upgrading, you'd essentially end up with a UI/output that is similar to the Testing profile used by tests: The lack of a main menu/primary links is probably not a big deal — but the lack of the account menu/secondary links very potentially is :-) One possible way to work around the situation would be to change the final link in update.php (that gets you back to the updated site) to point to the /user path instead of the front page. That way, you at least find your way to the login page ;)

Oh lovely to see that block entities/plugins are completely ignoring default block plugin settings...

That inherently means that our test coverage for blocks is very incomplete and fragile — if default settings are not merged, then settings may be undefined, so either all runtime code is currently using conditional fallback values, or lots of tests will blow up as soon as you merge in default settings in the block manager.

We've been generally categorizing conversion issues as tasks, not features. If there's a reason this needs to be considered a feature, please explain.

@sun please re-roll #61 and what's left here to finish conversion?

Bump. I was just checking into #1079010: The secondary links heading, "Secondary menu", is wrong and realized that this issue's still hanging.

just noting that this is a blocker for SCOTCH, and the work i'm currently doing in our princess branch.

I'd love a standalone patch that did this, if it's easy to separate. This has value even outside of SCOTCH.

This isn't in Spark's wheelhouse from what I can tell. Untagging.

@webchick I thought Spark wanted to put contextual links everywhere? Isn't this part of that effort.

Maybe eventually? But not really right now. We're trying to get our own features/problems done, rather than other peoples' atm. ;) I think it makes more sense as part of the "blocks everywhere" initiative since it's, well, putting blocks everywhere. :D

just for reference I completely agree with #70

Re-rolled against head

Cleanup for changes to annotation namespace

Should be green, passes locally

Screenshot of bartik

Fixes two comments longer than 80 chars identified by @swentel

ok, i've been through this. i'd feel better having someone who's actually GOOD at knowing all the coding standards, etc., RTBC it, but technically, it's good, so i'm gonna do it anyway. the only issue with it is the two new regions it introduces into Bartik to accommodate the new blocks, since they're really intended to just be single-purpose regions.

fortunately, we have a better solution for things like this in princess :)

There's a bit more here than coding standards issues. The changes to test coverage are especially worrisome.

+++ b/core/includes/menu.incundefined
@@ -1747,96 +1747,6 @@ function menu_list_system_menus() {
-function menu_navigation_links($menu_name, $level = 0) {

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/block/block/MenuNavigation.phpundefined
@@ -0,0 +1,129 @@
+  protected function blockBuild() {

I don't think we should kill this generic function and relegate it to a protected method of a block plugin.

Yes, we don't want to call out to a procedural function, but I think we should for now and move that code later.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -164,6 +166,7 @@ function addCustomMenu() {
+    $this->container->get('plugin.manager.block')->clearCachedDefinitions();

Is this needed? If so, needs a comment

+++ b/core/modules/menu/menu.moduleundefined
@@ -18,6 +18,7 @@
+use Drupal\menu\Plugin\block\block\MenuNavigation;

Unneeded

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDFunctionalTest.phpundefined
@@ -148,8 +148,8 @@ function testLogin() {
-    $this->submitLoginForm($identity);
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->submitLoginForm($identity, $this->web_user);
+    $this->assertUserIsLoggedIn($this->web_user);

@@ -167,7 +167,8 @@ function testLogin() {
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->web_user->session_id = $this->session_id;
+    $this->assertUserIsLoggedIn($this->web_user);

@@ -179,8 +180,8 @@ function testLogin() {
-    $this->submitLoginForm($identity);
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->submitLoginForm($identity, $this->web_user);
+    $this->assertUserIsLoggedIn($this->web_user);

@@ -220,7 +221,8 @@ function testLoginMaintenanceMode() {
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->web_user->session_id = $this->session_id;
+    $this->assertUserIsLoggedIn($this->web_user);

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDRegistrationTest.phpundefined
@@ -86,8 +86,8 @@ function testRegisterUserWithEmailVerification() {
-    $this->submitLoginForm($identity);
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->submitLoginForm($identity, $user);
+    $this->assertUserIsLoggedIn($user);

@@ -119,9 +119,13 @@ function testRegisterUserWithoutEmailVerification() {
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
-
     $user = user_load_by_name('john');
+    // @see WebTestBase::drupalLogin()
+    if (isset($this->session_id)) {
+      $user->session_id = $this->session_id;
+    }
+    $this->assertUserIsLoggedIn($user);

@@ -129,8 +133,8 @@ function testRegisterUserWithoutEmailVerification() {
-    $this->submitLoginForm($identity);
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
+    $this->submitLoginForm($identity, $user);
+    $this->assertUserIsLoggedIn($user);

@@ -260,9 +264,13 @@ function testRegisterUserWithAXButNoSREG() {
-    $this->assertLink(t('Log out'), 0, 'User was logged in.');
-
     $user = user_load_by_name('john');
+    // @see WebTestBase::drupalLogin()
+    if (isset($this->session_id)) {
+      $user->session_id = $this->session_id;
+    }
+    $this->assertUserIsLoggedIn($user);

+++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDTestBase.phpundefined
@@ -38,7 +38,7 @@ function setUp() {
-  function submitLoginForm($identity) {
+  function submitLoginForm($identity, $account = NULL) {
     // Fill out and submit the login form.
     $edit = array('openid_identifier' => $identity);
     $this->drupalPost('', $edit, t('Log in'), array(), array(), 'openid-login-form');
@@ -48,6 +48,9 @@ function submitLoginForm($identity) {

@@ -48,6 +48,9 @@ function submitLoginForm($identity) {
 
     // Submit form to the OpenID Provider Endpoint.
     $this->drupalPost(NULL, array(), t('Send'));
+    if (isset($account) && isset($this->session_id)) {
+      $account->session_id = $this->session_id;

+++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.phpundefined
@@ -72,7 +72,11 @@ function testUserPasswordReset() {
-    $this->assertLink(t('Log out'));
+    // @see WebTestBase::drupalLogin()
+    if (isset($this->session_id)) {
+      $this->account->session_id = $this->session_id;
+    }
+    $this->assertUserIsLoggedIn($this->account);

None of this should be changed, at all. This sort of change should definitely not directly impact Open ID

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -336,9 +336,6 @@ protected function drupalCreateContentType($settings = array()) {
-   * Note: Until this can be done programmatically, the active user account
-   * must have permission to administer blocks.
-   *
    * @param string $plugin_id
    *   The plugin ID of the block type for this block instance.
    * @param array $values
@@ -361,8 +358,7 @@ protected function drupalCreateContentType($settings = array()) {

@@ -361,8 +358,7 @@ protected function drupalCreateContentType($settings = array()) {
    * @return \Drupal\block\Plugin\Core\Entity\Block
    *   The block entity.
    *
-   * @todo
-   *   Add support for creating custom block instances.
+   * @todo Add support for creating custom block instances.
    */
   protected function drupalPlaceBlock($plugin_id, array $values = array(), array $settings = array()) {
     $values += array(
@@ -632,13 +628,23 @@ protected function drupalLogin($account) {

@@ -632,13 +628,23 @@ protected function drupalLogin($account) {
     if (isset($this->session_id)) {
       $account->session_id = $this->session_id;
     }
-    $pass = $this->assert($this->drupalUserIsLoggedIn($account), format_string('User %name successfully logged in.', array('%name' => $account->name)), 'User login');
+    $pass = $this->assertUserIsLoggedIn($account);

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.phpundefined
@@ -38,7 +47,7 @@ function testSecondaryMenu() {
     $this->drupalLogin($user);
-    $this->drupalGet('<front>');
+    $this->drupalGet('');

@@ -58,11 +67,16 @@ function testSecondaryMenu() {
-    $this->drupalGet('<front>');
+    $this->drupalGet('');

Out of scope

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -632,13 +628,23 @@ protected function drupalLogin($account) {
+   * @param $account

Needs a typehint, and @return

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.phpundefined
@@ -29,6 +29,15 @@ public static function getInfo() {
+  function setUp() {

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/MenuTest.phpundefined
@@ -20,6 +22,11 @@ public static function getInfo() {
+  function setUp() {

protected

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.phpundefined
@@ -29,6 +29,15 @@ public static function getInfo() {
+    $this->block = $this->drupalPlaceBlock('menu_navigation', array('region' => 'account'), array(

Is $block a documented object property already? If not, it needs to be

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/MenuTest.phpundefined
@@ -12,6 +12,8 @@
+  public static $modules = array('block', 'menu');

Missing a docblock:

/**
 * Modules to enable.
 *
 * @var array
 */

+++ b/core/profiles/standard/standard.installundefined
@@ -265,3 +266,51 @@ function standard_install() {
+function standard_update_8000() {

Don't we usually start with 8001?

+++ b/core/profiles/standard/standard.installundefined
@@ -265,3 +266,51 @@ function standard_install() {
+  $uuid = new UUID();
...
+  $uuid = new UUID();

Uuid, unfortunately

Gotta love seeing contextual links on these things :)

1. +++ b/core/profiles/standard/standard.installundefined
   @@ -265,3 +266,51 @@ function standard_install() {
   +function standard_update_8000() {
   

   Don't we usually start with 8001?

   8000 is correct, but more to the point, why is there an update function in the Standard install profile? I do not understand why there would be an update function that only sites running this profile would need.

   Plus the update function seems to be assuming the site is using Bartik for some reason...

2. +      <?php if ($page['account']): ?>
   +        <nav role="navigation">
   +          <?php print render($page['account']); ?>
   +        </nav>
   +      <?php endif; ?>
   .....
   +      'account' => 'Account links',
   

   Why is this region called "account links" rather than something like "secondary links" or "secondary menu"? There is no reason account-related links need to be placed there.

3. +      '#title' => t('Starting level'),
   +      '#default_value' => $this->configuration['level'],
   +      '#options' => drupal_map_assoc(array(0, 1, 2, 3, 4, 5, 6, 7, 8)),
   

   I would think it should start at "1", not "0"...

4. +    // Do not output this entire block if there are no visible/accessible links.
   +    // @todo This is insufficient for the real world. Respect #access.
   +    if (empty($links)) {
   +      return array();
   +    }
   

   I am not sure what this means, but "respecting #access" sounds a lot more important than a @todo :)

5. This patch seems to leave behind an entire administration page at admin/structure/menu/settings which now does absolutely nothing?.... I'm completely amazed it managed to pass tests with that :)
6. There is no (real) upgrade path here - that was discussed a bit in comments above, but what happened with the discussion?

This changes those things identified by @timplunkett.
Pretty sure we'll get 68/17 fails/exceptions on open id, even placing the block. No idea why this changes open id.

Even though the default configuration for bartik is to have the 'user account' menu as the default source for the secondary links it does seem confusing to have the region and block label to be called 'Account links' when the secondary menu concept continues to exist in the rest of the code and other menus could be substituted with actual use.
And though menu.settings.yml is virtually empty now it is still used within drupal. The form should be modified temporarily but not dropped as I expect there will be future use.

Plus the update function seems to be assuming the site is using Bartik for some reason...

standard.profile installs bartik as the theme. It needs to take care of updating bartik (unless bartik can take care of itself - I don't think it can). It doesn't know about any other themes the user has installed since.

There is no (real) upgrade path here - that was discussed a bit in comments above, but what happened with the discussion?

We cannot know what a contrib theme will call this region. The upgrade path for that theme will need to handle placing the block in the correct/relevant region. In most cases this will be a new region for that theme. So I guess we need a follow up to a) allow themes to implement update hooks (assuming thats not already possible) and b) remove the standard_update_8000() in favor of bartik_update_8000().

This patch seems to leave behind an entire administration page at admin/structure/menu/settings which now does absolutely nothing?.... I'm completely amazed it managed to pass tests with that :)

The attached patch removes that page, lets see what the tests say. The functions that use those settings are gone too, I think thats why the tests passed. There was only a test for a 200 response on the admin page but nothing else (that I can see). (i.e. we did not have test coverage that I can see) - but lets see what the bot says.

I would think it should start at "1", not "0"...

menu_navigation_links adds 1 to the passed level, we need to start at 0.

I am not sure what this means, but "respecting #access" sounds a lot more important than a @todo :)

Fixed

Why is this region called "account links" rather than something like "secondary links" or "secondary menu"? There is no reason account-related links need to be placed there.

Fixed

Couldn't we just 'empty' the menu.settings form for now and leave an issue to clean it up later before 8 goes final? It's quite possible a contrib module uses the settings form and I plan on using it as well. It seems a waste to remove it then have to restore it. I don't see menu.settings.yml itself going away.

A couple of suggestions. Eventually, everything in the bartik header will be blocks(ref: #1053648: Convert site elements (site name, slogan, site logo) into blocks). Couldn't the secondary links be put in the header region in bartik with enough weight that its block would always be first and then add css to style? Save introducing a new region to d8 that would have to be explained. Also, since main_menu might not always have 'Main menu' in it, why not use a more generic region name in bartik, e.g. 'menu_bar', that is in wide use and is conceptually what this is?
I know these are just cosmetic but it might make things easier down the road.

I'm not going to RTBC this, because I'm still uncertain of the standard_update_8000() parts.

menu_parent_options() indicates that override_parent_selector is on the chopping block anyway, so I'm not worried about deleting that form altogether.

I'm equally unconcerned about the naming of these regions now, as we add more of these, we can revisit the naming.

The tests look good.

Applied cleanly on a fresh install of 8.x. Still think block names main_navigation and menu_navigation are going to cause confusion and wtf's. Sad about the settings form. This issue needs to worry about #1961870: Convert page.tpl.php to Twig and #1938840: bartik.theme - Convert PHPTemplate templates to Twig because whatever lands 1st will cause changes in the others.

Those changes mostly looked good to me.

I still don't see the point of an update function that runs for one combination of install profile and theme only, though... Also, it doesn't seem to be respecting the site's existing settings - just putting two default menus in there?

I would think it could be something more like this (pseudo-code, and some of these functions probably aren't safe to use during updates):

function menu_update_80...() {
  // If the site had main or secondary menu links configured, then for any
  // enabled theme that supports the new 'main_menu' or 'secondary_menu'
  // regions, create a block corresponding to the menu that was used for the
  // links and put it in the appropriate theme region.
  foreach (array_keys(list_themes()) as $theme_name) {
    $regions = system_region_list($theme_name);
    if (($main_links_menu_name = config('menu.settings')->get('main_links')) && isset($regions['main_menu'])) {
      // Load the menu, create a block for it, and put it in the 'main_menu'
      // region.
      // ....
    }
    if (($secondary_links_menu_name = config('menu.settings')->get('secondary_links')) && isset($regions['secondary_menu'])) {
      // Load the menu, create a block for it, and put it in the
      // 'secondary_menu' region.
      // ....
      // (Also if $main_links_menu_name == $secondary_links_menu_name then
      // increment the level of this block by 1 to preserve the previous
      // behavior.)
    }
  }
}

This will not get themes that choose not to support the new default regions, but there's not much we can do about that, at least not now.

Actually, @sun in #59 already outlined something like this, but with some details I was missing (update function needs to be in System module, etc). To keep things simpler perhaps instead of enabling Menu and Block modules if they're not on, the update function could just skip running anything if they're not already on.

I think some sites probably will need to do manual steps here to get things the way they want.

I would think it should start at "1", not "0"...

menu_navigation_links adds 1 to the passed level, we need to start at 0.

I don't think it's a problem if we need to do some simple math behind the scenes before passing data into that function :) Humans looking at the user interface, however, should really see numbers that start counting at 1, not 0....

Yeah that is a better idea, match the configured primary/secondary menus instead of hard-coding account/main-menu.
Will sort the 0/1 issue - agree that is a better UI.
Thanks

Where do we sit with this one in regards to risk-release?
Still worthy of a re-roll or is this lumped with Scotch?
I think it stands on its own.

Ok, rerolling origin/8.x into this.

Had a few conflicts, merging was pretty straight forward.

Still a warning that I haven't been able to fix;

User warning: secondary_menu could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 88 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).

Changes to page.html.twig

+++ b/core/includes/theme.incundefined
@@ -2897,13 +2897,6 @@ function template_preprocess_page(&$variables) {
-  $variables['action_links']      = menu_get_local_actions();
...
-  $variables['tabs']              = menu_local_tabs();

This removes action links and tabs, but doesn't seem to add it anywhere else. Shouldn't those be left for http://drupal.org/node/507488?

Ok, here is a new patch in which I have re-added the items that @tim.plunkett mentioned, removed a preprocess hook that is no longer being used, and modified the CSS so that everything looks the same as it did before.

There is still an issue with the block definitions that needs to be fixed.

#103: menu-blocks-1869476.103.patch queued for re-testing.

#103: menu-blocks-1869476.103.patch queued for re-testing.

Removing the unnecessary B&L tag.

reroll

Not an endorsement, just rerolling after the removal of openid stuff.

Eclipse

vacation brain...

#112: menu-blocks-1869476.112.patch queued for re-testing.

...please don't shoot me, but I think @sun hasn't worked on this since back in March. Unassigning issues when not actually working on them helps get more eyes I think.

Needs reroll as per #116.

Updated issue summary.

This would still be cool to get in and then further explore 'menu block' options in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables.

Rerolling - let's see if I got this right.

This at least brings the links back (but they're ugly) - uncommented the upgrade path for now, it feels weird indeed being in standard.install.

Fixed the exceptions - couldn't get the right theming though

Worked on this related issue last night: [#/1053648].

Hoping the work there will help to move this issue forward. I read through the related issues, so I'll take a go at this in the next couple days.

Wouldn't it make a more sense to do markup changes in a template and not in hook_block_view_alter?

The regions are very specifically name, to me this is like a sidebar-user-login type region, which we would never do - the header is effectively going to end up with 4 regions, perhaps instead something like header.top header.bottom etc, personally I don't care much about the convention used but using very specific naming based on a single use case seems out of place.

Honestly I am still rather bewildered why we need this and don't instead heavily pursue #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, is it too late for that now?

My understanding is that by using the approach in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) for this issue, we'll essentially end up with the equivalent of menu_block in core. I was going to wait until that other issue was committed before starting in on this one. I do think we'll probably want to take a similar approach where the first step is getting the ability to add a menu block, while the second step is altering the regions to replace the markup in the templates with an installed menu block.

this approach is broken. we're conflating way too much into regions, per #127. while the patch @mdrummond links to in #128 is a good-enough solution for Bartik, it's not actually changing the way regions *work*. they're still generic containers, and the basic concept behind them needs to be enriched for the abstraction to still work well. designing for Bartik doesn't help us.

@sdboyer not sure or clear on what you mean by:

regions... and the basic concept behind them needs to be enriched for the abstraction to still work

"branding issue" is in! Suppose the primary reason for the issue is to decouple menu from theme layer.
Actually menu & menu_link modules could be disabled but theme is hard-wired.
The menu block is nice idea to get rid of SystemMenuBlock that now does not belongs to system but it's separate issue.

1. +++ b/core/includes/theme.inc
   @@ -2659,8 +2659,6 @@ function template_preprocess_page(&$variables) {
   -  $variables['main_menu']         = theme_get_setting('features.main_menu') ? menu_main_menu() : array();
   -  $variables['secondary_menu']    = theme_get_setting('features.secondary_menu') ? menu_secondary_menu() : array();
   

   variables could be empty
   theme settings?

2. +++ b/core/modules/menu/config/menu.settings.yml
   @@ -1,3 +1 @@
    override_parent_selector: false
   
   +++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
   @@ -749,13 +753,6 @@ private function verifyAccess($response = 200, $menu_name = 'tools') {
   -    $this->drupalGet('admin/structure/menu/settings');
   
   +++ b/core/modules/menu/menu.module
   @@ -79,12 +79,6 @@ function menu_menu() {
   -  $items['admin/structure/menu/settings'] = array(
   
   +++ b/core/modules/menu/menu.routing.yml
   @@ -1,10 +1,3 @@
   -  path: '/admin/structure/menu/settings'
   ...
   -    _form: 'Drupal\menu\MenuSettingsForm'
   

   But form still here and config schema too!

3. +++ b/core/modules/menu/lib/Drupal/menu/Plugin/Block/MenuNavigation.php
   @@ -0,0 +1,93 @@
   + * Contains \Drupal\menu\Plugin\Block\MenuNavigation.
   

   Why menu? it's optional module, and as block render links this should be moved to menu_link module

4. +++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
   @@ -35,6 +35,8 @@ public static function getInfo() {
   +    $this->drupalPlaceBlock('menu_navigation', array('region' => 'secondary_menu'), array('menu' => 'account'));
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.php
   @@ -129,6 +129,7 @@ protected function doTestDescriptionMenuItems() {
   +    $this->drupalPlaceBlock('menu_navigation', array('region' => 'main_menu'), array('menu' => 'main'));
   
   +++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/MenuTest.php
   @@ -20,6 +27,11 @@ public static function getInfo() {
   +    $this->drupalPlaceBlock('menu_navigation', array('region' => 'main_menu'), array('menu' => 'main'));
   

   and others... let's implement helper method for.

5. +++ b/core/modules/menu/menu.module
   @@ -653,6 +647,14 @@ function menu_preprocess_block(&$variables) {
   +  // @todo
   +  /*
   +  if ($variables['block']->id == 'menu_navigation') {
   +    $variables['attributes']['class'][] = 'links';
   +    $variables['attributes']['class'][] = 'inline';
   +    $variables['attributes']['class'][] = 'clearfix';
   +    $variables['title_attributes']['class'][] = 'element-invisible';
   +  }*/
    }
   

   needs to be addressed

6. +++ b/core/profiles/standard/config/block.block.bartik_account_links.yml
   @@ -0,0 +1,15 @@
   +uuid: a41e843f-d19c-4528-ab02-2fc335e12b1e
   
   +++ b/core/profiles/standard/config/block.block.bartik_main_menu.yml
   @@ -0,0 +1,15 @@
   +uuid: a30e843f-d19c-4528-ab02-2fc335e12b1e
   

   config now shipped without uuid

7. +++ b/core/profiles/standard/standard.install
   @@ -87,3 +88,52 @@ function standard_install() {
   +/*function standard_update_8000() {
   +  // Place main-menu.
   +  $block = \Drupal::config('block.block.bartik.main_menu');
   

   no more needed

Here's a completely reworked version of this patch based on the approach used in #1053648: Convert site elements (site name, slogan, site logo) into blocks.

In short, all this patch does is create the ability to place a block with any particular level for a particular menu. That's it. We can do a separate issue to create the regions needed to replace the primary menu and the secondary menu and to actually place menus by default.

Based on the discussion above, it seems like it would make sense to get this basic level in first, and then if we're still able to add some of the other configuration options that the Menu Block modules, we can do that in a separate issue.

I renamed this MenuLevelBlock, since that's what this does, and since there was some concern above about blocks named menu_navigation when there are main_navigation blocks too. We can easily change the block name if necessary or place this in menu_links. It should be noted as far as I can tell, menu_links is only required right now because of some work being done on it, at least according to a @todo that I found.

I tested this out, and it works well. It would do the job of secondary menus, in that if you specify level 1, for example, it will only show that level if you are on a page that has child items in level 1.

Hopefully this will help us get this done.

In case the new block introduced here then current SystemMenuBlock with derivative should be removed in favor of new one. Having both is confusing.
Also this new block is not covered with tests

EDIT there's a special issue for this block #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables so this one should focus on theme integration and probably wait for menu.inc removal

+++ b/core/modules/menu/lib/Drupal/menu/Plugin/Block/MenuLevelBlock.php
@@ -0,0 +1,135 @@
+   * Creates a SystemBrandingBlock instance.

hehe, de ja vú?

@andypost: Yes, there's definitely duplication of discussion in #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, although all the work on actual patches has been done in this issue, rather than that one, which is why I posted this here.

I'm aiming to use the same process as we used for #1053648: Convert site elements (site name, slogan, site logo) into blocks, which was to first get in a patch that allows for the actual creation of the block, and then have a follow-up issue which deals with removing the variables from the page templates and replacing them with a pre-installed block. Breaking those two things up seemed to make it more manageable so we could actually get things in. If we use #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables for the first part, which allows for adding a block, and this issue for removing the page variables and replacing them with the appropriate menu blocks, I'm fine with that. Whatever will get the job done.

I took a quick look at SystemMenuBlock, and it appears to be what allows for custom menus to have a block, which is somewhat different than this, which allows for a particular level of any menu block.

@tstoeckler: Oops. I used SystemBrandingBlock as a template for this revised block: missed changing that bit. I'll fix it in the next patch.

@andypost: After chatting with EclipseGc on IRC, we decided it would make sense to make the level selection tool available as an option for the existing menu blocks, which would mean adding this to SystemMenuBlock as far as I can tell, rather than having a separate MenuLevelBlock class. I think that's what you were saying, so my apologies if I misunderstood.

Should we remove: "Introduce regions as needed to place these elements." from the issue description?

After some further discussions, I'm going to move the work on patches to #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables, as this first step is much more about getting the essentials of menu_block into core. Once that's done we can come back here to replace the primary_links and secondary_links variables with blocks.

+1 Looking forward to it.

1. Situation: Fresh D8 HEAD, with a node created, and warmed cache.
2. Frontpage, warm cache: 222 DB queries.
3. Situation: same as above, but with main and secondary menus disabled
4. Frontpage, warm cache: 195 DB queries.

IOW: those two blocks are now responsible for 27 DB queries. As soon as they are blocks, ~10% of DB queries will be removed, because SystemMenuBlock are render cached by default.

Something seems broken with that number of database queries. Drupal 7 can just about serve an entire page with 27 database queries.

I'd be tempted to open a new issue for the regression, and have this as a sub-issue, but leaving this as is for now since we may well need to fix both the cached and uncached state separately anyway.

#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees should fix the performance issue here, so moving back to major.

This would still be good to have in its own right, so leaving as beta target. We could add the capability at any time, but can't remove the template variables post release candidate.

Just a note that I have a working patch up on #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables. Trying to get some reviews so we can get that in.

#474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables is now RTBC. Now let's get this one moving forward again! :)

Note: This is a soft blocker for #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, because it would make that patch a lot simpler: none of the hacky, one-off rendering of primary and secondary links in the theme layer and in menu.inc would need to be updated by #1805054 anymore, making that patch significantly simpler.

Hurray, #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables now got committed for realz!

Let's get this one moving forward again? :) I'd be happy to provide very frequent reviews!

So, here's a kickstart for this issue. This patch:

1. removes the primary and secondary menu links cruft from menu.inc, theme.inc and common.inc
2. removes admin/structure/menu/settings, since that's obsolete
3. adds a "Main navigation (level 1)" block and a "User account menu (level 1)" block to the standard install profile

Still left to do: make this match the current (HEAD) visually, i.e.:

1. figure out how to maintain the same accessibility (markup)
2. figure out if we should add regions, where they should live, what they should be named

(See the @todos.)

Let's see how spectacularly this test will fail… :D

Failed pretty good. :)

I tried locally & couldn't apply it either.

On track for the next big D8 core sprint — Sept 27, 2014 in Amsterdam! Wish I could be there. Subscribing to this issue in the meantime.

Straight reroll of #147.

As promised, I'm pushing this forward. I will definitely need your help though, mdrummond, to finish this patch.

The first thing I wanted to do is to get the main menu to look and behave the same. I got that working, including markup I didn't know existed that is used to "toggle" the main menu while responsive in Bartik, that was added in #1880488: Make menu collapsible on small screen resolutions.
Next, I tackled the secondary menu. The question is how we distinguish between the main and secondary menus. We don't need to care about a third menu in the header, because Bartik in HEAD also doesn't support that (well, it renders, but it's so ugly you'll run away screaming). Therefore I went with the simplest thing I could think of to determine which menu gets which styling: the first menu block in the header region gets the "main menu" styling, including the toggle. The second menu block in the header region gets the "secondary menu" styling. No work has been done yet to make it work for additional menus.

The above explains why the CSS selectors have gotten so much bigger: rather than having known IDs to target (#main-menu, #main-menu-links …), we now have to target the first/second menu block in the header, so that becomes #header .block-menu:first-child and #header .block-menu:nth-child(2).

Of course, Bartik was written with the assumption of primary and secondary menus being part of the theme, so the CSS still reflects that. Making the CSS cleaner, more generic and above all simpler is something that can be done in a follow-up, I believe.

Finally, this patch also fixes all test failures. Most of them were due to assuming the "account" menu was the secondary menu, and that it was available from the theme. That assumption no longer holds, but is easily fixed by just pldacing a block that contains the "account" menu.

A few minor nitpicks.

Also started to do some manual testing.

LOL, that red border was just there for debugging purposes, I forgot to remove it :P

I'm hoping the smart frontend folks here can come up with a solution when >2 menus are used. But note that also in HEAD, things break down in that case, even though it breaks less in HEAD.

+++ b/core/themes/bartik/css/style.css
@@ -547,19 +529,19 @@ h1.site-name {
-#main-menu {
+#header .block-menu:first-child {

Is this a really good idea? This seems really fragile. Our CSS standards intend to prevent this kind of fragility.

#162: I don't know at all if this is a good idea. My goal was to get the patch to a point where it works. I'm hoping you and others with much stronger CSS & Twig skills than I have can push it to the finish line. If you don't have the bandwidth or interest to do that, I'm hoping you can provide guidance/reviews so I can keep pushing it forward.

:)

I'll try to take a look tonight. Thanks for your work on this Wim!

In this reroll:

• Fixed test failures. (That broken contextual link in Stark was HELL to figure out. Turns out it's because system_theme() didn't set 'render element' => 'elements'. That means contextual links on the system branding block is currently broken in HEAD also.)
• Removed red outline.
• Minor code cleanup.

#164: Thanks!

I think at least some of the CSS fixes might need this to be done before: #2318611: Menu block template doesn't provide the menu name at all

I did some testing and seems like the blocks are getting attached on installation but now menu is in the header region which is wrapped with header. Is that ok for us or should we create new region for the menu?

All the attempts at this in the past created new regions for both menus, usually both in the header element with the header region.

Fixed some more CSS. Hopefully @rteijeiro gets his dreditor working to keep trolling going on.

1. +++ b/core/themes/bartik/css/style.css
   @@ -547,19 +529,19 @@ h1.site-name {
   +.menu-main {
   
   @@ -652,35 +644,45 @@ body:not(:target) #nav:target ~ #main-menu-links .menu-hide a {
   +.menu-account {
   

   This is much better! One change that would be more inline with our standards is .menu-main -> .menu--main and .menu-account -> .menu--account

2. +++ b/core/themes/bartik/css/style.css
   @@ -547,19 +529,19 @@ h1.site-name {
   +.menu-main .menu li {
   
   @@ -567,8 +549,7 @@ h1.site-name {
   +.menu-main .menu a {
   
   @@ -579,31 +560,42 @@ h1.site-name {
   +[dir="rtl"] .menu-main .menu a {
   ...
   +.menu-main .menu a,
   ...
   +.menu-main .menu a:hover,
   ...
   +.menu-main .menu a:active {
   ...
   +.menu-main .menu li a.active {
   
   @@ -652,35 +644,45 @@ body:not(:target) #nav:target ~ #main-menu-links .menu-hide a {
   +.menu-account .menu li {
   ...
   +.menu-account .menu a {
   ...
   +.menu-account .menu a:hover,
   

   There are a few situations where we can make these selectors shorter and weaker but removing unnecessary .menu's, li's, etc in the selector.

Changed the class name to one that Lewis suggested and fixed block names on tests.

+++ b/core/themes/bartik/css/style.css
@@ -547,19 +529,19 @@ h1.site-name {
+.menu-main {
@@ -652,35 +644,45 @@ body:not(:target) #nav:target ~ #main-menu-links .menu-hide a {
+.menu-account {

This is much better! One change that would be more inline with our standards is .menu-main -> .menu--main and .menu-account -> .menu--account

Actually, unless I'm mistaken, this is very wrong :(

This ties this styling to a specific menu: the "main" menu. That menu just happens to be created by default as part of the Standard install profile. What if you want to use a different menu as the main menu? What happens if you don't have a menu called main?

This is precisely why I went with the order-based approach. This represents a huge regression compared to HEAD…

@WimLeers Ok, if we want to mimic the current styling in HEAD then we just convert the #main-menu-links into a class, and add that in a template override. We can still called it .menu--main as long as it isn't being automatically generated from the machine name right?

But how are you going to determine when to add that class?