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.

Proposed solution

Either outright remove these special variables from core or introduce them as specific blocks that can be moved around. A possible reason for the later is all the custom styling and markup that themes are used to provide to major menus on the page. In short:

  • Remove special case items from themes.
  • (Optionally) introduce them as specific blocks that can be moved around.
  • Introduce regions as needed to place these elements.

Related issues

The code should depend on blocks as plugins (#1535868: Convert all blocks into plugins), however, we should not mark this postponed so we can do work on this in parallel and be ready for commits once that lands.

Files: 
CommentFileSizeAuthor
#132 menu-block-1869476-132.patch3.82 KBmdrummond
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,277 pass(es).
[ View ]
#123 menu-blocks-1869476.123.patch28.43 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 59,201 pass(es), 9 fail(s), and 3 exception(s).
[ View ]
#123 interdiff.txt4.97 KBswentel
#112 menu-blocks-1869476.112.patch34.84 KBEclipseGc
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 ]
#110 menu-blocks-1869476.110.patch38.33 KBhosef
FAILED: [[SimpleTest]]: [MySQL] 56,598 pass(es), 74 fail(s), and 8,647 exception(s).
[ View ]
#103 menu-blocks-1869476.103.patch38.33 KBhosef
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 ]
#103 interdiff.txt5.83 KBhosef
#100 menu-blocks-1869476.100.interdiff.txt3.54 KBlarowlan
#100 menu-blocks-1869476.100.patch33.88 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,392 pass(es), 244 fail(s), and 8,867 exception(s).
[ View ]
#98 1987692-inderdiff-98.txt19.18 KBcbiggins
#98 menu-blocks-1869476.98.patch30.34 KBcbiggins
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#87 menu-blocks-1869476.87.interdiff.txt15.58 KBlarowlan
#87 menu-blocks-1869476.87.patch33.66 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,431 pass(es).
[ View ]
#85 menu-blocks-1869476.84.interdiff.txt21.33 KBlarowlan
#85 menu-blocks-1869476.84.patch29.11 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,419 pass(es).
[ View ]
#81 menu-blocks-1869476.81.interdiff.txt1.77 KBlarowlan
#81 menu-blocks-1869476.81.patch36.14 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,537 pass(es).
[ View ]
#80 Screen Shot 2013-04-15 at 4.52.21 PM.png30.79 KBlarowlan
#79 menu-blocks-1869476.79.interdiff.txt6.34 KBlarowlan
#79 menu-blocks-1869476.79.patch36.13 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 54,376 pass(es).
[ View ]
#77 menu-blocks-1869476.77.interdiff.txt1.35 KBlarowlan
#77 menu-blocks-1869476.77.patch34.03 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,345 pass(es), 2 fail(s), and 12 exception(s).
[ View ]
#74 menu-blocks-1869476.74.interdiff.txt560 byteslarowlan
#74 menu-blocks-1869476.74.patch34.16 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#72 menu-blocks-1869476.71.patch34.16 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#61 menu.links_.61.patch32.77 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,734 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#61 interdiff.txt757 bytessun
#58 menu.links_.58.patch32.66 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,740 pass(es), 1 fail(s), and 305 exception(s).
[ View ]
#58 interdiff.txt8.1 KBsun
#54 menu.links_.54.patch27.47 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,875 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#54 interdiff.txt8.22 KBsun
#35 menu.links_.35.patch25.05 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,341 pass(es), 17 fail(s), and 4 exception(s).
[ View ]
#35 interdiff.txt5.33 KBsun
#33 menu.links_.33.patch22.74 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,258 pass(es), 12 fail(s), and 8 exception(s).
[ View ]
#33 interdiff.txt4.72 KBsun
#22 menu.links_.22.patch17.43 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,393 pass(es), 15 fail(s), and 5 exception(s).
[ View ]
#22 interdiff.txt4.44 KBsun
#18 1869476-17.patch14.44 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 47,570 pass(es), 2,287 fail(s), and 14,615 exception(s).
[ View ]
#1 global-menus-as-blocks-1.patch4.65 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 49,295 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

Comments

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.

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

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

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

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

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

Eclipse

Note:

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

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

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

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

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

Please also note that one of my primary goals for D8 was #474004: Move menu_block module functionality into core, since Menu Block module replaces the entire primary/secondary links plus all menu blocks with configurable menu block instances, which are able to solve a dozen of different real world use-cases.

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

I'd suggest to do the following:

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

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

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

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

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

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

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

EDIT related #1882552: Get rid of menu_list_system_menus()

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 (title, tabs, actions, messages) 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.

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

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

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

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

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

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.

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.

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

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: Move menu_block module functionality into core 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.

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

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.

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

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

@sun

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

Eclipse

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

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

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

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

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.

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.

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

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.

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

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

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

If so I must be doing something wrong.

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

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

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

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

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

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

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

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

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

Eclipse

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

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

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

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

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

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

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

Thanks for the clarification @dcrocks, I found it.

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

Issue tags:+Spark

Tagging.

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

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

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.

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.

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.

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.

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.

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.

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

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

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

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

Issue tags:-Spark

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

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

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

just for reference I completely agree with #70

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.

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.

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.

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

StatusFileSize
new30.79 KB

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

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

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

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

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?

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.

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.

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

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

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

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

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

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

The tests look good.

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

Those changes mostly looked good to me.

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

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

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

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

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

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

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

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

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

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

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

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

Status:Needs work» Needs review

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

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

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.

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.

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.

Issue tags:-B&L

Removing the unnecessary B&L tag.

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.

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

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.

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.

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.

Issue tags:+Needs reroll

Needs reroll as per #116.

Issue summary:View changes

Updated issue summary.

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: Move menu_block module functionality into core.

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.

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.

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.

Issue summary:View changes

Updated issue summary.

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

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

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

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

Honestly I am still rather bewildered why we need this and don't instead heavily pursue #474004: Move menu_block module functionality into core, is it too late for that now?

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

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

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

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

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

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.

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: Move menu_block module functionality into core so this one should focus on theme integration and probably wait for menu.inc removal

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

hehe, de ja vú?

@andypost: Yes, there's definitely duplication of discussion in #474004: Move menu_block module functionality into core, 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: Move menu_block module functionality into core for the first part, which allows for adding a block, and this issue for removing the page variables and replacing them with the appropriate menu blocks, I'm fine with that. Whatever will get the job done.

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

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

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

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

Status:Needs review» Postponed

After some further discussions, I'm going to move the work on patches to #474004: Move menu_block module functionality into core, as this first step is much more about getting the essentials of menu_block into core. Once that's done we can come back here to replace the primary_links and secondary_links variables with blocks.

+1 Looking forward to it.

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

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

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

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

Issue tags:+beta target