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
FileSize
1.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
FileSize
629 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
FileSize
1.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
FileSize
5.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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.