Battle plan (wiki-edited)

There are some "low-hanging fruits" that should be done first, before we consider any more radical changes to the menu system.

  1. #512962: Optimize menu_router_build() / _menu_router_save()
  2. #1170278: Introduce hook_menu_router_update()
  3. Now we can discuss further improvements, as discussed in groups.drupal.org
    Menu needs to be torn apart (chx, wiki)
    Router, tabs and breadcrumbs. (discussion)

Detailed explanation (original issue text)

The drupal menu system consists of two layers:

  • The menu_router layer, using hook_menu and the menu_router database table, is responsible for routing http requests to page callback functions. In other frameworks this thing would be called "router system", not "menu system".
  • The menu_links layer, using the menu_links table and the menu module, is responsible for producing nested html lists with menu links.

Both of these layers share the same file, includes/menu.inc.

While the first layer (the "router system") is vital for the Drupal framework to do anything, the second layer (the actual menu system) is not at all a required component. A site can technically work quite well without any menu links displayed anywhere (except for usability, which would suck if you are left with url guessing). And, everything related to menu_links could easily be replaced by a different system using a different table structure and different theme functions.

I think it would be logical, and beneficial to Drupal's architecture, to move anything related to menu_links into an optional core module. This can be the menu module itself, or a new module called menu_links. This would allow contrib developers to write alternative menu systems which could totally kick the ass of the core menu system.

Another core module could define the system_navigation menu. Items in system_navigation are automatically generated from information in menu_router, but can also be modified manually. Rebuilding this menu is a major performance bottleneck on a bunch of admin pages, especially for CCK content type and field management. If system_navigation was an optional module, it could be easily replaced by a light-weight contrib alternative.

hook_menu_rebuild()

To make this possible, it is necessary to introduce a new hook, that notifies core and contrib modules about changes in the menu_router table. The hook would be implemented by system_navigation, but could be implemented by any contributed module.

The hook signature can look like this:

<?php
function hook_menu_rebuild($menu, $changes) {}
?>

where $menu would be the menu array as we know it from menu_rebuild, and
$changes['update'], $changes['delete'] and $changes['insert'] would contain the changes to the menu_router table from the last call to menu_rebuild, keyed by path.

The call to _menu_navigation_links_rebuild() in menu_rebuild() would be replaced by
module_invoke_all('menu_rebuild', $menu, $changes).

(The $changes array is actually an optional gimmick, that would require a
different implementation of _menu_router_save(). The basic idea would work without that.)

Difficulties

I am sure there are other places where the two layers interact with each other: breadcrumb creation, menu translation, menu link access checking etc. So, I guess it will be a lot of work to get really separate layers.

The benefit will be a smaller and thus better manageable core router system, and the possibility for contrib developers to come up with innovative alternatives to the menu_links layer.

---

See also #550254-36: Menu links are sometimes not properly re-parented.

Files: 
CommentFileSizeAuthor
#10 menu_link_save.patch6.34 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_link_save.patch.
[ View ]
#10 hook_menu.txt3.65 KBchx
#5 hook_menu_rebuild.d7.very-very-simple.patch843 bytesdonquixote
PASSED: [[SimpleTest]]: [MySQL] 18,811 pass(es).
[ View ]

Comments

Subscribing.

I think we need a tree.inc to abstract the hierarchical functionality in the menu links and then re-use it for book, menu links, etc.

I'm not sure I see the advantage of making this a module - though I suppose you could if you want to enable a super bare-bones core.

Modules can be switched off and replaced with something else, and modules can talk to each other via hooks. Modules can implement hook_theme, which means a more tidy drupal_common_theme().

My initial idea was to move the system_navigation into a module. The hook_menu_rebuild seems like a good way to disentangle it from the rest of the menu system.

For the rest, I think that menu_links should be further separated from menu_router. The argument for putting it into a module would be that someone else could write a contrib module that would totally replace menu_links with something else. But I don't have a strong opinion about this one.

An alternative contrib menu_links system could bring the following innovations, for example:
- Better multilanguage handling.
- Placeholder url strings, that are dynamically replaced with something else.
- For instance, there could be a placeholder string that includes menu A into menu B at the position of the placeholder.
- There could be placeholder strings to include functional widgets, such as, a search box, in the position of the placeholder.

A lot of this can already be done, so I'm not sure if it is really necessary.

Status:Active» Needs review
StatusFileSize
new843 bytes
PASSED: [[SimpleTest]]: [MySQL] 18,811 pass(es).
[ View ]

A very simple first patch. The only thing it does is introduce a hook_menu_rebuild between menu_rebuild and _menu_navigation_links_rebuild.

In this patch I let the system module implement this hook, but obviously it will make more sense to let a new module do this (or the menu module?).

I mark this as needs review to see what the test bots say. I don't seriously recommend to commit this.

