Skip to content

Commit ce4d8bb

Browse files
committed
fix(modules): a self-registered class is no longer wired twice by a later scan
CodeRabbit found a real bug in the registerClass() I added, and it was mine. register() did `$this->mappings = $this->scanner->scan($paths)` — assignment, not merge. So a package that called registerClass() from its own provider had that record thrown away the moment the framework's scan ran, which meant the idempotency guard inside registerClass() had nothing left to see. If the scan also found the class, it wired a second listener and the handler ran twice on every event. The order is not hypothetical. A package provider's register() runs whenever Laravel gets to it, which may be before LifecycleEventProvider's — so the sequence that breaks is the ordinary one, not a corner. Proven before fixing, with the fixture registering both ways: assertCount(1, $displaced) Failed asserting that actual size 2 matches expected size 1 register() now merges into what is already there and skips a class already registered for that event — the same guard addPaths() has always had, which is where I should have looked before writing a second registration path. ./vendor/bin/pest --testsuite=Feature,Unit 267 -> 268 passed, 0 failed ./vendor/bin/pest --testsuite=Module 448 failed / 308 passed, unchanged vendor/bin/pint --test pass vendor/bin/phpstan analyse no errors Co-Authored-By: Virgil <virgil@lethean.io>
1 parent db5a6e2 commit ce4d8bb

2 files changed

Lines changed: 39 additions & 5 deletions

File tree

src/Core/ModuleRegistry.php

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -106,12 +106,18 @@ public function register(array $paths): void
106106
return;
107107
}
108108

109-
$this->mappings = $this->scanner->scan($paths);
110-
111-
foreach ($this->mappings as $event => $listeners) {
112-
$sorted = $this->sortByPriority($listeners);
109+
// Merged into what is already here, not assigned over it. A package that
110+
// calls registerClass() from its own provider may well do so before this
111+
// runs — provider order is Laravel's to decide — and assigning threw that
112+
// record away, so the guard below could not see it. The class was then
113+
// wired a second time and its handler ran twice on every event.
114+
foreach ($this->scanner->scan($paths) as $event => $listeners) {
115+
foreach ($this->sortByPriority($listeners) as $moduleClass => $config) {
116+
if (isset($this->mappings[$event][$moduleClass])) {
117+
continue;
118+
}
113119

114-
foreach ($sorted as $moduleClass => $config) {
120+
$this->mappings[$event][$moduleClass] = $config;
115121
Event::listen($event, new LazyModuleListener($moduleClass, $config['method']));
116122
}
117123
}

tests/Feature/ModuleRegistryTest.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,4 +206,32 @@ public function test_register_class_ignores_a_class_without_listens(): void
206206

207207
$this->assertSame([], $registry->getModules());
208208
}
209+
210+
/**
211+
* A self-registered class must not be registered a second time by a later scan.
212+
*
213+
* register() replaced $mappings wholesale, which threw away the record that
214+
* registerClass() had already wired a class — so the idempotency guard could
215+
* not see it, and a class that both self-registers and is scanned got two
216+
* listeners and ran its handler twice. The order is not hypothetical: a
217+
* package provider's register() runs whenever Laravel gets to it, which may
218+
* be before the framework's own scan.
219+
*/
220+
public function test_register_does_not_duplicate_a_self_registered_class(): void
221+
{
222+
$registry = new ModuleRegistry(new ModuleScanner());
223+
224+
$registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class);
225+
$registry->register([__DIR__.'/../Fixtures/Mod']);
226+
227+
$event = new WebRoutesRegistering();
228+
Event::dispatch($event);
229+
230+
$displaced = array_filter(
231+
$event->viewRequests(),
232+
fn (array $request): bool => $request[0] === 'displaced',
233+
);
234+
235+
$this->assertCount(1, $displaced, 'the handler ran more than once');
236+
}
209237
}

0 commit comments

Comments
 (0)