diff --git a/backend/app/Services/ModuleManager.php b/backend/app/Services/ModuleManager.php index 8f2b9875..e2e566f2 100644 --- a/backend/app/Services/ModuleManager.php +++ b/backend/app/Services/ModuleManager.php @@ -382,7 +382,18 @@ public function loadEnabled(\Illuminate\Foundation\Application $app): void $provider = $manifest['provider'] ?? null; if ($provider && class_exists($provider)) { - $app->register($provider); + // Wrapped rather than registered directly: an exception from the + // provider's boot() is thrown while Laravel boots every provider, + // long after this method returned, so the caller's try/catch + // cannot see it — and one bad module would 502 the whole app. + // + // Registered with force, because the container deduplicates by + // class name: without it the second module's guard would be + // mistaken for the first one's and silently skipped. + $app->register( + new \App\Support\ModuleProviderGuard($app, new $provider($app), $name), + force: true, + ); } } } diff --git a/backend/app/Support/ModuleProviderGuard.php b/backend/app/Support/ModuleProviderGuard.php new file mode 100644 index 00000000..3923572f --- /dev/null +++ b/backend/app/Support/ModuleProviderGuard.php @@ -0,0 +1,73 @@ +register()` inside a + * try/catch, and the comment there promised "a bad module never prevents the + * application from booting". It only half held: `register()` was covered, but + * Laravel calls a provider's `boot()` later, while booting every provider, and + * an exception thrown there escapes that catch entirely. A module built + * against a newer core — calling a menu helper with an argument this version + * does not accept, say — took the entire installation down with a 502, on + * every route, including the admin screen needed to disable it again. + * + * Both phases run here instead, each guarded. A module that throws simply does + * not register its routes, menus or listeners; every other page keeps working + * and the reason is in the log rather than in an nginx error page. + */ +class ModuleProviderGuard extends ServiceProvider +{ + public function __construct( + Application $app, + private readonly ServiceProvider $inner, + private readonly string $module, + ) { + parent::__construct($app); + } + + public function register(): void + { + $this->guard('register'); + } + + public function boot(): void + { + $this->guard('boot'); + } + + /** + * Run one phase of the wrapped provider, swallowing anything it throws. + * + * `boot()` is resolved through the container because a provider may type-hint + * its dependencies there, which is how Laravel calls it too. `register()` is + * always defined by the base class; `boot()` is not, so it is checked. + */ + private function guard(string $phase): void + { + if (! method_exists($this->inner, $phase)) { + return; + } + + try { + $this->app->call([$this->inner, $phase]); + } catch (\Throwable $e) { + Log::error("module.provider_{$phase}_failed", [ + 'module' => $this->module, + 'provider' => $this->inner::class, + 'exception' => $e->getMessage(), + ]); + + // Also to the app's error reporting, so this is visible wherever + // exceptions normally are and not only to whoever tails the log. + report($e); + } + } +} diff --git a/backend/tests/Feature/ModuleProviderGuardTest.php b/backend/tests/Feature/ModuleProviderGuardTest.php new file mode 100644 index 00000000..c0152b0d --- /dev/null +++ b/backend/tests/Feature/ModuleProviderGuardTest.php @@ -0,0 +1,132 @@ +app, new ThrowsOnRegisterProvider($this->app), 'Broken'); + + $guard->register(); + + Log::shouldHaveReceived('error')->withArgs( + fn (string $message, array $context) => $message === 'module.provider_register_failed' + && $context['module'] === 'Broken', + ); + } + + public function test_an_exception_from_boot_does_not_escape(): void + { + Log::spy(); + + $guard = new ModuleProviderGuard($this->app, new ThrowsOnBootProvider($this->app), 'Broken'); + + $guard->boot(); + + Log::shouldHaveReceived('error')->withArgs( + fn (string $message, array $context) => $message === 'module.provider_boot_failed' + && $context['module'] === 'Broken', + ); + } + + public function test_a_working_provider_still_registers_and_boots(): void + { + $inner = new WorkingProvider($this->app); + $guard = new ModuleProviderGuard($this->app, $inner, 'Working'); + + $guard->register(); + $guard->boot(); + + $this->assertTrue($inner->registered, 'register() was not passed through'); + $this->assertTrue($inner->booted, 'boot() was not passed through'); + } + + /** + * The container deduplicates providers by class name. Every module's guard + * is the same class, so without force the second module would be treated as + * already registered and never load at all. + */ + public function test_two_modules_both_load_despite_sharing_a_guard_class(): void + { + $first = new WorkingProvider($this->app); + $second = new WorkingProvider($this->app); + + $this->app->register(new ModuleProviderGuard($this->app, $first, 'First'), force: true); + $this->app->register(new ModuleProviderGuard($this->app, $second, 'Second'), force: true); + + $this->assertTrue($first->registered, 'first module did not register'); + $this->assertTrue($second->registered, 'second module was swallowed by provider deduplication'); + } + + /** A provider with no boot() at all is common and must not error. */ + public function test_a_provider_without_boot_is_fine(): void + { + $inner = new RegisterOnlyProvider($this->app); + $guard = new ModuleProviderGuard($this->app, $inner, 'NoBoot'); + + $guard->register(); + $guard->boot(); + + $this->assertTrue($inner->registered); + } +} + +class RegisterOnlyProvider extends ServiceProvider +{ + public bool $registered = false; + + public function register(): void + { + $this->registered = true; + } +} + +class ThrowsOnRegisterProvider extends ServiceProvider +{ + public function register(): void + { + throw new \RuntimeException('register blew up'); + } +} + +class ThrowsOnBootProvider extends ServiceProvider +{ + public function boot(): void + { + throw new \ArgumentCountError('Unknown named parameter $badge'); + } +} + +class WorkingProvider extends ServiceProvider +{ + public bool $registered = false; + + public bool $booted = false; + + public function register(): void + { + $this->registered = true; + } + + public function boot(): void + { + $this->booted = true; + } +}