Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#57 | renderable-menu-links-283723-57.patch | 12.53 KB | pwolanin |
#55 | renderable-menu-links-283723-55.patch | 12.53 KB | pwolanin |
#44 | renderable-menu-links-283723-43.patch | 11.19 KB | pwolanin |
#42 | renderable-menu-links-283723-42.patch | 10.77 KB | pwolanin |
#41 | renderable-menu-links-283723-41.patch | 10.57 KB | pwolanin |
Comments
Comment #1
cburschkaI 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.
Comment #2
aether CreditAttribution: aether commentedArancaytar:
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.
Comment #3
dave.mecha CreditAttribution: dave.mecha commentedYes, 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.
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
Comment #4
PasqualleComment #5
pwolanin CreditAttribution: pwolanin commented@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.:
Comment #6
eddified CreditAttribution: eddified commentedWhat 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.
Comment #7
eddified CreditAttribution: eddified commentedComment #8
eddified CreditAttribution: eddified commented---was a duplicate comment---
Comment #9
lilou CreditAttribution: lilou commentedComment #10
aether CreditAttribution: aether commented@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.
Comment #11
catch#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.
Comment #12
pwolanin CreditAttribution: pwolanin commentedchx 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.
Comment #13
cbovard CreditAttribution: cbovard commentedHello 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
Comment #14
Freso CreditAttribution: Freso commentedSubscribing!
Comment #15
ckngMarking #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.
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.
Comment #16
sunI 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.
Comment #17
pwolanin CreditAttribution: pwolanin commented@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.
Comment #18
pwolanin CreditAttribution: pwolanin commentedHere's a first pass - seems at least to be working for menu blocks.
Comment #20
pwolanin CreditAttribution: pwolanin commentedonly 30 - not bad for a 1st try
Comment #21
pwolanin CreditAttribution: pwolanin commentedfixed rendering in book module
Comment #23
pwolanin CreditAttribution: pwolanin commentedoops - had broken all tabs. Does generating the link really need a theme function that wraps l()?
Comment #25
pwolanin CreditAttribution: pwolanin commentedoops git prefixes in the patch.
Comment #27
sunThe data below should be added as proper child of $element, so drupal_render() performs the rendering.
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().
Preceding space in class string.
I'm on crack. Are you, too?
Comment #28
pwolanin CreditAttribution: pwolanin commented@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
Comment #29
pwolanin CreditAttribution: pwolanin commentedodd 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.
Comment #30
pwolanin CreditAttribution: pwolanin commentedah, missed removing
theme('menu_item_link', $item);
a couple more places.Comment #31
pwolanin CreditAttribution: pwolanin commentedAdd class to the a tag also
Comment #32
pwolanin CreditAttribution: pwolanin commentedper 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.
Comment #33
sunKilled 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.
Comment #34
sunok, 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:
Those elements are not alterable at all. By introducing #type = 'link', it we could use (only wrapped into multi-line for clarity)
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.
Comment #35
pwolanin CreditAttribution: pwolanin commented@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().
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commentedshould be just render() instead of drupal_render(). thats what we do in template land.
blocks can and should return renderable arrays so remove the drupal_render() here.
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.
Comment #37
pwolanin CreditAttribution: pwolanin commented@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()
Comment #38
sun.core CreditAttribution: sun.core commentedComment #39
pwolanin CreditAttribution: pwolanin commentedimprove title and address Moshe's comments above.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedtypically, 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.
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']
This looks a bit awkward. Could we add some Doxygen explaining what we are doing here?
Comment #41
pwolanin CreditAttribution: pwolanin commentedchanged 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.
Comment #42
pwolanin CreditAttribution: pwolanin commentedminor tweak to use mlid as the array key
Comment #43
moshe weitzman CreditAttribution: moshe weitzman commentedI went through this a few times with pwolanin in irc and it is rtbc now.
Comment #44
pwolanin CreditAttribution: pwolanin commentedfinal little doxygen tweak in the book template file discussed in IRC w/ moshe.
Comment #45
pwolanin CreditAttribution: pwolanin commentedthis is really a feature, since it allows menus tree to be altered in hook_page_alter.
Comment #46
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #47
pwolanin CreditAttribution: pwolanin commentedComment #48
Dries CreditAttribution: Dries commentedIs 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?
It is not immediately clear why these values might already be set. Is 'below' like 'children'?
Comment #49
pwolanin CreditAttribution: pwolanin commentedWe 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.
Comment #50
Dries CreditAttribution: Dries commentedLet's add a TODO to document this snafu.
Comment #51
sunTagging.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedSubscribing. 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().
Comment #53
Dries CreditAttribution: Dries commentedStill waiting for a TODO item.
Comment #54
pwolanin CreditAttribution: pwolanin commentedwill have it soon
Comment #55
pwolanin CreditAttribution: pwolanin commentedLooking 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.
Comment #56
sunThe 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.
Here we could use $link instead of $output. :)
This review is powered by Dreditor.
Comment #57
pwolanin CreditAttribution: pwolanin commentedI used $output instead of $link, since all other uses of $link (or similar) are for an array.
Comment #58
sunComment #59
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD! Thanks pwolanin and sun.
Comment #60
mgiffordLooking at the final patch it looks like theme_menu_item_link() just got nixed in favour of template_preprocess_menu_tree():
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.
Comment #61
pwolanin CreditAttribution: pwolanin commentedYou'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
Comment #62
pwolanin CreditAttribution: pwolanin commentedhttp://drupal.org/update/theme/6/7#menu_tree
Comment #63
mgiffordThanks!
Comment #64
julrich CreditAttribution: julrich commentedHi,
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
Comment #65
pwolanin CreditAttribution: pwolanin commentedthis is a new feature - new features are only for D7, or soon for only D8
Comment #67
c960657 CreditAttribution: c960657 commentedFYI: There is a suggestion to rename
#href
to#path
in #656614: Rename #href to #path.