Offshoot from #276751: Allow to alter/customize/add links in administration menu:

1) theme_admin_menu_links() is a bit cumbersome currently. It's a mixture of theme_links() and theme_item_list(), and was intended to render a LI that (usually) already contains an A. However, due to this mix, understanding the element properties is not easy. Additionally, add-on modules cannot easily inject a different element somewhere below, for example a #form or straight #markup (that drupal_render() would normally render just fine here).

2) theme_admin_menu_links() contains a new, awkward $depth argument, to avoid outputting a UL on the top-level (because we need it statically/hard-coded there for CSS/JS interaction and general container), but still outputting ULs for sub-menus deeper in the menu.

3) The actual menu is not processed and not rendered in theme_admin_menu_links(). This is done in theme_admin_menu_tree_output() instead. Add-on modules cannot alter the menu, because it is already rendered HTML.

To fix this mess, I'm proposing to introduce

  • #type [admin_menu_]list_item: Renders a single LI.
  • #type [admin_menu_]link: Renders a link (A).

If possible, when #type list_item identifies that a element has #children of the #type list_item, then it automatically wraps those with a UL. Alternative proposals highly appreciated.

To picture this:

Current code:

  $links['icon'] = array(
    '#title' => theme('admin_menu_icon'),
    '#attributes' => array('class' => array('admin-menu-icon')),
    '#href' => '<front>',
    '#options' => array(
      'html' => TRUE,
    ),
  );
  $links['icon']['cron'] = array(
    '#title' => t('Run cron'),
    '#weight' => 50,
    '#access' => user_access('administer site configuration'),
    '#href' => 'admin/reports/status/run-cron',
  );

New code following this proposal:

  $links['icon'] = array(
    '#type' => 'list_item',
    '#attributes' => array('class' => array('admin-menu-icon')),
  );
  $links['icon'][] = array(
    '#type' => 'link',
    '#title' => theme('admin_menu_icon'),
    '#href' => '<front>',
    '#options' => array(
      'html' => TRUE,
    ),
  );
  $links['icon']['cron'] = array(
    '#type' => 'list_item',
    '#attributes' => array('class' => array('admin-menu-icon')),
    '#weight' => 50,
    // Oh my.  Rather belongs to the link, but needs to be applied on the LI... :(
    '#access' => user_access('administer site configuration'),
  );
  $links['icon']['cron'][] = array(
    '#title' => t('Run cron'),
    '#href' => 'admin/reports/status/run-cron',
  );

Alternatively, we could also alter theme_admin_menu_links() to check for #type and #theme when iterating over element_children()... when one of the properties is encountered for a child, processing is handed over to drupal_render(), but the resulting markup gets still wrapped with a LI.

CommentFileSizeAuthor
#17 admin_menu-HEAD.render-children.patch5.71 KBsun
#16 admin_menu.render-custom-16.patch4.28 KBsun
#15 admin_menu.render-custom-15.patch4.04 KBsun
#14 admin_menu.render-custom.patch3.67 KBsun
#10 admin_menu.render-menu-10-d6.patch8.38 KBsun
#9 admin_menu.render-menu-d6.patch7.2 KBsun
#8 admin_menu.render-menu-8.patch8.73 KBsun
#7 admin_menu-527908-7-D7.patch5.59 KBmarkus_petrux
#6 admin_menu-527908-6-D7.patch5.23 KBmarkus_petrux
#4 admin_menu.render-menu.patch5.18 KBsun

Comments

markus_petrux’s picture

markus_petrux commented

In the latest patch, you have replaced $content['links'] by $content['icon'] and $content['user'].

    $content['icon'] = admin_menu_links_icon();
    $content['icon']['#theme'] = 'admin_menu_links';
    $content['user'] = admin_menu_links_user();
    $content['user']['#theme'] = 'admin_menu_links';

This makes a little confusing the implementation of hook_admin_menu_output_alter() against $content. Here's a snippet on the implementation I had to do for our backend menu:

