Remove module menus on uninstall

Menu modules is optional now, since most of its functions are in menu.inc.

I think we have to do this in system.module.

We should standardize the code, since #320392: DX: Remove module tables automatically on uninstall is just one of a number of similar new patches.

@Dave reid - need to look in more detail, but indeed menu module is optional and hence not the right palce.

Also, this code deals with menu links - "menu items" is a term we should stop using since it's not clear if it means router items, or menu links. I realize that the code comments in the menu system sorely need to be cleaned up on this front.

I was looking at this patch again, and I do not believe that removing the menu links here is actually required. The uninstall routine is only called under certain conditions, and only after a module has already been disabled. When a module is disabled -- in fact, any time the module page loads -- registry_rebuild() is called, which clears and rebuild both the {menu_router} and {menu_links} tables.

So the routine to remove menu items originally found in drupal_uninstall_modules() seems redundant. The only use I can see for that function is in the (rare?) cases where modules are disabled and uninstalled from a batch or command-line function. (That is, without loading the admin/build/modules page and firing registry_rebuild().

But even in such a case, module_disable() has to be called first. And module_disable() itself also forces a registry_rebuild. So I think we can remove this routine altogether, which the attached patch does.

(So this patch actually removes code that is not needed.)

Marking as bug, since this is redundant code AFAIK.

Revised patch for testbot.

@agentrickard: what is removed when calling menu_rebuild() is menu routers and menu links that are defined in the hook_menu() implementation of the module. Menu *links* that can be created using menu_link_save() are *not* removed by a call to menu_rebuild().

For an example of an actual use of menu_links (independently of a menu router entry) is: http://api.drupal.org/api/function/menu_enable/6

Dave approach in #5 makes total sense.

OK, I see that, but should those link entries not be removed on module _disable_ rather than uninstall?

Anyone able to revive this?

I just stumbled over this code. I do not understand why it exists, because:

- Menu router items of a module are no longer registered in the first place, because hook_menu() is not invoked in disabled modules, and a module needs to be disabled before it can be uninstalled.

- I was under the impression that a menu rebuild would already clean up and remove all stale router items that no longer exist. And that a menu rebuild would be executed whenever modules are disabled.

- The current code loads a disabled module to invoke its hook_menu(). In combination with #151452: Uninstalling modules does not follow dependencies and can lead to WSOD, this reveals that a module's hook_menu() implementation may contain runtime code that may rightfully expect that the module is enabled.

Overall, if this manual cleaning of menu router paths is really required for some reason, then I think the proper way would be to add a 'module' column to {menu_router}, so as to properly record a module name reference and avoid loading modules during an uninstallation process.

@sun I think I had the same confusion. The existing code (and patch approach) isn't about removing items from {menu_router}; it's about removing dead items from {menu_links} which belong to a {menu_link}.router_path defined by an uninstalled or disabled module.

I just tried a quick test and the problem I ran into is that my 'orphan' menu link (/admin/structure/domain/settings) was simply reparented to /admin/structure.

Not sure this is fixable in D7 and may need to be abandoned or bumped to D8.

Subscribe.

Thanks for clarifying. In that case, this bug is indeed major/critical and needs to be fixed.

I don't think we can call a module function during module uninstallation, as then dependencies might not be enabled any more. I already ran into this problem with profile2, see #1008346: Uninstallation of profile pages fails if profile2 is disabled.

More momentum at #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it). I think this is a dupe.