Spin-off from #1850352: config_import_invoke_owner() should check whether a module exists before invoking its hooks
module_hook() is based on a pure function_exists() only, which can easily trigger false-positives, depending on what PHP code has been loaded.module_exists() check before returning anything.| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drupal8.module-hook.13.patch | 5.41 KB | sun |
| #11 | drupal8.module-hook.11.patch | 1.19 KB | sun |
| #9 | drupal8.module-hook.9.patch | 629 bytes | sun |
| #4 | drupal8.module-hook.4.patch | 1.54 KB | sun |
| drupal8.module-hook.0.patch | 487 bytes | sun |
Comments
Comment #1
tim.plunkettLooks like #1850988: Should module_hook() check module_exists()? beat you to the punch just barely!
sun++
Comment #2
sunFurther research/analysis:
1)
module_implements()is based onmodule_list(). Granted, it's cached, but at least if the cache is correct, then it should not yield false-positives.2)
module_hook_info()is equally based onmodule_list(). Also cached, but only statically.3)
module_invoke_all()is based onmodule_implements(), thus, circling back into 1).4)
drupal_alter()is based onmodule_implements(), thus, circling back into 1).Actually, there are only 10 calls to module_hook() throughout core.
The only noteworthy one out of those is the call from module_invoke(), which in turn is called a lot. But will this additional call to
module_exists()be measurable? I guess not, but obviously we need to bench it.Comment #4
sunHrm. That makes it two function calls. Both are quick, but still...
Comment #6
hefox commentedFor unrelated reasons, I recently tested in d6 calling function_exists vs module_exists to see which was faster and discovering function_exists was better by an extreme amount. Adding a module_exists to what current a function_exists does make it quite heavy, I think. Does anywhere call module_hook on disabled modules?
Comment #7
sunYes, that's what triggered this issue; see #1850352: config_import_invoke_owner() should check whether a module exists before invoking its hooks
Comment #8
sunRelated: #1883658: Remove the concept of disabled modules
Comment #9
sunWith #1331486: Move module_invoke_*() and friends to an Extensions class, this is the much simpler revised version.
The originally offending
module_hook()call disappeared from HEAD in the meantime. Nevertheless, this is still a good idea.Comment #11
sunnode_hook()supports a strange fake-hook-naming pattern based on the "base" property of a node type, which is 'node_content' for all custom node types created via Node module's UI. But 'node_content' is not a module that exists.Comment #13
sunThere are a few implementations of
hook_schema(), which are calling intohook_schema()of another module, and the corresponding DUTB tests did not enable those modules as dependencies.Comment #14.0
(not verified) commentedUpdated issue summary.
Comment #27
smustgrave commentedWith module_hook() being replaced by service in https://www.drupal.org/node/1894902 wonder if this is still a valid task?
Comment #29
quietone commentedThere has been no work on this task for 12 years. It is likely that it is no longer needed or has been implemented. Therefor, closing.
If there is work to do here, then either re-open the issue and add a comment or open a new issue and reference this one.