diff --git a/AGENTS.md b/AGENTS.md index 904da34..321587a 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 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 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 @@ -611,7 +617,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 @@ -759,9 +767,9 @@ 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 -README back. They are written for a host developer integrating the library, not for a maintainer: +`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 `README.md` is not, so a link from the README into `docs/` must be an absolute repository URL; links 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..cd70b27 --- /dev/null +++ b/src/Traits/Reports_Errors.php @@ -0,0 +1,93 @@ +getMessage() + ), + '1.0.0' + ); + } + } +} diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 5b4a4b2..b3b8a7d 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -592,8 +592,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,7 +681,16 @@ 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(); } @@ -759,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(); 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