diff --git a/CLAUDE.md b/CLAUDE.md index 9d27836..8bd41e8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -96,6 +96,41 @@ class Boot Scaffold new modules with artisan: `make:mod`, `make:website`, `make:plug`. +**Modules inside this package or the consuming app** are found by `ModuleScanner`, +which walks the configured `core.module_paths` plus this package's own `src/Core` +and `src/Mod`. Declaring `$listens` is enough. + +**Modules in any other package are not scanned by default.** Their path can be +added to `core.module_paths` — the scanner reads each `Boot.php`'s declared +namespace, so a package laid out any way at all resolves correctly once its path +is configured. But that puts the burden on every consumer to know about the +package, so a package should register itself instead: + +```php +class Boot extends ServiceProvider +{ + public static array $listens = [ + AdminPanelBooting::class => 'onAdmin', + ]; + + public function register(): void + { + $this->app->make(ModuleRegistry::class)->registerClass(static::class); + } +} +``` + +A `$listens` array on a class nothing scans is dead code that reads as live: the +handlers are declared, never called, and nothing reports a problem — the feature +is simply absent. `registerClass()` takes the name from `static::class`, so no +directory convention has to be true for it to work. + +`registerClass()` is preferred over configuring a path because it needs nothing +from the consumer: the package declares its own participation, and a consumer +that merely installs it gets working behaviour. Configuring `core.module_paths` +works, but it means every application must be told about every package, and a +package that is installed and not configured looks installed and does nothing. + ### Namespace Mapping | Path | Namespace | diff --git a/src/Core/LifecycleEventProvider.php b/src/Core/LifecycleEventProvider.php index 23f993f..f9d213b 100644 --- a/src/Core/LifecycleEventProvider.php +++ b/src/Core/LifecycleEventProvider.php @@ -275,10 +275,27 @@ protected static function processViews(LifecycleEvent $event): void /** * Register Livewire components collected by a lifecycle event. + * + * Installed and wired are different questions, and only the second one + * matters here. `class_exists(Livewire::class)` is true the moment the + * package is in vendor/, which says nothing about whether its service + * provider has run — and registering a component resolves Livewire's own + * services out of the container, so on an application that ships Livewire + * without booting it the call throws BindingResolutionException rather than + * doing nothing. + * + * The container check is deliberately the facade's own binding rather than + * any particular internal service, so it does not have to be revisited when + * Livewire moves those around between versions. It confirms the registration + * path is reachable; it is not a claim that Livewire has finished booting. + * + * A module asking to register a component in an application that has no + * Livewire should be a no-op, the same way {@see processViews} skips a view + * path that is not there. */ protected static function processLivewire(LifecycleEvent $event): void { - if (! class_exists(Livewire::class)) { + if (! class_exists(Livewire::class) || ! app()->bound('livewire')) { return; } diff --git a/src/Core/ModuleRegistry.php b/src/Core/ModuleRegistry.php index b740eeb..c3ea01d 100644 --- a/src/Core/ModuleRegistry.php +++ b/src/Core/ModuleRegistry.php @@ -106,12 +106,18 @@ public function register(array $paths): void return; } - $this->mappings = $this->scanner->scan($paths); - - foreach ($this->mappings as $event => $listeners) { - $sorted = $this->sortByPriority($listeners); + // Merged into what is already here, not assigned over it. A package that + // calls registerClass() from its own provider may well do so before this + // runs — provider order is Laravel's to decide — and assigning threw that + // record away, so the guard below could not see it. The class was then + // wired a second time and its handler ran twice on every event. + foreach ($this->scanner->scan($paths) as $event => $listeners) { + foreach ($this->sortByPriority($listeners) as $moduleClass => $config) { + if (isset($this->mappings[$event][$moduleClass])) { + continue; + } - foreach ($sorted as $moduleClass => $config) { + $this->mappings[$event][$moduleClass] = $config; Event::listen($event, new LazyModuleListener($moduleClass, $config['method'])); } } @@ -215,4 +221,62 @@ public function addPaths(array $paths): void } } } + + /** + * Register one Boot class's `$listens` by name, without scanning for it. + * + * This is how a package outside the scanned tree takes part in lifecycle + * events — which in practice means every package in vendor/. + * + * // in the package's Boot::register() + * $this->app->make(ModuleRegistry::class)->registerClass(static::class); + * + * ## Why by name rather than by path + * + * Scanning has to work out a class name from a directory, and a package may + * lay itself out however it likes: php-uptelligence puts `Core\Mod\Uptelligence` + * at its package root, php-commerce keeps `Core\Service\Commerce` under + * `Service/`, php-admin has `Core\Mod\Hub` under `src/Mod/Hub`. No directory + * convention describes all of those, and one that guesses wrong does not + * fail loudly — it produces a name that does not exist, `class_exists()` + * returns false, and the module is skipped in silence. + * + * `static::class` is not a guess. The Boot class already knows what it is + * called, and every one of these packages is already a ServiceProvider that + * Laravel has constructed, so there is a moment where the name is simply + * available. That is the moment to use it. + * + * ## The trap this exists to close + * + * A `$listens` array on a class nothing scans is dead code that reads as + * live. It declares handlers, they are never called, and nothing anywhere + * reports a problem — the feature is just quietly absent. If you are writing + * a package with a `$listens` array, call this; declaring the array is not + * enough on its own. + * + * ## Ordering + * + * Priorities order listeners registered together in one pass. A module that + * registers itself is appended when its provider runs, so its priority + * orders it against others registered in the same call and not against the + * scanned tree. If two modules must run in a fixed order relative to each + * other, that is a reason for them to be scanned together rather than to + * lean on this. + * + * Registering the same class twice is a no-op, so a package that both is + * scanned and calls this does not get its handlers run twice. + * + * @param class-string $class The Boot class to register + */ + public function registerClass(string $class): void + { + foreach ($this->scanner->extractListens($class) as $event => $config) { + if (isset($this->mappings[$event][$class])) { + continue; + } + + $this->mappings[$event][$class] = $config; + Event::listen($event, new LazyModuleListener($class, $config['method'])); + } + } } diff --git a/src/Core/ModuleScanner.php b/src/Core/ModuleScanner.php index 9414f16..43cbdca 100644 --- a/src/Core/ModuleScanner.php +++ b/src/Core/ModuleScanner.php @@ -46,11 +46,12 @@ * * ## Namespace Detection * - * The scanner automatically determines namespaces based on path: - * - `/Core` paths map to `Core\` namespace - * - `/Mod` paths map to `Mod\` namespace - * - `/Website` paths map to `Website\` namespace - * - `/Plug` paths map to `Plug\` namespace + * The class name is read out of the file's own `namespace` declaration, so a + * package may lay itself out however it likes and still be found. Only a + * Boot.php that declares no namespace falls back to the directory convention + * (`/Core` → `Core\`, `/Mod` → `Mod\`, `/Website` → `Website\`, `/Plug` → `Plug\`). + * + * {@see classFromFile} says why the convention stopped being the primary rule. * * ## Usage Example * @@ -170,9 +171,68 @@ private function normalizeListens(array $listens): array } /** - * Derive fully qualified class name from file path. + * Determine the fully qualified class name a Boot.php file declares. + * + * Read out of the file, not guessed from its path. The file says which + * namespace it is in; nothing else has to agree with it. + * + * It used to be derived from the path — `/Core` meant `Core\`, `/Mod` meant + * `Mod\` — and that is a convention rather than a fact. It held for a + * consuming application laid out the way the scaffolding lays one out, and + * broke everywhere else, including in this framework: `src/Mod/Trees/Boot.php` + * declares `Core\Mod\Trees` and the path rule produced `Mod\Trees\Boot`. + * + * That failure was not a miss. In an application that happens to own a + * module of the same name — host.uk.com has an `app/Mod/Trees` — the wrong + * name *resolves*, and the scanner wires the consumer's unrelated class in + * place of this one, silently, for a Boot file it read from vendor. A rule + * that can attribute one package's file to another package's class is not a + * rule worth keeping. * - * Maps file paths to PSR-4 namespaces based on directory structure: + * The path convention survives only as a fallback for a Boot.php with no + * namespace declaration at all. + * + * @param string $file Absolute path to the Boot.php file + * @param string $basePath Base directory path (e.g., app_path('Mod')) + * @return string|null Fully qualified class name, or null if it cannot be determined + */ + private function classFromFile(string $file, string $basePath): ?string + { + $declared = $this->namespaceFromSource($file); + + if ($declared !== null) { + return $declared.'\\'.basename($file, '.php'); + } + + return $this->classFromPath($file, $basePath); + } + + /** + * Read the namespace a file declares, without loading it. + * + * Only the head of the file is read: a namespace declaration is required to + * be the first statement, so 8KB is more than enough, and this runs for + * every Boot.php on every request. + * + * @return string|null the declared namespace, or null for the global one + */ + private function namespaceFromSource(string $file): ?string + { + $head = @file_get_contents($file, false, null, 0, 8192); + + if ($head === false) { + return null; + } + + return preg_match('/^\s*namespace\s+([A-Za-z0-9_\x80-\xff\\\\]+)\s*;/m', $head, $matches) === 1 + ? $matches[1] + : null; + } + + /** + * The old path-derived name, kept for a Boot.php that declares no namespace. + * + * Maps file paths to namespaces by directory convention: * * - `app/Mod/Commerce/Boot.php` becomes `Mod\Commerce\Boot` * - `app/Core/Cdn/Boot.php` becomes `Core\Cdn\Boot` @@ -183,7 +243,7 @@ private function normalizeListens(array $listens): array * @param string $basePath Base directory path (e.g., app_path('Mod')) * @return string|null Fully qualified class name, or null if path doesn't match expected structure */ - private function classFromFile(string $file, string $basePath): ?string + private function classFromPath(string $file, string $basePath): ?string { // Normalise paths $file = str_replace('\\', '/', realpath($file) ?: $file); diff --git a/tests/Feature/ModuleRegistryTest.php b/tests/Feature/ModuleRegistryTest.php index 7dad549..eef4dfa 100644 --- a/tests/Feature/ModuleRegistryTest.php +++ b/tests/Feature/ModuleRegistryTest.php @@ -135,4 +135,103 @@ public function test_register_fires_events_to_listeners(): void // The Example module registers views $this->assertNotEmpty($event->viewRequests()); } + + /** + * The vendor registration path: a class registers itself by name. + * + * No path, no directory convention, no guess — the Boot class already knows + * what it is called. + */ + public function test_register_class_wires_a_boot_class_by_name(): void + { + $registry = new ModuleRegistry(new ModuleScanner()); + + $registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class); + + $this->assertContains( + \Core\Tests\Fixtures\Mod\Displaced\Boot::class, + $registry->getModules(), + ); + $this->assertArrayHasKey( + \Core\Tests\Fixtures\Mod\Displaced\Boot::class, + $registry->getListenersFor(WebRoutesRegistering::class), + ); + } + + /** + * The handler actually runs when the event fires — registering is not the + * same claim as being called, which is the whole reason this method exists. + */ + public function test_register_class_listener_runs_when_the_event_fires(): void + { + $registry = new ModuleRegistry(new ModuleScanner()); + $registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class); + + $event = new WebRoutesRegistering(); + Event::dispatch($event); + + $namespaces = array_map(fn (array $request): string => $request[0], $event->viewRequests()); + $this->assertContains('displaced', $namespaces); + } + + /** + * A package that is both scanned and self-registering must not run twice. + */ + public function test_register_class_is_idempotent(): void + { + $registry = new ModuleRegistry(new ModuleScanner()); + + $registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class); + $registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class); + + $event = new WebRoutesRegistering(); + Event::dispatch($event); + + $displaced = array_filter( + $event->viewRequests(), + fn (array $request): bool => $request[0] === 'displaced', + ); + + $this->assertCount(1, $displaced, 'the handler ran more than once'); + } + + /** + * A class with no $listens registers nothing rather than erroring. + */ + public function test_register_class_ignores_a_class_without_listens(): void + { + $registry = new ModuleRegistry(new ModuleScanner()); + + $registry->registerClass(\Mod\NoListens\Boot::class); + + $this->assertSame([], $registry->getModules()); + } + + /** + * A self-registered class must not be registered a second time by a later scan. + * + * register() replaced $mappings wholesale, which threw away the record that + * registerClass() had already wired a class — so the idempotency guard could + * not see it, and a class that both self-registers and is scanned got two + * listeners and ran its handler twice. The order is not hypothetical: a + * package provider's register() runs whenever Laravel gets to it, which may + * be before the framework's own scan. + */ + public function test_register_does_not_duplicate_a_self_registered_class(): void + { + $registry = new ModuleRegistry(new ModuleScanner()); + + $registry->registerClass(\Core\Tests\Fixtures\Mod\Displaced\Boot::class); + $registry->register([__DIR__.'/../Fixtures/Mod']); + + $event = new WebRoutesRegistering(); + Event::dispatch($event); + + $displaced = array_filter( + $event->viewRequests(), + fn (array $request): bool => $request[0] === 'displaced', + ); + + $this->assertCount(1, $displaced, 'the handler ran more than once'); + } } diff --git a/tests/Feature/ModuleScannerTest.php b/tests/Feature/ModuleScannerTest.php index 0f5ad56..71c6e68 100644 --- a/tests/Feature/ModuleScannerTest.php +++ b/tests/Feature/ModuleScannerTest.php @@ -231,4 +231,57 @@ public function test_scan_aggregates_multiple_paths(): void // Should have multiple listeners for WebRoutesRegistering $this->assertGreaterThanOrEqual(2, count($result[WebRoutesRegistering::class])); } + + /** + * The class name comes from the file, not from the directory above it. + * + * A Boot.php under a /Mod path that declares something else entirely is not + * exotic — this framework's own src/Mod/Trees declares Core\Mod\Trees, and + * php-commerce keeps Core\Service\Commerce under Service/. + */ + public function test_scan_reads_the_declared_namespace_not_the_path(): void + { + $modules = $this->scannedClasses([__DIR__.'/../Fixtures/Mod']); + + $this->assertContains( + \Core\Tests\Fixtures\Mod\Displaced\Boot::class, + $modules, + 'the fixture declares its namespace and the scanner must use it', + ); + $this->assertNotContains('Mod\Displaced\Boot', $modules); + } + + /** + * The regression this replaces a convention for. + * + * src/Mod/Trees/Boot.php declares Core\Mod\Trees. The old path rule derived + * Mod\Trees\Boot — a name this package does not own. In a consuming + * application that happens to have its own app/Mod/Trees, that name + * *resolves*, so the scanner wired the consumer's unrelated class for a Boot + * file it read out of vendor. Nothing failed. The wrong code ran. + */ + public function test_scan_does_not_attribute_framework_boot_files_to_consumer_classes(): void + { + $modules = $this->scannedClasses([__DIR__.'/../../src/Mod']); + + $this->assertContains(\Core\Mod\Trees\Boot::class, $modules); + $this->assertNotContains('Mod\Trees\Boot', $modules); + } + + /** + * @param array $paths + * @return array + */ + private function scannedClasses(array $paths): array + { + $classes = []; + + foreach ((new ModuleScanner())->scan($paths) as $listeners) { + foreach (array_keys($listeners) as $class) { + $classes[$class] = true; + } + } + + return array_keys($classes); + } } diff --git a/tests/Fixtures/Mod/Displaced/Boot.php b/tests/Fixtures/Mod/Displaced/Boot.php new file mode 100644 index 0000000..a5dfd89 --- /dev/null +++ b/tests/Fixtures/Mod/Displaced/Boot.php @@ -0,0 +1,30 @@ + 'onWebRoutes', + ]; + + public function onWebRoutes(WebRoutesRegistering $event): void + { + $event->views('displaced', __DIR__.'/Views'); + } +}