Integrate primary links with menu navigation

The way I /intended/ to write this, was that the local task CSS would be used, unless you theme the function. Or unless you provide your own CSS to override the local task CSS. For this purpose you had -by default- to wrap the primary links in another span or div, called something like "main menu".

But we should nto add more primary links CSS in drupal.css. that file is already way too cluttered an big. We should just have *one* small chunk of CSS that handles *all* horizontal LIs.

  <div class="list">
    <ul id="primary-links">
      <li class="item $i++"><span>.....</span></li>
    </ul>
  </div>

This makes all themable possibilities possible imo and very easy...
I vote for a markup like this...

I tried this patch without looking at the code, but it does not seems to work. After clicking around for 5 minutes, I found this little note on the 'global theme settings page':

Navigation settings
You have only one container. The primary items are automatically set to use that container.

This message is officially nominated for the "most obscure status message I've seen in months"-award. No seriously, I've no idea what to do with this.

1. It took me a while to locate the menu setting (message). Not the most intuitive place IMO.
2. The message seems to suggest it uses a default container of some sort. (What is is 'container'? What are 'primary items'? What has this to do with 'navigation'?)
3. What does this 'container' contain? It still see 'edit primary links' and 'edit secondary links'.
4. No further instructions? (I don't even have the menu module enabled so I can spend hours looking for 'containers' and 'primary items'.)

The UI and integration need work IMO.

Looking at the code, I'd rather not have the system module iterate over the $menu array. Ditto for theme.inc -- no way someone is ever going to theme that function. The $menu array is supposed to be private to menu.inc/menu.module. Don't emit tabs (\t\t). There is also a TODO in the code?

This code needs a lot of work.

Stefan: why not use something like the following? There's no reason for the extra DIV and SPAN elements.

  <ul class="list" id="primary-links">
    <li class="item $i++">.....</li>
  </ul>

This code needs a lot of work.

Above all it needs some decisions to be made. Then the best preactices for these decisions can be chosen, and only then work on the code should be done!

Please lets not rush this code in, but let us make it future proof and above all usable. So lets first look for what we want and **only then** code it. I know about the silver-gold rule. But imo Talk is silver, Code is gold, but having both the gold AND silver, instead of gold OR silver makes you the richest.

We must decide on:
* The DOM: do we follow the local tasks (imo not the ideal), or do wee design a new DOM.
* Nested vs non nested: all listamatic examples (and nearly all oters) use non neested ULs.
* Empty ULS: I know that a lot of designs will look very ugly or even break if there is no empty DIV, in place where normally the secondary rows will show up.
* Optional parameters: an often heard request, for example, is to render *all* levvels, for DHTML purposes.
* Dependency on menu.module. Adrian kinda convinced me that thats not a good thing. But still id like to hear other opinions. We could simply say "primary links use the navigation menu by default. If you want to change that, you must enable menu.module". [1]
* (How) Do we provide, the defaults. Drupal should offer sensible, useable and 60%-of-the-cases-already-correct primary links. IMO >60% of the users should not have to bother about changing them, but can use the defaults.

We then must find best preactices:
* Do we use one themable function with more parameters, or rather more funtions with less parameters. Its like: menu_items_nested($menu) and menu_items_unnested($menu) VS. menu_items($menu, $nested = TRUE);
* Where do we put the GUI. It now lives in theme settins, and should remain there, untill that whole theme settings stuff gets sorted out. But IMO navigation settings should live with the menu settings.
* Optimise help, names and descriptions. primary links was chose by me, simply because we have always named them like this. But its not a good name, at all.
* Optimise code and speed, minimise memory usage, minimise iteration loops. **we should end with this, not start this. Technology should follow functionality, never the other way around**

[1] This is the case anyway. The system now uses a menu as "container" for primary links. By default you have only one menu/block/container: navigation. If, for example you want Blogs, Forum and My CV in your primary links, you must create a new menu, called e.g. "Primary Menu". Menu module will make this a new block too. You then add the three menu *items* to that menu, possibly with children.
Now, in the current approach you can use the theme settings to set that "Primary Menu" as your container, for primary links.
Conclusion: for this to make sense you need menu.module anyway!

hey, don't blame me for copy-pasting bad code ;)

Furthermore, the css used sucks .. and we will need someone to write proper default css for us to make it at least presentable in the absence of theme css.

Despite having helped clean up the css in that patch a little I think it's better just not added. All themes will style these major links, when does anybody view the menu without a theme enabled? It's yet more code in drupal.css for theme developers to override.

These links are presented as lists precisely because the degrade well in the absence of CSS, sure they look ugly, but they still work. We'd be better off either just using the local task css as suggested, or adding to the core themes CSS to style these in a theme specific manner.

I dont get it: What is all the fuzz abuot this CSS about? m HTML will be rendered with the *already available* CSS for local tasks. why do we need additional CSS? Are these local tasks not good enough?

I have to abandon this, I spent way too much time on this already, but have trouble with the bordering levels. I simply do not manage to get the proper iterations of the -very complex- menu array.

Is there anyone who has enough advanced array knowledge, so that he/she can finish this? If not,we will not have more advanced pirimary menus in core for 4.7.

You can find the -working- module:
http://cvs.drupal.org/viewcvs/drupal/contributions/modules/primary_links/

Patch doesn't apply anymore.

Here's a fresh patch which applies to HEAD. All the code written to date for this feature is contained in this one patch. The module and the old patches have all been merged into this patch and are no longer needed.

Warning... this patch doesn't actually work. As far as I can tell it's at the same stage as Bér left off back in June. But it still needs a lot of work. I'm going to be actively working on this code over the next few days. If you don't hear back from me within a week I'll be lost deep in the menu array... please send a rescue team!

My understanding of the requirements for this feature are:

• to remove the "primary links" and "secondary links" from the theme settings
• to allow the menu system to be used to maintain the primary and secondary links
• to generate a single-level ul of primary links
• to generate a single-level ul of secondary links containing the menu items which are children of the current node
• to optionally allow a fixed single-level ul of secondary links, no context, useful for people who use this as a footer menu or something
• come up with some CSS that renders the ul horizontally and isn't completely horrible to look at. I expect this will be overridden by every theme so the fact that I'm a complete CSS dunce shouldn't be a big problem :)
• the item for the current page(s) should be highlighted
• add some settings to allow the admin to specify which menu is to be used to generate links as well as controlling some other features

If you want anything else from this code, get your feature requests in quick.

Richard++;

We'll rescue you if necessary. :-)

Richard. Great to hear someone picked this up.
If you have questions about my code, feel free to get me on jabber (ber@jabber.org.uk) or by my drupal contact form.

Here's a new patch which works pretty well. All of the features I had specified above are fully implemented.

The primary_links module has been merged into menu.module and menu.inc. It makes no sense to keep them separate because without menu.module there is no meaningful menu from which to generate the links.

For the CSS I duplicated all the primary/secondary tab styles in misc/drupal.css, renamed them and tweaked them a little. I realise this isn't ideal, but I'm absolutely hopeless with CSS and I was having a really hard time making Bèr's old styles work. At least these ones work, even if they are inefficient. If any CSS wizards want to help out with simplification of the styles I'd appreciate it.

I have removed the notion of generating nested unordered lists. The code I have written here can be refactored/extended and used to generate such lists. But for the purposes of generating primary, secondary and even other generic horizontal navigation bars this was an unnecessary complication. Such nested lists would require some really nasty CSS/DHTML to be useful and I'm not volunteering to write that :)

I have also included the ability to specify that any menu displays horizontally whenever it is displayed in a block. I figured it would be trivial to allow custom horizontal link blocks (for example a row of links in the footer). But as it turns out this was pretty complicated because blocks weren't designed to be displayed without a heading and the themes made assumptions about the content of specific blocks. I think this feature needs a little more work, but is well worth it because it extends the notion of primary and secondary links to allow whatever horizontal menus the user needs.

The changes to the user interface compared to HEAD comprise:

• addition of admin > settings > menus item where the admin can choose from which menu the primary links are generated
• on admin > menus > add menu or edit menu I have added a "horizontal" checkbox to specify that the menu is to be rendered horizontally.
• removed the references to links from theme settings

No database alterations are required.

And of course 4.6.3 themes will need to be modified to display links correctly.

The admin - settings - menus page appears to be empty? I can't test it.

Would I be able to create the exact same navigation structure as we're using on Drupal.org?

thanks for picking this up, richard ... i am wondering why we need code specifically to render a horizontal menu. shouldn't that be the same UL with special CSS? i am referring to theme_tabs_menu_row(). perhaps there is a reason.

no time to test the patch right now. sorry.

It certainly can do the Drupal site top navigation. See attached... only a few minor tweaks to the CSS from drupal.org were required. (But note how the menu renders with the top element at the right instead of the left... I hate CSS!)

I have simplified the Settings code so hopefully that will work now.

I'll have a look at refactoring the theme_tabs_menu_row code to use some of the existing tab functions with some new CSS and post a new patch later tonight. I still have a few remnants of the nested UL code in this patch which I guess I can do away with.

I think there will still be a fair bit of new code in menu.inc. But hopefully I can make this a bunch of useful little utility functions.

I can see I'm going to have to polish up my CSS skills to complete this patch :(

The patch didn't apply cleanly for me. It kept complaining about wrong file names. Here's a copy rerolled that should apply better.

Big +1 on functionality. Interface still needs some work. More specifically, There needs to be some better way to make it clear to the user how to edit the navigation links. A default menu with a link to the admin/menu page, to mimic the old themes method? Rename admin/settings/menu to admin/settings/links? I'm not sure here, but it needs to be more obvious.

Also, this naturally opens up the possibility to have an arbitrary number of link bars, or to have the link bar vary with the page (by section.module or other). That would be very cool.

As for the CSS, I'm not sure putting it in drupal.css is a good idea. That should be something left primarily to the theme, I'd think. The default should be just a very very simple render-inline-not-with-bullets code block.

That means there's also no need for a force-horizontal-in-blocks option. I'm not entirely sure where that would be useful, actually, since it would get very wide very fast. :-) It would be better if the ID given to the block was based on the menu's name rather than on the ID, making it easier for a site admin to theme.

