From d9bf4a03b7c8eadc26dca0e88462ef22e6e5d005 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 14:54:48 +0200 Subject: [PATCH 1/4] 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 62cdd06..7196da2 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -554,22 +554,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 => $filename) { $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 56ca253bef133c9b46c3c3e943da36635ba193e2 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:21:31 +0200 Subject: [PATCH 2/4] 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 7196da2..ec32b6f 100644 --- a/core/lib/Drupal/Core/Extension/ModuleHandler.php +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -518,23 +518,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]; @@ -573,6 +560,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 cbb73a788bd324db3e652f56f793488617a889d1 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:01:03 +0200 Subject: [PATCH 3/4] 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 ec32b6f..9e9088d 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 @@ -507,6 +515,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; } @@ -517,12 +526,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 55de8f24830117bc1e8601d683d796d0054238b3 Mon Sep 17 00:00:00 2001 From: Andreas Hennings Date: Fri, 16 May 2014 15:46:34 +0200 Subject: [PATCH 4/4] ModuleHandler::buildImplementationInfo() gets a separate verification logic with array_diff_assoc(). --- core/lib/Drupal/Core/Extension/ModuleHandler.php | 25 ++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/core/lib/Drupal/Core/Extension/ModuleHandler.php b/core/lib/Drupal/Core/Extension/ModuleHandler.php index 9e9088d..d0ef571 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; @@ -524,9 +525,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. @@ -561,10 +565,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