function darwin_backend_menu_admin_menu_output_alter(&$content) {
  global $base_url;

  if (variable_get('darwin_backend_admin_menu', 0) && ($darwin_backend_menu_name = variable_get('darwin_backend_menu_name', NULL))) {

    // Add a top level item for the backend menu itself.
    $content['darwin'] = array(
      '#theme' => 'admin_menu_links',
    );
    $content['darwin']['backend'] = array(
      '#title' => t('Darwin backend'),
      '#href' => $base_url,
      '#weight' => 49,
    );

    // Copy the create content submenu to our backend menu.
    $content['darwin']['backend']['create-content'] = array(
      '#title' => t('Create content'),
      '#href' => 'node/add',
      '#weight' => -10,
    );
    foreach (node_get_types() as $type) {
      $type_url_str = str_replace('_', '-', $type->type);
      $content['darwin']['backend']['create-content'][$type_url_str] = array(
        '#title' => drupal_ucfirst($type->name),
        '#href' => 'node/add/'. $type_url_str,
      );
    }

// ...more code goes here that appends internal backend stuff (custom).
}

a) I need to create 2 levels in the array to append a top level menu item to admin_menu.
b) I need to specify the #theme attribute of the top level element I'm adding to the $content array.

1) Maybe these could be simplified a little?
2) Maybe $content['icon'] and $content['user'] could be inserted by a hook_admin_menu_output_alter() implemented by admin_menu itself? That way we would have an example on how to use this hook properly.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Thanks for your feedback!

a) I need to create 2 levels in the array to append a top level menu item to admin_menu.

Yes, but I don't see a way around that. If you look at admin_menu_links_user(), it adds multiple top-level items, and to allow for that, we need a proper recursive/hierarchical structure.

Also, I guess it may be easier to grok this part, if the 'icon' sub-menu would be added as $content['menu']['icon'], instead of an entirely separate structure? That, however, directly relates to 3) in the original post, because the actual menu is pre-rendered into HTML and stuffed into $content['menu'] currently.

b) I need to specify the #theme attribute of the top level element I'm adding to the $content array.

Basically, we could assume theme_admin_menu_links() as #theme by default, if no custom #theme or #type is defined. However, that is also closely tied to what I outlined in the original post -- i.e. whether we want to keep the entire (and a bit confusing) theme_admin_menu_links() at all.

However, we can and probably definitely should move the two '#theme' definitions into admin_menu_links_icon()/admin_menu_links_user() to clarify this bit.

1) Maybe these could be simplified a little?

Well, that's basically the intention of this issue. ;) One related question is whether we want to optimize and simplify for menu items (while making custom additions/widgets such as the user counter and shortcut bar a bit more confusing) or whether to do the opposite, i.e. optimize and simplify for additions/widgets (by using a more sane, normalized, and clean array structure that follows common DX regarding drupal_render()).

2) Maybe $content['icon'] and $content['user'] could be inserted by a hook_admin_menu_output_alter() implemented by admin_menu itself? That way we would have an example on how to use this hook properly.

This would mean that third-party modules could not rely on the default menu additions of the core admin_menu. For example, hooks of a module like "activity" may be invoked before "admin_menu" (due to alphabetical ordering). So I'm not sure whether that is a good idea.

markus_petrux’s picture

markus_petrux commented

This would mean that third-party modules could not rely on the default menu additions of the core admin_menu.

Yes and no. Those who run after admin_menu could find them there. But anyway, agree these items are easy to alter if they are "builtin".

Maybe it is time to do something about $content['menu'] now. menu_tree_all_data('admin_menu') would have to be transformed into an structure of data compatible with theme_admin_menu_links, or something on that direction. I believe once all elements have the same format, then it will be easier to figure out the whole thing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs work
StatusFileSize
new admin_menu.render-menu.patch5.18 KB

oh yes, it feels good to remove superfluous code. :)

Works - only the #weights are getting hi-jacked (and I don't know why).

markus_petrux’s picture

markus_petrux commented

The icon is misplaced because it lacks a #weight, and this one is mixed with all the stuff that comes from the menu tree.

// This is a mini-patch over your patch to admin_menu_links_icon()

-  $links = array(
-    '#weight' => -100,
-  );
   $links['icon'] = array(
     '#title' => theme('admin_menu_icon'),
     '#attributes' => array('class' => array('admin-menu-icon')),
     '#href' => '<front>',
     '#options' => array(
       'html' => TRUE,
     ),
+    '#weight' => -100,
   );

I have a question/doubt now :P

