theme_menu_tree() removes helpful data for theming custom menu trees

We have http://api.drupal.org/api/function/template_preprocess_menu_tree/7 and http://api.drupal.org/api/function/theme_menu_tree/7 now, so this should be relatively easy to do.

Basically means that $variables['tree'] either needs a parent element (why are we removing it in the first place and only pass #children?) to assign a proper #type with #attributes, or we just want a #prefix and #suffix.

Can you roll a patch?

This would be good (verbose subscribe...).

Hello, I am new to Drupal and I have been studying it online for about 2 months. I have to migrate a site to Drupal, but I have to keep its design which is based on tables. This means that a lot of theming is needed and I already checked some of the core code to get a better idea what is actually hapenning down there... It's nice code, well done to the contributors!

So I tried to use menu_tree.tpl.php and THEME_preprocess_menu_tree(&$vars, $hook) in my theme and I came across the function menu.inc:template_preprocess_menu_tree(). Basically what it does is to hide a lot of information from the next preprocessor functions ($variables['tree'] = $variables['tree']['#children']; ???). I commented this row out and I could use $vars['tree']['#block'] in my preprocess function as desired.

Is this hack dangerous? :)

Unless I'm missing something, I think we need to go back to menu_tree_output() to find something that we can use as a class (the menu_name).

Might be sensible to add an array of classes that can be imploded in template_preprocess_menu_tree().

Thoughts?

I'm currently in the same situation:
- I want to add classes to the menu HTML output depending on the region they're in
- so I've implemented "THEME_preprocess_menu_tree()" but noticed that I don't get any info at all
- apparently "template_preprocess_menu_tree()" is called before, and indeed removes a lot of useful information
- so now I have no way of checking in which region the current menu_tree resides and it seems I'm either forced to hack the core or work around this through HTML and CSS changes...

Hence my question; why is "template_preprocess_menu_tree()" called before "THEME_preprocess_menu_tree()"? Shouldn't the THEME_preprocess_menu_tree() function replace / override the template_preprocess_menu_tree() one?

Edit: I decided to modify the core function as such:

/**
 * Preprocesses the rendered tree for theme_menu_tree().
 */
function template_preprocess_menu_tree(&$variables) {
  $variables['original_tree'] = $variables['tree'];  // NEW LINE, don't want to lose all info...
  $variables['tree'] = $variables['tree']['#children'];
}

I dont know exactly if this now belongs to 7.x or 8.x and maybe I'm already way too late.

Anyways, I had the same problem and I think, I've found a solution for that without overriding core function.
Since I'm working on 7.x, I can only tell you how it's working there. It may work on 8.x as well.

The whole magic lies in the hook_theme_registry_alter() function.
Within this function we search for that 'ugly' preprocess function which is defined in menu.inc and remove it from the list.

That made, my own preprocess function works without interruption and I can do whatever i want.

In the following case it just takes the delta value and puts it in $variables which you afterwards can use in hook_menu_tree().

/**
 * Alter the theme registry information returned from hook_theme().
 *
 * @param $theme_registry
 *   The entire cache of theme registry information, post-processing.
 */
function YOURMODULE_theme_registry_alter(&$theme_registry) {
  foreach ($theme_registry['menu_tree']['preprocess functions'] as $key => $value) {
    if ($value == 'template_preprocess_menu_tree') {
      unset($theme_registry['menu_tree']['preprocess functions'][$key]);
    }
  }
}

/**
 * Preprocesses the rendered tree for theme_menu_tree().
 */
function YOURMODULE_preprocess_menu_tree(&$variables) {
  $variables['delta'] = (empty($variables['tree']['#block'])) ? '' : (' ' . $variables['tree']['#block']->delta);
  $variables['tree'] = $variables['tree']['#children'];
}

/**
 * Returns HTML for a wrapper for a menu sub-tree.
 *
 * @param $variables
 *   An associative array containing:
 *   - tree: An HTML string containing the tree's items.
 *
 * @see template_preprocess_menu_tree()
 * @ingroup themeable
 */
function YOURTHEME_menu_tree($variables) {
  return '<ul class="menu'.$variables['delta'].'">' . $variables['tree'] . '</ul>';
}

Cheers
~Nexo

Thanks Nexotap! I've stumbled upon this issue again after updating core...
Your solution seems to work like a charm!

Thanks!

Almost a year later and Drupal version 7.15... Problem just throttled a fair hour or two out of me... LoL

