From f3e63d906e390779f73b80cba36446f93ebe6d21 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 20:49:53 +0200 Subject: [PATCH] Never let a hook throw, whatever the host's code does Configuration still throws -- register() rejects a bad config array where the host can see its own call in the stack trace. Everything the library puts on a hook now catches Throwable, reports it with _doing_it_wrong() and abandons that step: the plugins_loaded step in Boot\Scheduler, and render_notices() on all_admin_notices. The load pass catches per sub-plugin as well. Past that catch is somebody else's code -- the enabled and dependency_check callables, the conflict policy and its filter, the notice messages, should_load, and the bundled file a require runs top to bottom -- and a throw from one sub-plugin took the whole site down, front end included, along with every sub-plugin behind it in the registration order. The step reporter takes the step's name and what the site got instead, because the sequence gains a second step at the priority ahead of this one and both have to report the same way. A re-declaration is the one failure none of this catches, because PHP does not raise it as a Throwable. The guard constant, checked before the require, is what has always prevented that one. --- CLAUDE.md | 13 +++++ src/Absorber.php | 14 ++++- src/Boot/Scheduler.php | 46 +++++++++++++++- src/Loader.php | 25 ++++++++- tests/_support/Traits/WithBundledPlugins.php | 25 +++++++++ tests/unit/AbsorberTest.php | 23 ++++++++ tests/unit/Boot/SchedulerTest.php | 42 ++++++++++++++ tests/unit/LoaderTest.php | 58 ++++++++++++++++++++ 8 files changed, 243 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 717cf17..933596f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -315,6 +315,19 @@ treatment. Any older sketch showing `Config::reset()` or `Absorber::reset()` mea ## Invariants — do not "simplify" these away +- **Nothing reached from a hook is allowed to throw.** Configuration still throws: `Absorber::register()` + rejects a bad config array on the spot, at a call in the developer's own stack trace, before + anything is hooked. Past that point this library is code on somebody's live site, and a white screen + is never the better answer — so every entry point it puts on a hook catches `Throwable`, reports + with `_doing_it_wrong()` and abandons that step alone: the `plugins_loaded` step in + `Boot\Scheduler`, and `Absorber::render_notices()` on `all_admin_notices`. `Loader::load_all()` + catches *per sub-plugin* as well, because one sub-plugin's throw + must not take the ones behind it in the registration order with it. Everything past those catches is + somebody else's code — `enabled`, `dependency_check`, `activation_callback`, `conflict_policy`, the + notice messages, the `should_load` filter, the bundled file a `require` runs top to bottom, and the + standalone's own deactivation hook. The one failure none of this can catch is a re-declaration + fatal, which PHP does not raise as a `Throwable`; the guard constant, checked before the require, is + what prevents that one. - **The guard constant and the standalone basename are two separate keys.** No constant does double duty as both a load guard and a path resolver. - **`get_hook_name()` and `get_option_name()` do not share a normalisation.** Folding case into the diff --git a/src/Absorber.php b/src/Absorber.php index 5870100..75f1d65 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -150,7 +150,19 @@ public static function render_notices(): void { return; } - self::notices()->render(); + // The queue is a rebindable seam and the messages inside it are host callables, so rendering + // runs somebody else's code -- on all_admin_notices, which every admin screen fires. A throw + // out of here would white-screen wp-admin, which is exactly where a site owner would go to + // undo whatever caused it. The notice is worth less than the screen it would be read on. + try { + self::notices()->render(); + } catch ( Throwable $thrown ) { + _doing_it_wrong( + self::class . '::render_notices', + sprintf( 'The notices could not be rendered: %s', $thrown->getMessage() ), + '1.0.0' + ); + } } /** diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index b483be4..297344a 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -8,6 +8,7 @@ use Nexcess\PluginAbsorber\Absorber; use Nexcess\PluginAbsorber\Loader; use StellarWP\ContainerContract\ContainerInterface; +use Throwable; use WP_Hook; /** @@ -118,12 +119,55 @@ private function sequence(): array { [ 'priority' => self::LOAD_PRIORITY, 'run' => static function () use ( $container ): void { - $container->get( Loader::class )->load_all(); + self::load( $container ); }, ], ]; } + /** + * The load step. + * + * Static, and handed the container rather than reading one, so the closure in sequence() stays a + * closure over the container. + * + * @since 1.0.0 + * + * @param ContainerInterface $container Container the load pass is resolved from. + * + * @return void + */ + private static function load( ContainerInterface $container ): void { + // plugins_loaded fires on every request a site serves, so a throw out of this step is a white + // screen on all of them. `Loader::load_all()` already reports per sub-plugin and carries on, + // so what is left for this to catch is the pass itself -- a container that cannot build it + // above all, which is the shape a host's own broken binding takes. + try { + $container->get( Loader::class )->load_all(); + } catch ( Throwable $thrown ) { + self::report_a_step_that_threw( 'load pass', 'no sub-plugin was loaded', $thrown ); + } + } + + /** + * Tell the developer which step was abandoned, and why. + * + * @since 1.0.0 + * + * @param string $step Step that threw, named as the sequence names it. + * @param string $consequence What the site got instead. + * @param Throwable $thrown What came out of the step. + * + * @return void + */ + private static function report_a_step_that_threw( string $step, string $consequence, Throwable $thrown ): void { + _doing_it_wrong( + self::class, + sprintf( 'The %s threw, so %s: %s', $step, $consequence, $thrown->getMessage() ), + '1.0.0' + ); + } + /** * Whether it is already too late to wire the load hook. * diff --git a/src/Loader.php b/src/Loader.php index 5d05101..17df35a 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -8,6 +8,7 @@ use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Throwable; /** * The load pass: every registered sub-plugin, in registration order, gated one at a time. @@ -88,7 +89,29 @@ public function load_all(): void { } foreach ( $sub_plugins as $sub_plugin ) { - $this->load( $sub_plugin ); + // Everything past this line is somebody else's code: the enabled and dependency_check + // callables, the host's should_load filter, and the bundled file itself, which a require + // runs from top to bottom. Any of it may throw, and this loop runs inside plugins_loaded + // on every request a site serves -- so an escaping throw is a white screen on the front + // end and in wp-admin alike, over one sub-plugin, and it takes every sub-plugin behind it + // in the registration order with it. The developer is told which sub-plugin, and the + // loop carries on with the next. + // + // A re-declaration is the one failure this cannot catch, because PHP does not raise it as + // a Throwable -- which is what the guard constant, checked before any of this, is for. + try { + $this->load( $sub_plugin ); + } catch ( Throwable $thrown ) { + _doing_it_wrong( + self::class, + sprintf( + 'The sub-plugin "%s" threw while loading, so it was abandoned: %s', + $sub_plugin->get_slug(), + $thrown->getMessage() + ), + '1.0.0' + ); + } } } diff --git a/tests/_support/Traits/WithBundledPlugins.php b/tests/_support/Traits/WithBundledPlugins.php index 6d96ef2..ca1b73a 100644 --- a/tests/_support/Traits/WithBundledPlugins.php +++ b/tests/_support/Traits/WithBundledPlugins.php @@ -51,6 +51,31 @@ protected function make_bundled_plugin_file( string $constant ): string { return $path; } + /** + * Write a bundled plugin that throws as it is included, the way a broken build does. + * + * It counts its load first, so a test can tell "the require never happened" from "the require + * happened and the file threw". + * + * @since 1.0.0 + * + * @return string + */ + protected function make_throwing_bundled_plugin_file(): string { + $path = sys_get_temp_dir() . '/absorber-' . uniqid( '', true ) . '.php'; + + file_put_contents( + $path, + 'bundled_plugin_files[] = $path; + + return $path; + } + /** * A guard constant name no other test can collide with. * diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 405cf9f..9343a33 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -391,6 +391,29 @@ static function () use ( $notices ): Queue_Interface { $this->assertSame( 1, $notices->render_calls ); } + /** + * The notice messages are host callables and the queue itself is a rebindable seam, so rendering + * runs host code — on `all_admin_notices`, which every admin screen fires. A throw out of it + * white-screens wp-admin, which is exactly where a site owner would go to undo whatever caused + * it, so it is reported and the render is abandoned instead. + */ + public function test_render_notices_cannot_end_the_admin_request(): void { + $this->expect_incorrect_usage(); + + $container = new Test_Container(); + $container->singleton( + Queue_Interface::class, + static function (): Queue_Interface { + throw new RuntimeException( 'the notice option held something unreadable' ); + } + ); + $this->set_up_container( $container ); + + Absorber::render_notices(); + + $this->assert_the_library_reported_incorrect_usage(); + } + public function test_render_notices_does_nothing_without_a_hook_prefix(): void { $container = $this->set_up_container(); diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index ccf13a1..21087fe 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -11,6 +11,7 @@ use Nexcess\PluginAbsorber\Absorber; use Nexcess\PluginAbsorber\Boot\Scheduler; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Loader; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; use Nexcess\PluginAbsorber\Tests\Support\Config_State; @@ -20,6 +21,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; use ReflectionClass; +use RuntimeException; use WP_Hook; /** @@ -105,6 +107,46 @@ public function test_the_load_step_runs_early_in_plugins_loaded(): void { $this->assertSame( 2, $this->load_priority() ); } + /** + * The outermost guarantee, and the reason it lives here rather than inside the pass: whatever the + * step reaches — a collaborator a host's factory could not build, a pass that got past its own + * guard — `plugins_loaded` fires on every request a site serves, and a throw out of it is a white + * screen on all of them. The step is reported and abandoned on its own. + * + * @dataProvider throwing_steps + * + * @param string $id Binding the step resolves, bound to a factory that throws. + */ + public function test_a_step_that_throws_cannot_end_the_request( string $id ): void { + $this->expect_incorrect_usage(); + + Absorber::boot(); + + // After boot, because `Provider::bind_once()` rebinds a class id whatever a host put there + // first -- it cannot tell a deliberate binding from a container's willingness to autowire the + // class. Bound before it, this factory would be replaced by the real collaborator and the step + // would run perfectly well. + $this->container()->singleton( + $id, + static function (): object { + throw new RuntimeException( 'the host factory needed a database connection' ); + } + ); + + do_action( 'plugins_loaded' ); + + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * A provider from the start, because every step the sequence gains has to answer this one. + * + * @return Generator + */ + public static function throwing_steps(): Generator { + yield 'the load step' => [ Loader::class ]; + } + public function test_it_wires_the_load_step_at_the_load_priority(): void { $this->register_sub_plugin(); diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index fc2d112..b2a19f1 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -22,6 +22,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; +use RuntimeException; /** * The load loop and its gate chain. @@ -138,6 +139,63 @@ public function test_it_loads_the_registry_it_was_handed(): void { $this->assertTrue( defined( $constant ) ); } + /** + * A host callable is arbitrary code, and this one runs inside `plugins_loaded` on every request a + * site serves. Letting a throw out would white-screen the whole site — front end included — over + * one sub-plugin's mistake, and take every sub-plugin behind it in the registration order down + * with it. Reported to the developer, and the loop carries on. + */ + public function test_a_sub_plugin_that_throws_does_not_stop_the_others(): void { + $this->expect_incorrect_usage(); + + $this->register( + [ + 'slug' => 'give-recurring', + 'enabled' => static function (): bool { + throw new RuntimeException( 'the licence server was unreachable' ); + }, + ] + ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + $this->loader()->load_all(); + + $this->assertSame( + 1, + $this->bundled_plugin_loads(), + 'The sub-plugin behind the one that threw still has to load.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The bundled file is host code too, and a `require` of a file that throws — or one with a syntax + * error, which is a catchable ParseError on an include — must not be the end of the request. + */ + public function test_a_bundled_file_that_throws_does_not_stop_the_others(): void { + $this->expect_incorrect_usage(); + + $constant = $this->make_guard_constant(); + $path = $this->make_throwing_bundled_plugin_file(); + + Absorber::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + $this->loader()->load_all(); + + // Two: the throwing fixture counts its own load before it throws, and the sub-plugin behind + // it still loaded. One would mean the require never happened; without the guard neither + // number is ever read, because the throw ends the request. + $this->assertSame( 2, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + public function test_it_requires_the_bundled_file_exactly_once(): void { $this->register();