From 67e2f58a6523b7372dacfa754b231cc1ca575c20 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 13:57:44 +0200 Subject: [PATCH 1/4] Announce every failure this library already reports src/ made nine _doing_it_wrong() calls and no do_action() at all. Core's _doing_it_wrong() fires doing_it_wrong_run and then emits nothing unless WP_DEBUG is on, so on a production site every failure this library detects -- a typo'd bundled_plugin_file, a sub-plugin that threw during require, a conflict that could not be resolved, a duplicate slug, a boot that came too late to wire -- happened in complete silence. A host debugging "the add-on isn't there" had nothing to pull on. One action, named through Config::get_hook_name() like the filters: error( string $message, ?Sub_Plugin ). _doing_it_wrong() stays exactly as it was; it is the developer channel, and taking it away would regress every WP_DEBUG site. Traits\Reports_Errors joins the two channels in one method, so the failure this library is likeliest to add next -- a new gate -- cannot report down only one of them. It never throws whatever a listener does: the error action fires from inside handlers whose entire purpose is that nothing escapes them, and a diagnostic that could white-screen plugins_loaded would be a worse bug than the silence it replaces. A listener that throws is reported with a plain _doing_it_wrong(), never a second announcement, so one that throws every time cannot recurse. The hook prefix is what names these hooks, which makes a bootstrap that never set one the single failure the error action cannot carry. Traits\Guards_Hook_Prefix reports it through the shared method anyway: the name is built inside a try, and the case is stated in one place rather than left as a bare _doing_it_wrong() at a call site somebody has to notice. Conflict\Resolver goes over with the rest. Its per-sub-plugin catch is the one report whose silence a site owner feels directly -- a standalone left running beside the bundled copy after the pass that was supposed to deal with it -- so leaving it on the developer channel alone would have left the channel blind to the half of the library a fatal depends on. docs/actions.md rather than a heading in docs/filters.md: the two answer opposite questions -- how do I change what the library does, against how do I find out what it did. Six of the seven source files here are that swap and carry no argument of their own, which is more than the PR size cap allowed. AGENTS.md names the exception in the same commit, rather than leaving the rule and the diff disagreeing. --- AGENTS.md | 4 +- README.md | 2 + docs/actions.md | 37 ++++++ docs/filters.md | 3 +- src/Absorber.php | 12 +- src/Boot/Scheduler.php | 27 ++++- src/Conflict/Resolver.php | 10 +- src/Loader.php | 17 ++- src/Registry/Reader.php | 11 +- src/Traits/Guards_Hook_Prefix.php | 9 +- src/Traits/Reports_Errors.php | 85 +++++++++++++ tests/unit/Boot/SchedulerTest.php | 36 ++++++ tests/unit/Conflict/ResolverTest.php | 51 ++++++++ tests/unit/LoaderTest.php | 173 +++++++++++++++++++++++++++ tests/unit/Registry/ReaderTest.php | 38 ++++++ 15 files changed, 492 insertions(+), 23 deletions(-) create mode 100644 docs/actions.md create mode 100644 src/Traits/Reports_Errors.php diff --git a/AGENTS.md b/AGENTS.md index 904da34..c710c72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -611,7 +611,9 @@ Branch names are `NN-topic`. Never open PR N+1 before PR N's branch exists. `mai after every merge. - **PR size cap:** ≤10 files, tests and test infrastructure excluded. No logic-bearing PR exceeds 4 - source files. + source files — with one exception: a change that wires one decision through every site that + already does the same job may exceed it, where the added files are call-site swaps carrying no + argument of their own. Say so in the body, and say which files those are. - **Commits: no co-author trailers, ever.** - **PR body is exactly three parts, nothing else** — no boilerplate headings, no restating the diff, no checklists, and no "Verify" section: the commands are in this file and the coverage is in the diff --git a/README.md b/README.md index f99b0a5..2ad38a3 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,7 @@ two sub-plugins, every optional key. releases. - [Conflict handling][conflicts] — the policies, when they run, and the guard's limits. - [Filters][filters] — the runtime overrides for policies and notice text. +- [Actions][actions] — the failures the library announces, and what each one carries. - [Notices][notices] — where the queue lives, who may see it, and how to render it yourself. - [Extending][extending] — swapping out a piece of the library. - [Tests][tests] — running the suite, the fixtures and traits it offers, and every scenario it drives @@ -73,6 +74,7 @@ source. [recipes]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/recipes.md [conflicts]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/conflict-handling.md [filters]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/filters.md +[actions]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/actions.md [notices]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/notices.md [extending]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/extending.md [tests]: https://github.com/stellarwp/plugin-absorber/blob/main/tests/README.md diff --git a/docs/actions.md b/docs/actions.md new file mode 100644 index 0000000..8e01318 --- /dev/null +++ b/docs/actions.md @@ -0,0 +1,37 @@ +# Actions + +What the library tells you as it runs. [Filters](filters.md) are the other direction — the values +you override. `{prefix}` is the value passed to `Config::set_hook_prefix()`. + +| Action | Arguments | Fires when | +|---|---|---| +| `{prefix}/plugin_absorber/error` | `string $message`, `Sub_Plugin\|null $sub_plugin` | Something went wrong that a developer has to fix. | + +Everything announced here also goes to `_doing_it_wrong()`, which is silent unless `WP_DEBUG` is on. +This is the channel that is not — reach for it for a log line, a health check, or a support tool. + +## Errors + +`error` carries the sentence a developer needs, and the sub-plugin it belongs to when it belongs to +one — a duplicate slug, a broken bundled file, a sub-plugin whose own code threw, a conflict that +could not be resolved. It is `null` for a failure that belongs to no single registration: a boot +that came too late to wire, a pass that threw before it reached any sub-plugin, notices that could +not be rendered. + +```php +add_action( 'give/plugin_absorber/error', function ( $message, $sub_plugin ) { + error_log( 'plugin-absorber: ' . $message ); +}, 10, 2 ); +``` + +**A bootstrap with no hook prefix cannot be announced.** The prefix is what names this action, so +the one failure `error` can never carry is a missing `Config::set_hook_prefix()`. That one goes to +`_doing_it_wrong()` alone. + +## Your listener cannot take the site down + +`error` fires from inside `plugins_loaded`, and from inside the handlers that keep a failing +sub-plugin from white-screening the site, so a listener that throws is caught rather than allowed +out. A throw from it costs nothing at all and is itself reported through `_doing_it_wrong()`. That +is a backstop, not a licence — a listener here runs on every request the site serves, so keep it +cheap and keep it quiet. diff --git a/docs/filters.md b/docs/filters.md index a748871..3178dc8 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -1,6 +1,7 @@ # Filters -`{prefix}` is the value passed to `Config::set_hook_prefix()`. +What you override. [Actions](actions.md) are the other direction — what the library tells you as it +runs. `{prefix}` is the value passed to `Config::set_hook_prefix()`. | Filter | Arguments | Purpose | |---|---|---| diff --git a/src/Absorber.php b/src/Absorber.php index 60f28cc..24cedd8 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -17,6 +17,7 @@ use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Registry\Reader; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; use Throwable; /** @@ -34,6 +35,7 @@ */ final class Absorber { use Guards_Hook_Prefix; + use Reports_Errors; /** * Whether the hooks have been wired. @@ -180,10 +182,9 @@ public static function render_notices(): void { try { self::collaborator( Presenter::class )->render(); } catch ( Throwable $thrown ) { - _doing_it_wrong( + self::report_error( self::class . '::render_notices', - sprintf( 'The notices could not be rendered: %s', $thrown->getMessage() ), - '1.0.0' + sprintf( 'The notices could not be rendered: %s', $thrown->getMessage() ) ); } } @@ -215,10 +216,9 @@ public static function filter_activation_error_markup( $markup ): string { try { return self::collaborator( Rewriter::class )->rewrite( $markup ); } catch ( Throwable $thrown ) { - _doing_it_wrong( + self::report_error( self::class . '::filter_activation_error_markup', - sprintf( 'The activation error notice could not be rewritten: %s', $thrown->getMessage() ), - '1.0.0' + sprintf( 'The activation error notice could not be rewritten: %s', $thrown->getMessage() ) ); return $markup; diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index 4320b20..a5b27ef 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -12,6 +12,7 @@ use Nexcess\PluginAbsorber\Conflict\Detector; use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Loader; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; use StellarWP\ContainerContract\ContainerInterface; use Throwable; use WP_Hook; @@ -28,6 +29,8 @@ * @since 1.0.0 */ class Scheduler { + use Reports_Errors; + /** * plugins_loaded priority the load pass runs at. * @@ -121,10 +124,14 @@ public function wire(): void { // hook mistake there is -- would otherwise mean nothing loads at all, with no warning and // a site that looks entirely healthy. if ( $this->wiring_window_has_closed() ) { - _doing_it_wrong( + // The one report in this library raised before a hook has fired rather than from inside + // one, and the only one a host can still act on in the same request. It reaches the error + // action only when the prefix is already set: `boot()` requires a container and not a + // prefix, so a host that skipped `set_hook_prefix()` gets the developer channel alone -- + // the same answer the prefix guard gives on every other path, for the same reason. + self::report_error( Absorber::class . '::boot', - 'Absorber::boot() must run before plugins_loaded priority 5. Resolving and loading inline instead.', - '1.0.0' + 'Absorber::boot() must run before plugins_loaded priority 5. Resolving and loading inline instead.' ); // In the order the hooks would have run them. @@ -252,6 +259,15 @@ private static function load( ContainerInterface $container ): void { /** * Tell the developer which step was abandoned, and why. * + * Called from inside both backstop `catch` blocks, which is the sharpest place an announcement + * can sit: a listener throwing here would escape the handler whose entire purpose is that + * nothing escapes it, on `plugins_loaded`, on every request. `report_error()` catches its own + * listeners for exactly this call site. + * + * No sub-plugin is named. What threw is the step — a gate, the probe, a host's resolver, or a + * collaborator the container could not build — and by the time it reaches here there is no + * telling which registration, if any, it was about. + * * @since 1.0.0 * * @param string $step Step that threw, named as the sequence names it. @@ -261,10 +277,9 @@ private static function load( ContainerInterface $container ): void { * @return void */ private static function report_a_step_that_threw( string $step, string $consequence, Throwable $thrown ): void { - _doing_it_wrong( + self::report_error( self::class, - sprintf( 'The %s threw, so %s: %s', $step, $consequence, $thrown->getMessage() ), - '1.0.0' + sprintf( 'The %s threw, so %s: %s', $step, $consequence, $thrown->getMessage() ) ); } diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 8f54b3d..a20ef2d 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -15,6 +15,7 @@ use Nexcess\PluginAbsorber\Registry\Reader; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; use Throwable; /** @@ -37,6 +38,7 @@ */ class Resolver implements Resolver_Interface { use Guards_Hook_Prefix; + use Reports_Errors; /** * @since 1.0.0 @@ -137,14 +139,18 @@ public function resolve_all(): void { $standalone_gone = true; } } catch ( Throwable $thrown ) { - _doing_it_wrong( + // Announced as well as reported, and with the sub-plugin the conflict belongs to. A + // standalone still active after a pass that was supposed to deal with it is the + // failure a host is likeliest to hear about as "the site is broken", and on a + // production site the developer channel says nothing at all. + self::report_error( self::class, sprintf( 'The conflict for "%s" threw while being resolved, so it was abandoned: %s', $sub_plugin->get_slug(), $thrown->getMessage() ), - '1.0.0' + $sub_plugin ); } } diff --git a/src/Loader.php b/src/Loader.php index e26ef2f..625ca11 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -12,6 +12,7 @@ use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Registry\Reader; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; use Throwable; /** @@ -25,6 +26,7 @@ */ class Loader { use Guards_Hook_Prefix; + use Reports_Errors; /** * @since 1.0.0 @@ -94,17 +96,21 @@ public function load_all(): void { // // 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. + // + // Reporting the failure cannot add one of its own: report_error() swallows whatever a + // listener on the error action throws, so the announcement of a sub-plugin that died + // cannot be what kills the request. try { $this->load( $sub_plugin ); } catch ( Throwable $thrown ) { - _doing_it_wrong( + self::report_error( self::class, sprintf( 'The sub-plugin "%s" threw while loading, so it was abandoned: %s', $sub_plugin->get_slug(), $thrown->getMessage() ), - '1.0.0' + $sub_plugin ); } } @@ -148,14 +154,17 @@ private function load( Sub_Plugin $sub_plugin ): void { $file = $sub_plugin->get_bundled_plugin_file(); if ( ! is_file( $file ) || ! is_readable( $file ) ) { - _doing_it_wrong( + // The sub-plugin travels with the sentence, because this failure belongs to exactly one + // registration: a listener told only that a bundled file is missing would have to parse + // the path back out to know which of them to act on. + self::report_error( self::class, sprintf( 'The bundled plugin file for "%s" is missing or unreadable: %s', $sub_plugin->get_slug(), $file ), - '1.0.0' + $sub_plugin ); return; diff --git a/src/Registry/Reader.php b/src/Registry/Reader.php index 4ff10dd..332ddcb 100644 --- a/src/Registry/Reader.php +++ b/src/Registry/Reader.php @@ -10,6 +10,7 @@ use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; /** * Every registered sub-plugin, as something a pass can be handed rather than reach for. @@ -31,6 +32,8 @@ * @since 1.0.0 */ class Reader { + use Reports_Errors; + /** * Sub-plugins registered but not yet handed to the registrar. * @@ -162,14 +165,18 @@ protected function flush(): void { // what became of it is now the consequence -- the site runs one of those two files // and silently does not run the other. Every other report in this library says what // the outcome was; this one has to as well. - _doing_it_wrong( + // + // The refused registration is what goes on the action, not the one that stands: a + // listener is being told which object was thrown away, and the one already in the + // registrar is readable from every other path there is. + self::report_error( self::class, sprintf( '%1$s The registration already held was kept; %2$s was discarded.', $exception->getMessage(), $sub_plugin->get_bundled_plugin_file() ), - '1.0.0' + $sub_plugin ); } } diff --git a/src/Traits/Guards_Hook_Prefix.php b/src/Traits/Guards_Hook_Prefix.php index de0970f..62c6382 100644 --- a/src/Traits/Guards_Hook_Prefix.php +++ b/src/Traits/Guards_Hook_Prefix.php @@ -22,9 +22,16 @@ * @since 1.0.0 */ trait Guards_Hook_Prefix { + use Reports_Errors; + /** * Whether a hook prefix has been set, reporting to the developer when it has not. * + * Reported through the shared channel even though this is the one failure the error action can + * never carry — the prefix is what names that action too, so there is nothing to fire it under. + * The alternative, a bare `_doing_it_wrong()` here, would read as an oversight and would be one + * the moment somebody made the prefix optional. + * * @since 1.0.0 * * @return bool @@ -33,7 +40,7 @@ private static function has_hook_prefix(): bool { try { Config::get_hook_prefix(); } catch ( Config_Exception $exception ) { - _doing_it_wrong( self::class, $exception->getMessage(), '1.0.0' ); + self::report_error( self::class, $exception->getMessage() ); return false; } diff --git a/src/Traits/Reports_Errors.php b/src/Traits/Reports_Errors.php new file mode 100644 index 0000000..d0fa959 --- /dev/null +++ b/src/Traits/Reports_Errors.php @@ -0,0 +1,85 @@ +getMessage() + ), + '1.0.0' + ); + } + } +} diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index cfb1286..44797b9 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -713,6 +713,42 @@ static function () use ( $path, $constant ): void { $this->assert_the_library_reported_incorrect_usage(); } + /** + * The late boot is the one report this library makes before a hook of its own has fired, and the + * only one a host can still act on in the same request — so it is worth as much on a production + * site, where `_doing_it_wrong()` prints nothing, as it is under WP_DEBUG. + */ + public function test_booting_too_late_announces_the_mistake(): void { + $this->expect_incorrect_usage(); + + $announced = []; + + $this->add_tracked_action( + 'give/plugin_absorber/error', + static function ( $message ) use ( &$announced ): void { + $announced[] = $message; + } + ); + + $this->add_tracked_action( + 'plugins_loaded', + static function (): void { + Absorber::boot(); + }, + self::load_priority() + 1 + ); + + do_action( 'plugins_loaded' ); + + $this->assertCount( 1, $announced ); + $this->assertStringContainsString( + 'must run before plugins_loaded priority 5', + is_string( $announced[0] ) ? $announced[0] : '', + 'The announcement carries the same sentence the developer channel does.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + /** * @return Generator */ diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index b365fee..b26f375 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -239,6 +239,57 @@ public function test_a_sub_plugin_that_throws_does_not_stop_the_others(): void { $this->assert_the_library_reported_incorrect_usage(); } + /** + * `_doing_it_wrong()` prints nothing without WP_DEBUG, and an abandoned conflict is a standalone + * still running beside the bundled copy — the failure a site owner reports as the site being + * broken. So it is announced as well, carrying the sub-plugin it belongs to. + */ + public function test_a_sub_plugin_that_throws_is_announced_with_the_sub_plugin_it_belongs_to(): void { + $this->expect_incorrect_usage(); + $this->standalone_is( true ); + + $announced = []; + + add_action( + 'give/plugin_absorber/error', + static function ( $message, $sub_plugin ) use ( &$announced ): void { + $announced[] = [ + 'message' => $message, + 'sub_plugin' => $sub_plugin, + ]; + }, + 10, + 2 + ); + + $this->register( + [ + 'conflict_policy' => static function (): string { + throw new RuntimeException( 'the policy option could not be read' ); + }, + ] + ); + + $this->resolve_all(); + + $this->assertCount( 1, $announced ); + $this->assertStringContainsString( + 'the policy option could not be read', + is_string( $announced[0]['message'] ) ? $announced[0]['message'] : '', + 'The announcement carries the same sentence the developer channel does.' + ); + + $sub_plugin = $announced[0]['sub_plugin']; + + $this->assertInstanceOf( Sub_Plugin::class, $sub_plugin ); + $this->assertSame( + 'give-recurring', + $sub_plugin->get_slug(), + 'A conflict that was abandoned belongs to one sub-plugin, and a listener has to be told which.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + public function test_the_collaborators_come_from_the_container(): void { $detector = new class() extends Detector { /** diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index fb7d8f6..5144566 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -64,6 +64,13 @@ class LoaderTest extends WPTestCase { */ private $should_load_calls = []; + /** + * Every `error` firing, as the message and whatever sub-plugin came with it. + * + * @var array + */ + private $error_calls = []; + public function setUp(): void { parent::setUp(); @@ -75,6 +82,7 @@ public function setUp(): void { $this->clear_activations(); $this->reset_bundled_plugin_loads(); $this->should_load_calls = []; + $this->error_calls = []; } public function tearDown(): void { @@ -755,6 +763,147 @@ static function () use ( $notices ): Writer_Interface { ); } + /** + * The diagnostic channel a production site actually has. `_doing_it_wrong()` prints nothing + * without WP_DEBUG, so until this action existed a host had no way to be told that a bundled + * plugin it ships never made it into memory. + */ + public function test_it_announces_a_missing_bundled_file_with_the_sub_plugin_it_belongs_to(): void { + $this->record_error_action(); + $this->expect_incorrect_usage(); + + $path = $this->missing_bundled_plugin_file(); + + Absorber::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + + $this->loader()->load_all(); + + $this->assertCount( 1, $this->error_calls ); + $this->assertStringContainsString( + $path, + is_string( $this->error_calls[0]['message'] ) ? $this->error_calls[0]['message'] : '', + 'The error action carries the same sentence the developer channel does.' + ); + + $announced = $this->error_calls[0]['sub_plugin']; + + $this->assertInstanceOf( Sub_Plugin::class, $announced ); + $this->assertSame( + 'give-recurring', + $announced->get_slug(), + 'A failure that belongs to one sub-plugin has to name it, or a listener cannot act on it.' + ); + } + + /** + * A sub-plugin that threw is announced too, with the sub-plugin it threw for — and the + * announcement happens from inside the catch that keeps the request alive. + */ + public function test_it_announces_a_sub_plugin_that_threw(): void { + $this->record_error_action(); + $this->expect_incorrect_usage(); + + $this->register( + [ + 'enabled' => static function (): bool { + throw new RuntimeException( 'the licence server was unreachable' ); + }, + ] + ); + + $this->loader()->load_all(); + + $this->assertCount( 1, $this->error_calls ); + $this->assertStringContainsString( + 'the licence server was unreachable', + is_string( $this->error_calls[0]['message'] ) ? $this->error_calls[0]['message'] : '' + ); + } + + /** + * The sharpest case: the `error` action fires from inside handlers whose whole purpose is that + * nothing escapes them. A listener throwing there would defeat the guard by way of the thing + * reporting it, so the announcement catches its own listeners. + */ + public function test_a_throwing_error_listener_does_not_take_the_request_down(): void { + $this->expect_incorrect_usage(); + + add_action( + 'give/plugin_absorber/error', + static function (): void { + throw new RuntimeException( 'the log server was unreachable' ); + } + ); + + Absorber::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + $this->loader()->load_all(); + + $this->assertSame( + 1, + $this->bundled_plugin_loads(), + 'The sub-plugin behind the broken one still has to load.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'A listener on give/plugin_absorber/error threw', + 'The listener has to be reported down the one channel it cannot break.' + ); + } + + /** + * The hook prefix is what names the error action, so the bootstrap that never set one is the + * single failure the action cannot carry. It still reaches the developer channel, and the + * request still survives. + */ + public function test_a_missing_hook_prefix_is_reported_but_cannot_be_announced(): void { + $this->record_error_action(); + $this->register(); + + $loader = $this->loader(); + $container = $this->container(); + + // The prefix goes, the container stays: a library that reached the container first would fail + // this for the other reason. + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + $loader->load_all(); + + $this->assertSame( [], $this->error_calls ); + $this->assert_the_library_reported_incorrect_usage(); + + // The recorder has to be shown to work, and with the prefix back it is the same listener on + // the same hook: without this, a listener that never attached passes the assertion above for + // a reason that has nothing to do with the missing prefix. + Config::set_hook_prefix( 'give' ); + + Absorber::register( + [ + 'slug' => 'give-fee-recovery', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + + $loader->load_all(); + + $this->assertCount( 1, $this->error_calls, 'The recorder must catch an error that really happened.' ); + } + /** * @return void */ @@ -782,6 +931,30 @@ private function loader(): Loader { return $this->resolve( Loader::class ); } + /** + * Listen to the `error` action, keeping both of its arguments. + * + * The closure takes a reference to the property and is `static`: uopz is not involved here, but + * the same shape keeps a listener from holding the test object alive on a hook. + * + * @return void + */ + private function record_error_action(): void { + $errors = &$this->error_calls; + + add_action( + 'give/plugin_absorber/error', + static function ( $message, $sub_plugin ) use ( &$errors ): void { + $errors[] = [ + 'message' => $message, + 'sub_plugin' => $sub_plugin, + ]; + }, + 10, + 2 + ); + } + /** * Record every should_load call, so a test can assert there were none. * diff --git a/tests/unit/Registry/ReaderTest.php b/tests/unit/Registry/ReaderTest.php index 119d092..17b9898 100644 --- a/tests/unit/Registry/ReaderTest.php +++ b/tests/unit/Registry/ReaderTest.php @@ -172,6 +172,44 @@ public function test_a_duplicate_slug_is_reported_from_the_read_rather_than_thro ); } + /** + * A duplicate slug is reported through `_doing_it_wrong()`, which prints nothing on a production + * site, so it is also announced — and the sub-plugin it announces is the registration that was + * refused, not the one that stands. A listener is being told which object was thrown away; the + * one the registrar kept is readable from every other path there is. + */ + public function test_a_duplicate_slug_announces_the_registration_that_was_refused(): void { + $this->set_up_container(); + $this->register( 'give-recurring' ); + $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); + + $this->expect_incorrect_usage(); + + $announced = []; + + add_action( + 'give/plugin_absorber/error', + static function ( $message, $sub_plugin ) use ( &$announced ): void { + $announced[] = $sub_plugin; + }, + 10, + 2 + ); + + $this->reader()->all(); + + $this->assertCount( 1, $announced ); + + $refused = $announced[0]; + + $this->assertInstanceOf( Sub_Plugin::class, $refused ); + $this->assertSame( + '/tmp/give-recurring-again.php', + $refused->get_bundled_plugin_file(), + 'The announcement has to carry the registration that lost, or a listener cannot find it.' + ); + } + /** * The buffer is emptied before it is handed over, so a collision that aborted the hand-over would * take everything registered behind the colliding entry with it: the buffered copies are gone, the From 65d09a10a68a3efd0777e549456856c86dbe40b5 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:32:01 +0200 Subject: [PATCH 2/4] Record the error action where the file table, the keys and the invariant list it --- AGENTS.md | 20 ++++++++----- src/Traits/Reports_Errors.php | 6 ++-- tests/unit/AbsorberTest.php | 56 +++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c710c72..a3808f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,7 +200,7 @@ that drives the whole of it against a real WordPress is `tests/unit/Scenario/`. | `src/Registry/` | `Registrar` (holds registered `Sub_Plugin` objects), `Reader` (the registration buffer, drained into the registrar on the way past; the object every pass reads the registry through), `Contracts\Registrar_Interface`. | | `src/Activator.php` | Runs a sub-plugin's activation callback once ever, recorded in one option. | | `src/Conflict/` | `Detector` (whether a standalone is in the way), `Resolver` (which policy branch to take), `Gatekeeper` (which requests, and which users, may have one resolved), `Redirector` (where the user lands afterwards), `Rewriter` (rewrites the activation-error screen for a registered standalone), `Contracts\Resolver_Interface`. | -| `src/Traits/` | `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing). Cross-cutting only: a trait used by one folder lives in that folder. | +| `src/Traits/` | `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing), `Reports_Errors` (the one way a failure is announced: `_doing_it_wrong()` and the `error` action, and it swallows what a listener throws). Cross-cutting only: a trait used by one folder lives in that folder. | | `src/Notices/` | `Writer` (what a notice says, stored under `slug:type` — `merge`, `conflict`, `stranding`, `dependency`), `Presenter` (who may consume it, render-then-clear), `Store` (keeps it), `Renderer` (draws it, `notice-error` for `dependency` and `notice-warning` for the rest), `Contracts\Writer_Interface`. | | `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Activator_Interface`, `Config_Exception`. | @@ -403,11 +403,14 @@ runnable inline as well as wirable. `{$hook_prefix}/plugin_absorber/conflict_notice_message`, `{$hook_prefix}/plugin_absorber/dependency_notice_message` and `{$hook_prefix}/plugin_absorber/stranding_notice_message` (all four `Sub_Plugin`) +- Actions: `{$hook_prefix}/plugin_absorber/error` (`Traits\Reports_Errors`, from every reporting site + in the library) - Options: `{$option_prefix}_plugin_absorber_activations` (`Activator`), `{$option_prefix}_plugin_absorber_notices` (`Notices\Store`) -Both are built in `Config` — `get_hook_name()` and `get_option_name()` — so nothing else assembles -the segment between the host's prefix and the key's own name. The two differ in one respect: +The hooks are built in `Config` — `get_hook_name()` and `get_option_name()` — so nothing else assembles +the segment between the host's prefix and the key's own name. Hook names and option names differ in +one respect: `{$option_prefix}` is the hook prefix lowercased with hyphens folded to underscores, because the prefix validator admits `A-Z` and `-` and a hook-naming value should not reach a storage key verbatim. Hook names keep the host's casing exactly as it passed it. @@ -524,8 +527,11 @@ against real WordPress state. `Bootstrap_Test_Case.php` is the abstract parent o `Conflict\Resolver::resolve_all()` catch *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 + notice messages, the `should_load` filter, the bundled file a `require` runs top to bottom, the + standalone's own deactivation hook, and every listener on the actions this library fires. + `Traits\Reports_Errors` is the exception that catches its own: reporting a failure may not raise a + second one, so a throw from an `error` listener is swallowed there rather than handed back up to + the step that was already failing. 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 @@ -761,8 +767,8 @@ under a `Nexcess\SubPluginLoader\` namespace, with a `Config::set_version()` tha `ob_start()` approach the `wp_admin_notice_markup` filter replaced. Human-facing docs are `README.md` plus `docs/installing.md`, `docs/configuration.md`, -`docs/recipes.md`, `docs/conflict-handling.md`, `docs/filters.md`, `docs/notices.md` and -`docs/extending.md`. Keep them short and keep rationale here or in code comments — do not grow the +`docs/recipes.md`, `docs/conflict-handling.md`, `docs/filters.md`, `docs/actions.md`, +`docs/notices.md` and `docs/extending.md`. Keep them short and keep rationale here or in code comments — do not grow the README back. They are written for a host developer integrating the library, not for a maintainer: `docs/extending.md` is the only one that names internal classes, and every other file describes behaviour instead. `docs/` is `export-ignore`d and diff --git a/src/Traits/Reports_Errors.php b/src/Traits/Reports_Errors.php index d0fa959..fbdbbe8 100644 --- a/src/Traits/Reports_Errors.php +++ b/src/Traits/Reports_Errors.php @@ -25,9 +25,9 @@ * library is likeliest to add next is a new gate, and a new gate that reports through only one of * the two channels is invisible in exactly the way this exists to fix. * - * Cross-cutting rather than folder-scoped — the load pass, the boot sequence, the facade, the - * registry read and the hook-prefix guard all report — so it lives here beside the other trait every - * one of those uses. + * Cross-cutting rather than folder-scoped — the load pass, the conflict pass, the boot sequence, the + * facade, the registry read and the hook-prefix guard all report — so it lives here beside the other + * trait every one of those uses. * * @since 1.0.0 */ diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 5b4a4b2..0eccd71 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -57,6 +57,13 @@ class AbsorberTest extends WPTestCase { */ private $plugins_loaded_count = null; + /** + * Whether a test attached a listener to the error action, so tearDown knows to take it off. + * + * @var bool + */ + private $error_listener_attached = false; + public function setUp(): void { parent::setUp(); @@ -67,6 +74,12 @@ public function setUp(): void { } public function tearDown(): void { + if ( $this->error_listener_attached ) { + remove_all_actions( 'give/plugin_absorber/error' ); + + $this->error_listener_attached = false; + } + // The counter is process-global, so a test that left it rewound would tell the next one it is // still early enough to wire a plugins_loaded callback. if ( $this->plugins_loaded_count !== null ) { @@ -592,8 +605,21 @@ static function (): Presenter { } ); + // The channel that is on in production, asserted here because this is one of the two report + // sites that fire off an admin hook rather than off plugins_loaded: a trampoline wired to + // catch but not to announce would leave a white screen it prevented invisible on any site + // without WP_DEBUG, which is every site this matters on. + $announced = []; + + $this->announce_errors_into( $announced ); + Absorber::render_notices(); + $this->assertCount( 1, $announced ); + $this->assertStringContainsString( + 'the notice option held something unreadable', + is_string( $announced[0] ) ? $announced[0] : '' + ); $this->assert_the_library_reported_incorrect_usage(); } @@ -668,10 +694,40 @@ public function test_the_activation_error_trampoline_cannot_end_the_admin_reques $rewriter = $this->bind_rewriter(); $rewriter->failure = new RuntimeException( 'two sub-plugins were registered under one slug' ); + $announced = []; + + $this->announce_errors_into( $announced ); + $this->assertSame( '

