Skip to content

Commit 601783f

Browse files
author
catch
committed
Issue #3491386 by nicxvan: Using hooks_converted container parameter changes $dir during hook collection breaking collection of oop hooks.
1 parent 393b244 commit 601783f

File tree

4 files changed

+83
-60
lines changed

4 files changed

+83
-60
lines changed

‎lib/Drupal/Core/Hook/HookCollectorPass.php

+52-59
Original file line numberDiff line numberDiff line change
@@ -168,74 +168,67 @@ protected function collectModuleHookImplementations($dir, $module, $module_preg,
168168
$hook_file_cache = FileCacheFactory::get('hook_implementations');
169169
$procedural_hook_file_cache = FileCacheFactory::get('procedural_hook_implementations:' . $module_preg);
170170

171-
// Check only hook classes.
172-
if ($skip_procedural) {
173-
$dir = $dir . '/src/Hook';
174-
}
175-
176-
if (is_dir($dir)) {
177-
$iterator = new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::UNIX_PATHS | \FilesystemIterator::FOLLOW_SYMLINKS);
178-
$iterator = new \RecursiveCallbackFilterIterator($iterator, static::filterIterator(...));
179-
$iterator = new \RecursiveIteratorIterator($iterator);
180-
/** @var \RecursiveDirectoryIterator | \RecursiveIteratorIterator $iterator*/
181-
foreach ($iterator as $fileinfo) {
182-
assert($fileinfo instanceof \SplFileInfo);
183-
$extension = $fileinfo->getExtension();
184-
$filename = $fileinfo->getPathname();
171+
$iterator = new \RecursiveDirectoryIterator($dir, \FilesystemIterator::SKIP_DOTS | \FilesystemIterator::UNIX_PATHS | \FilesystemIterator::FOLLOW_SYMLINKS);
172+
$iterator = new \RecursiveCallbackFilterIterator($iterator, static::filterIterator(...));
173+
$iterator = new \RecursiveIteratorIterator($iterator);
174+
/** @var \RecursiveDirectoryIterator | \RecursiveIteratorIterator $iterator*/
175+
foreach ($iterator as $fileinfo) {
176+
assert($fileinfo instanceof \SplFileInfo);
177+
$extension = $fileinfo->getExtension();
178+
$filename = $fileinfo->getPathname();
185179

186-
if (($extension === 'module' || $extension === 'profile') && !$iterator->getDepth() && !$skip_procedural) {
187-
// There is an expectation for all modules and profiles to be loaded.
188-
// .module and .profile files are not supposed to be in subdirectories.
189-
190-
include_once $filename;
180+
if (($extension === 'module' || $extension === 'profile') && !$iterator->getDepth() && !$skip_procedural) {
181+
// There is an expectation for all modules and profiles to be loaded.
182+
// .module and .profile files are not supposed to be in subdirectories.
183+
// These need to be loaded even if the module has no procedural hooks.
184+
include_once $filename;
185+
}
186+
if ($extension === 'php') {
187+
$cached = $hook_file_cache->get($filename);
188+
if ($cached) {
189+
$class = $cached['class'];
190+
$attributes = $cached['attributes'];
191191
}
192-
if ($extension === 'php') {
193-
$cached = $hook_file_cache->get($filename);
194-
if ($cached) {
195-
$class = $cached['class'];
196-
$attributes = $cached['attributes'];
192+
else {
193+
$namespace = preg_replace('#^src/#', "Drupal/$module/", $iterator->getSubPath());
194+
$class = $namespace . '/' . $fileinfo->getBasename('.php');
195+
$class = str_replace('/', '\\', $class);
196+
if (class_exists($class)) {
197+
$attributes = static::getHookAttributesInClass($class);
198+
$hook_file_cache->set($filename, ['class' => $class, 'attributes' => $attributes]);
197199
}
198200
else {
199-
$namespace = preg_replace('#^src/#', "Drupal/$module/", $iterator->getSubPath());
200-
$class = $namespace . '/' . $fileinfo->getBasename('.php');
201-
$class = str_replace('/', '\\', $class);
202-
if (class_exists($class)) {
203-
$attributes = static::getHookAttributesInClass($class);
204-
$hook_file_cache->set($filename, ['class' => $class, 'attributes' => $attributes]);
205-
}
206-
else {
207-
$attributes = [];
208-
}
209-
}
210-
foreach ($attributes as $attribute) {
211-
$this->addFromAttribute($attribute, $class, $module);
201+
$attributes = [];
212202
}
213203
}
214-
elseif (!$skip_procedural) {
215-
$implementations = $procedural_hook_file_cache->get($filename);
216-
if ($implementations === NULL) {
217-
$finder = MockFileFinder::create($filename);
218-
$parser = new StaticReflectionParser('', $finder);
219-
$implementations = [];
220-
foreach ($parser->getMethodAttributes() as $function => $attributes) {
221-
if (StaticReflectionParser::hasAttribute($attributes, StopProceduralHookScan::class)) {
222-
break;
223-
}
224-
if (!StaticReflectionParser::hasAttribute($attributes, LegacyHook::class) && preg_match($module_preg, $function, $matches)) {
225-
$implementations[] = ['function' => $function, 'module' => $matches['module'], 'hook' => $matches['hook']];
226-
}
204+
foreach ($attributes as $attribute) {
205+
$this->addFromAttribute($attribute, $class, $module);
206+
}
207+
}
208+
elseif (!$skip_procedural) {
209+
$implementations = $procedural_hook_file_cache->get($filename);
210+
if ($implementations === NULL) {
211+
$finder = MockFileFinder::create($filename);
212+
$parser = new StaticReflectionParser('', $finder);
213+
$implementations = [];
214+
foreach ($parser->getMethodAttributes() as $function => $attributes) {
215+
if (StaticReflectionParser::hasAttribute($attributes, StopProceduralHookScan::class)) {
216+
break;
217+
}
218+
if (!StaticReflectionParser::hasAttribute($attributes, LegacyHook::class) && preg_match($module_preg, $function, $matches)) {
219+
$implementations[] = ['function' => $function, 'module' => $matches['module'], 'hook' => $matches['hook']];
227220
}
228-
$procedural_hook_file_cache->set($filename, $implementations);
229-
}
230-
foreach ($implementations as $implementation) {
231-
$this->addProceduralImplementation($fileinfo, $implementation['hook'], $implementation['module'], $implementation['function']);
232221
}
222+
$procedural_hook_file_cache->set($filename, $implementations);
233223
}
234-
if ($extension === 'inc') {
235-
$parts = explode('.', $fileinfo->getFilename());
236-
if (count($parts) === 3 && $parts[0] === $module) {
237-
$this->groupIncludes[$parts[1]][] = $filename;
238-
}
224+
foreach ($implementations as $implementation) {
225+
$this->addProceduralImplementation($fileinfo, $implementation['hook'], $implementation['module'], $implementation['function']);
226+
}
227+
}
228+
if ($extension === 'inc') {
229+
$parts = explode('.', $fileinfo->getFilename());
230+
if (count($parts) === 3 && $parts[0] === $module) {
231+
$this->groupIncludes[$parts[1]][] = $filename;
239232
}
240233
}
241234
}

‎modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module

+5
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,8 @@ function hook_collector_skip_procedural_cache_flush(): void {
1616
// Set a global value we can check in test code.
1717
$GLOBALS['skip_procedural_all'] = 'skip_procedural_all';
1818
}
19+
20+
function hook_collector_skip_procedural_custom_function(): void {
21+
// Set a global value we can check in test code.
22+
$GLOBALS['test_run'] = 'test_run';
23+
}

‎modules/system/tests/modules/hook_collector_skip_procedural/src/Hook/SkipProceduralHooks.php

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Drupal\hook_collector_skip_procedural\Hook;
6+
7+
use Drupal\Core\Hook\Attribute\Hook;
8+
9+
/**
10+
* Hook implementations for hook_collector_skip_procedural.
11+
*/
12+
class SkipProceduralHooks {
13+
14+
/**
15+
* Implements hook_cache_flush().
16+
*/
17+
#[Hook('cache_flush')]
18+
public function cacheFlush(): void {
19+
// Set a global value we can check in test code.
20+
hook_collector_skip_procedural_custom_function();
21+
$GLOBALS['skipped_procedural_oop_cache_flush'] = 'skipped_procedural_oop_cache_flush';
22+
}
23+
24+
}

‎tests/Drupal/KernelTests/Core/Hook/HookCollectorPassTest.php

+2-1
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,13 @@ public function testProceduralHooksSkippedWhenConfigured(): void {
114114
$this->assertFalse(isset($GLOBALS['procedural_attribute_skip_has_attribute']));
115115
$this->assertFalse(isset($GLOBALS['procedural_attribute_skip_after_attribute']));
116116
$this->assertFalse(isset($GLOBALS['procedural_attribute_skip_find']));
117+
$this->assertFalse(isset($GLOBALS['skipped_procedural_oop_cache_flush']));
117118
drupal_flush_all_caches();
118119
$this->assertFalse(isset($GLOBALS['skip_procedural_all']));
119120
$this->assertFalse(isset($GLOBALS['procedural_attribute_skip_has_attribute']));
120121
$this->assertFalse(isset($GLOBALS['procedural_attribute_skip_after_attribute']));
121-
// This is the only one that should be found.
122122
$this->assertTrue(isset($GLOBALS['procedural_attribute_skip_find']));
123+
$this->assertTrue(isset($GLOBALS['skipped_procedural_oop_cache_flush']));
123124

124125
}
125126

0 commit comments

Comments
 (0)