Convert primary and secondary menus to blocks

I think this is a bad idea. The whole concept of primary and secondary menus should go. It is a theme setting, and should be a theme option.

Also note that secondary menus can appear as child menu (below) the primary menu. This is a feature that's not possible with this solution. I agree that this whole feature should be scrapped, but you need to be aware it exists as themes must be changed to support this removal.

Here's start on removing this feature. Removed from everywhere I could find, CNR for the test bot.

Forgot about user links being in the secondary links. Back to CNW - need to sort those out somehow if we do this.

A big +1 on this. It's a huge source of confusion for newbies ("why do i have two primary menus?") and is more consistent with the way other 'special' elements like mission and footer_message are also becoming blocks in D7.

So do these blocks have titles off by default?

This is great, should simplify things a little more and make it easier to move menus around the site.

Makes a lot of sense to me. Converting the menus to blocks removes special casing (improving usability) and is more flexible. It also dovetails nicely with #351249: Finer control over the Parent Menu select box. The combination of these two patches would allow for the removal of the confusing menu settings page.

This is an updated patch.

Removed from everywhere I could find the main and secondary links feature. (mostly from the patch in #3)
(In Garland) Created a 'navigation' region via garland.info (Do I need to add regions[page_top] & regions[page_bottom] explicitly as I did?).
(In Garland) Put 'main menu block' inside the newly created 'navigation' region. Via system.install
(In Garland) Put 'secondary menu block' inside the 'footer' region. Via system.install
(In Garland) Whatever menu block you put inside the 'navigation' region is styled like primary links in D6 and supports two levels menu out of the box. Second level menus are styled like the secondary links in D6. You can assign more than menu block to the navigation region. They won't break but live side by side.
(In Garland) Whatever menu block you put inside the 'footer' navigation is styled horizontally and supports two levels menu.

Notes:
I had to modify Garland CSS to have this working but I haven't been able to test this modification in IE on windows (especially IE6).

Let me know if you like this approach and have a look at the code.This is my first big patch. I'm pretty sure I'm missing something huge. Be kind.

These are some (hopefully) explanatory screenshots.

How long does it take to have a patch tested from the testbot? I posted the patch on February 1st. I wonder if there's something wrong in the patch...

It seems testbot is having problems, not your fault.

1104 fails! Wow! It was better when the testbot didn't work. I guess I'll have to browse the documentation to understand what all those fails mean.
Anyhow I won't be able to bring home this patch alone. Is there anybody out there willing to review, at least the approach taken?

As far as I can tell this is Drupal 8 material, we're only fixing bugs for D7 now, not changing/reworking features anymore.

I don't see it as a new feature and it does not change API, so this seems as D7 material to me.

Edit : forgot the "not", uh.

.

People are already porting themes to Drupal 8, and those almost invariably use the primary and secondary links variables, so it's an API change for themers, if not for module developers. So also think this is Drupal 8 now - although it should go in really, really early if at all possible since these are now even more inconsistent with other template variables now that help and mission have been moved to blocks.

The problem is that in the current state this feature is broken, at least in a UX point of view. I tried to summarize what's wrong here #698014: Theme settings for main/secondary variables mismatch with menu settings. Many people is complaining now and will once we ship D7. If we really think that we can't change it now I'll try to patch what's possible to make it usable. But I'd like to hear one more voice before giving up.

I agree. Maybe the patch could simply reproduce the current behavior, that is not having the sub-menus provided by patch from #14.

Menus are the only thing we cannot manage through blocks in Garland.
Moreover, although I am not aware how this will be handled by D7 version of the i18n module, in D6, you have to use menus as blocks to enable translations or it won't work.
IMO, all core themes should do this.

To preserve theme API, we should simply modify Garland (and other core themes) to use blocks and let the variables as they are.

Setting this back to D7 on behalf of this new proposal.

+1

It doesn't make sense to leave these hardcoded given all the improvements that have been made so far. Also, given the odd default placement of the secondary menu (a user menu in the footer, really?), many people will want to change this. Why make users have to edit template files to do so when they can just move blocks around in the UI?

As far as this affecting themers, IMO the change is positive. Especially for the links in the footer, which (a) are currently impossible to shut off without editing page.tpl.php because theme settings are broken and (b) do not contain a wrapper div, which makes it difficult to target only that menu, and its title.

There are already primary and secondary menu blocks created by default, so what actually needs to happen here? Patch to Garland and the standard profile?

Right now:

- garland/page.tpl.php prints the main and secondary menus in the header region.
- system/page.tpl.php (think Stark & CSS-only themes) prints the main menu in the header and the secondary (user) menu in the footer region.

I think the header region is the most appropriate default place for both menus.

The hardcoding should be removed from these files, and blocks could be placed in the header by default.

I just totally confused myself looking at admin/structure/menu and seeing both a "user" and "secondary" menu.

$secondary_links in page.tpl.php will either be the "user menu" or the "secondary menu." See: http://api.drupal.org/api/function/menu_secondary_menu/7

Looking at the markup in Stark <ul id="secondary-menu"> I thought I was looking at the "secondary menu." In fact I was looking at the "user menu" with an inaccurate ID.

...and the WTF moments continue.

A secondary menu makes a lot more sense in the footer by default. In any case, converting these to blocks would be a million times better.

@Jide
The problem with leaving the variables is that we can't get rid of the menu settings. We can't have settings for something that isn't printed by the default theme. It's a nonsense even if I understand it's a way to preserve the legacy.
For what I see there are 2 options:

• 1. We go with my patch at #14 gaining consistency and support for two level menus out of the box (but someone needs to help cause I don't understand why there are all those test failures)
• 2. As pointed by catch since we're late and can't modify theme API we can at least try to clean up the code. These are the problems:
  • A. the variables that store the settings in theme.inc are called main_menu and secondary_menu (they should be primary_links and secondary_links)
  • B. the theme hook to print the links in page.tpl.php is "links__system_main_menu" (it should be "links__system_primary_links"). Even the id is set to 'main-menu' (it should be 'primary-links')
  • C. Garland mess things even more by providing its own variables to print them. In template.php: primary_nav and secondary_nav (they should/could be primary_links and secondary_links) and again it hardcodes the headings to 'Main Menu' and 'Secondary Menu' (they should be 'Primary links' and 'Secondary links')

  Ideally I'd like to find different terms that don't overlap (If we call the containers 'Primary links' and 'Secondary links' the menus shouldn't contain the same terms, something like 'Main Menu' and 'Auxilliary Menu' ?)

Does it make sense?

A couple concerns:

If this made it in, what would happen to the D6 feature that lets you set primary links and secondary links to the same menu, so secondary links automatically just displays children of the current primary nav item? #7 says we'd be keeping that page, but it seems to be removed in the latest patch. If that got thrown out, how would local nav be implemented?

Also, would this patch remove the ability for a theme to specify where menus go? If menus are strictly blocks, then contrib themes will all have to tell you where to put the menu blocks instead of just putting the menus there by themselves, which would be pretty awkward. It would remove the "download, enable, and it just works" paradigm of contrib themes.

Yeah I'd forgotten about that secondary links feature when typing this, that's only really doable with http://drupal.org/project/menu_block at the moment. So this really does feel like D8 now, but I already moved it there once.

I agree.

I suspect at that point themes will start adding primary and secondary links *regions* to themes. That idea rubbed me the wrong way at first but then started making a whole lot of sense considering all the use cases where people want to add more content to this area, such as an inline search box beside the menu, two menus side by side, different menus depending on page visibility or role or other factor. What we really need is a way for themes to specify the blocks that go in regions by default, but that's another issue.

Having the menus support two uses cases, including the feature mentioned in #32 with child items has always been a WTF for users, and menu_block gives you that functionality and more.

This would still be adding a heap of configuration to something that right now is simple to get working (confusing UI and labelling aside...).

With two regions for menus, how much work would it be to get the current couple Primary and Secondary links feature? I'm guessing you'd need to put both blocks in, then some settings on both blocks, etc etc etc. That's a big retrograde step for the UI.

In fact my patch in #14 completely remove the primary and secondary links coupling feature.
But the navigation region supports two level horizontal menus, so you can replicate that feature simply by creating a two level menu (if you do it right now you won't see the childrens in primary links).
In my opinion this is much more intuitive and consistent than how it is now and there are LESS settings to tweak because everything works out of the box.
Moreover if we provide the navigation region as a default one (something I haven't done in the patch, I just added it via garland.info) contrib themes will still be able to decide where to put the main menu by positioning the navigation region.

Now that I talk about it I'm thinking of going even further and convert both primary and secondary links to default regions (primary and secondary navigation?) and put main menu in the first one and secondary menu in the latter.

For what I see the only advantage of the current implementation is that you can assign only menus to primary and secondary links, something you can't control once you convert them to regions.

But the navigation region supports two level horizontal menus, so you can replicate that feature simply by creating a two level menu

It sounds like you're talking about a hierarchical menu, and I'm talking about actual local navigation (as in, a local sub-menu that can be placed anywhere and is not a part of the main nav). Is that right? Would your method allow me to have my main nav at the top and my local nav in the left sidebar, as two completely different UL's?

Ok, I will try to make time to try the patch.

@dodazzi: Have you seen http://drupal.org/project/menu_block ?

@mcrittenden
You're right that's not possible but:
1. You can still do that using a separate (local) menu and block visibility settings.
2. You can't place it anywhere because primary and secondary links are hardcoded in the page.tpl.php (for example in Garland it's not possible to have it in a sidebar unless you change the code)

I still believe this should be part of D7. If not I'd like to make the actual implementation cleaner by making the labels less confusing and by hiding the menus assigned to primary and/or secondary links from the block settings (is it possible?), to avoid menus showing in two places

#3: remove_primary_secondary.patch queued for re-testing.

#14: primarymenu.patch queued for re-testing.

Agreed with the existing patch (entire removal). Agreed with the pointer to http://drupal.org/project/menu_block, as that's literally all we're talking about here.

Adding a pointer to http://drupal.org/project/skinr, as both menu blocks received a horizontal link list styling.

Totally important D8 material. Needs work then.

> pointer to http://drupal.org/project/menu_block, as that's literally all we're talking about here.

Basically, yes. And also no.

What the current theme system does is give you two magic $foo_links variables, which are not menus in two important ways:

1. They are horizontal lists of links run through theme_links(), not the menu tree system
2. They are not a menu tree, but a *slice* through a menu.
(3. They also couple together, but once we abstract the slice concept, you can just do that by setting up a block for each of the 1st and 2nd level slices.)

Menu blocks does slices, but it outputs them as trees (ie vertical lists). So we need to take care of the horizontal theming too -- in other words, allow a special case for a single-depth menu slice.

Righto, slices first. Second, attach styles. <ul class="links inline"> is what you do now, thus skinr, or whatever, though that's more generic than menu blocks, but in the end, the actual demand.

Once people start to use menu_block, they completely forget that core once provided this weirdness. #620618: Optimize menu tree building and use it for toolbar landed too late, but introduced base and playground for menu_block in core.

Subscribing. For D7, maybe we can still do something to clean up the usability issues (and messed-up markup issues) related to this issue that are discussed above, via #698014: Theme settings for main/secondary variables mismatch with menu settings. And then kill it altogether in Drupal 8 :)

The primary and secondary links variables still use menu_tree_page_data() - on a cache miss that's 3-4 MySQL queries, when IME only the final one ever actually returns anything - this might not be the case when using primary links as the source for both sets of links but would be good to optimize this a bit too. I have a replacement for menu_secondary/primary_links() in performance_hacks module which handles the basic case (down to one query which is then cached for all pages).

*sigh*
Subscribe.
I've just been tooling round with a prototype in stark, and ended up with an unwanted 'Secondary Menu' in the footer region that I couldn't get rid of because it wasn't showing in block management pages.
So I tried turning off "Secondary menu" in "Toggle display" of the theme settings, and I still couldn't get rid of it.

So I trace code through stark theme, phptemplate, theme system, preprocessors, and finally find
system/page.tpl.php

    <div id="footer"><div class="section">
      <?php print theme('links__system_secondary_menu', array('links' => $secondary_menu, 'attributes' => array('id' => 'secondary-menu', 'class' => array('links', 'clearfix')), 'heading' => t('Secondary menu'))); ?>
      <?php print render($page['footer']); ?>
    </div></div> <!-- /.section, /#footer -->

:-(
I thought messiness and special cases like that was eliminated from everywhere by now
:-(

Not only, but the freaking menu heading is hard-coded! Boo.

... In the end, it turns out I had 'user-menu' set as the "Source for the Secondary links" in menu settings - but the hard-coded heading was leading me down blind alleys.

Now.., given that discussion here says that secondary menus are being retained as a special case indefinitely ... shouldn't that wacky chunk of code at least be moved into preprocess_page and out of the tpl?

#48 sounds like it'd be a good cause for a new issue.

I would suggest moving most of #48 to #698014: Theme settings for main/secondary variables mismatch with menu settings, as it seems related to that.

So I tried turning off "Secondary menu" in "Toggle display" of the theme settings, and I still couldn't get rid of it.

This part sounds like its own very special bug, and it should probably get its own very special issue :)

I ended up finding #410646: "Secondary menu" exists but is no longer the default source for the secondary links as an issue for this that is still on the active list, so maybe I'll be following it there in the hope of a fix.

The reason that toggling off 'secondary menu' in the theme not working is probably because the 'secondary menu' it was theming was not actually the 'secondary menu' but instead the 'user menu' ... it just was displaying with the hardcoded label "secondary menu".
Hence the WTF.

On general principles, I'm unhappy with tpl pages calling further functions (even just theme_* functions) when ample opportunities exist within preprocessors to cook those values. tpl.php files should do nothing more than print($value) IMO.

Subscribing.

I am +1 in favour of getting rid of $primary_ and $secondary_ links.

I am -1 in favour of losing the ability to place "sub-navigation" (nested menu items) into the site layout. The coupling of primary and secondary links used to provide this functionality. Let's not lose it or I'll have to keep telling people to use the Book module to get sub-navigation working "properly."

See also: #737136: Put together a list of must-have features for new core themes and set a final deadline for implementing them

@52 http://drupal.org/project/menu_block is very good and a better bet than book module.

And also requires that I teach people how to install contrib modules. This is not always appropriate for the types of sites that people are building. The functionality exists in core. Let's not lose it.

Surely if we convert 1ary and 2ary menus to blocks, that entails taking most if not all of menu_block's functionality into core.

#878772: Primary and Secondary links headings are hard coded also works around the issue that these menus are not blocks.

#44 - Menu blocks does slices, but it outputs them as trees (ie vertical lists). So we need to take care of the horizontal theming too -- in other words, allow a special case for a single-depth menu slice...

What I would not like to see is core assuming because its a single-depth slice that it should automatically get the inline class, we need to stop assuming what themers and designers want by semi-styling the front end for them, let the themer theme, adding CSS such as this often just gets in the way. What would be pretty cool is if we had depth classes on menu UL's, e.g. .level-1, .level-2 etc.

I would be very happy if this entire main menu / secondary menu thing was totally nuked out of core and we bring in a simplified Menu Block module to take over the coupling duties. Two different issues yes: see #1102462: Add Menu Block module to core

I would be very happy if this entire main menu / secondary menu thing was totally nuked out of core and we bring in a simplified Menu Block module to take over the coupling duties. Two different issues yes: see #1102462: Add Menu Block module to core

I agree with that as the main objective here.

See also #474004: Add options to system menu block so primary and secondary menus can be blocks rather than variables. I see this issue as a separate discussion and not a duplicate of that one.

So, how are we doing here? Latest patch is 2.5 years old :) There's consensus about the direction that yes, primary and secondary menus should become blocks. What are the to-do's if somebody wanted to write a new patch for this? Seems #14 provides a solid outline.

I think #1869476: Convert global menus (primary links, secondary links) into blocks is probably a duplicate. Some discussion of the topic there too. This is much earlier but that has a much more up to date blocks as plugins based focus. Maybe we want to close this down and move there?

Marking as duplicate of #1869476: Convert global menus (primary links, secondary links) into blocks.