And if I sound over-critical, I'm sorry. I really do think this is a cool idea. :-) And you're right, the potential for sub-menus is even cooler, pending the CSS/JS to do it.

In this patch I have done a lot of work on menu.inc. The local_task tabs and the primary/secondary links are now generated from pretty much the same code. IMHO the code is now easier to follow and more flexible. I grepped through all the core Drupal code and none of the functions I eliminated were being used from outside menu.inc. It's possible contrib modules were using them I guess, but they shouldn't have been :)

Re: special code for horizontal menus... The HTML generated for horizontal menus is a bit different than that generated for the vertical menus. menu.inc already generated two flavours of HTML, one for the menu tree and one for the local tasks. I had a hack at the menu tree code, adding a "no_recursion" parameter and tweaking the class attributes generated. My intention being to use the same code to generate ALL the menus. But I started having strange problems with the tree-style menus falling apart. Rather than forcing a re-write of all the CSS for these menus I think it's best to leave the horizontal and vertical menus separate at this stage.

Re: the patch not applying... it worked fine for me with patch -p1 < patchfile. I'll roll this new patch so that the -p1 is hopefully not needed. I really should follow the instructions about generating patches from cvs.

Re: force horizontal blocks... The "arbitrary link bars" feature Crell mentioned is why I implemented this flag. I was thinking specifically of the case where a menu is required in the footer block, for example "Disclaimer | Privacy | Terms of Use". With this new code I could create a menu "footer menu". Set it to display horizontally. Add the three links to it. Enable the block, placed in the footer. This results in HTML for the new horizontal menu being generated instead of the vertical one and it will automatically display with the same appearance as the primary links. I think this is a nice feature and I can't envisage a simple way to allow this without including a bunch of extra CSS in each theme just on the off-chance the user wants a horizontal menu somewhere. And even then the CSS would be trying to render the HTML designed for a vertical menu tree into a horizontal bar.

Re: hard-to-find settings. Yes, this needs some more thought. Would adding a "default menu with a link to the admin/menu page" require manually inserting menu records into the database? I'm not sure I want to go near a process like that!

Re: drupal.css... I've ripped all the extra styles out of here. The links menu uses the styles for the primary local tasks with a few settings over-ridden in theme stylesheets if necessary.

at the botton of our db setup scripts we already instead data into url alias table and several others. it is perfectly acceptable to add a line there for primary links if thats what needed.

at the botton of our db setup scripts we already instead data into url alias table and several others. it is perfectly acceptable to add a line there for primary links if thats what needed.

at the botton of our db setup scripts we already instead data into url alias table and several others. it is perfectly acceptable to add a line there for primary links if thats what needed.

I think we are really overcomplicating this.
My reason for the past route/implemantation, wéas because blocks could only appear in sidebars. That is no longer the case.

So here is a far easier route to take:
* Introduce two new blocks:
** primary links
** secondary links
* add a few lines of CSS to the themes (NOT to drupal.css) #header .menu { ... } ...

The two blocks will render a non-hierarchical menu, where 'primary links' llists the in the admin defined primary links, and 'secondary links' gets filled with any links active under the current active primary item.
example:
- foo
- about us
- - Cool stuff
- - About our Big Boss

if 'foo' is active or any other link, secondary block is empty
if 'about us' is empty, seconday block contains 'Cool stuff and About our Big Boss' links.

All other advanced stuff like deep hierarchical tabs, or 'static' tabs (non changin secondary links) can be achieved with bloks, now! I tried it, it works. You really only need some configuration and a few lines of CSS and a line to define a header region, to do this in your theme now.

Bèr

To Bèr

Simple is good. I'm happy to code up a simpler solution once we nail it down :)

Do you mean to introduce Primary Links and Secondary Links as blocks, so that the Admin enables those blocks in administer >> blocks? This raises a few issues:

• Blocks are always displayed with a title. Some changes to block.module would be required to allow titles to be turned off.
• It makes the admin use the Blocks interface to enable a Menu. Poor usability.
• Need to add another two regions to the themes. Not a big issue by itself, except...
• It would give admins the option of placing other content into the region intended for links. Which may or may not be a good thing.
• It would give admins the option of placing the Links into any region. Which again may or may not be a good thing... for sure the horizontally-formatted Links aren't going to work well in a sidebar!

