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 pure function_exists() only, which can easily trigger false-positives, depending on what PHP code has been loaded.

Proposed solution

  1. Simply add a module_exists() check before returning anything.
Files: 
CommentFileSizeAuthor
#13 drupal8.module-hook.13.patch5.41 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,352 pass(es), 189 fail(s), and 304 exception(s).
[ View ]
#11 drupal8.module-hook.11.patch1.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] 49,106 pass(es), 224 fail(s), and 143 exception(s).
[ View ]
#9 drupal8.module-hook.9.patch629 bytessun
FAILED: [[SimpleTest]]: [MySQL] 47,622 pass(es), 1,008 fail(s), and 533 exception(s).
[ View ]
#4 drupal8.module-hook.4.patch1.54 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
drupal8.module-hook.0.patch487 bytessun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

tim.plunkett’s picture

Status:Needs review» Closed (duplicate)

Looks like #1850988: Should module_hook() check module_exists()? beat you to the punch just barely!
sun++

sun’s picture

Status:Closed (duplicate)» Needs review

Further research/analysis:

1) module_implements() is based on module_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 on module_list(). Also cached, but only statically.

3) module_invoke_all() is based on module_implements(), thus, circling back into 1).

4) drupal_alter() is based on module_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.

Status:Needs review» Needs work

The last submitted patch, drupal8.module-hook.0.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Hrm. That makes it two function calls. Both are quick, but still...

Status:Needs review» Needs work

The last submitted patch, drupal8.module-hook.4.patch, failed testing.

hefox’s picture

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?

sun’s picture

sun’s picture

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new629 bytes
FAILED: [[SimpleTest]]: [MySQL] 47,622 pass(es), 1,008 fail(s), and 533 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.module-hook.9.patch, failed testing.

sun’s picture

Assigned:Unassigned» sun
Status:Needs work» Needs review
StatusFileSize
new1.19 KB
FAILED: [[SimpleTest]]: [MySQL] 49,106 pass(es), 224 fail(s), and 143 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.module-hook.11.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new5.41 KB
FAILED: [[SimpleTest]]: [MySQL] 50,352 pass(es), 189 fail(s), and 304 exception(s).
[ View ]

There are a few implementations of hook_schema(), which are calling into hook_schema() of another module, and the corresponding DUTB tests did not enable those modules as dependencies.

The last submitted patch, drupal8.module-hook.13.patch, failed testing.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.