Writing in to thank Nexotap for the solution posted. Worked like a charm indeed!

Hope this gets fixed in a future version of D7... Does anyone know if it will?

EDIT:

Just realised that my minor adjustment to Nexotap's code might be handy for someone else. Also want to mention that hook_theme_registry_alter(&$theme_registry) can be used in a Theme's template.php file as well - a custom module is not needed for it.

The following is my version... I'm just pulling out the menu name and adding it as a class. My use case was that I'm using Panels Everywhere and have my Menus in several Panes... and wanted an simpler way to differentiate between them other than the classic ul class="menu". I should mention that I've stripped out all the excess div elements that Panels inputs by default.

// SAME AS NEXOTAP'S
/**
 * Alter the theme registry information returned from hook_theme().
 *
 * @param $theme_registry
 *   The entire cache of theme registry information, post-processing.
 *
 * @see
 *   Added this due to a fault with the current Drupal 7 implementation of template_preprocess_menu_tree()
 *   http://drupal.org/node/767404
 *
 */
function YOURTHEMENAME_theme_registry_alter(&$theme_registry) {
  foreach ($theme_registry['menu_tree']['preprocess functions'] as $key => $value) {
    if ($value == 'template_preprocess_menu_tree') {
      unset($theme_registry['menu_tree']['preprocess functions'][$key]);
    }
  }
}

// PULL THE FIRST MENU ITEM AND GET THE MENU NAME FROM IT
/**
 * Preprocesses the rendered tree for theme_menu_tree().
 * http://drupal.org/node/767404
 */
function YOURTHEMENAME_preprocess_menu_tree(&$variables) {
  $pop = array_slice($variables['tree'], 0, 1);
  $menu_item = array_pop($pop);
  $variables['menu_name'] = $menu_item['#original_link']['menu_name'];
  $variables['tree']      = $variables['tree']['#children'];
}

// PRINT THE MENU NAME WE PASSED ON
/**
 * IMPLEMENTATION OF: theme_menu_tree()
 *
 * Returns HTML for a wrapper for a menu sub-tree.
 *
 * @param $variables
 *   An associative array containing:
 *   - tree: An HTML string containing the tree's items.
 *
 * @see     template_preprocess_menu_tree()
 * @ingroup themeable
 */
function YOURTHEMENAME_menu_tree($variables) {
  return '<ul class="menu' . (($variables['menu_name']) ? ' ' . $variables['menu_name'] : '') . '">' . $variables['tree'] . '</ul>';
}

Hope this helps someone :o)

I think the current version is plain wrong:

function template_preprocess_menu_tree(&$variables) {
  $variables['tree'] = $variables['tree']['#children'];
}

This plain removes the ability to (re-)use existing information in specialized processors or in the theme hook, which is absolutely silly.
For now I totally agree with the approach of BenVercammen in #6
The attached patches integrate the approach but with a slightly other naming.
This is not likely the final solution to make theme_menu_tree() more themeable, but at least it's not impossible to reuse available meta-data for the theming.

Hold on, I think we want to postpone this on #891112: Replace theme_item_list()'s 'data' items with render elements and afterwards, revamp the entire menu tree rendering into a dead-simple item list.

@sun D7 too?! Because I just added the D8 patch to comply with the current set version here.
I'm more interested in fixing this in D7.

oh, hm. I guess we can do this as a bug fix to backport to D7, and handle the total menu tree theming revamp in a separate issue (which will potentially remove the additional data being added here for D8 then).

So for this patch, the only thing I'm concerned about is performance (specifically memory consumption) due to the additional copy of the entire menu tree array in the new #tree theme variable. I think we should bench/profile this.

I agree that this will add some overhead. Unfortunately I can't think of another solution (at least none which doesn't include picking specific information).
The total memory consumption should increase since the full $variables['tree'] is kept longer and $variables['tree']['#children'] is copied.
If there are "parallel" uses of template_preprocess_menu_tree() it's likely the total memory consumption increases quite a bit.

So, what would be an acceptable increase and how shall I test it?
Is a simple D7/D8 fresh install with around 50 menu items enough?
Multiple menus?
How many hierarchy levels?

Test:
50 menu links in the main-menu.
All menus on first level, this means all 50 links are displayed on one page.
2 menu blocks - makes 100 menu links on the same page.
Used profiler: xhprof

