From b0eda32d83f1a901a407136a54767a9ea461b000 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Thu, 31 Jul 2014 11:25:57 +0100 Subject: [PATCH] Issue #2263365 by donquixote, longwave, alexpott: Second loop in module_implements() being repeated for no reason. --- .../Drupal/Core/Extension/ModuleHandler.php | 122 +++++++++++++----- .../Module/ModuleImplementsAlterTest.php | 92 +++++++++++++ .../module_test.implementations.inc | 10 ++ .../modules/module_test/module_test.module | 20 +++ 4 files changed, 214 insertions(+), 30 deletions(-) create mode 100644 core/modules/system/src/Tests/Module/ModuleImplementsAlterTest.php create mode 100644 core/modules/system/tests/modules/module_test/module_test.implementations.inc diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 0ef41c3bcca..6270c8ef780 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -10,6 +10,7 @@ namespace Drupal\Core\Extension; 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 Drupal\Core\Entity\Schema\EntitySchemaProviderInterface; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -49,6 +50,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. * @@ -510,14 +519,16 @@ class ModuleHandler implements ModuleHandlerInterface { * @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)) { $this->implementations = array(); + $this->verified = array(); if ($cache = $this->cacheBackend->get('module_implements')) { $this->implementations = $cache->data; } @@ -526,27 +537,18 @@ class ModuleHandler implements ModuleHandlerInterface { // 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; } - 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; - } + 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. + $this->cacheNeedsWriting = TRUE; } + $this->verified[$hook] = TRUE; } return $this->implementations[$hook]; } @@ -557,30 +559,90 @@ class ModuleHandler implements ModuleHandlerInterface { * @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. + * + * @throws \RuntimeException + * Exception thrown when an invalid implementation is added by + * hook_module_implements_alter(). * * @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 => $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. 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 + // 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); + // 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)) { + throw new \RuntimeException(String::format('An invalid implementation @function was added by hook_module_implements_alter()', array('@function' => $module . '_' . $hook))); + } + } } - return $this->implementations[$hook]; + return $implementations; + } + + /** + * 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; } /** diff --git a/core/modules/system/src/Tests/Module/ModuleImplementsAlterTest.php b/core/modules/system/src/Tests/Module/ModuleImplementsAlterTest.php new file mode 100644 index 00000000000..319e3be3ee3 --- /dev/null +++ b/core/modules/system/src/Tests/Module/ModuleImplementsAlterTest.php @@ -0,0 +1,92 @@ +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 non-existing + * 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'); + $this->fail('An exception should be thrown for the non-existing implementation.'); + } + catch (\RuntimeException $e) { + $this->pass('An exception should be thrown for the non-existing implementation.'); + $this->assertEqual($e->getMessage(), 'An invalid implementation module_test_unimplemented_test_hook was added by hook_module_implements_alter()'); + } + } + +} 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 00000000000..63c866ea1b1 --- /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\ModuleImplementsAlterTest::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 non-existing 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; + } +}