Posted by sun on November 26, 2012 at 5:46pm
5 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | sun |
| Status: | needs work |
| Issue tags: | API clean-up, Testing system |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal8.module-hook.0.patch | 487 bytes | Idle | FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. | View details | Re-test |
Comments
#1
Looks like #1850988: Should module_hook() check module_exists()? beat you to the punch just barely!
sun++
#2
Further 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.#3
The last submitted patch, drupal8.module-hook.0.patch, failed testing.
#4
Hrm. That makes it two function calls. Both are quick, but still...
#5
The last submitted patch, drupal8.module-hook.4.patch, failed testing.
#6
For 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?
#7
Yes, that's what triggered this issue; see #1850352: config_import_invoke_owner() should check whether a module exists before invoking its hooks
#8
Related: #1883658: Remove the concept of disabled modules
#9
With #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.#10
The last submitted patch, drupal8.module-hook.9.patch, failed testing.
#11
node_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.#12
The last submitted patch, drupal8.module-hook.11.patch, failed testing.
#13
There 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.#14
The last submitted patch, drupal8.module-hook.13.patch, failed testing.