Results:
The CPU values differ heavily between each run, so no qualified result here.
Memory consumption after the change:

Cold Cache

• Global memory: 0.0%
• Function specific memory: +18.8%

Warm Cache

• Global memory: -0.2%
• Function specific memory: +18.8%

Following functions had 0.0% changes too:
bartik_menu_tree
menu_tree
menu_tree_output

@Nexotap (#7): many thanks for this!
A small code improvement:
Instead of

<?php
function YOURTHEMENAME_theme_registry_alter(&$theme_registry) {
  foreach ($theme_registry['menu_tree']['preprocess functions'] as $key => $value) {
    if ($value == 'template_preprocess_menu_tree') {
      unset($theme_registry['menu_tree']['preprocess functions'][$key]);
    }
  }
}
?>

Use:

function YOURTHEMENAME_theme_registry_alter(&$theme_registry) {
  $theme_registry['menu_tree']['preprocess functions'] = array_diff($theme_registry['menu_tree']['preprocess functions'], array('template_preprocess_menu_tree'));
}

Thanks Nexotap and Anrikun,

how can I check the region in this case?

#10: drupal-template_preprocess_menu_tree.d7.patch queued for re-testing.

Apologies, I haven't had time to read this entire thread but the simplest answer here has to be to rename template_preprocess_menu_tree to template_process_menu_tree doesn't it?

Then other preprocess functions will get a full render array for the tree?

Removing location tag (and adding SprintWeekend2015) -- there was a discussion (see 2407325#comment-9513533 and decided to use one tag to keep the autocomplete list of tags short.

I dont understand what this is supposed to be doing in Drupal 8 since theme_menu_tree() doesn't exist in D8 anymore.

I think this needs to go back to D7 because I can't see that its relevant anymore.

No longer in D8, so bringing back to D7:
https://api.drupal.org/api/drupal/8/search/theme_menu_tree

I ran into this problem theming a hierarchical menu tree. If all that needs to be passed downward is the #attributes, then simply checking if #attributes have been set and then copying that is a relatively efficient way to resolve this problem.

I am submitting a patch to do just this, and can't see any drawbacks.

I'm new to this patching thing. What does it take to get a little change like this reviewed?

Fixed few of the indentation issues in the patch.

Bump. What does it take to get this proposed change reviewed / tested and incorporated into Drupal?

I have successfully gotten an appropriate modification merged into the main branch of Zurb Foundation (added a-tag class qualifier ":not(.follow)" to allow accordion a-tags to reference...)

That modification, coupled with this and some theme code allow accordions to be opened and closed and attribute information to be passed in the hierarchy of a menu tree without wholesale copying of entire menu tree branches.

I see that Nitesh Sethia has a link for "Add test". Are these tests something I can accomplish? If so, would someone please guide me in how to accomplish them?

RTBC - I am generally fine with this change.

I am not sure we absolutely need tests for this functionality.

It would be better though.

--

Caveat: It might be that this still needs to be fixed in D8 first (even though the actual function does no longer exist.)

Actually I checked the whole issue again and what we really want is:

  $variables['#tree'] = $variables['tree'];
  $variables['tree'] = $variables['tree']['#children'];

to make it available to other preprocessors down the road.

The memory usage is no concern as it is temporary (and the parent still has the data structure in memory anyway and arrays are shared in memory, not copied).

Can someone please roll a new patch based on #10 and update the issue summary?

Thanks!

Hi @Fabianx,

Please find attached the patch based on #10.

If I understand #15 correctly, this approach causes a performance hit, because it is a shotgun blast solution to the problem. I hope the performance of this patch will be tested.

I think that if attributes are what need to be passed, then let's give the opportunity to pass attributes alone.

#35: There is no performance hit in memory for passing around a variable that is in memory anyway still (the caller still has the whole array in memory).

The whole tree is usually needed, attributes are just a special case. This is an unfortunate side effect of theme wrappers and how they work.

--

This still needs a change record and and IS update: Could someone try to write those please?

I am very happy to review the changes / change record.

Bump:

- Could a novice write a change record and update the issue summary, please?

Created a draft change notice https://www.drupal.org/node/2827134 and updated issue summary. Feedback on change notice welcome (could it use example code?).

Splendid! Both work for me.

Some example code in the change notice would be great - yes!

Marking for commit.

Change notice looks good.

Committed and pushed to 7.x, thanks!

I went ahead and published the change record.