Don't use menu_tree_page_data for $main_menu and $secondary_menu

Marked as duplicate #1446170: Main menu show disable menu link.

Look at menu_navigation_links how the hide disabled menu.

But instead of making your own system it will be certainly possible to overwrite theme_menu_link .

I find than menu in block use correct theme function. So the problem is only if you use the main_menu and secondary_menu variable in your template (in fact $secondary_nav and $primary_nav with this theme).

I am subtheming twitter_bootstrap, in the subtheme i just put the main_menu back to default and add in the class .nav and things should work relatively fine.

function mytheme_preprocess_page(&$vars) {

  if (isset($vars['main_menu'])) {
    $vars['primary_nav'] = theme('links__system_main_menu', array(
      'links' => $vars['main_menu'],
      'attributes' => array(
        'class' => array('links', 'inline', 'main-menu', 'nav'),
      ),
      'heading' => array(
        'text' => t('Main menu'),
        'level' => 'h2',
        'class' => array('element-invisible'),
      )
    ));
  }
}

There are a few issues in theme.inc that comes with twitter_bootstrap,
On the main menu, the h2 is inside the ul, the li don't have classes on them, etc. If anyone is interested, I can fix up a patch that makes it use the default drupal theme function links__system_main_menu to create the main menu.

Patch to make the main menu use the default drupal theme function links__system_main_menu. It fixes the problems introduced in twitter_bootstrap_twitter_bootstrap_links() like invalid markup of h2 in ul, missing active and other classes on li and a.

The only visual difference I can see is now the active class triggers the css for .navbar .nav .active

We should delete twitter_bootstrap_menu_navigation_links function.

Also perhaps take a look at https://github.com/mpezzi/bootstrap.

makangus' patch (#4) fixes another problem - menus are not properly localized in the current version of twitter_bootstrap - but it means that multi-level menus no longer work.

Thanks for this I patched this on my subtheme

makangus' patch (#4) has also fixed a problem we had with discarding purl in some menus.

