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.
Comment | File | Size | Author |
---|---|---|---|
#4 | drupal-1216758-4.patch | 1.02 KB | tim.plunkett |
#1 | drupal-1216758-1.patch | 543 bytes | tim.plunkett |
Comments
Comment #1
tim.plunkettAttached.
Comment #2
neclimdulBy 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
Comment #3
chx CreditAttribution: chx commentedNeeds a comment above the unset otherwise good.
Comment #4
tim.plunkettComment modeled after the one in
drupal_get_form()
where the same thing is done.Also noticed the same exact thing in
module_invoke_all()
.Comment #5
sunExcellent, thanks!
This can be backported to D7 and perhaps even to D6, too.
Comment #6
xjmTagging issues not yet using summary template.
Comment #6.0
xjmUpdated issue summary.
Comment #6.1
xjmClarification.
Comment #6.2
xjmMarkup fail.
Comment #7
xjmSummary added. Cool patch by the way; this one made me go "Oh duh!"
Comment #8
sunThis patch can actually be ported back to Drupal 4.7.x...
Comment #9
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #10
tim.plunkettJust to note, the commit message got mangled: http://drupalcode.org/project/drupal.git/commit/e98e818
Comment #11
tim.plunkettFor D7 and D6.
Comment #12
sunShould apply cleanly against D7, too.
Comment #13
webchickSeems like a decent DX improvement.
Committed and pushed to 7.x. Thanks!
Comment #14
neclimdulYay! Thanks!
Comment #15.0
(not verified) CreditAttribution: commentedLink to patch comment.