Problem/motivation

Drupal 7 has most global site and page elements hardwired into templates, and gives no control to users to move them around. The primary and secondary link placements are hardwired to core and contrib themes alike. We should stop special-casing these, so they can be moved around and selectively hidden or replaced as needed.

Additionally, this causes code duplication, prevents efficient caching and causes a confusing site builder experience (/admin/structure/menu/settings is not exactly discoverable).

Proposed solution

  1. Primary and secondary menus as new regions in the default page.html.twig and in the default (Bartik) theme.
  2. The site builder can then put any menu block in the "Primary menu" region and it'll be styled as the primary menu. Similar for the "Secondary menu" region.
  3. All the special snowflake stuff can be removed:
    the obsolete code in menu.inc
    the hacky code in common.inc to associate the cache tags for the primary and secondary menus, necessary because they're rendered in a backward way
    the global menu settings (the route, the form, the config schema, the config file) to choose "THE primary menu" and "THE secondary menu".
The goal: look identical to HEAD, which looks like this:
With this patch: mission succeeded.

API changes

  • The primary and secondary menus are converted into blocks, and therefore no longer available as theme settings or as variables in the page template.
  • The following API functions are removed:
    • function menu_main_menu()
    • menu_secondary_menu()
    • _menu_get_links_source()
    • menu_navigation_links()
  • The primary and secondary menu settings are removed from menu_ui.settings and theme.settings.
  • Two new regions in Bartik: primary_menu and secondary_menu.
Files: 
CommentFileSizeAuthor
#231 Screen Shot 2014-09-28 at 3.33.22 PM.png10.27 KBwebchick
#228 global_menus_as_blocks-1869476-226.patch51.61 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,706 pass(es).
[ View ]

Comments

Gábor Hojtsy’s picture

StatusFileSize
new4.65 KB
FAILED: [[SimpleTest]]: [MySQL] 49,295 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Given #1535868: Convert all blocks into plugins, it probably does not make much sense at this point to convert these to blocks as blocks exist pre-plugins. However, @EclipseGC actually did lots of the conversion work that is involved with these blocks in interim patches at #1787846: Themes should declare their layouts. It was not actually related to introducing layouts as a concept, and I want to make sure we have the patch around, so here it is. All the code included is from @EclipseGC.

Blocks included:
- primary links
- secondary links

Technically this issue does not make much sense until #1535868: Convert all blocks into plugins however, I think it would make sense to parallelise work.

I don't agree with the ways the theming / markup changes are done in plugins (and the way themes need to override them), it is not my code, I just wanted to save it for @EclipseGC and start a discussion :)

Status:Needs review» Needs work

The last submitted patch, global-menus-as-blocks-1.patch, failed testing.

andypost’s picture

+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"

EclipseGc’s picture

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).

dcrocks’s picture

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.

EclipseGc’s picture

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

sun’s picture

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.

dcrocks’s picture

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.

andypost’s picture

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: Get rid of menu_list_system_menus()

Gábor Hojtsy’s picture

I propose we group at #1053648: Convert site elements (site name, slogan, site logo) into blocks first which is much simpler, but still needs to set up the pattern of introducing the blocks, new regions as needed, removal of existing theme variables, blocks added in install profiles, etc. That pattern can be replicated here and in #507488: Convert page elements (local tasks, actions) into blocks but both this one and the breadcrumb/message one need more discussion as to what we do with associated specific problems, while the logo/slogan and site name could work flawlessly.

dcrocks’s picture

#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?

Gábor Hojtsy’s picture

@dcrocks: seems to me like this one and that one are clearly duplicates?! What would be the difference?!

dcrocks’s picture

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.

dcrocks’s picture

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

Gábor Hojtsy’s picture

@dcrocks: my reading of #503810: Convert primary and secondary menus to blocks is that there is no consensus; also there were only 2 posts the whole of 2012 and the last one before that was in 2011 March. I think that is safely closed as a duplicate. There is no consensus in here either what to do. :)

dcrocks’s picture

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.

Gábor Hojtsy’s picture

Yes, if a specific block for primary and secondary menus is not introduced, we still need to remove the theme settings related to this. The menus should be displayed and used through blocks. Even in Seven/Stark or any other theme. No special theme variables anymore, that is the whole point of the blocks and layouts initiative.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new14.44 KB
FAILED: [[SimpleTest]]: [MySQL] 47,570 pass(es), 2,287 fail(s), and 14,615 exception(s).
[ View ]

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

sun’s picture

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. ;)

Status:Needs review» Needs work

The last submitted patch, 1869476-17.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review

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

sun’s picture

StatusFileSize
new4.44 KB
new17.43 KB
FAILED: [[SimpleTest]]: [MySQL] 49,393 pass(es), 15 fail(s), and 5 exception(s).
[ View ]

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 Hojtsy’s picture

@sun: I think that is fine as long as enabling and placing the block is tested too. :)

Status:Needs review» Needs work

The last submitted patch, menu.links_.22.patch, failed testing.

sun’s picture

Status:Needs work» Postponed

@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

EclipseGc’s picture

@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

sun’s picture

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? :)

dcrocks’s picture

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?

sun’s picture

@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.

dcrocks’s picture

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.

sun’s picture

Status:Postponed» Needs review
Issue tags:-Blocks-Layouts, -B&L

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

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts, +B&L

The last submitted patch, menu.links_.22.patch, failed testing.

sun’s picture

Assigned:Unassigned» sun
Status:Needs work» Needs review
StatusFileSize
new4.72 KB
new22.74 KB
FAILED: [[SimpleTest]]: [MySQL] 49,258 pass(es), 12 fail(s), and 8 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, menu.links_.33.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new5.33 KB
new25.05 KB
FAILED: [[SimpleTest]]: [MySQL] 49,341 pass(es), 17 fail(s), and 4 exception(s).
[ View ]

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. ;)

tim.plunkett’s picture

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

Status:Needs review» Needs work

The last submitted patch, menu.links_.35.patch, failed testing.

EclipseGc’s picture

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

sun’s picture

Status:Needs work» Needs review

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

mgifford’s picture

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.

dcrocks’s picture

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?

dcrocks’s picture

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

