While searching for the hook_menu invocation, I came across something pretty weird: Rather than module_invoke, menu.inc pastes together $module . '_menu' and calls call_user_func.

I was almost completely sure that is the wrong way to do it, and I wanted to find out why on Earth we do it.

Turns out that this goes all the way back to May 2007 and #140218: Make Drupal smaller: move page callbacks to their own include files.

-  $callbacks = module_invoke_all('menu');
+  // We need to manually call each module so that we can know which module a given item came from.
+  $callbacks = array();
+  foreach (module_implements('menu') as $module) {
+    $items = call_user_func($module . '_menu');
+    if (isset($items) && is_array($items)) {
+      foreach (array_keys($items) as $path) {
+        $items[$path]['module'] = $module;
+      }
+      $callbacks = array_merge($callbacks, $items);
+    }
+  }

So I want to ask, is there any reason we don't use module_invoke($module, 'menu') here?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cburschka’s picture

Status: Active » Needs review
FileSize
698 bytes

Here is a patch to change this.

moshe weitzman’s picture

Status: Needs review » Closed (works as designed)

Whats the advantage of module_invoke() here? All it does is make sure that the module implements the specified hook but we already know it does - thats the point of module_implements(). So, I consider module_invoke() slightly less direct and thus less desireable. IMO, this is By Design

RobLoach’s picture

Status: Closed (works as designed) » Needs review

#1: drupal-menu-inc-module_invoke-626134-1.patch queued for re-testing.

...... I'd opt to use the API Drupal provides rather then relying on call_user_func(). Or maybe switch to a $function() call?

Damien Tournoud’s picture

Title: call_user_func() used instead of module_invoke(). » call_user_func() used instead of $function()
Category: bug » task
Status: Needs review » Needs work

Traditionally, we do that:

foreach (module_implements('menu') as $module) {
  $function = $module . '_menu';
  $items = $function();
}

So this implementation is slightly inconsistent. We could fix this, but for sure there is no point in using module_invoke() here.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

So let's move to the $function() call. This patch also does two other things:

  1. Assumes the hook_menu call returns an array so that we are not babysitting broken code
  2. Reduces the amount of times it constructs/deconstructs the array.
Damien Tournoud’s picture

Let's keep the type checking here. There are enough ways to brick a site during development, we don't need to add some more.

catch’s picture

Status: Needs review » Needs work

Needs work per #6.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

Updated the patch with the suggestion in #6.

Niklas Fiekas’s picture

Status: Needs review » Needs work
+++ includes/menu.incundefined
@@ -2625,10 +2626,12 @@ function menu_router_build() {
-  list($menu, $masks) = _menu_router_build($callbacks);
-  _menu_router_cache($menu);
 
-  return array($menu, $masks);
+  // Build the router and cache the new menu array from it.
+  $router = _menu_router_build($callbacks);
+  _menu_router_cache($router[0]);
+
+  return $router;

What is the argument here? Doesn't this change what is actually returned?