Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
base system
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
21 Jan 2013 at 20:57 UTC
Updated:
29 Jul 2014 at 21:48 UTC
Jump to comment: Most recent, Most recent file
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 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!