Ensure reliable link structure and contents for alter hooks (to not break on customizations)

...

As we are talking about it, would it be possible and make sense to add legacy support for older modules that still use hook_admin_menu?

Hmm, I did a dsm($content) in my implementation, and $content['menu'] is a nested array with a bunch of numeric keys. I don't see a $content['links'], as suggested in the documentation.

What is the best way to work with the $content['menu'] array?
I want to add some submenu items under Site building > Menus > List menus (menu_editor module).

Of course I could just search through the tree and look at each href, to finally find the place I want to work with..

I think it needs better documentation. (see #3)

Title is misleading..
Maybe I will find the answer for myself, in this case I might close the issue. For now I just fix the title.

As said in #502500-98: Re-add Create content and also in #3, I find the new hook a bit unconvenient, compared to the old one. I don't like to work with mlids as array keys.

So, what about something like this in hook_menu:

function mymodule_menu() {
  $items = array();
  if (module_exists('admin_menu')) {
    $types = ...;
    foreach ($types as $slug => $title) {
      $items["admin/content/node-add/$slug"] = array(
        'page callback' => 'drupal_goto',
        'page arguments' => array("node/add/$slug"),
        ...
      );
    }
  }
  return $items;
}

Later admin_menu has to look for drupal_goto page callbacks, and replace the item's path with the goto destination.
Alternatively, we could introduce a page callback 'admin_menu_goto_callback', to make sure we don't do unintended stuff.

This works quite ok for menus items that are already part of system navigation.
I would like to use it for menu_editor, where this is not the case.

With hook_admin_menu_output_alter, if the items are not already in another menu, I have to do my own access checking, and build a structure exactly as it is supposed to look for the admin_menu output function. Well, maybe I can get used to that.

For now I have solved this with my own module, where you can see the drupal_goto idea in action.
The only difference is that I have to call admin_menu_output_alter to explicitly walk through those items and replace the link destination.
(see attachment)

--------------

As mentioned over in #502500: Re-add Create content, that issue just needs someone to properly retrieve the already existing menu link tree for and below the path node/add and just put that into the renderable array structure. Extra bonus points for implementing that as node_admin_menu_output_alter().

See #502500-97: Re-add Create content.
So far it fetches the full menu, instead of just a submenu. I could reuse some parts of menu_block, but I'm not sure if it produces the same structure that admin_menu uses.

The point is, I want to add items into the admin_menu tree that would belong somewhere else according to their paths.

Unfortunately, the core menu system does not allow to customize the "parent path" of a router item. The path of a router item is the only thing that defines where it is placed in the menu tree.

Now, for menu_editor, I want the menu edit form to be linked in two places:
1. In a local task at Site building > Menus > (pick a menu) > "Poweredit".
2. A shortcut in the admin menu under Site building > Menus > List menus > [menu name].

This used to be possible with the old hook_admin_menu(), but not with the core hook_menu or hook_menu_alter.
The closest I can get with the core menu system is a redirect, using a drupal_goto callback or a custom function that does the same. This works, but is not the ideal way to do things.

Fortunately, admin_menu provides a hook that allows to replace the path, after the item has already been placed somewhere in the tree. The item can now have a link path that does not have to reflect its position in the menu. However, we need a custom implementation of hook_admin_menu_output_alter to actually make this happen.

The idea was to automate this process, and always replace the path if the page callback is drupal_goto. This would happen before the access checking.

So, why not in core?
If we would be talking about Drupal core, I would rather suggest a dedicated functionality that allows to set either an optional "menu parent" (that can be different from the parent guessed from the array key) or an optional "path" (that can be different from the item's array key) directly in hook menu, or hook_menu_alter. Unfortunately, D6 is feature frozen (and D7 is too nowadays, or not?). And even if it wasn't, it would probably take half a year of discussion to get this feature.

As we are not talking about Drupal core, we can't fundamentally change how hook_menu or hook_menu_alter works, but we can use the existing hook protocol to express extra information.

but you should primarily use this only to change CSS classes, add sub-links, or relocate certain links (although that should ideally be limited to "own" links).

Yes. This is why I was asking for something in addition to hook_admin_menu_output_alter, that happens earlier in the process, and where we can play with paths instead of mlids. As admin_menu is no longer involved in menu_rebuild (or is it?), I can only think of core hook_menu and hook_menu_alter.

A middle way would be a hook that fires before admin_menu_links_menu(), with the tree from menu_tree_all_data(). Or, if we can further split up menu_tree_all_data (which I already did somewhere), we could fire a hook somewhere in that process, before the access checking. We would still be dealing with mlids then, so I'm not sure if this would be much of a benefit.

That said, it looks like you just want to register your regular items/links via hook_menu(), using a path below admin/*, and use hook_admin_menu_output_alter() to move/copy/fork them into other locations of admin_menu.

Almost. The original router items are local tasks, with a parent that is not in system navigation, and is not supposed to be there either.
So, I would have to hook_menu_alter to put the parent items (admin/build/menu-customize and admin/build/menu-customize/[menu name]) into system navigation / admin menu, and then remove it again via hook_admin_menu_output_alter.
I think the drupal_goto approach is easier, because then I only have to change the #href.

until D8 or D9, when we will entirely revamp core's menu system and completely separate menu router items from menu links

I have a slightly different opinion on this..

EDIT: My objections start in #13.
#653784-13: Separate out menu links from hook_menu

Ultimately, however, exactly this conversion is also the cause for further trouble, since administrative menu links are _not_ automatically updated/maintained anymore if site administrators are renaming/changing/moving administrative links.

This is true, but: The reason for this conflict is not the menu_router to menu_links conversion. The real cause is the mix of hardcoded and hand-built structure.

In some way modules need a possibility to indicate menu tree locations and menu link titles for their configuration pages. In D6 and D7 this happens via hook_menu, and in D8 there might be a separate hook_menu_links. Or it could all be in hook_install, where modules would have to do things like menu_link_save() etc (which would seriously suck).

Think of display suite, or ubercart. Each of them have a very deep menu structure, which has to be defined in some way. And you seriously don't want a user to click around and build this structure herself.

So, let's assume this would all be in hook_menu_links() or in hook_install or something else. Now there can still be a user who wants to do his customizations via the menu links backend. We get the same conflict that we had with hook_menu in D6. Nothing changed. Code vs configuration.

Any solution we can find to solve this conflict will probably work just as well with old hook_menu.

My own solution to the problem is to never configure system nav or admin menu via the backend. And I imagine that most other people do the same.

Therefore, the presumption that there would be a valid menu link tree that's keyed by paths instead of mlids is wrong. Something like that does not exist.

I am aware of that.
Nevertheless, there exists a structure that the tree of menu links is based on. This structure lives in code, and code does not know about mlids.

I understand why the tree has to be keyed by mlid. On the other hand, every implementation of hook_admin_menu_output_alter will have to look for paths, if it wants to target specific items (as you can see very nicely in the "not a patch" in the other issue).

I think the helper functions in the attachment above and in the other issue are quite useful already.

If you want, you can look into dqx_adminmenu, which has always done it like that. This also allows to do modifications like "re-add create content" via the api, instead of being hardcoded in the menu building code.
Not sure if this helps.
(the D7 version is very rough-edge atm)

I did not experience any hickups, but this is mainly because dqx follows an entirely different philosophy.

Most importantly, it does not do anything with menu_links. It fetches directly from menu_router. As a result, if you work with the API, you do usually know quite well where the items are, because no use can mess with them.

It would be difficult to say, how well this works with the existing structure of admin_menu pulling from menu_links.

The main problem, I guess, is duplicate paths resulting in duplicate keys.
menu_router naturally delivers unique paths. Any duplicates can only come from plugins or, in your case, from users editing the menu.
In dqx, plugins can discard that "path = key" idea, and register their own custom submenus and custom links under made-up keys. Unless, they really want to override existing keys, in which case they will use the existing path/key.

Also notice the different way that the menu structure is kept in variables:
Instead of a deeply nested array, dqx uses two rather flat arrays:
$submenus, and $items.
The structure is all in $submenus, and basically that is a $parent_key => $child_keys map.
Yes, this could theoretically result in a loop, but it would be not too difficult to detect such a loop.

The good thing about these flat arrays is that plugins don't need to dive into a deeply nested structure.

Lastly, also note that #502500: Re-add Create content finally bit the dust.

I did notice :)

Any duplicates can only come from plugins or, in your case, from users editing the menu.

I guess, duplicates can also come from the horrible menu_navigation_links_rebuild process. I remember seeing some duplicates in the admin/structure/menu. Or was it in themes?

For customized links, maybe the "customized" flag could be used as an indicator to key by mlid, or by href + mlid, whereas uncustomized links would be keyed by path. This would mean that any plugins can only target non-customized links.

The other approach would be to let the plugins kick in before menu_navigation_links_rebuild, so that any manual customization would be on top of that. (this is what admin_menu used to do in the past)

For my part, I once decided that any justified manual customization indicates a deficiency in the pre-built structure, and should rather be done by a plugin.

Of course, it's a pretty wild assumption to believe that every $href is unique, but I do believe that one $href should be unique on a single tree level of the menu.

Menu rebuild will kill you when you sleep.

You need a "what happens if" strategy for duplicate keys, if you key by href or path, if you continue to fetch from menu links.
OR, you key by href + path, like this:
$items[$path][$mlid] = $item;
Then it would be up to plugin authors to decide which item they want to target.

Is that what you had in mind?

Hi,
sorry I didn't reply earlier.
I am going to comment, probably this is no different from what you were thinking yourself.

So what we do here is (simplified)

• Fetch from menu_links, with mlid as keys (naturally), with user modifications
• Use href as the key (actually the term href is misleading, better would be system path - but this is how the menu_links system names it. The structure is a deep nested array.
• Apply some programmatic modifications, and mix with other href-keyed submenus.

I think this is fine, but for a user it will be possible to rearrange or modify the menu links in a way that the programmatic modifications no longer work.
For instance, changing the href of one the top-level items, or moving an item one level up or down, or into a different submenu - each of this can make the programmatic modifications stop working.

The assumption is that users modify the admin menu "at their own risk". I think that is fine.