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.Spin-off from #1850352: config_import_invoke_owner() should check whether a module exists before invoking its hooks
Problem
- Unlike all other module_* functions,
module_hook()is based on a purefunction_exists()only, which can easily trigger false-positives, depending on what PHP code has been loaded.
Proposed solution
- Simply add a
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 CreditAttribution: 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) CreditAttribution: commentedUpdated issue summary.
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedWith module_hook() being replaced by service in https://www.drupal.org/node/1894902 wonder if this is still a valid task?