dcrocks’s picture

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?

dcrocks’s picture

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.

Gábor Hojtsy’s picture

@dcrocks: re #44 those are unrelated pre-existing bugs or just current behaviours with the blocks system.

@sun: I agree with tim.plunkett that this looks really good. Great direction :)

dcrocks’s picture

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.

EclipseGc’s picture

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

sun’s picture

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.

dcrocks’s picture

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.

mgifford’s picture

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.

sun’s picture

webchick’s picture

Issue tags:+Spark

Tagging.

mgifford’s picture

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?

sun’s picture

StatusFileSize
new8.22 KB
new27.47 KB
FAILED: [[SimpleTest]]: [MySQL] 50,875 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
  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.

Status:Needs review» Needs work
Issue tags:-Blocks-Layouts, -Spark, -B&L

The last submitted patch, menu.links_.54.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts, +Spark, +B&L

The last submitted patch, menu.links_.54.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new8.1 KB
new32.66 KB
FAILED: [[SimpleTest]]: [MySQL] 50,740 pass(es), 1 fail(s), and 305 exception(s).
[ View ]
  1. Fixed UserPasswordResetTest.
  2. Fixed Views Wizard/MenuTest.
  3. Fixed MenuRouterTest.
  4. Fixed MenuTest.
  5. Fixed OpenIDRegistrationTest.
  6. Various clean-ups.
sun’s picture

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 ;)

Status:Needs review» Needs work

The last submitted patch, menu.links_.58.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new757 bytes
new32.77 KB
FAILED: [[SimpleTest]]: [MySQL] 50,734 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

effulgentsia’s picture

Category:feature» task

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.

Status:Needs review» Needs work

The last submitted patch, menu.links_.61.patch, failed testing.

andypost’s picture

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

mgifford’s picture

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

sdboyer’s picture

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

webchick’s picture

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

webchick’s picture

Issue tags:-Spark

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

Bojhan’s picture

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

webchick’s picture

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

EclipseGc’s picture

just for reference I completely agree with #70

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new34.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-rolled against head

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.71.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new34.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new560 bytes

Cleanup for changes to annotation namespace

Status:Needs review» Needs work
Issue tags:-Blocks-Layouts, -B&L

The last submitted patch, menu-blocks-1869476.74.patch, failed testing.

Issue tags:+Blocks-Layouts, +B&L

The last submitted patch, menu-blocks-1869476.74.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new34.03 KB
FAILED: [[SimpleTest]]: [MySQL] 54,345 pass(es), 2 fail(s), and 12 exception(s).
[ View ]
new1.35 KB

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.77.patch, failed testing.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new36.13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,376 pass(es).
[ View ]
new6.34 KB

Should be green, passes locally

larowlan’s picture

StatusFileSize
new30.79 KB

Screenshot of bartik
Screen Shot 2013-04-15 at 4.52.21 PM.png

larowlan’s picture

StatusFileSize
new36.14 KB
PASSED: [[SimpleTest]]: [MySQL] 54,537 pass(es).
[ View ]
new1.77 KB

Fixes two comments longer than 80 chars identified by @swentel

sdboyer’s picture

Status:Needs review» Reviewed & tested by the community

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 :)

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs work

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

David_Rothstein’s picture

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?
larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new29.11 KB
PASSED: [[SimpleTest]]: [MySQL] 54,419 pass(es).
[ View ]
new21.33 KB

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.

dcrocks’s picture

Status:Needs review» Needs work

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.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new33.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,431 pass(es).
[ View ]
new15.58 KB

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

dcrocks’s picture

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.

dcrocks’s picture

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.

tim.plunkett’s picture

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.

dcrocks’s picture

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.

David_Rothstein’s picture

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):

<?php
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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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....

larowlan’s picture

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

larowlan’s picture

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.

Gábor Hojtsy’s picture

I think this stands alone, not dependent on scotch at all.

cbiggins’s picture

Status:Needs review» Needs work
StatusFileSize
new30.34 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new19.18 KB

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).

andyceo’s picture

Status:Needs work» Needs review
larowlan’s picture

StatusFileSize
new33.88 KB
FAILED: [[SimpleTest]]: [MySQL] 57,392 pass(es), 244 fail(s), and 8,867 exception(s).
[ View ]
new3.54 KB

Changes to page.html.twig

tim.plunkett’s picture

+++ 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?

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.100.patch, failed testing.

hosef’s picture

Status:Needs work» Needs review
StatusFileSize
new5.83 KB
new38.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-blocks-1869476.103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Blocks-Layouts, -B&L

The last submitted patch, menu-blocks-1869476.103.patch, failed testing.

hosef’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.103.patch, failed testing.

cbiggins’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts, +B&L

The last submitted patch, menu-blocks-1869476.103.patch, failed testing.

oadaeh’s picture

Issue tags:-B&L

Removing the unnecessary B&L tag.

hosef’s picture

Status:Needs work» Needs review
StatusFileSize
new38.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,598 pass(es), 74 fail(s), and 8,647 exception(s).
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.110.patch, failed testing.

EclipseGc’s picture

StatusFileSize
new34.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-blocks-1869476.112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review

vacation brain...

Status:Needs review» Needs work
Issue tags:-Blocks-Layouts

The last submitted patch, menu-blocks-1869476.112.patch, failed testing.

dcrocks’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts

The last submitted patch, menu-blocks-1869476.112.patch, failed testing.

klonos’s picture

Assigned:sun» Unassigned

...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.

jibran’s picture

Issue tags:+Needs reroll

Needs reroll as per #116.

jibran’s picture

Issue summary:View changes

Updated issue summary.

swentel’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new28.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,539 pass(es), 53 fail(s), and 1,447 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.119.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new28.26 KB
FAILED: [[SimpleTest]]: [MySQL] 59,931 pass(es), 28 fail(s), and 2,053 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, menu-blocks-1869476.121.patch, failed testing.

swentel’s picture

Status:Needs work» Needs review
StatusFileSize
new4.97 KB
new28.43 KB
FAILED: [[SimpleTest]]: [MySQL] 59,201 pass(es), 9 fail(s), and 3 exception(s).
[ View ]

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

