Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#1331486: Move module_invoke_*() and friends to an Extensions class left out module_invoke()
for some reason, and no one noticed. :)
Related: #1850992: Make module_hook() check for module_exists() first
Comment | File | Size | Author |
---|---|---|---|
#1 | drupal8.module-invoke.1.patch | 3.48 KB | sun |
drupal8.module-invoke.0.patch | 3.54 KB | sun | |
Comments
Comment #1
sunSorry, that was the wrong discovery method. We only need to check for the particular $module, not all modules.
Comment #2
catchThis 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.
Comment #3
sunNot 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:
Comment #4
catchHmm 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
Comment #5
sunI 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:...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.
Comment #6
catchYeah agreed. We can open a novice follow-up to remove the wrapper once it's in.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedWhen 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.
Comment #8
catchIt'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.
Comment #9
sun#1: drupal8.module-invoke.1.patch queued for re-testing.
Comment #10
sunAny chance to move forward here? :)
Comment #11
webchickThis 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!