Core.

', Absorber::filter_activation_error_markup( '

Core.

' ) ); + $this->assertCount( 1, $announced ); + $this->assertStringContainsString( + 'two sub-plugins were registered under one slug', + is_string( $announced[0] ) ? $announced[0] : '' + ); $this->assert_the_library_reported_incorrect_usage(); } + /** + * Collect what the `error` action carries into a list the caller can assert on. + * + * Removed in tearDown by `remove_all_actions()` rather than by identity, because a listener left + * attached would keep filling an array belonging to a test that has already finished. + * + * @param array $announced Filled with each message announced, in order. + * + * @return void + */ + private function announce_errors_into( array &$announced ): void { + add_action( + 'give/plugin_absorber/error', + static function ( $message ) use ( &$announced ): void { + $announced[] = $message; + } + ); + + $this->error_listener_attached = true; + } + /** * The other half of the same guarantee: a rewriter the container cannot build at all is a host's * broken binding, and it arrives on the same screen with the same consequence. From 63f614d85a853b8592ad56ad21cde67a6ec7e419 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:39:16 +0200 Subject: [PATCH 3/4] Promise only what report_error controls --- src/Traits/Reports_Errors.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Traits/Reports_Errors.php b/src/Traits/Reports_Errors.php index fbdbbe8..cd70b27 100644 --- a/src/Traits/Reports_Errors.php +++ b/src/Traits/Reports_Errors.php @@ -35,10 +35,18 @@ trait Reports_Errors { /** * Tell the developer, and tell anyone listening. * - * Never throws, whatever a listener does, because every caller is either inside a `catch` whose - * whole purpose is that nothing escapes it or on a path where nothing was catching in the first - * place. An error action that could take the request down would turn the library's diagnostics - * into its worst failure mode. + * Never throws, whatever a listener on the error action does, because every caller is either + * inside a `catch` whose whole purpose is that nothing escapes it or on a path where nothing was + * catching in the first place. An error action that could take the request down would turn the + * library's diagnostics into its worst failure mode. + * + * `_doing_it_wrong()` is deliberately not wrapped in the same way, and the promise above is + * worded to say so. It fires core's `doing_it_wrong_run`, which is core's hook rather than this + * library's: a listener throwing there is already breaking WordPress from every one of the dozens + * of places core calls it, so swallowing it here would hide a site-wide fault at one call site + * out of hundreds. It is also the hook the test framework raises its own failures through, and a + * `catch` around it would turn every assertion about a report this library makes into a silent + * pass. * * @since 1.0.0 * From 6eb99bdf80d88ef46adf72a7e2f100340f0e1430 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 14:45:54 +0200 Subject: [PATCH 4/4] Put the recorder with the other helpers, and name the option key an option --- AGENTS.md | 6 ++--- tests/unit/AbsorberTest.php | 54 ++++++++++++++----------------------- 2 files changed, 23 insertions(+), 37 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a3808f8..321587a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -408,7 +408,7 @@ runnable inline as well as wirable. - Options: `{$option_prefix}_plugin_absorber_activations` (`Activator`), `{$option_prefix}_plugin_absorber_notices` (`Notices\Store`) -The hooks are built in `Config` — `get_hook_name()` and `get_option_name()` — so nothing else assembles +The names are built in `Config` — `get_hook_name()` and `get_option_name()` — so nothing else assembles the segment between the host's prefix and the key's own name. Hook names and option names differ in one respect: `{$option_prefix}` is the hook prefix lowercased with hyphens folded to underscores, because the @@ -768,8 +768,8 @@ under a `Nexcess\SubPluginLoader\` namespace, with a `Config::set_version()` tha Human-facing docs are `README.md` plus `docs/installing.md`, `docs/configuration.md`, `docs/recipes.md`, `docs/conflict-handling.md`, `docs/filters.md`, `docs/actions.md`, -`docs/notices.md` and `docs/extending.md`. Keep them short and keep rationale here or in code comments — do not grow the -README back. They are written for a host developer integrating the library, not for a maintainer: +`docs/notices.md` and `docs/extending.md`. Keep them short and keep rationale here or in code +comments — do not grow the README back. They are written for a host developer integrating the library, not for a maintainer: `docs/extending.md` is the only one that names internal classes, and every other file describes behaviour instead. `docs/` is `export-ignore`d and `README.md` is not, so a link from the README into `docs/` must be an absolute repository URL; links diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 0eccd71..b3b8a7d 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -57,13 +57,6 @@ class AbsorberTest extends WPTestCase { */ private $plugins_loaded_count = null; - /** - * Whether a test attached a listener to the error action, so tearDown knows to take it off. - * - * @var bool - */ - private $error_listener_attached = false; - public function setUp(): void { parent::setUp(); @@ -74,12 +67,6 @@ public function setUp(): void { } public function tearDown(): void { - if ( $this->error_listener_attached ) { - remove_all_actions( 'give/plugin_absorber/error' ); - - $this->error_listener_attached = false; - } - // The counter is process-global, so a test that left it rewound would tell the next one it is // still early enough to wire a plugins_loaded callback. if ( $this->plugins_loaded_count !== null ) { @@ -707,27 +694,6 @@ public function test_the_activation_error_trampoline_cannot_end_the_admin_reques $this->assert_the_library_reported_incorrect_usage(); } - /** - * Collect what the `error` action carries into a list the caller can assert on. - * - * Removed in tearDown by `remove_all_actions()` rather than by identity, because a listener left - * attached would keep filling an array belonging to a test that has already finished. - * - * @param array $announced Filled with each message announced, in order. - * - * @return void - */ - private function announce_errors_into( array &$announced ): void { - add_action( - 'give/plugin_absorber/error', - static function ( $message ) use ( &$announced ): void { - $announced[] = $message; - } - ); - - $this->error_listener_attached = true; - } - /** * The other half of the same guarantee: a rewriter the container cannot build at all is a host's * broken binding, and it arrives on the same screen with the same consequence. @@ -815,6 +781,26 @@ static function () use ( $presenter ): Presenter { * * @return Spy_Rewriter */ + /** + * Collect what the `error` action carries into a list the caller can assert on. + * + * Nothing takes the listener off again, for the reason no other listener in this suite does + * either: wp-browser's teardown restores `$wp_filter` wholesale from the snapshot it took on the + * first setUp, so a hook added during a test cannot outlive it. + * + * @param array $announced Filled with each message announced, in order. + * + * @return void + */ + private function announce_errors_into( array &$announced ): void { + add_action( + 'give/plugin_absorber/error', + static function ( $message ) use ( &$announced ): void { + $announced[] = $message; + } + ); + } + private function bind_rewriter(): Spy_Rewriter { $rewriter = new Spy_Rewriter();