Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Sorry, that was the wrong discovery method. We only need to check for the particular $module, not all modules.

catch’s picture

This is slightly changing the functionality of module_invoke()

1. module_invoke() has never respected module_implements() or module_implements_alter()

2.people use module_invoke() to all non hooks, as an alternative to if/else around module_exists(). This will pollute the implements cache a bit if they keep doing that.

Both of these might not be important, but it's an API change beyond simply converting it.

sun’s picture

Not sure I follow?

#1850992: Make module_hook() check for module_exists() first will slightly change its behavior, but this conversion here appears to be 100% identical to me:

Before ModuleHandler HEAD + this patch
function module_invoke($module, $hook) {
  $args = func_get_args();
  // Remove $module and $hook from the arguments.
  unset($args[0], $args[1]);
  if (module_hook($module, $hook)) {
    return call_user_func_array($module . '_' . $hook, $args);
  }
}
  public function invoke($module, $hook, $args = array()) {
    if (!$this->implementsHook($module, $hook)) {
      return;
    }
    $function = $module . '_' . $hook;
    return call_user_func_array($function, $args);
  }
function module_hook($module, $hook) {
  $function = $module . '_' . $hook;
  if (function_exists($function)) {
    return TRUE;
  }
  // If the hook implementation does not exist, check whether it may live in an
  // optional include file registered via hook_hook_info().
  $hook_info = module_hook_info();
  if (isset($hook_info[$hook]['group'])) {
    module_load_include('inc', $module, $module . '.' . $hook_info[$hook]['group']);
    if (function_exists($function)) {
      return TRUE;
    }
  }
  return FALSE;
}
  public function implementsHook($module, $hook) {
    $function = $module . '_' . $hook;
    if (function_exists($function)) {
      return TRUE;
    }
    // If the hook implementation does not exist, check whether it lives in an
    // optional include file registered via hook_hook_info().
    $hook_info = $this->getHookInfo();
    if (isset($hook_info[$hook]['group'])) {
      $this->loadInclude($module, 'inc', $module . '.' . $hook_info[$hook]['group']);
      if (function_exists($function)) {
        return TRUE;
      }
    }
    return FALSE;
  }
catch’s picture

Hmm I'd not actually looked at the code for implementsHook(), assumed that was checking the implementations. Not sure we actually want to keep that behaviour but yeah it makes no difference changing it here then :p

sun’s picture

I think we should simply retain it as-is.

The usage of module_invoke($module, $callback_that_isnt_a_hook) indeed isn't exactly in line with its purpose, and I know that it is used throughout contrib for optional integration code like this:

  // Do not output admin_menu on this page, in case it is enabled.
  module_invoke('admin_menu', 'suppress');

...but I think that's OK-ish... Perhaps we should evaluate a completely new and dedicated DrupalKernel::call($module, $callback, $args = array()) helper for that use-case, but we can do that in a follow-up issue.

I think this patch is RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yeah agreed. We can open a novice follow-up to remove the wrapper once it's in.

effulgentsia’s picture

When it comes to module_invoke(), I don't really get what the difference is between a hook and a $callback_that_isnt_a_hook. For example, is hook_enable() a hook or not? Agreed that we can discuss this in a different issue, just commenting here while it's on my mind.

catch’s picture

It's less callbacks vs. hooks, it's more random functions that other modules define that weren't designed to be either. module_invoke() also gets used for "I'm going to call this random specific function defined by foo module and if it doesn't exist I don't care and will just get FALSE". In that case the calling code could use module_exists() and a real function call instead which is more readable IMO. But yeah it's not really relevant here.

sun’s picture

#1: drupal8.module-invoke.1.patch queued for re-testing.

sun’s picture

Any chance to move forward here? :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a pretty straight-forward follow-up and it sounds lie catch and eff are in favour, soooo....

Committed and pushed to 8.x. Thanks!

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