Or do you mean to have the Links represented internally as blocks but displayed through some special code that places these blocks into a certain area in the theme. Because that's pretty much what this patch already does :)

In order to render a non-hierarchical menu some extra code is required in menu.inc. The current menu.inc doesn't have the ability to return HTML for a single level of a menu. It always follows the hierarchy. Enabling that functionality is at the core of my changes to menu.inc. I have chosen to implement that by extending and refactoring the tabs code. It could just as easily have been done by extending the tree code to add a "no-recursion" flag.

Deep hierarchical tabs I have already scrapped from this patch because they are beyond the scope of this issue. I don't think anything I've done would make implementation of a dynamic menu system any harder. In fact the refactored functions in menu.inc might be of use.

"Static tabs" are a bit complicated but worth investigating further as part of this issue.

I think being able to add a link bar in the footer (or header) managed through the menu system should be part of the Drupal core and shouldn't require extra CSS to be written. I have tried to implement some links in the footer as a "proof-of-concept" of this patch, as well as to make sure my code isn't so specialized that it's only useful for the primary/secondary links and cannot be used in other interesting ways.

I tried to do what you have done, and implement such a footer with a block and CSS and I failed miserably. I ran into conflicts between the CSS for the sidebar navigation and the footer navigation. Although that's probably just me being a CSS-newbie. I'd be interested to see how you managed to do it... if I can work it out I'll try to roll it into this patch in a simpler and more elegant way than my current implementation.

Ber: I don't understand what you're after with the block stuff.

Richard: I'd love to see this patch hit core. I wanted to try it, but it no longer applies. Mind to re-roll it against HEAD? Glad to see one can mimick the drupal.org navigation structure. Great work. Can this be accomplished from the node submission page?

Adrian: mind to have a look at this patch too?

Dries, my (confusing) post about block regions boils down to the following:

my primary_links module and thus the code in this thread, is very complex.
Most of that complexity achieves the following: get slices of LIs out of the menu, to display them horizontally.

But, that is now no longer neccesary. We can now say in our stylesheet. If a menu-block appears in the head region of our theme, then render the LIs horizontal.

In other words, quite some of the previous complexity is no longer neccesary. "All" we need is a way to say "make a new menu" (which is not a menu entry!). then that menu is a new block. Then "all" we need to say is "put this new block in the header".

We can say all that from within Drupal. But we need to find an automated way to do this on installation of a new drupal.

The current menu system does not allow this.

Yes, I'll roll a new patch against HEAD. I think I can work out now exactly what is required/expected of this patch so I'll also try to finalise the interface and set up some default settings so that the whole system is enabled and useful by default. This will probably take me a couple of days as I have some real work to do that has to take priority.

Dries, setting up the top navigation on the Drupal site could be done using the menu options on the node submission page. But I rather think the Drupal nav is a special case... much simpler than most sites. I would usually expect a site to have a complete hierarchical menu displayed vertically down the left. The top nav would be different to this menu with links pulled from multiple levels of the hierarchy and perhaps with additional links added from outside the tree.

As Adrian points out, the menu options only allow a node to be added to one menu. So managing a Drupal.org-style top nav through this interface will only be possible if the top nav is identical to level-1 of the menu tree.

In fact the menu management options available on the node submission pages are very limited and not of much practical use for managing the menu. All they allow is for a new page to be added to the menu. I have (which probably won't apply any more either) which completes the functionality of this interface, allowing a full hierarchical navigation tree to be managed exclusively from the node/edit pages. I think these two patches complement each other and they should be committed together. I'll also re-roll the other one.

I'll drop the "horizontal" block option for now. All it's doing is adding unnecessary complexity to this patch.

@Ber:

The menu.inc in HEAD offers no way to extract a single level of a multiple-level menu hierarchy as a UL. This is essential functionality because the primary and secondary links must be generated from a hierarchy to allow secondary links (and tertiary even) to follow the context of the active trail.

Most of the code already existed to allow this to be done, but it was locked away in the special-purpose functions that generated the primary and secondary tabs in the admin pages.

My changes in menu.inc are really only a refactoring of these special-purpose functions. I have extracted duplicate and single-purpose code into a couple of new utility functions and extended the these functions to make them more general-purpose.

The primary and secondary tabs and the primary and secondary links are now generated by the same code-path through menu.inc. If I do say so myself, this is very slick indeed :)

Sorry, I had some bad HTML in my last post... and I can't see how to edit a post!

Continued...

I have another patch in the queue (which probably won't apply any more either) which completes the functionality of this interface, allowing a full hierarchical navigation tree to be managed exclusively from the node/edit pages. I think these two patches complement each other and they should be committed together. I'll also re-roll the other one.

I'll drop the "horizontal" block option for now. All it's doing is adding unnecessary complexity to this patch.

@Ber:

The menu.inc in HEAD offers no way to extract a single level of a multiple-level menu hierarchy as a UL. This is essential functionality because the primary and secondary links must be generated from a hierarchy to allow secondary links (and tertiary even) to follow the context of the active trail.

Most of the code already existed to allow this to be done, but it was locked away in the special-purpose functions that generated the primary and secondary tabs in the admin pages.

My changes in menu.inc are really only a refactoring of these special-purpose functions. I have extracted duplicate and single-purpose code into a couple of new utility functions and extended the these functions to make them more general-purpose.

The primary and secondary tabs and the primary and secondary links are now generated by the same code-path through menu.inc. If I do say so myself, this is very slick indeed :)

I just want to make sure I'm understanding what Ber et al are saying. Please thwap me if I'm wrong here. :-)

There are three ways that "link collections" can be displayed: Static, constant links; full navigation hierarchy; and nested section-oriented static links.

Static, constant links are the theme links from 4.6. As Ber said, those can now be done via a block in 4.7. Just create a menu, get a simple <ul> out of it, and throw CSS at it to make it look horizontal. That also allows an arbitrary number of link bars instead of just primary and secondary. Sounds great, but I agree the UI should be a bit clearer than it would be now to do it that way. Perhaps something as simple as the settings page from Robert's patch with more inline documentation (help text) and a bit richer UI? Some default CSS would be helpful, too. (Eg, whatever menu is flagged as "show as link bar" gets a "link-bar" class class added to it auto-magically.)

Full navigation hierarchy is the current Navigation block. No changes required. Yay.

OK, so the interesting one is nested section-oriented links. That is, a "primary" link bar with About, Forum, and Blogs, then when viewing Forum a secondary link bar appears that is sub-sections under Forum but while viewing Blogs a different set of sub-navigation is displayed. Defining the structure here is the same as the static links (and other menus), but there needs to be some fancier logic for display. Potentially, this could be an arbitrary number of sub-menus, and they may not be shown next to each other. That means simply nesting <ul>s together is not always going to work. I'm not sure of the best way to handle this, though, since it would need so much buy-in from the current theme.

I'm going to think "aloud" for a moment here, please bear with me. :-)