The last submitted patch, menu-blocks-1869476.123.patch, failed testing.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

mdrummond’s picture

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.

Jeff Burnz’s picture

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?

mdrummond’s picture

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.

sdboyer’s picture

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.

Jeff Burnz’s picture

@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

andypost’s picture

Issue tags:+Needs reroll

"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

mdrummond’s picture

Status:Needs work» Needs review
StatusFileSize
new3.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,277 pass(es).
[ View ]

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.

andypost’s picture

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

tstoeckler’s picture

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

hehe, de ja vú?

mdrummond’s picture

@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.

mdrummond’s picture

@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.

lokapujya’s picture

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

mdrummond’s picture

Status:Needs review» Postponed

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.

dcrocks’s picture

+1 Looking forward to it.

Wim Leers’s picture

  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.

catch’s picture

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.

catch’s picture

Issue tags:+beta target
catch’s picture

Priority:Critical» Major

#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.

mdrummond’s picture

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.

Wim Leers’s picture

Status:Postponed» Needs work

#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.

Wim Leers’s picture

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!

Wim Leers’s picture

Status:Needs work» Needs review
Issue tags:+sprint
StatusFileSize
new25.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch global_menus_as_blocks-1869476-147.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 147: global_menus_as_blocks-1869476-147.patch, failed testing.

mgifford’s picture

Failed pretty good. :)

I tried locally & couldn't apply it either.

thinkyhead’s picture

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.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new25.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,314 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Straight reroll of #147.

Status:Needs review» Needs work

The last submitted patch, 151: global_menus_as_blocks-1869476-151.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new51.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,307 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
new29.51 KB

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.

Gábor Hojtsy’s picture

Issue tags:+frontend

Amazing. Looks like needs frontend reviews first and foremost :)

rteijeiro’s picture

StatusFileSize
new48.87 KB
new3.5 KB

A few minor nitpicks.

Also started to do some manual testing.

Gábor Hojtsy’s picture

StatusFileSize
new355.31 KB
new158.89 KB

Good trick to tie the CSS to the position of the block in the region IMHO, although not sure if this will be evident to people. BTW as-is visually looks good except the red border:

+++ b/core/themes/bartik/css/style.css
@@ -652,35 +641,46 @@ body:not(:target) #nav:target ~ #main-menu-links .menu-hide a {
+#header .block-menu:nth-child(2) {
+    border: 1px solid red;

Don't think this red border was intended?

Also wondered what happens when they place a third menu? It sort of falls apart :/

Gábor Hojtsy’s picture

BTW I also noticed no "Edit menu" contextual link on the block, but that is not on the tools block or the footer menu block either post/pre patch. Duh. How do we not have a test for that... One of the great side effects of this patch will be you can edit items right from a contextual link on the primary/secondary menus. Quickest menu customization ever. If it works :) Should be fixed in a separate issue as it is pre-existing.

Wim Leers’s picture

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.

Gábor Hojtsy’s picture

Its right, we don't necessarily need to resolve the 3rd menu looking bad because in head already the first one will look bad :D Confirmed.

Status:Needs review» Needs work

The last submitted patch, 153: global_menus_as_blocks-1869476-153.patch, failed testing.

The last submitted patch, 155: global_menus_as_blocks-1869476-155.patch, failed testing.

LewisNyman’s picture

+++ 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.

Wim Leers’s picture

#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.

:)

mdrummond’s picture

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

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new54.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,311 pass(es).
[ View ]
new8.96 KB

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!

lauriii’s picture

Status:Needs review» Needs work

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

lauriii’s picture

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?

dcrocks’s picture

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

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new12.54 KB
new51.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,612 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
lauriii’s picture

StatusFileSize
new3.15 KB
new51.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,612 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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

The last submitted patch, 169: global_menus_as_blocks-1869476-169.patch, failed testing.

LewisNyman’s picture

Status:Needs review» Needs work
  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.

The last submitted patch, 170: global_menus_as_blocks-1869476-170.patch, failed testing.

lauriii’s picture

Status:Needs work» Needs review
StatusFileSize
new8.67 KB
new51.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch global_menus_as_blocks-1869476-174.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 174: global_menus_as_blocks-1869476-174.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 174: global_menus_as_blocks-1869476-174.patch, failed testing.

lauriii’s picture

StatusFileSize
new51.44 KB
lauriii’s picture

Status:Needs work» Needs review
Wim Leers’s picture

+++ 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…

LewisNyman’s picture

@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?

The last submitted patch, 178: global_menus_as_blocks-1869476-175.patch, failed testing.

Wim Leers’s picture

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

joelpittet’s picture

@Wim Leers that seems to be the responsibility of the block to add that class, no?

Which in d7 could be exposed via something like https://www.drupal.org/project/block_class

Likely in the template I'll be doing something along the lines of

<nav class="menu--primary">{{ navigation_primary_region }}</nav>

And dump 1, 2 or more menu blocks into that region.
Followed by a few more regions for posterity:)

<nav class="menu--utility">{{ navigation_utility_region }}</nav>
<nav class="menu--footer">{{ navigation_footer_region }}</nav>
<nav class="menu--side">{{ navigation_side_region }}</nav>

Not totally sure yet if this patch accounts for all the use-cases but by the @todo, I think not quite yet.

Thanks for pushing this issue along though, I intend to experiment with it next weekend!

xjm’s picture

+++ b/core/config/schema/core.data_types.schema.yml
@@ -161,12 +161,6 @@ theme_settings:
-        main_menu:
-          type: boolean
-          label: 'Main menu'
-        secondary_menu:
-          type: boolean
-          label: 'Secondary menu

