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.

Files: 
CommentFileSizeAuthor
#4 drupal-1216758-4.patch1.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,553 pass(es).
[ View ]
#1 drupal-1216758-1.patch543 bytestim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,540 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new543 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,540 pass(es).
[ View ]

Attached.

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

Status:Needs review» Needs work

Needs a comment above the unset otherwise good.

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 33,553 pass(es).
[ View ]

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().

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.

Tagging issues not yet using summary template.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Clarification.

Issue summary:View changes

Markup fail.

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

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

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

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

Status:Fixed» Needs review

For D7 and D6.

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

Should apply cleanly against D7, too.

Status:Reviewed & tested by the community» Fixed

Seems like a decent DX improvement.

Committed and pushed to 7.x. Thanks!

Yay! Thanks!

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

Issue summary:View changes

Link to patch comment.