The user defines a menu named "Links" that has three levels to it. (Reasonably large site.) That automatically gets a block that corresponds to it. If nothing else is done, then it renders as nested-<ul>s with no special styling. If the admin places that block on the side, great, no change. If they place it in a horizontal-friendly location (header, footer, etc.), then it's up to them to throw CSS at it to turn it into a horizontal nav bar and hide whatever parts they want. (Most likely if he just wants a single-line nav bar he'll only make a single-level menu.)

Now add admin/settings/links (or similar). In it, have a single select box to pick the "menu to use for navigation". Then there's a checkbox to "break sub-menus out into their own block". If that's set, then instead of creating a single block it will create N blocks, one for each level in the selected block. They will be named for CSS as menu-level-1, menu-level-2, etc. When the admin now goes to the blocks page, there will be Links-1, Links-2, etc. as different blocks. The sub-level blocks auto-populate with that particular sub menu, based on the current location. He can now position them wherever, and theme them with CSS the same way as before.

Does that make any sense to anyone? Am I onto something, or way off base?

I now await proper thwappage. :-)

The way I envisage this working in the context of a fairly large site is:

The admin creates a new menu. I always call this "Site Content" because CEOs who want to drive my Drupal installations understand that term. That makes a block which I enable in the left-hand sidebar. This becomes my main site navigation. I intend to set up some defaults in my next patch so that this is set up automatically.

The admin also has the option of setting this as the menu from which primary links are generated. The last patch creates admin/settings/menu which shows some help text and a drop-down menu which contains a default of "No primary links" as well as listing all the available menus. Once again the next patch will contain a default setting pointing to the "Site Content" menu.

I hope that this will be all the administration that's required to enable a hierarchical sidebar menu as well as the primary links (and secondary, tertiary etc as implemented by the theme).

I have these all being generated from the one massive hierarchical menu because that means the admin/editor only has to maintain one menu hierarchy. And by using the menu_otf features now in core, all new pages may be added to this menu in the appropriate place in the hierarchy. With my other menu patch rolled in, the menu_otf features should be capable of maintaining all aspects of this big hierarchical menu.

So far Crell and I are very much on the same wavelength. But hang on while I go grab my thwap (or is that "a thwapper"?).

I think it should be left to the theme to decide how many levels deep the horizontal menus are displayed. The default themes mostly have two which should be sufficient for most sites. If someone has a site complex enough to warrant three levels, they probably have already been exposed to the dirty side of Drupal and they shouldn't have too many troubles with the fairly simple process of adding another level to this hierarchy.

I think the extra complexity to introduce a "break sub-menus out into their own block" feature as well as the raft of changes required to the default themes, extra help text and so on is unwarranted. This feature can fairly easily be implemented in a theme. As an example, with the new menu.inc, extracting a UL containing the 5th-level deep row of links along the active path looks like: theme('menu_links', NULL, 5). This can be called from a phptemplate and that UL can be wrapped in a div to allow custom CSS to apply to each level. Easy.

I think this last paragraph of Crell's post might describe something that some people would find useful (or even just interesting to play with). But I don't think it's really a "core-quality" feature. More something themers can play with.

At this stage I think this patch should do little more than implement the revisions to menu.inc to allow a single-level UL to be retrieved from a menu tree, remove the configure-navigation-in-theme-settings and related cruft and set up some useful defaults.

I'm going to be pretty much unavailable to work on Drupal from the start of next week until mid-November. So I'll get to work on cleaning this patch up and hopefully have it committed before I have to leave off.

I'm way too new to be posting to this topic, so i will keep it very brief. Is a lot of this functionality relating to work that is being done on the taxonomy module, to allow the admin to set up a primary site heirarchy?
http://drupal.org/node/23730
Currently I achieve the 'outcome' of primary links and secondary links on the lhs mostly through the bastardisation of the books node type, but it doesn't work well for dynamic content.
Is there a crossover between the taxonomy module and the menu module that needs to be addressed? Or are we talking about completely different functionality or motives?
(be gentle - im relatively new, but trying to get involved ;)

This feature is unrelated to that other issue.

This one is much more generic and relates entirely to the way the menu can be displayed.

Anything that can be inserted into a menu will show up in the Primary and Secondary links panels. Which includes both taxonomies and books.

Here's a new patch against HEAD.

Can I attach files yet, or is that still broken? If there's no attachment, get it from here:

http://mel01.juggernaut.com.au/primary_navigation_5.diff

Just uploading this patch now attachments are working again.