+++ b/core/includes/menu.inc
@@ -472,72 +454,6 @@ function menu_list_system_menus() {
-function menu_main_menu() {
...
-function menu_secondary_menu() {
...
-function _menu_get_links_source($name, $default) {
...
-function menu_navigation_links($menu_name, $level = 0) {

+++ b/core/includes/theme.inc
@@ -1784,8 +1784,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();

+++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
@@ -234,31 +234,4 @@ public function flatten(array $tree) {
-  public function extractSubtreeOfActiveTrail(array $tree, $level) {

+++ b/core/modules/menu_ui/config/schema/menu_ui.schema.yml
@@ -4,12 +4,6 @@ menu_ui.settings:
-    main_links:
-      type: string
-      label: 'Main links'
-    secondary_links:
-      type: string
-      label: 'Source for secondary links'

Here are the API and config schema changes I saw scanning the patch. I added API and data model changes to the summary -- please check that they are complete/correct.

This change will need a change record. There is also a BC break in the public menu API, so we need to consider doing this by beta 1, preserving some form of BC, or whether this is an okay BC break in a beta.

xjm’s picture

Issue tags:+Pre-AMS beta sprint

Tagging as a priority for a pre-AMS beta sprint.

mdrummond’s picture

I looked this over last night and noodled on it overnight, and I think we may want to approach this a different way.

The concept of the primary menu and the secondary menu is that you could select any menu for each, and it would appear in a specific place within the theme and behave a certain way.

Because we're switching to using blocks, I think we need to change the way a site builder can make those same choices. The sanest way to do so is to have a specific region for the primary menu and a specific region for the secondary menu.

We can certainly have default menu blocks that go into those regions, but this approach would make it pretty simple for somebody to easily place a different menu block in the primary menu region and another in the secondary menu region.

This is also a nice way to handle things because each theme can do this however they please. If they want a primary menu region, great. Don't want a secondary menu region? No problem. Want a tertiary menu region? Sure. If a themer wants to set up a certain set of styles for menus that appear in a particular region, they can do so.

Once challenge worth reviewing is what happens if somebody places multiple menu blocks in a menu region. I think the answer to that though is that if it looks weird when somebody does that, a site builder will be able to see that easily and avoid doing that if it causes problems.

I'll try to find some time to work on a region-based approach to primary and secondary menus. If you have concerns with that approach, feel free to shout about it, though!

Wim Leers’s picture

#184:

@Wim Leers that seems to be the responsibility of the block to add that class, no?

No, because that implies we'd need to expose a checkbox for menu blocks that essentially says "render this as the main menu". That's very problematic. But, fortunately mdrummond provided comprehensive thinking around this in #187, so let's continue the discussion using #187 as a frame of reference.


#187:

Because we're switching to using blocks, I think we need to change the way a site builder can make those same choices.

And this is why I went with the approach I went with. It allows you to make the same choices, without the problems associated with introducing special-purpose regions.

The sanest way to do so is to have a specific region for the primary menu and a specific region for the secondary menu.

We can certainly have default menu blocks that go into those regions, but this approach would make it pretty simple for somebody to easily place a different menu block in the primary menu region and another in the secondary menu region.

This is also a nice way to handle things because each theme can do this however they please. If they want a primary menu region, great. Don't want a secondary menu region? No problem. Want a tertiary menu region? Sure. If a themer wants to set up a certain set of styles for menus that appear in a particular region, they can do so.

… this is all true, but …

Once challenge worth reviewing is what happens if somebody places multiple menu blocks in a menu region.

…Voila! That's the problem with special-purpose regions! And it's not limited to that: nothing prevents the user from putting blocks other than menu blocks in this special purpose region.

For this to work well, without problems, we'd need the concept of a region that only accepts certain types of blocks. But that's also problematic (what if you have a module that provides an alternative, more customizable menu block? Then those blocks could not be placed in this special purpose region, which is also a WTF).

If you have concerns with that approach, feel free to shout about it, though!

I hope the concerns above are clear to you.

In conclusion…

The thing is: I don't see a way to do this without something being problematic:

  1. in HEAD, main/secondary menus are hardwired into the theme/template, which is precisely what this issue tries to solve. So we want menus to be movable blocks.
  2. in my patch (#165), menus are blocks, and there's special meaning for the first and second menu blocks in the header region. It seems that's problematic because 1) ugly & less efficient CSS, 2) not so great discoverability/understandability.
  3. in lauriii's patch (#178), the styling is tied to a specific menu, so you cannot choose which menu should be presented as "the main menu", and it's completely broken when you're not using the Standard install profile
  4. in mdrummond's proposal (#187), we have one or two special purpose regions, which are intended to only be used by zero or one menu blocks, but we cannot control that, which means there can be 2 menu blocks in there (in which case behavior/styling would be "undefined" probably) or a menu block and other block(s) (also "undefined behavior" probably). At best, we can enforce this by doing something like .main-menu-region .block { display: none; } .main-menu-region .block:first-child { display: block; }, which is … very dubious and will also have a terrible SX (Sitebuilder Experience).

So the SX is objectively bad in all four cases:

  1. unable to move the main menu
  2. unable to know that there's special styling depending on the order
  3. unable to choose which menu should be the main menu
  4. unable to enforce "correct use" of these special purpose regions

Options 1 is clearly fundamentally broken. Option 3 is acceptable for a site-specific theme, because there it is known which menu will be the "main" menu. Options 2 and 4 are both viable, because they're both solutions for themes that aren't site-specific: they allow the user to still choose the "main" menu.

I don't see a 5th or 6th option. If anybody sees another way of doing this, please let us know!

So that leaves option 2 and 4.

  • Option 4 is clearly the easiest to understand. But it's the most difficult to enforce correct use of.
  • Option 2 is easy to enforce correct use of (there simply isn't a way to use it incorrectly), but it's not the easiest to understand.

Perhaps we can get away with option 4 and a simple solution to its problem after all: attach some JavaScript to admin/structure/block/%theme that shows a dialog to the user whenever a second block is added to this special purpose region, and that ensures the placed block is implemented by the SystemMenuBlock plugin?

mdrummond’s picture

I think you summed up the issue very well, Wim. There isn't a perfect solution.

Adding some sort of warning with option 4, seems decent. I think this gets partially solved by the fact that people will see pretty clearly what happens if they put in more than one menu block, if it looks weird. Also the fact that the regions have menu in the name makes it clear you shouldn't put non-menu things in there.

People can do all sorts of silly things, and we can't prevent all of them. Having a somewhat sensible what way for people to make a good choice is what should be our aim.

I got started on a patch, but headed off for the afternoon/evening. I will try to wrap up the patch tonight and get it posted then. Might be easier to evaluate options once it is is easy to see how this would work.

dcrocks’s picture

This certainly has changed over the last 2 years. I don't see why bartik, as a theme, can't have 2 special regions to host the primary and secondary menus. It is not an unusual practice of themers to place menus in their layouts via regions. The purpose of these regions is for layout, not functional. Seven doesn't use these menus, but Stark does, indirectly. Modules/System/Templates/page.html.twig still calls the old variables, even after the patch in #165, though a @todo was added. Stark inherits the menus from there. That seems to be more of a problem here.

mdrummond’s picture

StatusFileSize
new53.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,154 pass(es), 832 fail(s), and 134 exception(s).
[ View ]
new18 KB

This is my first stab at a revised version of this patch that relies upon regions rather than the header region.

I got an error on install with this patch applied so clearly something else still needs to be fixed. Hopefully this is a starting point at least.

When I created this patch, I actually stepped through and recreated it line by line, because I wanted to avoid missing some change that I didn't understand. It's possible I made an error somewhere while doing that. The interdiff will likely show if that happen.

I think one thing to look at is all the changes in the CSS to the header region. I'm not sure if all of those changes are necessary. Was that part of the thought that menus would go into the header region? If we're not doing that we may want to revert those changes.

Anyhow, hopefully this helps move this in the right direction. If somebody wants to take a stab at whatever awful errors testbot finds, please go right ahead!

Status:Needs review» Needs work

The last submitted patch, 191: global-menus-as-blocks-1869476-191.patch, failed testing.

Wim Leers’s picture

Looks like the majority of the test failures (hopefully all!) are due to a simple mistake:

Uncaught PHP Exception Twig_Error_Syntax: "A template that extends another one cannot have a body in "core/modules/system/templates/block--system-menu-block.html.twig" at line 54."

:)

mdrummond’s picture

Oh, duh. Whoops! That should be an easy fix. I may be able to roll a new patch for that today.

mdrummond’s picture

StatusFileSize
new53.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,544 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new1.2 KB

So the upside is that this is now working. The menus are being placed in new regions. Yay!

The downside is that the appearance looks wrong, so there's more work to be done there.

mdrummond’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 195: global-menus-as-blocks-1869476-195.patch, failed testing.

Wim Leers’s picture

Assigned:Unassigned» Wim Leers

I'll try to get this green again.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new56.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,603 pass(es).
[ View ]
new3.2 KB

For inexplicable reasons, the latest patch lost the following changes of mine:

  1. consistent naming of the cache tags compared to the block names, thereby causing failures in PageCacheTagsIntegrationTest
  2. an expected cache tag addition to PageCacheTagsIntegrationTest, thereby causing failures in PageCacheTagsIntegrationTest
  3. render element in system_theme() was renamed to render_element, thereby causing the failure in \Drupal\menu_ui\Tests\MenuTest

The failure in BlockUiTest was due to that test being very silly, unable to cope with new regions being added. Easy fix.

This should be green again! :)


I'll now take a stab at fixing the appearance.

Wim Leers’s picture

StatusFileSize
new73.99 KB
new83.61 KB
new74.06 KB
new73.85 KB
new56.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,605 pass(es).
[ View ]
new10.34 KB
new510 bytes
new2.5 KB

This reroll brings appearance fixes.

#199: BAD!
The goal: look identical to HEAD, which looks like this:
With this patch: mission succeeded.
Except for contextual links…
This is inherent about Bartik's styling, because it's always styled the secondary menu using overflow: hidden, which causes the contextual links to be clipped. The only way to fix this, is to change the CSS quite drastically, which is out of scope for this issue. Let's fix contextual links in a follow-up — the fact that contextual links appear at all for the secondary menu is already an improvement.

Attached: 3 interdiffs, applied in succession to go from #199 to this patch.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
+++ b/core/modules/system/templates/page.html.twig
@@ -50,6 +47,8 @@
+ * - page.primary_menu: Items for the primary menu region.
+ * - page.secondary_menu: Items for the secondary menu region.

@@ -90,17 +89,13 @@
+  {{ page.primary_menu }}
+  {{ page.secondary_menu }}

I don't think we should add regions to System module's page.html.twig? That implies every theme should support this, which is false. I think we should revert the changes to System module's page.html.twig, and only keep those in Bartik.


Finally, I looked into doing this:

Perhaps we can get away with option 4 and a simple solution to its problem after all: attach some JavaScript to admin/structure/block/%theme that shows a dialog to the user whenever a second block is added to this special purpose region, and that ensures the placed block is implemented by the SystemMenuBlock plugin?

It's definitely possible. But that'll bring JS into this patch, which is hard to find reviewers for. Plus, it'll probably cleaning up block.js a bit, which has been unchanged since Drupal 7 except for coding style updates. So any change there is going to amount to a fair amount of bikeshedding probably. In any case, this can easily be a follow-up, because it requires no API changes. Opened #2343297: Add JS to prevent non-menu blocks from being added to Bartik's two special-purpose menu regions.

Plus, that's not even absolutely necessary, as mdrummond said in #189:

Adding some sort of warning with option 4, seems decent. I think this gets partially solved by the fact that people will see pretty clearly what happens if they put in more than one menu block, if it looks weird. Also the fact that the regions have menu in the name makes it clear you shouldn't put non-menu things in there.

People can do all sorts of silly things, and we can't prevent all of them. Having a somewhat sensible what way for people to make a good choice is what should be our aim.

dcrocks’s picture

(1)I built a new clone of D8.0 and the patch applied successfully. Everything looks OK except the new menu blocks don't appear on the 'Place Blocks' side bar. If an admin, for some reason, wanted to place an extra copy of these menus, or add them to a theme which currently didn't have them for some reason, they would be out of luck.
(2)All themes inherit blocks from Bartik when they are installed. If there is no corresponding region they are inserted into the 'default' region, usually the first defined region. So every theme added to an installation will gain those blocks via that route by default. Which would probably lead to some weird formatting unless the css didn't need the region definition and the css was in core css.
(3)It is currently common practice for themers to have a local copy of page.html.twig, which at this point still will likely refer to the old menu variables. That would make for an important change notice.
(4)I don't know how stark will get those blocks in minimal install and test scenarios unless they are in system's page.html.twig.

Wim Leers’s picture

#202: Thanks for the review!

  1. They're there, they're called "Main menu" and "User account menu". The labels you see in the block listing are the labels the user has chosen (in the case of this patch: "Main navigation" and "User account menu (level 1)"), which is probably why you thought you couldn't find them :) We can change those labels to match the original names, that's probably clearer anyway.
  2. Wow, I didn't know that. That being said, no weird formatting will occur; these are menu blocks like any other, i.e. just like the "Tools" menu you see in the sidebar. If they have styling for menu blocks, it will work.
  3. There's plenty of big changes to page.html.twig and other templates already, so I'm not concerned there :)
  4. Again, they're just menu blocks. In the minimal install profile, no blocks are enabled by default except the bare minimum. We could add blocks like these there too, and we could also make similar changes to Stark as the changes we made for Bartik (i.e. add a "Primary menu" region and a "Secondary menu" region) if we want to though.
dcrocks’s picture

I don't think anyone will want to make any changes to stark, otherwise it wouldn't be stark anymore. I tried a couple of contributed themes and the output didn't match bartik's, but themers should be able to fix that easily. And I noticed people surrounding their use of the old menu variables in local copies of page.html.twig with 'if' statements, so there actually may not be many issues there. I guess these are documentations issues more than anything else.
Also, if you don't have any regions specified in the theme's info.yml(e.g. stark) you get a default set of regions.

mdrummond’s picture

#199: Sorry I missed those. The perils of manually redoing a complicated patch.

#200: Thanks for fixing those. Before we RTBC, I want to give one more look at how we're handling the secondary menus now versus how we handled the header region before. The header region is still a valid place to put content, but I think some of the earlier changes overwrote some of the rules for the header region. Just want to make sure we don't have a regression for non-menu content.

#201: The page.html.twig template in system is also what's used in Stark, and yes that's inherited by contrib themes that don't override it. Again, to go back to what I said in #187, previously a site builder could choose a menu that would go into the primary menu area and another menu (or a second level of the same menu) that would go into the secondary menu area. By putting these regions in system/Stark, we're allowing site builders to do something similar. This wasn't a choice that was just available in Bartik. If a contrib theme wants to set up different menu regions or no menu regions at all, great, but I think we should aim to duplicate what could be done previously as much as possible.

#203.4: Did system/Stark have menus assigned to the primary and secondary menus by default? If so we should consider assigning the same menu blocks to the primary/secondary menu regions by default.

If we want to have follow-ups to discuss changing those defaults or whether or not there should be menu regions by default, I think that's fine. But I think that in this issue, we should be aiming to duplicate as much as possible what was available with the main/secondary menu variables that we're removing.

Wim Leers’s picture

The perils of manually redoing a complicated patch.

No problem! If you manually redid this, wow, then you missed very little!

Just want to make sure we don't have a regression for non-menu content.

Yes! I forgot to do this. But you'll be able to judge this much better than I could, so: awesome! :)

By putting these regions in system/Stark, we're allowing site builders to do something similar. […] but I think we should aim to duplicate what could be done previously as much as possible.

That's a great reason, thanks for answering that! :)

Did system/Stark have menus assigned to the primary and secondary menus by default? If so we should consider assigning the same menu blocks to the primary/secondary menu regions by default.

This patch still assigns the same menus as before, so I don't think there's a problem there.

So what's left to be done here?

  1. Another review round of the CSS to make sure I didn't screw up :P
  2. Change the labels of the blocks to match the menu names (see #203.1).
  3. A change record.
  4. Anything else?
cosmicdreams’s picture

Has a change record been started, but far from done: https://www.drupal.org/node/2343643

xjm’s picture

  1. +++ b/core/includes/menu.inc
    @@ -472,72 +453,6 @@ function menu_list_system_menus() {
    -function menu_main_menu() {
    ...
    -function menu_secondary_menu() {
    ...
    -function _menu_get_links_source($name, $default) {
    ...
    -function menu_navigation_links($menu_name, $level = 0) {

    Just a note that removing these functions is beta-deadline, so if this doesn't land before beta 1, we should consider what to do with them instead.

  2. +++ b/core/themes/bartik/templates/block--system-menu-block.html.twig
    @@ -0,0 +1,21 @@
    \ No newline at end of file

    oopsie.

LewisNyman’s picture

StatusFileSize
new1.96 KB
new54.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,603 pass(es).
[ View ]

Great work! I fixed few errors thrown up in csslint (see #2222049: Add a .csslintrc file that's in line with our CSS standards) and a few RTL errors.

Wim Leers’s picture

Issue summary:View changes
Issue tags:-Needs change record

Thanks, Lewis! Am I correct if I assume that you reviewed the patch and think it's good to go from a front-end perspective?

I've refined the change notice further — thanks cosmicdreams for getting that started!

That leaves only the changing of the labels to match the menu names to be done. I'll do that later today.

Please review so we can get this to RTBC ASAP!

Wim Leers’s picture

Issue summary:View changes

Updated the issue summary to actually reflect the current patch.

Wim Leers’s picture

StatusFileSize
new47.51 KB
new56.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,602 pass(es).
[ View ]
new2.11 KB

Changes:

  1. #208.2: fixed.
  2. Labels of the blocks now match the menu names:
  3. Also fixed two indentation errors in the CSS.

41 files changed, 277 insertions(+), 508 deletions(-)

:)

mdrummond’s picture

Status:Needs review» Needs work
  1. +++ b/core/themes/bartik/css/layout.css
    @@ -24,15 +24,7 @@ body,
    -.region-header {
    -  float: right; /* LTR */
    -  margin: .5em 5px .75em;
    -  border: 1px solid #ccc;
    -}
    -[dir="rtl"] .region-header {
    -  float: left;
    -}

    +++ b/core/themes/bartik/css/style.css
    @@ -441,30 +441,7 @@ h1.site-name {
    -/* Region header block menus. */
    -.region-header .block-menu {
    -  border: 1px solid;
    -  border-color: #eee;
    -  border-color: rgba(255, 255, 255, 0.2);
    -  padding: 0;
    -  width: 208px;
    -}
    -.region-header .block-menu li a {
    -  display: block;
    -  border-bottom: 1px solid;
    -  border-bottom-color: #eee;
    -  border-bottom-color: rgba(255, 255, 255, 0.2);
    -  padding: 3px 7px;
    -}
    -.region-header .block-menu li a:hover,
    -.region-header .block-menu li a:active,
    -.region-header .block-menu li a:focus {
    -  text-decoration: none;
    -  background: rgba(255, 255, 255, 0.15);
    -}
    -.region-header .block-menu li:last-child a {
    -  border-bottom: 0;
    -}

    @@ -1874,8 +1864,8 @@ div.admin-panel .description {
    - .region-header {
    -    margin: .5em 5px .75em;
    +  .region-header {
    +    margin: 0 5px 0;

    We aren't removing the header region (nor should we) so I don't think these changes are probably necessary. It's still possible people could put blocks in the header region, so I don't even know that we should remove those menu styles for the header region..

  2. +++ b/core/themes/bartik/templates/page.html.twig
    @@ -79,12 +78,9 @@
    -  <header id="header" class="{{ secondary_menu ? 'with-secondary-menu' : 'without-secondary-menu' }}" role="banner" aria-label="{{ 'Site header'|t}}"><div class="section clearfix">

    I suspect that we'd want to look at these classes in particular. We should be able to change that to to page.secondary_menu ? at the beginning I'd think, so we could keep any shifts due to the presence of secondary menus the same.

Wim Leers’s picture

Assigned:Unassigned» Wim Leers

I'm working on this, but it will be some time until I post a patch. Assigning to me to prevent duplicate work.

  1. Agreed. Already fixed this locally.
  2. I suspect that we'd want to look at these classes in particular. We should be able to change that to to page.secondary_menu ? at the beginning I'd think, so we could keep any shifts due to the presence of secondary menus the same.

    But there is zero CSS that uses this, so there's no value in keeping it around?

The only reason I haven't posted a patch yet, is because I also found regressions for the primary menu's appearance in the narrow and mobile breakpoints. Fixing those also. The patch that I'll then post will hopefully be RTBC-worthy.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
Status:Needs work» Needs review
StatusFileSize
new54.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch global_menus_as_blocks-1869476-215.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.12 KB
new7.54 KB
new25.57 MB

The CSS issues were caused by the changes made in the rerolls by either of you frontend folks :)

I'd carefully tuned the selectors to match the original intent: #main-menu targeted the <nav>, #main-menu-links targeted the <ul>. In the new HTML structure, that means you need to target the <nav> using .region-primary-menu .block-menu and the <ul> using .region-primary-menu .block-menu .menu (or, alternatively, the shorter: .region-primary-menu .block-menu .menu).

Unfortunately, the latest CSS targeted the <ul> using .region-primary-menu .block-menu, which is the one to target the <nav>, not the <ul>. While this works for some things, it doesn't work for everything. Notably, system.theme.css styles ul.menu and its children, and Bartik wants to override that.

This probably happened because of the (great) goal to have shorter, simpler and therefore faster CSS selectors. But it was subtly wrong, hence causing subtle breakage.

In this reroll, I've made that wholly consistent with the old selectors again:

  1. #main-menu -> .region-primary-menu .block-menu
  2. #main-menu-links -> .region-primary-menu .menu (short for .region-primary-menu .block-menu .menu
  3. #secondary-menu -> .region-secondary-menu .block-menu
  4. #secondary-menu-links -> .region-secondary-menu .menu (short for .region-secondary-menu .block-menu .menu

A 1.5 minute annotated screencast to make it easier to verify all use cases work correctly

Screencast showing before/after (because otherwise you need to manually compare screenshots in 3 breakpoints, of the reference, the previous patch, and the current patch — it's easier to show/explain in a video):

Setup
We begin in stme (simplytest.me), this is a Drupal 8 HEAD instance, i.e. the reference, the goal to match pixel-for-pixel.
Then I show tres, this is my patched local site (I have three local versions of Drupal 8: unus, duo, tres, to work on patches in parallel).
I will continuously switch between the sites, overlaid on top of each other, so that it's easy to see the pixel-perfect matching.
I begin with tres + the patch in #212. You can clearly see the problems. Then I apply the changes for the patch attached to this comment. You can see how everything matches perfectly.
bartik.wide already correct + header region broken (00:05–00:15)
This shows the problem caused by #213.1. I use a menu block as an example. Clearly broken.
At the same time, note how the primary and secondary menus match perfectly already at this breakpoint.
bartik.narrow: already correct (00:15–00:30)
The alignment of the text in menu links is wrong. The header region is also still broken at this breakpoint.
bartik.mobile: already correct (00:30–00:20)
What isn't wrong here? The toggle doesn't consume 100% of the width, has rounded corners while it shouldn't.
Apply this patch… (00:40–00:55)
… and reload both stme and tres, plus reset them to bartik.wide. Now we can restart our comparison.
bartik.wide still correct + header region fixed (00:55–01:05)
This shows #213.1 has been fixed. I use a menu block as an example, and show that this looks exactly the same.
bartik.narrow is now correct (01:05–01:15)
Pixel-perfect match now!
bartik.mobile is now correct (01:15–01:30)

Whew. Hope that helped.


interdiff-header.txt shows the changes just to fix #213.1. The other interdiff shows the rest. That one is not very reviewable though. I optimized it to make the *actual patch* easier to review (least changes possible). So please review all the Bartik CSS changes not via the interdiff, but via the actual patch.
(This is also why the new patch is smaller than the previous ones.)

Status:Needs review» Needs work

The last submitted patch, 215: global_menus_as_blocks-1869476-215.patch, failed testing.

Wim Leers’s picture

StatusFileSize
new54.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,618 pass(es).
[ View ]

Straight reroll.

Wim Leers’s picture

Status:Needs work» Needs review

I'm such a noob.

Wim Leers’s picture

We probably need to reroll this now that #1777332: Replace theme_menu_link() and menu-tree.html.twig with a single Twig template and #1972122: Remove the DIV tag around block content have landed. But, that doesn't mean we can get a preliminary RTBC for #217 in the mean time, so that only the interdiff for a reroll will need to be reviewed still.

rteijeiro’s picture

Assigned:Unassigned» rteijeiro
Status:Needs review» Needs work

Re-rolling...

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new49.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,589 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolled (I hope so)

Status:Needs review» Needs work

The last submitted patch, 221: global_menus_as_blocks-1869476-221.patch, failed testing.

lokapujya’s picture

I think there is some unnecessary spaces.

  1. +++ b/core/themes/bartik/css/style.css
    @@ -630,62 +637,45 @@ body:not(:target) .main-menu-reveal:after {
    +.region-secondary-menu .menu  a:hover,

    after .menu.

  2. +++ b/core/themes/bartik/css/style.css
    @@ -1974,26 +1964,27 @@ div.admin-panel .description {
    +  [dir="rtl"] .region-primary-menu .menu li,
    +  [dir="rtl"]  body:not(:target)  .region-primary-menu .menu li {

    before and after body:not(:target)

* These were just minor; Doesn't mean there aren't bigger issues.

rteijeiro’s picture

Assigned:rteijeiro» Unassigned
Issue tags:+Novice, +Amsterdam2014

@lokapujya nice catches, thanks!

Tagging as novice and DrupalCon Amsterdam code sprint. Looking for a novice to contribute.

Wim Leers’s picture

Assigned:Unassigned» Wim Leers
Issue tags:-Novice

#221: is this a reroll to just chase HEAD or to address #219? I'm pretty sure it only chased HEAD?

#224: This is nowhere near a novice issue.

I'll get this green again.

lokapujya’s picture

lokapujya’s picture

Actually, summary looks ok.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
Status:Needs work» Needs review
StatusFileSize
new51.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,706 pass(es).
[ View ]
new3.24 KB

Reroll of #217, not #221.

joelpittet’s picture

Status:Needs review» Reviewed & tested by the community

Reviewed this with @Wim Leers in detail. And again after and I believe this patch is RTBC.

This patch provides:

  1. Allows global menus to be treated like blocks.
  2. Accounted for CSS for Bartik and cleans up some of the id selectors necessarily for the blocks to account for which region they are initialized within.
  3. Standardizing "<nav>" tags for menu blocks.
  4. Added depth limits and starting level to the menu blocks.
  5. Removes the confusing and often drupal support questioned 'feature' where secondary menu changes from user menu to secondary menu if the source menu is the same as primary.
  6. Menu cache tag test fixes.
  7. Two new regions to support the CSS to account for the areas of the pages designating areas for these two menu blocks.
  8. The CSS changes were visually confirmed to be unchanging between breakpoints in header, footer, sidebar areas of the page.
  9. Has change record and passes tests

Next Steps:

There likely needs to be a followup for migrate to set new mappings for d6. (not sure how hard those would be to implements maybe they could be added to this patch even.
@see core/modules/migrate_drupal/config/install/migrate.migration.d6_menu_settings.yml

Thank you all!

  • webchick committed daaccc3 on 8.0.x
    Issue #1869476 by rteijeiro, LewisNyman, lauriii, Wim Leers, mdrummond,...
webchick’s picture

Status:Reviewed & tested by the community» Fixed
StatusFileSize
new10.27 KB

This looks great!

I took this around the block (ha!) and back. Two issues I found:

#1: Overlapping contextual links

Unfortunately this is a fairly obvious user-facing bug. It'd be awesome if there were some way to fix it tomorrow during the QA sprint.

#2: The menu block depth selector and such doesn't seem to be working. If I tell it to show two menu levels, and put "log out" underneath "my account" then I still only see just "My Account" when on that page, not the sub-page. However, this also is present in HEAD. But if we could make sure there's a follow-up for this (if there isn't one already) that'd be wonderful.

In the interest of making the most of the QA sprint tomorrow, I'd like to get this in now, even though it's not been RTBC very long. I feel like more end-users testing this out will lead to better results with future conversions.

Therefore!

Committed and pushed to 8.x. Yeeeeehawwwwwww!

joelpittet’s picture

mdrummond’s picture

Oh wow, amazing to see this get in! It has been a very long road! Thanks for all the work from everyone to get this in!

tim.plunkett’s picture

This is slightly broken.

{% set heading_id = attributes.id ~ '-menu'|clean_id %}

attributes.id is optional, in tests I'm seeing <nav id="-menu" which is wrong.

I have no idea what ~ does, or why we're not just doing this in preprocess, but can someone open an issue?

joelpittet’s picture

@tim.plunkett ~ is concatenation in twig. Dot operator is used for objects and + operator for math... so they went with ~

Was the id defaulted because in twig you could do this:

{% set heading_id = attributes.id|default('aux') ~ '-menu' %}

Also that clean id is quite useless... due to operator precedence:
https://github.com/fabpot/Twig/issues/328

It should be

{% set heading_id = attributes.id|default('aux')|clean_id ~ '-menu' %}

And the finally, any questions regarding "why not doing things in preprocess" need to be routed to @JohnAlbin, @mdrummond, @davidhernandez and of course @mortendk

They are the banana experts... and are much better at explaining the plan.
https://www.drupal.org/node/2322163

@mdrummond do you mind opening that issue to fix that up?

Wim Leers’s picture

Strange, any chance they're only broken in tests? It works fine in the actual rendered pages for me (if it didn't, then the main menu toggle on very narrow viewports wouldn't work either), at least in 6ef8ba79091ba8ef07008a68ae11c8fcde7d1958.

mdrummond’s picture

We're not doing class declarations for templates in preprocess anymore, but this is an id, so slightly different. If the heading ID is optional, then we could tweak the template to fix that. Would be good to know if this is a test-only issue, though?

joelpittet’s picture

+++ b/core/modules/system/templates/block--system-menu-block.html.twig
@@ -0,0 +1,65 @@
+{% set heading_id = attributes.id ~ '-menu'|clean_id %}
+<nav{{ attributes.addClass(classes) }} role="navigation" aria-labelledby="{{ heading_id }}">

It is here, not in a test. :) We could like add a test to show that is broken in the fix up issue, I don't mind helping with that if you want? Just need to pass an unclean attribute.id to the template (all attribute key/values checkplained so not that kind of unclean). Likely should show both a fail and a success for the menu block.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.