From 756c93fb0bcbf991de629b81879d7fa374369eca Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Tue, 3 Dec 2024 10:29:23 +0000
Subject: [PATCH] Issue #3490355 by nicxvan, godotislate, catch: Add procedural
hook short circuit per module or file
---
.../Hook/Attribute/StopProceduralHookScan.php | 16 +++
.../Drupal/Core/Hook/HookCollectorPass.php | 123 ++++++++++--------
.../hook_collector_skip_procedural.info.yml | 5 +
.../hook_collector_skip_procedural.module | 18 +++
...ook_collector_skip_procedural.services.yml | 2 +
...llector_skip_procedural_attribute.info.yml | 6 +
...collector_skip_procedural_attribute.module | 42 ++++++
.../Core/Hook/HookCollectorPassTest.php | 22 ++++
8 files changed, 182 insertions(+), 52 deletions(-)
create mode 100644 core/lib/Drupal/Core/Hook/Attribute/StopProceduralHookScan.php
create mode 100644 core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.info.yml
create mode 100644 core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module
create mode 100644 core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.services.yml
create mode 100644 core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.info.yml
create mode 100644 core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.module
diff --git a/core/lib/Drupal/Core/Hook/Attribute/StopProceduralHookScan.php b/core/lib/Drupal/Core/Hook/Attribute/StopProceduralHookScan.php
new file mode 100644
index 000000000000..73f0ce6915bd
--- /dev/null
+++ b/core/lib/Drupal/Core/Hook/Attribute/StopProceduralHookScan.php
@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Hook\Attribute;
+
+/**
+ * Defines a StopProceduralHookScan attribute object.
+ *
+ * This allows contrib and core to mark when a file has no more
+ * procedural hooks.
+ */
+#[\Attribute(\Attribute::TARGET_FUNCTION)]
+class StopProceduralHookScan {
+
+}
diff --git a/core/lib/Drupal/Core/Hook/HookCollectorPass.php b/core/lib/Drupal/Core/Hook/HookCollectorPass.php
index 006ddbec0171..4fce50e085a0 100644
--- a/core/lib/Drupal/Core/Hook/HookCollectorPass.php
+++ b/core/lib/Drupal/Core/Hook/HookCollectorPass.php
@@ -10,6 +10,7 @@
use Drupal\Core\Extension\ProceduralCall;
use Drupal\Core\Hook\Attribute\Hook;
use Drupal\Core\Hook\Attribute\LegacyHook;
+use Drupal\Core\Hook\Attribute\StopProceduralHookScan;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
@@ -74,7 +75,7 @@ class HookCollectorPass implements CompilerPassInterface {
* {@inheritdoc}
*/
public function process(ContainerBuilder $container): void {
- $collector = static::collectAllHookImplementations($container->getParameter('container.modules'));
+ $collector = static::collectAllHookImplementations($container->getParameter('container.modules'), $container);
$map = [];
$container->register(ProceduralCall::class, ProceduralCall::class)
->addArgument($collector->includes);
@@ -123,6 +124,8 @@ public function process(ContainerBuilder $container): void {
* @param array $module_filenames
* An associative array. Keys are the module names, values are relevant
* info yml file path.
+ * @param Symfony\Component\DependencyInjection\ContainerBuilder|null $container
+ * The container.
*
* @return static
* A HookCollectorPass instance holding all hook implementations and
@@ -130,15 +133,18 @@ public function process(ContainerBuilder $container): void {
*
* @internal
* This method is only used by ModuleHandler.
+ *
+ * * @todo Pass only $container when ModuleHandler->add is removed https://www.drupal.org/project/drupal/issues/3481778
*/
- public static function collectAllHookImplementations(array $module_filenames): static {
+ public static function collectAllHookImplementations(array $module_filenames, ?ContainerBuilder $container = NULL): static {
$modules = array_map(fn ($x) => preg_quote($x, '/'), array_keys($module_filenames));
// Longer modules first.
usort($modules, fn($a, $b) => strlen($b) - strlen($a));
$module_preg = '/^(?<function>(?<module>' . implode('|', $modules) . ')_(?!preprocess_)(?!update_\d)(?<hook>[a-zA-Z0-9_\x80-\xff]+$))/';
$collector = new static();
foreach ($module_filenames as $module => $info) {
- $collector->collectModuleHookImplementations(dirname($info['pathname']), $module, $module_preg);
+ $skip_procedural = isset($container) ? $container->hasParameter("$module.hooks_converted") : FALSE;
+ $collector->collectModuleHookImplementations(dirname($info['pathname']), $module, $module_preg, $skip_procedural);
}
return $collector;
}
@@ -153,70 +159,83 @@ public static function collectAllHookImplementations(array $module_filenames): s
* @param $module_preg
* A regular expression matching every module, longer module names are
* matched first.
+ * @param $skip_procedural
+ * Skip the procedural check for the current module.
*
* @return void
*/
- protected function collectModuleHookImplementations($dir, $module, $module_preg): void {
+ protected function collectModuleHookImplementations($dir, $module, $module_preg, bool $skip_procedural): void {
$hook_file_cache = FileCacheFactory::get('hook_implementations');
$procedural_hook_file_cache = FileCacheFactory::get('procedural_hook_implementations:' . $module_preg);
- $iterator = new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::UNIX_PATHS | \FilesystemIterator::FOLLOW_SYMLINKS);
- $iterator = new \RecursiveCallbackFilterIterator($iterator, static::filterIterator(...));
- $iterator = new \RecursiveIteratorIterator($iterator);
- /** @var \RecursiveDirectoryIterator | \RecursiveIteratorIterator $iterator*/
- foreach ($iterator as $fileinfo) {
- assert($fileinfo instanceof \SplFileInfo);
- $extension = $fileinfo->getExtension();
- $filename = $fileinfo->getPathname();
+ // Check only hook classes.
+ if ($skip_procedural) {
+ $dir = $dir . '/src/Hook';
+ }
+
+ if (is_dir($dir)) {
+ $iterator = new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::UNIX_PATHS | \FilesystemIterator::FOLLOW_SYMLINKS);
+ $iterator = new \RecursiveCallbackFilterIterator($iterator, static::filterIterator(...));
+ $iterator = new \RecursiveIteratorIterator($iterator);
+ /** @var \RecursiveDirectoryIterator | \RecursiveIteratorIterator $iterator*/
+ foreach ($iterator as $fileinfo) {
+ assert($fileinfo instanceof \SplFileInfo);
+ $extension = $fileinfo->getExtension();
+ $filename = $fileinfo->getPathname();
- if (($extension === 'module' || $extension === 'profile') && !$iterator->getDepth()) {
- // There is an expectation for all modules and profiles to be loaded.
- // .module and .profile files are not supposed to be in subdirectories.
- include_once $filename;
- }
- if ($extension === 'php') {
- $cached = $hook_file_cache->get($filename);
- if ($cached) {
- $class = $cached['class'];
- $attributes = $cached['attributes'];
+ if (($extension === 'module' || $extension === 'profile') && !$iterator->getDepth() && !$skip_procedural) {
+ // There is an expectation for all modules and profiles to be loaded.
+ // .module and .profile files are not supposed to be in subdirectories.
+
+ include_once $filename;
}
- else {
- $namespace = preg_replace('#^src/#', "Drupal/$module/", $iterator->getSubPath());
- $class = $namespace . '/' . $fileinfo->getBasename('.php');
- $class = str_replace('/', '\\', $class);
- if (class_exists($class)) {
- $attributes = static::getHookAttributesInClass($class);
- $hook_file_cache->set($filename, ['class' => $class, 'attributes' => $attributes]);
+ if ($extension === 'php') {
+ $cached = $hook_file_cache->get($filename);
+ if ($cached) {
+ $class = $cached['class'];
+ $attributes = $cached['attributes'];
}
else {
- $attributes = [];
+ $namespace = preg_replace('#^src/#', "Drupal/$module/", $iterator->getSubPath());
+ $class = $namespace . '/' . $fileinfo->getBasename('.php');
+ $class = str_replace('/', '\\', $class);
+ if (class_exists($class)) {
+ $attributes = static::getHookAttributesInClass($class);
+ $hook_file_cache->set($filename, ['class' => $class, 'attributes' => $attributes]);
+ }
+ else {
+ $attributes = [];
+ }
+ }
+ foreach ($attributes as $attribute) {
+ $this->addFromAttribute($attribute, $class, $module);
}
}
- foreach ($attributes as $attribute) {
- $this->addFromAttribute($attribute, $class, $module);
- }
- }
- else {
- $implementations = $procedural_hook_file_cache->get($filename);
- if ($implementations === NULL) {
- $finder = MockFileFinder::create($filename);
- $parser = new StaticReflectionParser('', $finder);
- $implementations = [];
- foreach ($parser->getMethodAttributes() as $function => $attributes) {
- if (!StaticReflectionParser::hasAttribute($attributes, LegacyHook::class) && preg_match($module_preg, $function, $matches)) {
- $implementations[] = ['function' => $function, 'module' => $matches['module'], 'hook' => $matches['hook']];
+ elseif (!$skip_procedural) {
+ $implementations = $procedural_hook_file_cache->get($filename);
+ if ($implementations === NULL) {
+ $finder = MockFileFinder::create($filename);
+ $parser = new StaticReflectionParser('', $finder);
+ $implementations = [];
+ foreach ($parser->getMethodAttributes() as $function => $attributes) {
+ if (StaticReflectionParser::hasAttribute($attributes, StopProceduralHookScan::class)) {
+ break;
+ }
+ if (!StaticReflectionParser::hasAttribute($attributes, LegacyHook::class) && preg_match($module_preg, $function, $matches)) {
+ $implementations[] = ['function' => $function, 'module' => $matches['module'], 'hook' => $matches['hook']];
+ }
}
+ $procedural_hook_file_cache->set($filename, $implementations);
+ }
+ foreach ($implementations as $implementation) {
+ $this->addProceduralImplementation($fileinfo, $implementation['hook'], $implementation['module'], $implementation['function']);
}
- $procedural_hook_file_cache->set($filename, $implementations);
- }
- foreach ($implementations as $implementation) {
- $this->addProceduralImplementation($fileinfo, $implementation['hook'], $implementation['module'], $implementation['function']);
}
- }
- if ($extension === 'inc') {
- $parts = explode('.', $fileinfo->getFilename());
- if (count($parts) === 3 && $parts[0] === $module) {
- $this->groupIncludes[$parts[1]][] = $filename;
+ if ($extension === 'inc') {
+ $parts = explode('.', $fileinfo->getFilename());
+ if (count($parts) === 3 && $parts[0] === $module) {
+ $this->groupIncludes[$parts[1]][] = $filename;
+ }
}
}
}
diff --git a/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.info.yml b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.info.yml
new file mode 100644
index 000000000000..2ce2c7a886ee
--- /dev/null
+++ b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.info.yml
@@ -0,0 +1,5 @@
+name: 'Test skipping procedural for whole module'
+type: module
+description: 'Test skipping procedural for whole module.'
+package: Testing
+version: VERSION
diff --git a/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module
new file mode 100644
index 000000000000..5fe9407a0b22
--- /dev/null
+++ b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module
@@ -0,0 +1,18 @@
+<?php
+
+/**
+ * @file
+ * Implement hooks.
+ */
+
+declare(strict_types=1);
+
+/**
+ * This implements a hook but should not be picked up.
+ *
+ * We have set procedural_hooks: skip.
+ */
+function hook_collector_skip_procedural_cache_flush(): void {
+ // Set a global value we can check in test code.
+ $GLOBALS['skip_procedural_all'] = 'skip_procedural_all';
+}
diff --git a/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.services.yml b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.services.yml
new file mode 100644
index 000000000000..205fd7c2b670
--- /dev/null
+++ b/core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.services.yml
@@ -0,0 +1,2 @@
+parameters:
+ hook_collector_skip_procedural.hooks_converted: true
diff --git a/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.info.yml b/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.info.yml
new file mode 100644
index 000000000000..9bac70877910
--- /dev/null
+++ b/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.info.yml
@@ -0,0 +1,6 @@
+name: 'Test skipping procedural for part of the module'
+type: module
+description: 'Test skipping procedural for part of the module.'
+package: Testing
+version: VERSION
+procedural_hooks: scan
diff --git a/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.module b/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.module
new file mode 100644
index 000000000000..e7b4bf3f6829
--- /dev/null
+++ b/core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.module
@@ -0,0 +1,42 @@
+<?php
+
+/**
+ * @file
+ * Implement hooks.
+ */
+
+declare(strict_types=1);
+
+use Drupal\Core\Hook\Attribute\StopProceduralHookScan;
+
+/**
+ * This implements a hook and should be picked up.
+ *
+ * We have set procedural_hooks: scan.
+ */
+function hook_collector_skip_procedural_attribute_cache_flush(): void {
+ // Set a global value we can check in test code.
+ $GLOBALS['procedural_attribute_skip_find'] = 'procedural_attribute_skip_find';
+}
+
+/**
+ * This implements a hook but should not be picked up.
+ *
+ * This attribute should stop all procedural hooks after.
+ * We implement on behalf of other modules so we can pick them up.
+ */
+#[StopProceduralHookScan]
+function hook_collector_on_behalf_procedural_cache_flush(): void {
+ // Set a global value we can check in test code.
+ $GLOBALS['procedural_attribute_skip_has_attribute'] = 'procedural_attribute_skip_has_attribute';
+}
+
+/**
+ * This implements a hook but should not be picked up.
+ *
+ * The attribute above should prevent this from being found.
+ */
+function hook_collector_on_behalf_cache_flush(): void {
+ // Set a global value we can check in test code.
+ $GLOBALS['procedural_attribute_skip_after_attribute'] = 'procedural_attribute_skip_after_attribute';
+}
diff --git a/core/tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php b/core/tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php
index e05ca8ab6019..82e8734e44da 100644
--- a/core/tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php
@@ -101,4 +101,26 @@ public function testHooksImplementedOnBehalfFileCache(): void {
$this->assertTrue(isset($GLOBALS['on_behalf_procedural']));
}
+ /**
+ * Test procedural hooks for a module are skipped when skip is set..
+ */
+ public function testProceduralHooksSkippedWhenConfigured(): void {
+ $module_installer = $this->container->get('module_installer');
+ $this->assertTrue($module_installer->install(['hook_collector_skip_procedural']));
+ $this->assertTrue($module_installer->install(['hook_collector_on_behalf_procedural']));
+ $this->assertTrue($module_installer->install(['hook_collector_skip_procedural_attribute']));
+ $this->assertTrue($module_installer->install(['hook_collector_on_behalf']));
+ $this->assertFalse(isset($GLOBALS['skip_procedural_all']));
+ $this->assertFalse(isset($GLOBALS['procedural_attribute_skip_has_attribute']));
+ $this->assertFalse(isset($GLOBALS['procedural_attribute_skip_after_attribute']));
+ $this->assertFalse(isset($GLOBALS['procedural_attribute_skip_find']));
+ drupal_flush_all_caches();
+ $this->assertFalse(isset($GLOBALS['skip_procedural_all']));
+ $this->assertFalse(isset($GLOBALS['procedural_attribute_skip_has_attribute']));
+ $this->assertFalse(isset($GLOBALS['procedural_attribute_skip_after_attribute']));
+ // This is the only one that should be found.
+ $this->assertTrue(isset($GLOBALS['procedural_attribute_skip_find']));
+
+ }
+
}
--
GitLab