If the plan will be to keep using twitter_bootstrap_menu_navigation_links I have a patch for it to fix the purl issue and the problem mentioned by lathos (#6).

I agree, the navigation is pretty useless, active trails are not applied, hidden items are being displayed etc. I used the menu blocks module, works a lot better.

I try patch number 4 but not work, how can i disable the bootstrap navigation?

#4 do not work for sub menu.

#4 Should be in need review for a future commit as its waiting since April.

as say in #11 this do not works if we have submenu.

Well, to solve this issue we need to decide:

1. Do we want submenu's to work?

If so, then I think we need to continue using menu_tree don't we?

The original issue complains that the disabled items are being shown. I've posted a patch in:

#1825790: Menu still shows "disabled" menu items

So, that negates the initial complaint...

The other is sub menu items... I started playing with that.. it isn't perfect but my initial test patch is in:

#1706180: dropdown for main menu?

Happy for feedback though, I'm sure there may be better methods to resolve these issues.

We can use main-menu block in a region "top bar" instead of the primary_nav. Poeple than don't want this top bar should be able t just drag the main-menu in an other region...

What do you think?

Or something like that:

//in theme_links or preprocess_page...
function theme_links($variables) {
  if (array_key_exists('id', $variables['attributes']) && $variables['attributes']['id'] == 'main-menu-links') {
      $pid = variable_get('menu_main_links_source', 'main-menu');
      $tree = menu_tree($pid);
    return drupal_render($tree);
  }
  return theme_links($variables);
} 

In fact, the best thing would be to completely disable $primary_nav and $secondary_nav page variables which are ancient ghost of Drupal 5 menu system and use the menu blocks instead which are far more flexible.

Same for search

maybe we should just remove the vars and replace them by a region

ok added a patch that removes the vars and adds a region instead

i also think this patch is somhow related to http://drupal.org/node/1706180 and if accepted should be commited together

ok redone the patch, strange that it fails

removed also all the search form settings and theming

Thanks Frank.

I've converted one site of mine to using region for the main menu, so I'll just check and see how your patch affects it.

It works. Anyone see any problems having the nested region?

I understand the navigation region though, otherwise we'd need to include a custom region--header.tpl.php template file (which is what I did with another site to get menu blocks).

The patch really should be combined with #1706180: dropdown for main menu? to make it worthwhile. If I have time in a moment, I'll combine the two.

Minor adjustment to capitalization, same patch as #27 otherwise.

To move things along I have committed Frank's patch to 2.x-dev.

Menus can be added to the Navigation region via Blocks administration. It's recommended to set the menu block title to <none>

If you want to float a menu right (e.g. user-menu), then it is just a case of adding a theme override to include pull-right, such as:

function <your_subtheme>_menu_tree__user_menu($variables) {
  return '<ul class="menu nav pull-right">' . $variables['tree'] . '</ul>';
}

So, the latest commit did cause an issue #1860142: Notice: Undefined index: navigation in include() (line 23 of [...]/themes/bootstrap/templates/page.tpl.php).

As an interim (patch attached), I've added the original primary_nav and secondary_nav options back for those who don't have anything assigned to Navigation (or if Navigation doesn't exist).

Flagged for testing.

#1860142: Notice: Undefined index: navigation in include() (line 23 of [...]/themes/bootstrap/templates/page.tpl.php) is fixed but this means the original issue in #1 is still present.

I'll propose a new patch over the weekend to resolve this once and for all.

Ok, so I've looked into this for quite some time. I'm going to summarise the issue below.

The original issue asks:

I don't understand why the nav links pulled in again using menu_tree_page_data. This function will post disabled items.

I wasn't around when the theme was developed, however the #1 reason I can see for including primary_nav and secondary_nav is to include dropdown and submenu support. Perhaps due to bugs, this may or may not have been obvious when this issue was raised. Disabled items are no longer displayed. #1825790: Menu still shows "disabled" menu items

Why don't we just the links that are already in $variables['primary_menu'] and $variables['secondary_menu']?

To do this, people would need to be able to render dropdowns and submenus via a theme function (ie. bootstrap_links). It appears this doesn't work 100%. If someone wants to help fix that, then perhaps we can remove $primary_nav and $secondary_nav.

To close this issue I propose:

1. bootstrap_preprocess_page
Change bootstrap_preprocess_page to populate $primary_nav and $secondary_nav in the same way menu blocks would populate it, using menu_tree (similar to code posted by gargarine in #15). This will ensure that the main menu will support dropdowns and submenus as soon as the theme is installed - no need to assign blocks to regions unless people choose.

2. page.tpl.php
Keep $primary_nav and $secondary_nav menu's as defaults in page.tpl.php. If people want to remove them, they can easily untick Main Menu and Secondary Menu in the theme settings and assign block menus to the Navigation region. The Navigation region would remain, so out of the box it would be rendered as:

$primary_nav + [region: navigation ] + $secondary_nav

Search will need to be assigned to Navigation via blocks. Turning off Main menu and Secondary Menu in the theme settings would just leave people with the Navigation region.

3. In future we may be able to remove $primary_nav and $secondary_nav
At some later date we can consider removing $primary_nav and $secondary_nav completely. I think to do so, we need to ensure a theme function is available to render the Drupal main_menu and secondary_menu's with dropdown + submenu support, so that it works out of the box. (We can create a new issue for this).

Hope all this makes sense. I'll attach a patch as soon as I can.

Any problems/issues? Let me know.

This is how I handled dealing with default menus vs. block in navigation region in the Framework theme. This might be a useful way of handling the choice in the page.tpl

<?php if ($main_menu || $secondary_menu || !empty($page['navigation'])): ?>
  <nav id="navigation" role="navigation" class="clearfix">
    <?php if (!empty($page['navigation'])): ?> <!--if block in navigation region, override $main_menu and $secondary_menu-->
      <?php print render($page['navigation']); ?>
    <?php endif; ?>
    <?php if (empty($page['navigation'])): ?>
      <?php print $main_menu; ?>
      <?php print $secondary_menu; ?>
    <?php endif; ?>
  </nav> <!-- /#navigation -->
<?php endif; ?>

+1 for natted summary and solution, got my ok

Alright, so here is the patch I am proposing:

What has changed?

1. bootstrap_preprocess_page

• Adjusted the code to use the two menu's specified within menu.module settings and render the menu's with menu_tree in the same manner that is done with menu blocks.
• Added theme wrapper functions for the two main menus menu_tree__primary and menu_tree__secondary

Why change this?
The default menus now render out of the box in the same way as the menu block's, so they automatically should support dropdown menus and nested submenus. I've included the theme wrapper functions in case people want a simpler way to override the HTML wrapper for the menu's (allowing to change the style of menu button in a subtheme etc).

Default main menu theme wrapper:

<?php
/**
 * Bootstrap theme wrapper function for the primary menu links
 */
function bootstrap_menu_tree__primary(&$variables) {
  return '<ul class="menu nav">' . $variables['tree'] . '</ul>';
}
?>

Default secondary menu theme wrapper:

<?php
/**
 * Bootstrap theme wrapper function for the secondary menu links
 */
function bootstrap_menu_tree__secondary(&$variables) {
  return '<ul class="menu nav pull-right">' . $variables['tree'] . '</ul>';
}
?>

As mentioned above: page.tpl.php renders navigation as follows:

$primary_nav + [region:navigation] + $secondary_nav

This means, if you don't want $primary_nav and $secondary_nav, then just disable them in the theme settings and they are gone. You can then just use the Navigation region to assign block menu's. If you want a third menu, just assign it via blocks to Navigation... mix and match!

<?php 
      <?php if ($primary_nav || $secondary_nav || !empty($page['navigation'])): ?>
        <div class="nav-collapse">
          <nav role="navigation">
            <?php if ($primary_nav): ?>
              <?php print render($primary_nav); ?>
            <?php endif; ?>
            <?php if (!empty($page['navigation'])): ?>
              <?php print render($page['navigation']); ?>
            <?php endif; ?>
            <?php if ($secondary_nav): ?>
              <?php print render($secondary_nav); ?>
            <?php endif; ?>
          </nav>
        </div>
      <?php endif; ?>
?>

Out of the box, this is how it looks:

I'm sure the first think people will say is.. "the right hand user-menu is not a dropdown... ". So, to turn it into a dropdown you can either edit the menu and add a new item to nest the links .. e.g:

Or, the user might just decide to override their subtheme (this is just an example...):

<?php
function <subtheme>_menu_tree__secondary(&$variables) {
  // Let's display the title for this menu
  $secondary_menu = menu_load(variable_get('menu_secondary_links_source', 'user-menu'));
  $output = '';
  $output .= '<ul class="menu nav pull-right dropdown">';
  $output .= '<li>';
  $output .= '<a href="#" class="dropdown-toggle" data-toggle="dropdown" data-target="#">'.$secondary_menu['title'].'<span class="caret"></span></a>';
  $output .= '<ul class="dropdown-menu">' . $variables['tree'] . '</ul>';
  $output .= '</li>';
  $output .= '</ul>';
  return $output;
}
?>

Any thoughts? I've tested this and it appears to be working. I guess others can tell me if this change is worthwhile...

Like I mentioned, longer term - we may be able to remove $primary_nav and $secondary_nav if we can improve the bootstrap theme functions. I think though, we can create a separate issue for that if we feel it is really important to do so.

Great work, natted. +1 from me.

commited http://drupalcode.org/project/bootstrap.git/commit/e5db762

and also tested it couple of times, works like a charm now