Problem/Motivation

The doxygen function documentation for module_invoke() lists both $hook and $module as parameters, but these are currently retrieved with func_get_args() rather than being declared in the function definition. Adding the parameters to the function definitions would make them more self-documenting and would also provide more meaningful error messages when the correct parameters are not passed.

The same is true of module_invoke_all() for its $hook argument.

Proposed resolution

Patch in #1216758-4: Add $module and $hook as formal parameters in module_invoke() adds the arguments to the function definitions. So:
module_invoke()
becomes
module_invoke($module, $hook)

and
module_invoke_all()
becomes
module_invoke_all($hook)

Remaining tasks

None. The $module and $hook parameters are already documented in the functions' docblocks.

User interface changes

None.

API changes

The documented signatures for module_invoke() and module_invoke_all() would technically change, but there would be no functional change since both use func_get_args(). (These functions already trigger errors when the $hook or $module arguments are not passed.)

This change could be backported to D7 and even D6.

Original report by @tim.plunkett

Looking at module_invoke, I don't understand why $module and $hook aren't in the function definition.

I'm expecting there is a reason why this isn't done, but if so, can we document why it is done this way?

This would also make things break cleaner if someone tried to call module_invoke() alone.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
543 bytes

Attached.

neclimdul’s picture

By cleaner, a required parameter error for module_invoke is better than vague warnings and parameter error for module_hook(). Especially since module_hook() already has formal parameters this is a bit of a WTF likely caused by the historical implementation of module_invoke()(array_shift'ing in D5).

Patch looks good though I haven't actually reviewed it and the bot hasn't finished running so just a soft +1 and sub

chx’s picture

Status: Needs review » Needs work

Needs a comment above the unset otherwise good.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Comment modeled after the one in drupal_get_form() where the same thing is done.
Also noticed the same exact thing in module_invoke_all().

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Excellent, thanks!

This can be backported to D7 and perhaps even to D6, too.

xjm’s picture

Tagging issues not yet using summary template.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Clarification.

xjm’s picture

Issue summary: View changes

Markup fail.

xjm’s picture

Summary added. Cool patch by the way; this one made me go "Oh duh!"

sun’s picture

This patch can actually be ported back to Drupal 4.7.x...

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

tim.plunkett’s picture

Just to note, the commit message got mangled: http://drupalcode.org/project/drupal.git/commit/e98e818

tim.plunkett’s picture

Status: Fixed » Needs review

For D7 and D6.

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Should apply cleanly against D7, too.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Seems like a decent DX improvement.

Committed and pushed to 7.x. Thanks!

neclimdul’s picture

Yay! Thanks!

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

Anonymous’s picture

Issue summary: View changes

Link to patch comment.