I have also updated my other menu system patch at: http://drupal.org/node/32665
That patch complements this one, as it allows the site navigation menu to be more easily managed via the Menu Item Settings fieldset on the node/*/edit pages.

These two patches won't apply together because they both try to add admin/settings/menu. Perhaps I should merge these two patches together?

Some minor code comments:

- INSERT INTO variable VALUES ('menu_primary_menu, 'i:2'); has a missing quote. Both MySQL and PostgreSQL are affected.

- 'Site Content' should be 'Site content': don't capitalize each word.

- theme_menu_links() has no parameter $show_empty as advertised in the PHPdoc.

- "primary" should be 'primary': use single quotes where possible. Ditto for "secondary".

- Some code comments start with a capital letter, others do not.

- title'=>t('menus'),: there should be spaces around =>. Ditto for the other menu callback parameters.

- For consistency, %adminmenu should be %menu.

Otherwise this patch's code looks good! :-)

Next up, I tried the patch.

1. I can't seem to create a new menu. It seems to work with the default 'Navigation' menu though. Haven't tested adding menu items from the node submission form. (I'll have to investigate that menu problem.)

2. I'm not 100% convinced the menu setting is necessary. IMO, it would be easier to use if there was a hardcoded 'primary links' menu. Less configuration overhead.

3. I'm not 100% convinced with naming the menu 'Site contents'. Maybe it should be 'Top navigation' versus 'Block navigation' or something. Something which gives me a clue as to where I can expect the menu items to show up.

I fixed the menu problem on my localhost, and tested the various aspects of this patch once more. Everything works as expected. I'll commit this patch after one more round of updates/fixes. Thanks Richard.

Re: coding style errors... wow, that means nearly 10% of the new lines of code contained an error!

Re: the "Site content" name. I agree this is sub-optimal. And I still feel uneasy about creating a new menu on installation when it is unclear what this menu is for or how it needs to be populated.

It is a requirement of this patch that the menu used for Primary links contains a complete hierarchy of all the content in the site. This is the only way the context-sensitive secondary links can be created. "Hmm", I hear you say... "that sounds just like the Navigation menu".

So I have changed this patch so that the Navigation menu is the default. There will now be no configuration overhead for most people as this is a very sensible setting.

But the menu setting needs to remain so the admin can override this default if they want a custom or single-level Primary links system. Like for example the drupal.org one.

As for the reasons for wanting to create a "Site content" menu... this was so that the admin menu wasn't displayed in the secondary nav (it's waaaaaaaay too wide) and also so I could hide the admin items in the menu_otf menu. But I'll sort that issue out in another patch by adding a menu setting to enable filtering the admin menu items out of these menus.

The only other significant change in this patch is some re-wording of the admin/settings/menu text to make the use of "primary links" consistent.

I don't really like the term "primary links" but if we start debating that now we'll still be going in 6 months :)

Be careful not to introduce too many options; less options is better. Putting the navigation menu at the top sounds like a bad idea; it's way to dynamic (eg. tabs come and go based on your permissions). Therefore, an option to hide one particular tab, doesn't sounds like a workable solution.

Here's another patch, same as the last one but with a default menu called "Primary links". This keeps the name consistent throughout.

It would be rather confusing if a "primary links" setting made the "Site content" menu appear in the "Top navigation area".

• No upgrade path at all? Given that we are moving to PHPTemplate, porting over the old PHPTemplate links if they exist would be nice. Many people use PHPTemplate based themes.
• While being able to derive the secondary links from the primary links is a nice option, it is certainly not the only usage case. Often, secondary links are simply less important links that live next to the primary ones. I think it should be possible (but optional) to explicitly specify a different secondary menu that is not a hierarchy step below the primary.
• The themables seem a bit too scattered and unfocused.
  • menu_local_tasks() contains the <ul> tag, which some people might want to theme. But it delegates the actual menu items to:
  • theme_menu_local_task() which just wraps an <li> around each, which is fetched with:
  • menu_item_link() which calls:
  • theme_menu_item_link() which is a wrapper around l().

  Phew. Of course the last two are used for every menu item (not just tabs), but it would be nice if we could simplify it a bit. At the least, I think it makes sense to move the <ul> and its foreach into theme_menu_local_task() (which should then be renamed into theme_menu_list() or something). That way themers get a more familiar ul/li loop, similar to how primary/secondary links work now.

• theme('menu_local_task', ...) is still invoked with the now non-existant third parameter.
• Bluemarine lost the divs with the id's for the primary and secondary menu. The CSS now contains stale rules. Converting them to use the class instead (.primary and .secondary)should work, as each ul has a class by default.
• There are a bunch of changes in pushbutton CSS which seem out of place (e.g. changing a color or font size). Does pushbutton still look the same after this patch? Especially the footer menu HTML and CSS seems badly mangled. All the footer menu links are now in a cell labeled secondary menu.

I dont think we should bother about an upgrade path. It is going to be a hell to code. And really, who has more then 20 primary/secondary links? That takes about 10 mintus to copy paste. But it looks like it is going to take a huge effor to code in a update.inc (it needs hook_menu for one).

Hi Steven,

That's all valuable feedback, thank you!

Re: upgrade path... how about this:

• Add a setting to allow the admin to specify the source for Secondary links. The options would be:
  • No secondary links
  • Use primary links
  • and a list of possible menus
• New installations get set to generate primary links from the Primary links menu and for secondary links to use primary links. The Primary links menu gets created but is empty (I still don't like this much... it's not clear that this menu exists or what it's for).
• The upgrade process looks to see if legacy primary/secondary links are used.
• If primary links are used, create a Primary links menu and populate it with entries from the legacy primary links.
• If primary links were not used, set the new primary links to "No primary links".
• If secondary links are used, create a Secondary links menu and populate it with entries from the legacy secondary links.
• If secondary links were not used, set the new secondary links to "No secondary links".

I think this is possible, but with these settings stashed away inside variables instead of in their own database table it's going to be ugly, ugly code.

Re: themables... I'll make two points here:

• I have tried to keep this compatible with 4.6.3 as far as the function call used to obtain the tabs ul is concerned.
• I have chosen not to do more extensive refactoring in menu.inc even though the code that generates the menu tree could be partially merged with the code that generates these single-level uls.

I don't want to change my approach in either of these areas because that will require more changes throughout the core code, buy-in from more affected parties etc. I want this patch to make it to core so I want to keep this patch as simple as it can possibly be while introducing the new functionality.

That said, I'll look into the menu.inc again and see if I can increase the cohesion of these functions and make their names more appropriate.

Re: changes to CSS in themes:
I'm a complete CSS dunce and I'm really struggling with this side of the patch. If someone could chip in and help with this I'd be grateful. I would hate to see this patch rejected because I can't get my head around CSS.

And no, the pushbutton theme doesn't look the same. It used to be in the link | link | link style but I have no idea how to make that work in CSS so I made it more consistent with the other themes using the button-like styles from the tabs.

It would be great if you could give approval-in-principle for these alterations. I need the scope for this patch to be better-defined so I can spend the few hours I have available for Drupal work more productively.

Normally, people would have only 5 to 7 primary links, and at most 10. If they have more, they have a very cluttered interface.

I think that an upgrade path here is over kill. Just document the fact that you have to do so and so and that is it.

I too am not to worried about an upgrade path. If you have limited time, I'd focus on:

1. Providing support for secondary links.
2. Fine-tuning the themes:
   • make sure Bluemarine and Pushbutton look good and that people are 100% happy with the introduced changes: the safest path is not to introduce visual changes. The last thing we want is uglier themes.
   • make sure there is no redundant/left-over CSS code
3. Beautifying the (theme) API: making it easier to theme or use something is a plus.

This is what the headers (and footers) look like after applying this patch. Screenshot attached. I think there is room for improvement.

About the upgrade path, I still think we need one. When the theme system was made, we converted legacy XTemplate settings into the new system and even extracted the logo image source from the <img> tag that you had to set before. It is not that difficult:
http://drupaldocs.org/api/4.6/function/update_104

The main issue I see is that after this upgrade, your old links are all gone. There is no place to copy them from save for going into the database. Yes, we can warn them in advance, but we all know how well that has worked in the past. People will complain: remember the "categories" block? Except this time it's a feature that is used on every Drupal site.

Also, as far as the themability goes, the navigation links at the top and the actual local tasks at least need some default CSS id's that allow you to distinguish them without having to override the theme_ function. The namin is confusing too: "local tasks" are now the old local tasks, and the site-wide navigation links.

As far as the look goes, if we move the <ul> into the themable function as I discussed above, then we could just insert an old style implode(' | ', ...) in pushbutton to get the old look back. Due to IE6 not supporting CSS2, there is no way to do this with CSS only. Regardless, "room for improvement" is a bit of an understatement.

Starting Monday my time is utterly over-committed until November 15. For that month I won't have more than an hour here and there to work on Drupal.

I'll try to get another patch rolled up this weekend, addressing the code problems and the upgrade path.

I probably won't go near the CSS again... I'm sure there's someone out there that can knock that off in an hour whereas it would take me 8 and the job would be inferior.

Richard: any luck with that patch? If we want to include this in Drupal 4.7 (and we really do), this patch needs to hit core within the next 7 days.

Richard. If you cannot manage, please ping this thread. I will see if I manage to finish it then this sunday.

I have managed to do a bit more work on this... mostly reducing the scale of the changes.

I'll roll another patch up tonight.

Here's another patch.

I have removed all alterations to style sheets. And themes have only been changed so that the links are pulled out of the new menu.

I have reverted all the local_task functions in menu.inc and just added extra functions for this feature. There is some refactoring that could be done in menu.inc but the benefits are marginal and it's more important to complete this patch.

In menu.inc there exists the ability to generate either a menu in the same style as previously (link | link | link) or as an unordered list. This adds some complexity to this code, but it is necessary to both retain the previous behaviour and allow me to still answer "yes" to the question "Would I be able to create the exact same navigation structure as we're using on Drupal.org?". Actually accessing the ul-based list requires the theme to request this directly. I thought about adding a setting to control this but decided against it.

I have also added a new function in update.inc to import old primary/secondary links into new menus. I've given this a bit of a test but it really needs to be tested thoroughly since, as Steven points out, if this fails the user could lose data.

If I've done everything as I intended, after the upgrade the generated HTML should be identical to pre-upgrade.

Hm. I'm afraid I'm having a number of problems with the 8 patch.

I created a quick sample menu named Primary Links, with two top level entries and one second level entry. I then set that menu as the primary links menu. I then get a message at the top of every page

Warning: Invalid argument supplied for foreach() in /home/lgarfiel/public_html/drupal-cvs/includes/menu.inc on line 846

I suspect that's also the reason that the "secondary links based on primary link section" function isn't working. The secondary link is not showing up. It's caused by the line $mid = menu_follow_active_trail($menu, $pid, $start_level - 1); sometimes returning an int rather than an array, which then causes the foreach() loop to gag.

Also, this may be a design decision but it appears to be back to being placed only by the theme rather than using the blocks system, as earlier versions did. Did I miss that part of the discussion? :-) I also frankly see no reason to have the old XTemplate link list setup. Theming a ul list is the more conventional and frankly more powerful way.

And finally, admin/settings/menu should be named something that doesn't create automatic confusion with admin/menus. :-) I'd suggest admin/settings/links or site links or something like that.

I'd really like to see this functionality in 4.7, but like Rob I sadly don't have the time to commit to recoding it within the next week. This version of the patch is not ready yet, though. :-(

Hi Crell,

Thanks for taking a look at the patch. I can see where there's a possible problem but I can't make my installation break the way yours did.

If you still have it available, could you email me a dump of the contents of your menu and variable database tables?
drupal.org@juggernaut.com.au.

Thank you!

Steven/UnConeD was going to have a look at this.

A couple of points about Crell's testing:

1. Yes, it was decided that if you want a menu in a block that the existing blocks interface makes this trivial and therefore this patch didn't need to consider that use case.

2. There's a fundamental problem with my code. I have relied heavily on "menu_in_active_trail()". But in the case where the primary links menu has been specially created and contains menu items that are present in other menus, this fails. The active trail sometimes follows the items in the original menu instead of the primary links menu. This is dependent on the order the items are created... lower ID takes priority.

The obvious fix for this would be to create a new function with similar functionality to menu_in_active_trail but which restricts the search for the trail to a specified menu.

But I'm afraid I don't have time to work on this just now :(

I have attached a patch which applies against the current head (but head at the moment is a rapidly moving target!). And with a TODO at the point which is causing problems.

I have addressed the problem where menu_in_active_trail traversed the wrong part of the menu tree... I wrote a new function which generates an active trail beneath a specified menu item. Not a particularly elegant solution, but it does seem to work.

The upgrade function needs to be widely checked too. It seems to work fine for me on my simple test setup but testing on a more complex site might shake out a few bugs. If you need to report a bug on this, please post or email me a dump of the theme_settings variable (and any other variables you feel might be relevant) from your original database.

Note that the upgrade function copies the URLs from the theme settings straight across into the menu table. This might result in some paths being stored in the menu table that wouldn't normally be there. I have made a modest attempt to resolve any url_aliases, but... who knows what users might have typed into that text box!

If anyone can offer any suggestions as to further processing that should be done to these user-entered paths before inserting them into the menu table I'm all ears.

...Richard.

When I first installed the patch I did get a foreach() error on menu.inc briefly, but then it went away. I'm not entirely sure why, but I suspect it was because I didn't run any upgrade script.

Looks better now. As I see it, this addresses the maintenance of links aspect. The display of links, I think, could still be improved, but for time reasons we may want to save that for post-4.7. This does get links out of the theme system, where they don't belong, and the single level of section-specific links is a good start.

I'm not entirely wild about the interface of admin/settings/menu right now, but I don't have a better suggestion at the moment. :-) I still believe it should be renamed to admin/settings/links to avoid confusion with admin/menu. Perhaps it could have its own menu-editing interface for the single Primary Links menu, similar to how forum.module has its own interface for what comes down to a taxonomy vocabulary? Or am I making this too complicated? :-)

While I don't think these ideas will make it into 4.7, the direction I'd like to see this move eventually is to allow for an arbitrary depth site menus. Those would allow each level to be shown at an arbitrary location on the site, either nesting or as separate block or block-like entities. I suppose I'm describing the interface of a site I'm working on at work, sadly not using Drupal. It's an extra level of flexibility that would make Drupal even more capable of producting arbitrary designs and layouts. But I digress... :-)

Functionality-wise, I think this is sufficient for 4.7. I'd like to see admin/settings/menu renamed, for clarity. If someone has a brilliant idea on how to improve the interface there, I'd like to hear that, too.

I did run into one issue, although I don't know if it's related to this patch or a pre-existing bug. If I put a multi-level menu into a block and displayed it, it would never expand regardless of what path I was at. It didn't matter if that menu was selected for primary links. All of the entries in it were duplicates of the Navigation menu, though, since I just rebuilt my CVS site and it has only 2 nodes in it so far. :-) Disabling the display of the Navigation menu didn't have any effect. As I said, I don't know if that's related to this patch or not.

Hi Crell,

Thanks for taking another look at this patch.

In this version I have tightened up the error checking which has almost certainly fixed any possible instances where foreach was being passed a non-array. I have also rearranged the parameter order for a few functions to make it easier for themers to call them.

With this patch I have deliberately avoided adding too many bells and whistles. And the post-patch links menus should match the pre-patch versions to the pixel. This was effectively a requirement for the patch from Steven and Dries.

That said, the patch does provide the ability to produce each of the primary and secondary links in a single-level unordered list. So if a theme wants a nicer layout they can implement the required CSS and ask for a list. None of the current themes use that feature though.

It can also generate a ul for any specified level of the menu tree. You just need to call theme('menu_links_list', level) and you will get an ul containing the relevant submenu of the "Primary links" menu. I won't pretend this would be of any use if you wanted a full DHTML-driven menu system, but it's enough for all I'll ever need :)

I chose admin/settings/menu as the name of the admin menu item in part to help reinforce the fact that primary/secondary links are now an integral part of the menu system, but I'm happy to change that if it's confusing. But this raises another question... should these links still be called primary/secondary links or is there a better name to give them now? "Primary menu" perhaps?

The theming is really bad in the latest incarnation... the whole point of theming is decided on how to present something, so providing a separate 'list' and a 'horizontal' themable function is silly. This is the kind of thing that should be decided in the theme itself.

Instead, we should separate by logical function: theme local tasks separate from primary/secondary navigation links. They are different sections of the page which a themer will almost always want to theme separately. CSS-only theming is just one way (and a useful and quick one at that), but it should not be the only way.

For the themes that current have link | link | link, we override the themable function for primary/secondary links in template.php with one that does the old implode(' | ', ...) instead of outputting the <ul> and <li> list.

Richard, can you look into the theme issue posted by Steven? I'd really like to include this in 4.7!

I wholeheartedly agree with Steven about the theming approach.

As for the rest of the patch, it seems pretty clean in general. I do have reservations about the change to the semantics of what a "trail" is. In the current menu scheme, a given path has one and only one trail. When a path appears multiple places in the menu structure, we still have a unique trail to use for the purpose of presenting the breadcrumb and for expanding the appropriate parts of the navigation block. If I read this code right, now we have up to three different trails for a single menu item (main, primary, and secondary), but not an arbitrary number. I think this will work, but I worry about the complexity added to the already confusing code. I don't have an alternate suggestion at the moment, though.

Here's a new patch. It no longer tries to theme anything. It just creates the array of links and lets the theme manage the display. This is how the old primary links worked and it's the way this patch should have been written from the start. I wish I had started this patch from scratch instead of continuing development on the earlier version!

@Steven...

I can't see why we would implement lists as the default method of output and then override it with the implode version in every theme. So I have left the old theme_links function alone (apart from some sanity checking) and themers can override theme_links if they want something different.

There is already a documentation page for converting links into tabs. I added a comment to it containing some sample code for the override: http://drupal.org/node/31704

The only drawback of this approach is the ugly and potentially expensive stristr call required to assign a class to the active

I encourage you todo:

-  $primary_links = theme('links', theme_get_setting('primary_links'));
-  $secondary_links = theme('links', theme_get_setting('secondary_links'));
+  $primary_links = theme('primary_links', menu_primary_links());
+  $secondary_links = theme('secondary_links', menu_secondary_links());

function theme_primary_links($links) {
  if (is_array($links)) {
    return theme('links', $links);
  }
}

This will make it easier to theme the (primary|secondary)_links() apart from the node-links and comment links and each other.

Furthermore, I like your patch...

The reason we want the default to be an ordered list is because this is more semantic. Furthermore, many contributed themes use lists. The old-style implode is merely a concession to the core themes.

Here's a new version of this patch. Created with diff because cvs diff wouldn't pick up the new files.

I created a theme_menu_links function which generates a themed list for the links. So this patch can once again generate the drupal.org navigation. And it's separate from theme_links as suggested.

There seems to me to be no need to have both theme_primary and theme_secondary functions, as surely if one is being shown as a list the other will too. If this really is required by a theme, it can call menu_primary_links to obtain the array and do whatever it likes with it.

theme_menu_links is overridden in the core themes so they still look the way they used to.

I also added some trickiness to the index of the array generated by menu_primary_links so that theme_menu_links can tell when an li needs to be assigned class="active" even when the link isn't. While still maintaining backwards compatability with contrib themes.

And the ul now has class="links-n" where n is the number of levels deep we are in the menu. Generally this class wouldn't be required as the class or id of the enclosing div would be used as the selector. But it is available if a theme wants to provide CSS for a specific level. Also, the old class="primary" was clashing with the local tasks styles in drupal.css and confusing me no end.

There are two outstanding issues:

Should admin/settings/menu be renamed? What to?

The upgrade function needs to be thoroughly tested.

i have no problem with 'admin/settings/menu'. if pressed, i would suggest 'site navigation'

This patch is for drupal.css. it is real minimal style, the rest should be handled in the themes themeselves;
All this does, is make primary menus render horizontal and space them out a bit.

well, that is a *very* small patch :p trying again.

I'm not sure it's worth bloating drupal.css even more. Especially with styles that are always going to be overridden by a theme.

Perhaps it would be better to include a link in the source (or from admin/settings/menu) to a documentation page where a fully-worked example of using the list for links is shown. This patch makes obsolete http://drupal.org/node/31704 so perhaps that page should be updated with a more rigorous example of how to use the new code.

I have some CSS for bluemarine that works fairly nicely, which I'll attach.

The instructions would be an expansion of:

1. delete the phptemplate_menu_links function from template.inc
2. delete the existing #primary and #secondary lines from style.css
2. add the following CSS to styles.css in place of the above
3. tweak to your heart's content :)

Though I really agree with your ideas about CSS. I think we should focus primarily on getting this patch in. If that means that we have to do it the usual way (ie cluttering drupal.css) then so be it.
After 4.7 we will hget a 'campaign' going for a cleaner drupal.css. But let us please not hold up this patch by getting that CSS discussion into this issue :)

I don't think there's any need to add anything to drupal.css to get this patch into 4.7.

The patch is cunningly crafted so that it's completely back-compatible with 4.6.3. $primary_links contains an array of links, just like it used to. Existing themes will either process this array with a foreach or else pass the array to theme_links. Neither of these use cases require any additional css in drupal.css.

If themes want to display a horizontal menu, even if we provide something in core, they are always going to need to supply a lot of their own css . I don't see the point in making them override whatever we provide.

And... there's no guarantee that every theme is going to want to display this list horizontally. Personally I dislike horizontal navigation because it doesn't scale.

The main problem, however, is that I can't get this patch to work. When I click a node's edit link, the page appears to be loading for minutes, and then it timouts. While the page is loading, Apache takes almost all CPU so I think trigger some kind of race/loop. I'm not sure it's related to this patch but I first experienced it after I applied it and ran the upgrade script.

Note that I accidentically ran the upgrade script twice. I noticed there are multiple primary and secondary menus in my database. People are not supposed to run the update script twice, but I figured I'd point this out nonetheless. I cleared the menu table but that does not seem to fix the load problems.

Maybe I should try again with a new database; chances are something is corrupt now. :/

I tried this with a clean copy of HEAD (empty database). This time, the edit page seems to load, however, the available menu items are a bit weird. Maybe a glitch in the update path? See screenshot.

With a fresh install, the edit node form let me assign a node to the 'primary links'. However, it didn't seem to work; no primary navigation links appeared even though I saved the node multiple times. It took me 5 minutes (!) to figure out I had to enable the primary links. I figured it out because I looked at the patch.

1. Why do I have to enable this feature, even though it looks as if it can be used?
2. When I saw the menu settings screen I was very confused. See annotated screenshot. What am I supposed to be doing here? Assign 'primary links' to 'primary links'? Assign 'secondary links' to 'primary links'?

Creating primary links should be a no-brainer. It's one of the first thing new Drupal users will want to do, and it should be extremly easy to do. Assign the node to the 'primary links' and *poof* have it appear on the screen.

At this point, the patch paves the path to a better menu system (code/design-wise) yet the user experience requires more work. Something is very flawed with the 'Primary links settings' and their default configuration.

Nonetheless, this is one of my favorite patches.

(Also, the 'admin/menu' link seems to be gone; it's no longer part of the navigation block?)

Dries,

Most of what yyou point out is due to the (very complex) upgrade path. Ifthe upgrade was not ran, you wuold have seen an option in the select for every menu block we have. Whicn by default is -one-, called 'navigation'.

I really like the patch as it is. But I fear that we are getting a bit lost in the upgrade, atm. I still think we should just ignore the pgrade, esp if it ds so much comlpexity.
Really if people cannot copy-paste five to ten links, then they should use blogger.com :) No serious, when we went for phptemplate we ignored the links too. (people merging from foo to phptemlpate lost them too). so why should we care now. Esp becuase it is so hard to do right.

Ber: what are you talking about? How do my problems relate to the upgrade path? Please explain.

The smart answer would be that there are lots of things in Drupal that should be easy but aren't. But I'll try to be a bit more useful than that.

I'll have a think about the usability issues and double-check the default settings over the next few days.

The admin/menu item would be missing in a fresh install because the menu module is disabled by default. That does limit the usefulness of this patch, because by default there is no way to add items to the primary links. This is a regression from the old system where you just had to enter the links in the theme settings page.

I think the best solution would be to enable menu for new installs.

Otherwise I guess I need to check to see if menu is disabled and if so, return null (instead of the link array).

But then, on upgrading from 4.6.3 to 4.7, if the menu module is disabled even though all the old links have been imported into the menus the links would not show up. This is bad.

I'm not sure what to do about this. It seems to be beyond the scope of this patch.

I think it is reasonable to enable menu.module for upgraders and new installs alike. maybe even make it a required module? see system.module for that array.

+1 to enable menu module by default.

First up, I must apologize for the quality of my QA for this patch. I haven't put the time required to do proper testing into this simply because I don't have any spare time to put in. I shouldn't even be working on this patch!

The good news is that if this patch worked the way I intended it would meet all Dries' requirements. Most of the criticisms were a result of bugs, now duly squashed.

Re: The update script.

I agree with Steven that it is essential to copy old links into the new system. And I think the update script is nearly there.

Running the update script twice is not a good thing to do, but it should create no unrecoverable corruption. It will create a second Primary links menu and turn off the primary links feature. But the imported links will all still be present in the first menu. I could add a test to see if a menu called t("Primary links") already exists and bail out of the update if so. Would this test be correct? Is is worth while? What if the user has an existing "Primary links" menu?

I have done some more work on the update script. I think the installation I was testing with was strangely corrupt. Many of the theme setting variables had been stored as variables and I don't think this is supposed to be the case. It certainly isn't with a clean install. So now I think the update script should be better at pulling the links out of the old settings and cleaning up all the obsolete variables. It still needs to be thoroughly tested though.

Re: The infinite loop. I don't see how this could happen. The foreach loops in this patch all run through $menu and it doesn't have loops. I'll blame another patch Dries had installed for this until someone else reports the same problem. If this happens a database dump of menu and variable would be useful.

Re: the screen grab showing "--edit primary links" as an option. I've done some work on this and it should be fixed. The "edit primary links" hint is no longer created in the menu, it's generated as a special case if the primary links menu is enabled and empty.

Re: Having to enable Primary links. I initially blamed the menu.module being disabled for this problem, but that can't have been the case. Dries must have already enabled menu.module in order to be able to assign a page to the primary links menu in the edit node form. The error was in fact in database.mysql+pgsql. Every single line I added had an error of some sort. After fixing these problems, for new installations primary links is active with the links being pulled from the new 'Primary links' menu and secondary links being level 2 of the same menu. The Primary links menu is initially empty but as soon as an item is added it shows up in the nav. I think that using this feature in this way is pretty much a no-brainer.

Re: Enabling menu module. I have enabled the menu module in both the upgrade and install scripts. And also added some checking to make sure it is enabled before building the links. I don't see the need to make menu a required module... let the admin decide.

Re: The big one... Settings usabilty.

In this patch I have tried to improve the layout and wording of the help text. My expectation is that if a user comes to this page and they are confused that the help text will show them the Way. The new label above the drop-downs makes things clearer. I think it is logical to choose the same menu from both drop-downs when wanting the contextual secondary links (because both primary and secondary links are in the same menu). But there's a bit of extra help text there too, just in case.

Settings is a little complex because of the number of ways the primary links feature can be deployed:
- Both primary and secondary links disabled
- Primary links only enabled
- Secondary links only enabled
- Secondary links as level 2 of Primary links
- Secondary links from their own menu.

The current settings allow all those configurations to be specified with only two drop-downs which I think is pretty clever. I can't see how adding extra radio buttons or checkboxes would make this interface any clearer.

Picking on the name "Navigation" isn't fair. I didn't come up with that name and I can't remove it from the list because it's a valid setting. If the admin doesn't know what the Navigation menu is they probably have bigger problems than this patch can help them with.

I see in the Admin Survey results that there is perhaps a need to split the navigation menu into an admin block and the rest of the navigation items. This is already what I do on my sites. If this change made it into core the logical change in this feature would be to use the new non-admin navigation menu as the default for primary links. Being able to remove the 'Primary links' menu would simplify the settings greatly. Although there would still need to be the two drop-downs to retain the required configurability.

Committed to HEAD! Thanks. We can continue to refine this patch, and its themeability now it hit core.

The new files in this patch weren't committed:

themes/bluemarine/menu_links.tpl.php
themes/bluemarine/template.php
themes/pushbutton/menu_links.tpl.php
themes/pushbutton/template.php

What's the reason you couldn't extend themes/engines/phptemplate/phptemplate.engine?

Hmm. I was just following Steven's directions and didn't think about it too much.

There seems to be little point overriding theme_menu_links just to replicate the functionality of theme_links. Why not just pass the links array to theme_links?

The only reason I can see to use theme_menu_links is so people using one of the core templates as a starting point for their own template know of the existence of this function. Not really a good enough reason to have three copies of this code in core.

Here's a patch against the current head that calls theme_links to render the links.

would be nice if this cleans up the database from old primary/secondary links settings in db -> variable -> theme etc.

The update script is supposed to clean up the old links variables and theme settings.

If you find that this is not happening, please email me a database dump of the offending items.
drupal.org@juggernaut.com.au.

...Richard.

hehe i just purged the theme settings in the database. thanks anyway!

This is a great patch/update to drupal. It works like a charm! Keep it up!

Committed.

See http://drupal.org/node/36324. The upgrade path has a bug.

Why is it that update_sql doesn't support printf-style arguments?
Someone should post a feature request about that :)

Here's a quick fix that calls db_escape_string for each string argument.

Dries, could you please commit this patch even though nobody has reviewed it? This addresses the update failure when an old link contains a '.

The patch is trivial... it just adds a call to db_escape_string for each string argument to update_sql.

Version 17 of the patch should apply cleanly against the latest updates.inc and also fixes some whitespace nastiness in v.16.

#87 submitted by Richard Archer on November 3, 2005 - 21:54

The new files in this patch weren't committed:

themes/bluemarine/menu_links.tpl.php
themes/bluemarine/template.php
themes/pushbutton/menu_links.tpl.php
themes/pushbutton/template.php

menu_links.tpl.php is still missing. i guess its there you style the output of the links? shouldnt they be

separated instead of | by default?

menu_links is not anymore used
see #89 (this was commited)

Those files are not necessary. I made a last-minute change so the core themes call theme_links instead of theme_menu_links. This keeps the old link|link style without needing to override anything.

If a theme wants to display links as a list it should call theme_menu_links instead of implementing its own foreach. At some stage when I have more time (after 14 November) I will submit patches to all contrib themes (ack! what am I saying?!) which have their own foreach.

Committed. Thanks Richard.

This change needs to be documented in the theme update guide at http://drupal.org/node/25297.