Version:8.x-dev» 7.x-dev

Is this ignored because it's a D7 patch, while the issue says D8?
I don't have any D8 test install to play with atm, I am happy to have a D7..
Can be switched to D8 again when the test bots are finished.

Version:7.x-dev» 8.x-dev

Bot likes it. I support this as the first patch committed to d8

Totally awesome. Will work on this during DrupalCon. Note that menu has three pieces, the third being tabs #484234: Big task cleanup: Remove type and tasks from hook_menu()

OK now I read the issue :) moving out is easy but what we want is switch to use menu_link_save from install and submit if you need a link and that's it.

StatusFileSize
new3.65 KB
new6.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_link_save.patch.
[ View ]

First steps. Props to dmitrig01 for the menu conversion script.

Title:Move menu_links table into a module, and out of includes/menu.incSeparate out menu links from hook_menu

Subscribing.

@chx (#11):

I'm not sure if I support the direction indicated by the title change - but that might be due to a misunderstanding.

I like the way how hook_menu contains information both for the routing, and for the creation of a navigation menu. I just don't like the way how menu_links stuff is so tightly coupled into menu_router and hook_menu code.

Core menu_links and the core navigation menu is just one possible way to build a menu from hook_menu information. There could be alternative ways, that use a different storage format or whatever, and there could be sites that don't use this information at all.

One example is the admin menu, which is only slightly different from system_navigation. A site with admin_menu often doesn't need the core navigation menu.
Another example would be a custom menu that is a subset of system_navigation.

The breadcrumb system does not have any persistent data (afaik), but if it had, or if there was an alternative contrib breadcrumb system that has persistent data, this would be yet another example for a system that can listen to a hook_menu_rebuild, so it can update itself each time that menu_router is modified.

Finally, the local tasks / tabs. Currently we need to look into menu_router to find the tabs for a page. If they would go into a separate table controlled by its own subsystem, then this subsystem could be yet another listener for hook_menu_rebuild.

So: I don't ask for big changes in hook_menu. system_navigation and menu_links and tabs / local tasks and breadcrumbs can still use meta information from hook_menu, but the code for these things can be decoupled so they can be replaced or switched off.

Please correct me if I'm misreading you, chx.

---------

How do these subsystems work?
Should there be crud functions like menu_link_save() ?

The tricky thing with system_navigation is that it can be manipulated by users, and at the same time it has to follow the changes of menu_router. Other subsystems (local tasks, breadcrumbs) could have similar issues, if we allow users to manipulate them.

What I would suggest:
- When a user modifies a menu link (or a breadcrumb, or a local task, if that should be possible one day), we do that via crud functions such as menu_link_save(). These public crud functions need to set a "customized" flag for items that have been manually modified.
- An implementation of hook_menu_rebuild (such as system_navigation_menu_rebuild) will change the menu_links table, but it does not need to use the public "one item at a time" crud interface. Instead, it can use more efficient "private" functions, that update a whole bunch of items at once. Cleanup steps such as reparenting don't need to happen separately for each item, instead there can be a bulk cleanup after the bulk save operation. The internal mass save has to work a little differently from the public crud operations anyway, because it treats the "customized" flag in a different way.
- There should be no crud interface for menu_router. It just wouldn't make sense. The only thing that changes the menu_router table is a menu_rebuild, and we don't want to break that down to crud functions.

To maintain a consistency with both public crud and hook_menu_rebuild operations, we need to define some criteria and invariants for what is a healthy menu_links table, and what every operation is allowed to change.

I think the opposite actually -- we should split hook_menu up into routing and menu.

Hook_menu has lots of things that have nothing to do with the menu system: ajax callbacks, services, tabs for nodes, etc. It also does all sorts of weird things like MENU_SUGGESTED_ITEM simply in order to avoid messing up the user's Navigation block.

I suggest we split this into two:

hook_router, which specifies how Drupal should respond to a particular path:
- what kind of response this is: html, ajax, xmlrpc, etc
- who may access it
- optionally, title and description for hook_menu to inherit

hook_menu, which describes a tree of paths.

The current approach has two advantages:
1. It is easier/faster to do these two things in one go. In many cases you want to have both (especially with admin_menu)
2. It is guaranteed that every menu item from hook_menu is also a valid router path. This way, I don't have to keep both things in sync manually.
3. We avoid unreachable pages, that would certainly happen to exist out of lazyness, if it wasn't for the combined hook.

There can be more hooks for fine-tuning, such as hook_admin_menu is now, but that does not mean we have to radically change hook_menu.

---------

I had another idea for hook_menu, which is maybe not relevent here, but is interesting to think about:

<?php
function mymodule_menu(MenuDefinition $menu) {
 
$menu->addRouterItem('mymodule/%something', ...);
 
$menu->addLocalTask('mymodule/%something/edit', ...);
}
?>

This could be spiced up with method chaining, so we would have things like

<?php
  $menu
->addItem('user/%user/edit')
    ->
setTitle('Edit')
   
// set page callback with arguments
   
->setPageCallback('drupal_get_form', array('user_profile_form', 1))
   
// set arguments for user_access()
   
->requirePermission('administer content types')
?>

This way we would get a more strictly defined interface, with early checking for invalid values, and with support for code suggestions in Eclipse or other IDEs.

I'm not sure if this change is entirely positive, and if it would help any bit to make you worry less about the mixed menu definition. I think the current implementation is more transparent.

Agreed that there are advantages to having them in one place.

But it creates a false system where the routing definition of a path is the 'real' and only menu item for that path. Attempts to have the same path in other parts of the menu cause problems. This is an example I've just spotted: #784842: shouldn't the "post new blog entry" link have 'action-links' class?.

Also:
1. how many modules create menu items outside of the admin tree now anyway? Back in 4.7 and 5, every module used to 'helpfully' create Navigation menu items, but most don't do that any more as users just have to remove them anyway. So we could just say that anything in hook_router that's an admin path should go into the admin menu automatically. Which means you just have to tell it what are tab groups.
2. How often does a module change its menu paths?

So, what about the following approach:

  • hook_menu defines router paths with meta info for suggested menu items.
  • A menu subsystem (system_navigation, admin_menu, breadcrumbs, local tasks) can use all or a subset of the hook_menu meta info, and define additional hooks to alter that information or add additional menu items.
  • Alternatively, there could be a special syntax in hook_menu, that allows to add menu links for duplicate paths.

This way, the average module can do a normal hook_menu implementation, and those that do special stuff can use special syntax or extra hooks.

every module used to 'helpfully' create Navigation menu items, but most don't do that any more as users just have to remove them anyway.

My own approach (for my own sites) is to disable the system_navigation menu block altogether. Instead I have admin_menu, plus custom menus for public visitors and the client / content writers.

Which means, the menu building meta information in hook_menu is only useful for admin paths and for local tasks (and for breadcrumbs? I'm not sure).

On the other hand, most of the meta information is not for menu building, but for access checking etc, stuff we need anyway. So we don't gain much by removing menu building meta info.

I like the way how hook_menu contains information both for the routing, and for the creation of a navigation menu

I don't. I absolutely and irrevocably hate this. They SO have nothing to do with each and we suffer for the two being mushed together and I will separate them.

Ok then. So..
- What meta information will no longer have a place in hook_menu implementations? (or hook_router, if you prefer that)
- Based on which definitions will we construct tabs (local tasks)?
- Based on which definitions will we construct an admin menu?
- Based on which definitions will we construct a breadcrumb?

Will this be in additional hooks?

EDIT: More...
- How does a module such as display suite or ubercart define its (deep and rich) menu tree?
- How do we resolve the conflict of manual menu editing vs module-defined menu structure?

EDIT: And more..
- How many times will we see hook_page_route and hook_menu_links being out of sync, with menu links pointing to non-existent paths, or router paths (for administration pages) being unreachable through the menu? Simply because the module maintainer missed to think about menu links..
- How many times will we see hand-crafted navigation features, where a module maintainer did not bother or know how to build links for the admin menu?
- What about a hook_page_route_alter implementation that leaves menu_links unchanged?

I don't want to take this off-topic, but the way that we construct admin menus now is extremely problematic, #591682: Config pages are effectively hardcoded to some subcategory of 'admin/config' and many, many related issues deal with some of that. There's no clear path forward for fixing it, but one possibility is that system.module (or a new admin_ui.module) would invoke a hook_admin_pages() hook, and then build the actual structure of the menu on behalf of other modules. Modules should only really be concerned about providing admin forms and page callbacks, and a hint to categorisation, not worrying about the intricacies of Drupal's out of the box IA.

We should have lots of tea and beers and discuss this all night long.

#20 are really excellent questions. I can answer tabs #484234: Big task cleanup: Remove type and tasks from hook_menu() the rest we will deal when D8 is developed. I agree we need to answer those questions.

> - Based on which definitions will we construct tabs (local tasks)?

I had a thought about that.... needs a new issue probably.
But basically, *stuff* such as vocabs, terms, even menus, should define itself its base admin path, and then some operations to go below this. That would get used to build tabs and local tasks.

Issue tags:+MenuSystemRevamp

.

A first step:
#1170278: Introduce hook_menu_router_update()
This does not include any of the more severe changes discussed here, but it does also not deny any of them.

Depends on #512962: Optimize menu_router_build() / _menu_router_save(), which is also worth a visit (green patches waiting to be reviewed).

Status:Needs review» Needs work

The last submitted patch, menu_link_save.patch, failed testing.

Status:Needs work» Closed (duplicate)

Issue summary:View changes

Add a "battle plan" into the summary.