We have now $content['menu'] and $content['user'], but both sub-arrays contain items that are rendered on the top menu bar. Why? This is confusing, I think. It looks like the stuff in $content['user'] could also be placed in $content['menu'], just give it a weight that ensures it is rendered on the correct place.

Maybe the weight of the top level items in the menu bar that come from the menu tree need to be altered to ensure they are between -100 (icon stuff) and (say) 100 (user stuff).

markus_petrux’s picture

markus_petrux commented
Status: Needs work » Needs review
StatusFileSize
new admin_menu-527908-6-D7.patch5.23 KB

Nah, discard my previous comment. Sorry.

I think I got it. menu_tree_all_data() is giving us sorted menu items, so we don't need to sort them again during drupal_render().

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new admin_menu-527908-7-D7.patch5.59 KB

Added a weight 0 for the menu tree itself, as this ensures it is rendered between the icon on the left (weight -100) and the user links on the right (weight 100).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)
StatusFileSize
new admin_menu.render-menu-8.patch8.73 KB

Yay! You made it work - awesome!

Thanks for reviewing and testing! Committed attached patch to 7.x (including usage of this new functionality for admin_menu_toolbar).

Now, let's shoot D6...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new admin_menu.render-menu-d6.patch7.2 KB

Somehow works for me, but should not. (since element_children() works differently here)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new admin_menu.render-menu-10-d6.patch8.38 KB

This is the proper one. However, I don't fully grok why links below "Flush all caches" get sorted in reversed order.

markus_petrux’s picture

markus_petrux commented

hmm... I tested the patch I attached for D7 to my D6 installation, and it worked. The only difference is the admin menu name. Well, for some reason it worked, or it seems to me it did. :-|

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

Sure ;) It also "worked" for me, but some sub-elements were not sorted/ordered at all.

That said, two important issues:

  • The handling of #sorted and element_children() is very different in D6. As previously mentioned, I have no idea why the items below "Flush all caches" are a) not sorted at all (?) or b) sorted in reversed order. I'm not sure whether we want to deal with this is an a follow-up patch (or even issue).

    That said, the only difference between #9 and #10 is the additional sorting of elements in theme_admin_menu_links().

  • I entirely forgot to implement a custom processing for optional #type + #theme element properties. The point is: Whenever drupal_render() determines a #type, it passes on the processing of an element to the element type handler, but recursively processes sub-elements on its own. Whenever drupal_render() determines a #theme, the element and the entire structure below that element is passed on to the theme handler and drupal_render() does not recurse any further. The latter is the case for theme_admin_menu_links(). The original idea was to

    a) either replace the parent and recursively walking #theme property with #type properties on all elements in the $content array or

    b) add special support for custom #type/#theme properties in sub-elements, i.e. processing of sub-elements is handed over to drupal_render(), but the result is wrapped in a <li></li> to ensure and maintain our hierarchical menu structure.

    That said, by doing b), we could perhaps also move the #theme property to the top-level $content element.

To move forward, I've committed that last patch to 6.x-3.x.

markus_petrux’s picture

markus_petrux commented

Ah, ok. Well, I also think b) is the best way to go. This should allow further customizations as these can be overridden or altered during the rendering process.

Thanks for the hard work here. ;-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new admin_menu.render-custom.patch3.67 KB

Holy cow. (!)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new admin_menu.render-custom-15.patch4.04 KB

heh... that $depth + 1 was a bit b0rked, too ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new admin_menu.render-custom-16.patch4.28 KB

Minor fix to only apply the 'expandable' CSS class when children are admin menu links.

Not sure where to go from here.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work
StatusFileSize
new admin_menu-HEAD.render-children.patch5.71 KB

I've committed attached patch to 7.x and backported + committed it also to 6.x now. (without that nifty Devel switch user form ;) - separate issue)

Should we further evaluate whether #prefix + #suffix could be used for the surrounding LIs, and perhaps also the surrounding/conditional ULs?

At least, I'm not 100% happy with the new code yet.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Active

Proper status.

candelas’s picture

candelas commented

any news about this interesting feature?
thanks for the module :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +D7 stable release blocker
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Fixed

Let's call this fixed. Follow-up work happens in #1564934: Separate toolbar and dropdown menu markup

Status: Fixed » Closed (fixed)
Issue tags: -D7 stable release blocker

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +D7 stable release blocker

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.