From 0d4726990ae1c35630a0de554f35fbd296e772de Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 20 May 2014 04:33:08 +0200 Subject: [PATCH 1/7] Clarify return value of ModuleHandler::getImplementationInfo() and ModuleHandler::buildImplementationInfo() --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 62cdd06..2d6d826 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -499,10 +499,11 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) { * @param string $hook * The name of the hook (e.g. "help" or "menu"). * - * @return array + * @return mixed[] * An array whose keys are the names of the modules which are implementing - * this hook and whose values are either an array of information from - * hook_hook_info() or FALSE if the implementation is in the module file. + * this hook and whose values are either a string identifying a file in + * which the implementation is to be found, or FALSE, if the implementation + * is in the module file. */ protected function getImplementationInfo($hook) { if (!isset($this->implementations)) { @@ -546,10 +547,11 @@ protected function getImplementationInfo($hook) { * @param string $hook * The name of the hook (e.g. "help" or "menu"). * - * @return array + * @return mixed[] * An array whose keys are the names of the modules which are implementing - * this hook and whose values are either an array of information from - * hook_hook_info() or FALSE if the implementation is in the module file. + * this hook and whose values are either a string identifying a file in + * which the implementation is to be found, or FALSE, if the implementation + * is in the module file. * * @see \Drupal\Core\Extension\ModuleHandler::getImplementationInfo() */ -- 1.8.5.1 From bf921ebe9fdfea73d104e9524ccf5a8bb454c01e Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 20 May 2014 04:33:57 +0200 Subject: [PATCH 2/7] Clarify the nature of ModuleHandler::, where the values are extension objects, not file names. --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 2d6d826..018adb6 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -558,7 +558,7 @@ protected function getImplementationInfo($hook) { protected function buildImplementationInfo($hook) { $this->implementations[$hook] = array(); $hook_info = $this->getHookInfo(); - foreach ($this->moduleList as $module => $filename) { + foreach ($this->moduleList as $module => $extension) { $include_file = isset($hook_info[$hook]['group']) && $this->loadInclude($module, 'inc', $module . '.' . $hook_info[$hook]['group']); // Since $this->implementsHook() may needlessly try to load the include // file again, function_exists() is used directly here. -- 1.8.5.1 From 8a79658c1f9a32dffdf3cc2e4603c827f8908ac3 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 14:54:48 +0200 Subject: [PATCH 3/7] Use local variable instead of $this->implementations[$hook] in ModuleHandler::buildImplementationInfo(). --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 018adb6..5fe9be4 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -556,22 +556,22 @@ protected function getImplementationInfo($hook) { * @see \Drupal\Core\Extension\ModuleHandler::getImplementationInfo() */ protected function buildImplementationInfo($hook) { - $this->implementations[$hook] = array(); + $implementations = array(); $hook_info = $this->getHookInfo(); foreach ($this->moduleList as $module => $extension) { $include_file = isset($hook_info[$hook]['group']) && $this->loadInclude($module, 'inc', $module . '.' . $hook_info[$hook]['group']); // Since $this->implementsHook() may needlessly try to load the include // file again, function_exists() is used directly here. if (function_exists($module . '_' . $hook)) { - $this->implementations[$hook][$module] = $include_file ? $hook_info[$hook]['group'] : FALSE; + $implementations[$module] = $include_file ? $hook_info[$hook]['group'] : FALSE; } } // Allow modules to change the weight of specific implementations but avoid // an infinite loop. if ($hook != 'module_implements_alter') { - $this->alter('module_implements', $this->implementations[$hook], $hook); + $this->alter('module_implements', $implementations, $hook); } - return $this->implementations[$hook]; + return $implementations; } /** -- 1.8.5.1 From e2949dbe0f3ab5c37b00f2b8bc1bc5346bd01ccb Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:21:31 +0200 Subject: [PATCH 4/7] Factor verifyImplementations() out of getImplementationInfo() in ModuleHandler. --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 60 +++++++++++++++++------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 5fe9be4..86531bf 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -519,23 +519,10 @@ protected function getImplementationInfo($hook) { $this->implementations[$hook] = $this->buildImplementationInfo($hook); } else { - foreach ($this->implementations[$hook] as $module => $group) { - // If this hook implementation is stored in a lazy-loaded file, include - // that file first. - if ($group) { - $this->loadInclude($module, 'inc', "$module.$group"); - } - // It is possible that a module removed a hook implementation without - // the implementations cache being rebuilt yet, so we check whether the - // function exists on each request to avoid undefined function errors. - // Since ModuleHandler::implementsHook() may needlessly try to - // load the include file again, function_exists() is used directly here. - if (!function_exists($module . '_' . $hook)) { - // Clear out the stale implementation from the cache and force a cache - // refresh to forget about no longer existing hook implementations. - unset($this->implementations[$hook][$module]); - $this->cacheNeedsWriting = TRUE; - } + if (!$this->verifyImplementations($this->implementations[$hook], $hook)) { + // One or more of the implementations did not exist and need to be + // removed in the cache. + $this->cacheNeedsWriting = TRUE; } } return $this->implementations[$hook]; @@ -575,6 +562,45 @@ protected function buildImplementationInfo($hook) { } /** + * Verifies an array of implementations loaded from the cache, by including + * the lazy-loaded $module.$group.inc, and checking function_exists(). + * + * @param string[] $implementations + * Implementation "group" by module name. + * @param string $hook + * The hook name. + * + * @return bool + * TRUE, if all implementations exist. + * FALSE, if one or more implementations don't exist and need to be removed + * from the cache. + */ + protected function verifyImplementations(&$implementations, $hook) { + $all_valid = TRUE; + foreach ($implementations as $module => $group) { + // If this hook implementation is stored in a lazy-loaded file, include + // that file first. + if ($group) { + $this->loadInclude($module, 'inc', "$module.$group"); + } + // It is possible that a module removed a hook implementation without + // the implementations cache being rebuilt yet, so we check whether the + // function exists on each request to avoid undefined function errors. + // Since ModuleHandler::implementsHook() may needlessly try to + // load the include file again, function_exists() is used directly here. + if (!function_exists($module . '_' . $hook)) { + // Clear out the stale implementation from the cache and force a cache + // refresh to forget about no longer existing hook implementations. + unset($implementations[$module]); + // One of the implementations did not exist and needs to be removed in + // the cache. + $all_valid = FALSE; + } + } + return $all_valid; + } + + /** * Parses a dependency for comparison by drupal_check_incompatibility(). * * @param $dependency -- 1.8.5.1 From cf401f271d537624e623feb7bd203bc819b515c3 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:01:03 +0200 Subject: [PATCH 5/7] Add ModuleHandler->$verified. --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 86531bf..388062e 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -49,6 +49,14 @@ class ModuleHandler implements ModuleHandlerInterface { protected $implementations; /** + * List of hooks where the implementations have been "verified". + * + * @var true[] + * Associative array where keys are hook names. + */ + protected $verified; + + /** * Information returned by hook_hook_info() implementations. * * @var array @@ -508,6 +516,7 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) { protected function getImplementationInfo($hook) { if (!isset($this->implementations)) { $this->implementations = array(); + $this->verified = array(); if ($cache = $this->cacheBackend->get('module_implements')) { $this->implementations = $cache->data; } @@ -518,12 +527,13 @@ protected function getImplementationInfo($hook) { $this->cacheNeedsWriting = TRUE; $this->implementations[$hook] = $this->buildImplementationInfo($hook); } - else { + if (!isset($this->verified[$hook])) { if (!$this->verifyImplementations($this->implementations[$hook], $hook)) { // One or more of the implementations did not exist and need to be // removed in the cache. $this->cacheNeedsWriting = TRUE; } + $this->verified[$hook] = TRUE; } return $this->implementations[$hook]; } -- 1.8.5.1 From d73a2cd6dc1b64579c30787e6221f88ca9d3a359 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:46:34 +0200 Subject: [PATCH 6/7] ModuleHandler::buildImplementationInfo() gets a separate verification logic with array_diff_assoc(). Throw an exception, if hook_module_implements_alter() has added a nonexisting implementation. --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 27 ++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 388062e..95cf8d4 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -10,6 +10,7 @@ use Drupal\Component\Graph\Graph; use Drupal\Component\Serialization\Yaml; use Drupal\Component\Utility\NestedArray; +use Drupal\Component\Utility\String; use Drupal\Core\Cache\CacheBackendInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -525,9 +526,12 @@ protected function getImplementationInfo($hook) { // The hook is not cached, so ensure that whether or not it has // implementations, the cache is updated at the end of the request. $this->cacheNeedsWriting = TRUE; + // Discover implementations. $this->implementations[$hook] = $this->buildImplementationInfo($hook); + // Implementations are always "verified" as part of the discovery. + $this->verified[$hook] = TRUE; } - if (!isset($this->verified[$hook])) { + elseif (!isset($this->verified[$hook])) { if (!$this->verifyImplementations($this->implementations[$hook], $hook)) { // One or more of the implementations did not exist and need to be // removed in the cache. @@ -550,6 +554,8 @@ protected function getImplementationInfo($hook) { * which the implementation is to be found, or FALSE, if the implementation * is in the module file. * + * @throws \Exception + * * @see \Drupal\Core\Extension\ModuleHandler::getImplementationInfo() */ protected function buildImplementationInfo($hook) { @@ -563,10 +569,27 @@ protected function buildImplementationInfo($hook) { $implementations[$module] = $include_file ? $hook_info[$hook]['group'] : FALSE; } } - // Allow modules to change the weight of specific implementations but avoid + // Allow modules to change the weight of specific implementations, but avoid // an infinite loop. if ($hook != 'module_implements_alter') { + // Remember the original implementations, before they are modified with + // hook_module_implements_alter(). + $implementations_before = $implementations; + // Verify implementations that were added or modified. $this->alter('module_implements', $implementations, $hook); + // Verify new or modified implementations. + foreach (array_diff_assoc($implementations, $implementations_before) as $module => $group) { + // If drupal_alter('module_implements') changed or added a $group, the + // respective file needs to be included. + if ($group) { + $this->loadInclude($module, 'inc', "$module.$group"); + } + // If a new implementation was added, verify that the function exists. + if (!function_exists($module . '_' . $hook)) { + $function = String::checkPlain($module . '_' . $hook); + throw new \Exception("An invalid implementation '$function' was added by hook_module_implements_alter()"); + } + } } return $implementations; } -- 1.8.5.1 From c3025c9edac314e7ca3f4ed628fb03dd4fa5092b Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Tue, 20 May 2014 04:11:33 +0200 Subject: [PATCH 7/7] Add ModuleImplementsAlterTest. --- .../Tests/Module/ModuleImplementsAlterTest.php | 115 +++++++++++++++++++++ .../module_test/module_test.implementations.inc | 10 ++ .../tests/modules/module_test/module_test.module | 20 ++++ 3 files changed, 145 insertions(+) create mode 100644 core/modules/system/lib/Drupal/system/Tests/Module/ModuleImplementsAlterTest.php create mode 100644 core/modules/system/tests/modules/module_test/module_test.implementations.inc diff --git a/core/modules/system/lib/Drupal/system/Tests/Module/ModuleImplementsAlterTest.php b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleImplementsAlterTest.php new file mode 100644 index 0000000..6fb459e --- /dev/null +++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleImplementsAlterTest.php @@ -0,0 +1,115 @@ + 'Module implementation alteration.', + 'description' => 'Test hook_module_implements_alter().', + 'group' => 'Module', + ); + } + + /** + * Tests hook_module_implements_alter() adding an implementation. + * + * @see \Drupal\Core\Extension\ModuleHandler::buildImplementationInfo() + * @see module_test_module_implements_alter() + */ + function testModuleImplementsAlter() { + + // Get an instance of the module handler, to observe how it is going to be + // replaced. + $module_handler = \Drupal::moduleHandler(); + + $this->assertTrue($module_handler === \Drupal::moduleHandler(), + 'Module handler instance is still the same.'); + + // Install the module_test module. + \Drupal::moduleHandler()->install(array('module_test')); + + // Assert that the \Drupal::moduleHandler() instance has been replaced. + $this->assertFalse($module_handler === \Drupal::moduleHandler(), + 'The \Drupal::moduleHandler() instance has been replaced during \Drupal::moduleHandler()->install().'); + + // Assert that module_test.module is now included. + $this->assertTrue(function_exists('module_test_permission'), + 'The file module_test.module was successfully included.'); + + $this->assertTrue(array_key_exists('module_test', \Drupal::moduleHandler()->getModuleList()), + 'module_test is in the module list.'); + + $this->assertTrue(in_array('module_test', \Drupal::moduleHandler()->getImplementations('permission')), + 'module_test implements hook_permission().'); + + $this->assertTrue(in_array('module_test', \Drupal::moduleHandler()->getImplementations('module_implements_alter')), + 'module_test implements hook_module_implements_alter().'); + + // Assert that module_test.implementations.inc is not included yet. + $this->assertFalse(function_exists('module_test_altered_test_hook'), + 'The file module_test.implementations.inc is not included yet.'); + + // Trigger hook discovery for hook_altered_test_hook(). + // Assert that module_test_module_implements_alter(*, 'altered_test_hook') + // has added an implementation. + $this->assertTrue(in_array('module_test', \Drupal::moduleHandler()->getImplementations('altered_test_hook')), + 'module_test implements hook_altered_test_hook().'); + + // Assert that module_test.implementations.inc was included as part of the process. + $this->assertTrue(function_exists('module_test_altered_test_hook'), + 'The file module_test.implementations.inc was included.'); + } + + /** + * Tests what happens if hook_module_implements_alter() adds a nonexisting + * function to the implementations. + * + * @see \Drupal\Core\Extension\ModuleHandler::buildImplementationInfo() + * @see module_test_module_implements_alter() + */ + function testModuleImplementsAlterNonexistingImplementation() { + + // Install the module_test module. + \Drupal::moduleHandler()->install(array('module_test')); + + try { + // Trigger hook discovery. + \Drupal::moduleHandler()->getImplementations('unimplemented_test_hook'); + } + catch (\Exception $e) { + $this->assertEqual( + $e->getMessage(), + "An invalid implementation 'module_test_unimplemented_test_hook' was added by hook_module_implements_alter()", + 'An exception was thrown for the nonexisting implementation, and the exception message is as expected.' + ); + return; + } + $this->fail('An exception should have been thrown for the nonexisting implementation.'); + } + +} diff --git a/core/modules/system/tests/modules/module_test/module_test.implementations.inc b/core/modules/system/tests/modules/module_test/module_test.implementations.inc new file mode 100644 index 0000000..63c866e --- /dev/null +++ b/core/modules/system/tests/modules/module_test/module_test.implementations.inc @@ -0,0 +1,10 @@ +set('module_test.uninstall_order', $modules); } + +/** + * Implements hook_module_implements_alter() + * + * @see module_test_altered_test_hook() + * @see \Drupal\system\Tests\Module\ModuleApiTest::testModuleImplementsAlter() + */ +function module_test_module_implements_alter(&$implementations, $hook) { + if ($hook === 'altered_test_hook') { + // Add a hook implementation, that will be found in + // module_test.implementation.inc. + $implementations['module_test'] = 'implementations'; + } + if ($hook === 'unimplemented_test_hook') { + // Add the nonexisting function module_test_unimplemented_test_hook(). This + // should cause an exception to be thrown in + // \Drupal\Core\Extension\ModuleHandler::buildImplementationInfo('unimplemented_test_hook'). + $implementations['module_test'] = FALSE; + } +} -- 1.8.5.1