Keep a failing module from taking the application down

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,
+                );
             }
         }
     }

backend/app/Support/ModuleProviderGuard.php

@@ -0,0 +1,73 @@
+<?php
+
+namespace App\Support;
+
+use Illuminate\Contracts\Foundation\Application;
+use Illuminate\Support\Facades\Log;
+use Illuminate\Support\ServiceProvider;
+
+/**
+ * Wraps an installed module's service provider so a failure inside it costs
+ * that module, not the whole application.
+ *
+ * ModuleManager::loadEnabled() already ran `$app->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);
+        }
+    }
+}

backend/tests/Feature/ModuleProviderGuardTest.php

@@ -0,0 +1,132 @@
+<?php
+
+namespace Tests\Feature;
+
+use App\Support\ModuleProviderGuard;
+use Illuminate\Support\Facades\Log;
+use Illuminate\Support\ServiceProvider;
+use Tests\TestCase;
+
+/**
+ * A module's provider must not be able to take the application down with it.
+ *
+ * The case this comes from: a module built against a newer core called a menu
+ * helper with an argument this version does not accept. The throw happened in
+ * boot(), which Laravel calls while booting every provider — outside the
+ * try/catch in ModuleManager::loadEnabled() — so every route answered 502,
+ * including the admin screen needed to turn the module off again.
+ */
+class ModuleProviderGuardTest extends TestCase
+{
+    public function test_an_exception_from_register_does_not_escape(): void
+    {
+        Log::spy();
+
+        $guard = new ModuleProviderGuard($this->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;
+    }
+}