Make menu_tree_output() return renderable output
aether - July 17, 2008 - 05:48
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | pwolanin |
| Status: | closed |
| Issue tags: | API change, API clean-up |
Description
Seems like this would be a common request but I could not find an existing issue. This would make adding additional classes containing the item's 'mlid' or others that would be useful for certain themeing requirements.
#1
I was facing a similar problem with DHTML Menu. I solved it by hooking into the themeable parts of that function instead - menu_item_link and menu_item. That goes a long way (although understanding the order of recursion made my head spin). Take a look at the 6.x-3.x code; perhaps you can do what you need in a similar way.
#2
Arancaytar:
Thanks for your suggestion as it got me headed in the right direction / thinking in a different way. In my case I wanted the mlid to appear as a class on the link's list item so I ended up passing the full (modified) $link array from theme_menu_item_link() to theme_menu_item() and adding the class there.
Still it would be nice if menu_tree_output() were themeable. I'm curious if there is a reason why it is not.
#3
Yes, this would be a great feature.
I was searching for a simple way to change the propagation through the menu to change the output variables. Each menu is available as a variable in the page-template but i wanted to split the output whithout having two menus.
In my case, the first menu level schould be another variable then the others.
$navi_1 =>
<div><a>Level One - Link One</a> | <a>Level One - Link Two</a> | <a>Level One - Link Three</a></div>
$navi_2 =>
<ul><li><a>Level Two - Link One</a></li><li><a>Level Two - Link Two</a></li></ul>
For this a first step is to have easy control over the menu propagation. The next step would be an easy way to manipulate the menu variables available in the template. Some kind of preprocessor for the menu.
(I'm new to Drupal so please forgive me if there is an easy way to realize this ;) )
dave
#4
#5
@dave.mecha - you are talking about something separate.
@all - there is ongoing debate as to whether it's appropriate for theme fucntions to even to a foreach loop - menu tree output is a recursive function even, so I really don't see it as an appropriate candidate for being a theme function itself. However, it might be reasonable to have a theme function that calls menu_tree_output so that theme function could be a place to hook in to totally replace the output.
e.g.:
<?php
function menu_tree($menu_name = 'navigation') {
static $menu_output = array();
if (!isset($menu_output[$menu_name])) {
$tree = menu_tree_page_data($menu_name);
$menu_output[$menu_name] = theme('menu_tree', $tree);
}
return $menu_output[$menu_name];
}
function theme_menu_tree($tree) {
return menu_tree_output($tree);
}
?>
#6
What I need to do is to allow each level of the menu to have a different class.
So, the links that have no parents in the menu would have a class of "level-0". All children of the "level-0" links have a class of "level-1". All children of "level-1" links have a class of "level-2".... and so on and so forth. This is necessary because the design I was told to implement has different styles for each menu level. So what I ended up doing was adding a 2nd parameter called $level to function menu_tree_output() that is recursively incremented each time menu_tree_output is called on a child. See the attached patch file for full details. (yes, in the patch I know I did it the "wrong" way because I edited the core functions theme_menu_item() and theme_menu_tree() .... and the "right" way would have to just create themed functions for those ... but the patch is for illustrative purposes only)
I'm actually not sure if I did it the "wrong" way by editing menu_tree_output().... Is there a better way to do this? Perhaps by writing a tiny module? I'm new to drupal so please advise if there is a better way.
#7
#8
---was a duplicate comment---
#9
#10
@pwolanin - I understand the concern regarding complexity in theme functions. Your proposed solution here sounds like a reasonable way to get around this and still provide flexibility in themeing menu output.
#11
#7 deals with a different issue to the title of the issue, and also has tabs in. Seems like this one needs some work either way.
#12
chx and I are discussing a little. Better yet would be to make an item that could be rendered using drupal_render()
We'd have to make at least one minor change - unset $item['below'] rather than leaving it as something that evals to FALSE
marking: http://drupal.org/node/369300 as a duplicate.
#13
Hello All,
Basically what I needed is a way to get into and around each menu item in between the theme functions. A way to count the menu items (if needed) and put my own html (if needed). Leaving this the way it is seems to be constrictive in styling out a final theme in CSS.
chris
#14
Subscribing!
#15
Marking #387930: Make menu_tree() more theme-friendly as duplicate.
--
When trying to output a highly customized menu tree, the existing menu_tree() is not very theme-friendly. The convention always employed is to override calls to menu_tree() to a custom one and its children. Just notice that admin_menu is doing the same.
e.g.
<?php
function _my_menu_tree($menu_name = 'navigation') {
...
$menu_output[$menu_name] = _my_menu_tree_output($tree);
...
}
function _my_menu_tree_output($tree) {
...
$link = _my_top_menu_item_link($data['link']); // theme for 1st level menu
...
$output .= theme('menu_item', ..., _my_submenu_tree_output($data['below']), ...);
...
}
function _my_submenu_tree_output($tree) {
...
$link = _my_sub_menu_item_link($data['link']); // theme for sublevel menu
...
$output .= theme('menu_item', ..., _my_submenu_tree_output($data['below']), ...);
...
}
?>
which in turn need to duplicate the menu_tree_output() and often menu_item_link() as a custom function.
Another shortcoming is they are lacking the knowledge of the menu hierarchy (currently only come across menu_item_link may need that info). Often during themeing, we'll theme main menu differently especially a simple drop down menu, not to mention other more complex menu display (showing menu with inline description, themeing individual submenu differently, multiple columns etc).
It gets complicated during version upgrade as those codes need to be recoded if there are changes and could pose a security leak. A more theme-friendly menu_tree is needed in order to make drupal looks nicer.
#16
I have similar code in admin_menu 3.x, though of course, the menu tree data is parsed into a drupal_render-style array there afterwards only.
The main issue that needed to be tackled was the array structure to use - i.e. whether there should be a single #type/#theme menu_link that takes a specially crafted array structure, which contains both the information for the LI and the A, and wraps all sub-elements into a new UL. Or whether UL, LI, and A should be kept separate in the array structure, so #attributes and other properties could be set separately. Administration menu's theme_admin_menu_links() implements the former.
This makes the entire menu tree alterable.
<?php
/**
* Render a list of links.
*
* @param $elements
* A structured drupal_render()-array of links using the following keys:
* - '#attributes': Optional array of attributes for the list item, processed
* via drupal_attributes().
* Note that we use an array for 'class'.
* - '#title': Title of the link, passed to l().
* - '#href': Optional path of the link, passed to l(). When omitted, the
* element's '#title' is rendered without link.
* - '#description': Optional alternative text for the link, passed to l().
* - '#options': Optional alternative text for the link, passed to l().
*
* The array key of each child element itself is passed as path for l().
* @param $depth
* Current recursion level; internal use only.
*/
function theme_admin_menu_links(&$elements, $depth = 0) {
}
// Some example data:
$links['icon'] = array(
'#title' => theme('admin_menu_icon'),
'#attributes' => array('class' => array('admin-menu-icon')),
'#href' => '<front>',
'#options' => array(
'html' => TRUE,
),
'#weight' => 50,
'#access' => user_access('administer site configuration'),
);
?>
#17
@sun - well, I'd think the goal should be to keep the same # of theme() calls.
however, we are rather short on time for this, so I might also lean towards the simple solution above for D7 unless someone wants to start rolling the patch.
#18
Here's a first pass - seems at least to be working for menu blocks.
#19
The last submitted patch failed testing.
#20
only 30 - not bad for a 1st try
#21
fixed rendering in book module
#22
The last submitted patch failed testing.
#23
oops - had broken all tabs. Does generating the link really need a theme function that wraps l()?
#24
The last submitted patch failed testing.
#25
oops git prefixes in the patch.
#26
The last submitted patch failed testing.
#27
+++ includes/menu.inc@@ -824,23 +824,29 @@ function menu_tree_output($tree) {
+ $element['#below'] = $data['below'] ? menu_tree_output($data['below']) : $data['below'];
@@ -1284,29 +1281,29 @@ function theme_menu_tree($tree) {
+ function theme_menu_link(array $element) {
...
+ if ($element['#below']) {
+ $element['#attributes']['class'][] = 'expanded';
+ $sub_menu = drupal_render($element['#below']);
}
The data below should be added as proper child of $element, so drupal_render() performs the rendering.
+++ includes/menu.inc@@ -1284,29 +1281,29 @@ function theme_menu_tree($tree) {
+ elseif ($element['#has_children']) {
+ $element['#attributes']['class'][] = 'collapsed';
}
...
+ else {
+ $element['#attributes']['class'][] = 'leaf';
+ }
I wonder whether all of these classes shouldn't be set in the builder function already. (At least the 'active-trail' class should also be set on the anchor, which is another Good Thing™ that the Menu Trails module does).
'#wrapper_attributes' might be used as key (custom property).
By then moving the list item wrapper into a #theme_wrapper function, theme_menu_link() would turn into theme_link(), which either duplicates or invokes l().
+++ includes/menu.inc@@ -1284,29 +1281,29 @@ function theme_menu_tree($tree) {
+ $element['#attributes']['class'][] = ' active-trail';
Preceding space in class string.
I'm on crack. Are you, too?
#28
@sun - if we set #theme, then no child elements are rendered. - also, we want to have the child elements inside the
<li>so it won't work to have them rendered and added on after.Also, why do we want the class on the anchor? you shoudl be able
#29
odd that that node module test still fails - maybe we need to make sure localized options is an array?
Moved some code around here as sun suggests.
#30
ah, missed removing
theme('menu_item_link', $item);a couple more places.#31
Add class to the a tag also
#32
per sun, all class attributes are expected to be arrays in D7 (is there a note in the upgrade docs?), so we can insure that in _menu_link_translate() and simply the other code.
#33
Killed some nitpicks. RTBC, although I'll try to come up with another approach I mentioned in IRC. Either this is committed before I do so or not.
#34
ok, this is what I had in mind:
Next to turning the output of menu_tree_output() into a renderable array structure, we also have countless of forms + stuff elsewhere that uses hardcoded HTML output like so:
<?php$form[$id]['configure'] = array('#markup' => l(t('configure'), 'admin/settings/formats/' . $id));
$form[$id]['delete'] = array('#markup' => $default ? '' : l(t('delete'), 'admin/settings/formats/delete/' . $id));
?>
Those elements are not alterable at all. By introducing #type = 'link', it we could use (only wrapped into multi-line for clarity)
<?php$form[$id]['configure'] = array(
'#type' => 'link',
'#title' => t('configure'),
'#href' => 'admin/settings/formats/' . $id,
);
if ($default) {
$form[$id]['delete'] = array(
'#type' => 'link',
'#title' => t('delete'),
'#href' => 'admin/settings/formats/delete/' . $id,
);
}
?>
Hence, if you need to alter a link title or href or its options in a form or some renderable output, you can just alter it. Currently, you need to fork the full invocation of l() and entirely replace it.
Unless there is an agreement on this approach, #33 is still RTBC and does not need work.
#35
@sun - I think your proposal needs to be a separate issue, since it would have effects across core. Let's stick with #33 for now.
It also has potential performance implications, since every such 'link' would incur an extra theme() call on top of the l().
#36
+++ modules/book/book.module 28 Aug 2009 11:50:21 -0000@@ -703,7 +704,7 @@ function book_children($book_link) {
+ return $children ? drupal_render(menu_tree_output($children)) : '';
should be just render() instead of drupal_render(). thats what we do in template land.
+++ includes/menu.inc 28 Aug 2009 12:00:48 -0000@@ -1253,60 +1283,34 @@ function _menu_tree_data(&$links, $paren
+ $output = '<ul class="menu">';
+ foreach(element_children($tree) as $index) {
blocks can and should return renderable arrays so remove the drupal_render() here.
+++ includes/menu.inc 28 Aug 2009 12:00:48 -0000@@ -1253,60 +1283,34 @@ function _menu_tree_data(&$links, $paren
function theme_menu_tree($tree) {
- return '<ul class="menu">' . $tree . '</ul>';
+ $output = '<ul class="menu">';
+ foreach(element_children($tree) as $index) {
+ $output .= drupal_render($tree[$index]);
+ }
+ return $output . '</ul>';
it looks to me like theme_menu_tree should be a theme_wrapper on an element not a #theme. that would be more natural, since it would not have to call drupal_render() itself. a theme_wrapper can find rendered HTML in $element['#children']
This review is powered by Dreditor.
#37
@moshe - I was having some issues getting theme_wrappers to work - will try again.
I'm confused by your 2nd comment. There is no use of drupal_render in function _menu_tree_data()
#38
#39
improve title and address Moshe's comments above.
#40
+++ includes/menu.inc@@ -824,23 +834,43 @@ function menu_tree_output($tree) {
+ return $content;
typically, we use $build as a variable name when building up a renderable array. I would do that instead of $content. I'm not yet clear on why we ned both $content and $element here.
+++ includes/menu.inc@@ -1253,20 +1283,16 @@ function _menu_tree_data(&$links, $parents, $depth) {
+function template_preprocess_menu_tree(&$elements) {
+ $elements['tree'] = $elements['tree']['#children'];
}
It is a nit, but I find this a bit awkward. I would prefer using the variable name 'variables' in a preprocess and creating a new var called 'content'. So,
$variables['content'] = $variables['tree']['#children']
+++ modules/book/book.module@@ -890,6 +891,23 @@ function _book_link_defaults($nid) {
+ // Remove all non-renderable elements.
+ $elements = $variables['book_menus'];
+ $variables['book_menus'] = array();
+ foreach (element_children($elements) as $index) {
+ $variables['book_menus'][$index] = $elements[$index];
+ }
This looks a bit awkward. Could we add some Doxygen explaining what we are doing here?
#41
changed to $build, added comments.
Left $tree alone per discussion in IRC with moshe, since preprocess function are a little different in the behavior of the $variables for theme functions and templates.
#42
minor tweak to use mlid as the array key
#43
I went through this a few times with pwolanin in irc and it is rtbc now.
#44
final little doxygen tweak in the book template file discussed in IRC w/ moshe.
#45
this is really a feature, since it allows menus tree to be altered in hook_page_alter.
#46
tagging
#47
#48
+++ includes/menu.inc@@ -552,6 +552,11 @@ function _menu_check_access(&$item, $map) {
+ // All 'class' attributes are assumed to be an array, but links stored in the
+ // database may use an old string value.
+ if (isset($item['options']['attributes']['class']) && is_string($item['options']['attributes']['class'])) {
+ $item['localized_options']['attributes']['class'] = explode(' ', $item['options']['attributes']['class']);
+ }
Is that a temporary think? It sounds like there might be a missing TODO to clean up the database representation? Can we make this more clear in the comments?
+++ includes/menu.inc@@ -824,23 +834,47 @@ function menu_tree_output($tree) {
+ $element['#localized_options'] = !empty($data['localized_options']) ? $data['localized_options'] : array();
+ $element['#below'] = $data['below'] ? menu_tree_output($data['below']) : $data['below'];
It is not immediately clear why these values might already be set. Is 'below' like 'children'?
#49
We could potentially add validation or this sort of fix code to menu-link save so that we can remove it here, but we would potentially then need to add an update function to check existing DB values. We might add a @todo about some future options, but I'm not sure how else to make that robust.
'below' contains the visible children - i.e. if the link is expanded or in the active trail. We added that in D6, though webchick was confused by it too. Certainly we could do a later APi cleanup patch if you have a better name for it.
Note that there is a separate boolean has_children, since we display links that have non-expanded children differently from those that have no children.
#50
Let's add a TODO to document this snafu.
#51
Tagging.
#52
Subscribing. Since this is already RTBC, I haven't reviewed the implementation in detail, but huge +1 for the concept! I'm looking forward to this being committed soon, and us finding and fixing any remaining places where things are themed prior to drupal_render_page().
#53
Still waiting for a TODO item.
#54
will have it soon
#55
Looking at the code again - we should really put the call to l() inside the theme functions - otherwise it's not possible to add extra classes, etc, at the theme layer to the A tag.
Also added @todo.
Putting back to CNR for confirmation of these changes.
#56
+++ includes/menu.inc@@ -552,6 +552,14 @@ function _menu_check_access(&$item, $map) {
+ // @todo in order to remove this code we need to implement a database update
+ // including unserializing all existing link options and running this code on
+ // them, as well as adding validation to menu_link_save().
The todo should start with proper capitalization ("In") and if it spans more than one line, the following lines should be indented by 2 spaces to clarify where the @todo statement begins and ends.
+++ includes/menu.inc@@ -1276,56 +1309,47 @@ function theme_menu_tree($tree) {
+ $output = l($element['#title'], $element['#href'], $element['#localized_options']);
+ return '<li' . drupal_attributes($element['#attributes']) . '>' . $output . $sub_menu . "</li>\n";
Here we could use $link instead of $output. :)
This review is powered by Dreditor.
#57
I used $output instead of $link, since all other uses of $link (or similar) are for an array.
#58
#59
Committed to CVS HEAD! Thanks pwolanin and sun.
#60
Looking at the final patch it looks like theme_menu_item_link() just got nixed in favour of template_preprocess_menu_tree():
@@ -1254,20 +1291,16 @@ function _menu_tree_data(&$links, $parents, $depth) {
}
/**
- * Generate the HTML output for a single menu link.
+ * Preprocess the rendered tree for theme_menu_tree.
*
* @ingroup themeable
*/
-function theme_menu_item_link($link) {
- if (empty($link['localized_options'])) {
- $link['localized_options'] = array();
- }
-
- return l($link['title'], $link['href'], $link['localized_options']);
+function template_preprocess_menu_tree(&$variables) {
+ $variables['tree'] = $variables['tree']['#children'];
}
I don't see any documentation about why this decision was made here. I wasn't part of the IRC thread that is referred to in the comments above.
Just really not sure how we're supposed to pass along $active now as per:
http://drupal.org/node/521852#comment-2067364
Any thoughts here would be appreciated. Would be fine to re-roll the patch if we knew what the new logic is.
#61
You're not reading the patch right - that's just a diff artifact that makes it seem the one funciton replaced the other. This theme function was essentially replaced by http://api.drupal.org/api/function/theme_menu_link/7
Setting issue to CNW for needed documentation of theme function changes
#62
http://drupal.org/update/theme/6/7#menu_tree
#63
Thanks!
#64
Hi,
is this patch dependent upon any Drupal 7 features, or is it applicable to drupal 6, too?
Are there any plans to bring this to Drupal 6?
regards, Jonas
#65
this is a new feature - new features are only for D7, or soon for only D8
#66
Automatically closed -- issue fixed for 2 weeks with no activity.