diff --git a/AGENTS.md b/AGENTS.md index 653b680..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`. | @@ -230,12 +230,14 @@ whenever the host's bootstrap happens to run it. This is also why `Absorber::reg resolves nothing — registration at plugin-file scope is a shape a host is entitled to use, and it would otherwise register into the throwaway. -**A duplicate slug is `Registry\Registrar::register()`'s exception, not `Absorber::register()`'s.** What +**A duplicate slug is `Registry\Registrar::register()`'s refusal, not `Absorber::register()`'s.** What `Absorber::register()` throws is config validation, from the `Sub_Plugin` constructor, in the call -the host can see in its own stack trace. The buffer reaches the registrar at the first read — -`plugins_loaded` priority 5 on a request that passes the gatekeeper, priority 6 otherwise — so the -collision surfaces from inside a core action. Both are `Config_Exception`; only one of them can name -the line the host wrote. +the host can see in its own stack trace. A collision cannot be found there: the buffer reaches the +registrar at the first read — `plugins_loaded` priority 5 on a request that passes the gatekeeper, +priority 6 otherwise — long after both `register()` calls returned. So the registrar throws, and +`Registry\Reader::flush()` catches it per entry and reports it through `_doing_it_wrong()` naming the +registration that was discarded. The first registration under the slug stands, the second is dropped, +and everything registered behind it still reaches the registrar. **The too-late barrier measures against the first step in the sequence, not the last.** `Boot\Scheduler` compares the priority `plugins_loaded` is already dispatching against the lowest @@ -284,12 +286,15 @@ constructed with rather than through the registrar they could resolve for themse drains the pending registrations before it reads and a registrar asked directly would miss anything registered since the last flush. -**Both passes also catch `Config_Exception` around that read.** A duplicate slug is only found when -the buffer reaches the registrar, which is a read — long after both `register()` calls returned — and -it arrives inside `plugins_loaded`, the hook that exists to prevent a fatal, so this is the last place -allowed to cause one. The conflict pass needs the guard more than the load pass, not less: its request -gate means the only requests reaching it are admin page views, so an escaping throw lands on exactly -the screens the mistaken registration would have to be corrected from. +**Neither pass guards that read, because the read no longer raises.** The one exception it used to +carry was the duplicate slug, and that is now refused and reported inside `Registry\Reader::flush()`, +where it is found. A guard at the read was the wrong altitude for it: the first pass to read caught +it and stood down whole — the load pass loading nothing at all on the front end, the conflict pass +resolving nothing in wp-admin — over a registry that was intact and readable the entire time. One +mistaken registration is one sub-plugin's problem and the sub-plugins around it still have to load. +What remains are the backstops that were always the right altitude for an unexpected throw: the +`Throwable` catch on each `plugins_loaded` step in `Boot\Scheduler`, and the per-sub-plugin catch +inside `Loader::load_all()` and `Conflict\Resolver::resolve_all()`. The container is no longer the other half of that. A pass is handed a reader that already holds its registrar, so a container that cannot supply one fails while the *pass* is being built — where an @@ -398,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. @@ -519,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 @@ -606,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 @@ -754,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/configuration.md b/docs/configuration.md index 4059ee3..13ac09b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -61,8 +61,10 @@ container that was never taught about this library. Set the container once, befo Sub-plugins load in **registration order**, so register a dependency before anything that extends it at include time, and register each slug exactly once. A config array the library cannot use throws `Config_Exception` on the spot, in the call you can see in your own stack -trace; a duplicate slug is the exception that surfaces later, on `plugins_loaded`, since -registrations are buffered until the first read. +trace. A duplicate slug is found later, at the first read of the registry — normally on +`plugins_loaded` — since registrations are buffered until then: it is refused there and reported +through `_doing_it_wrong()`, the first registration under the slug stands, and the second is +discarded. Register unconditionally and put anything you cannot decide up front — a licence, a setting the site owner can change — in `enabled`, which is re-evaluated on every load. See 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/docs/recipes.md b/docs/recipes.md index 0054e1e..93a5566 100644 --- a/docs/recipes.md +++ b/docs/recipes.md @@ -65,9 +65,11 @@ foreach ( $sub_plugins as $slug => $constant ) { ``` An entry the library cannot use throws `Config_Exception` out of the `Absorber::register()` call it -is in, so a typo names itself in a stack trace pointing at your loop. A duplicate `slug` surfaces -later: registrations are buffered, and the collision is raised at the first read on -`plugins_loaded`. +is in, so a typo names itself in a stack trace pointing at your loop. A duplicate `slug` is found +later, at the first read of the registry — normally on `plugins_loaded` — because registrations are +buffered until then. It is refused there and reported through `_doing_it_wrong()`: the first +registration under the slug stands, the second is discarded, and every other sub-plugin loads as +normal. ## Choose a policy, and know what the site owner sees diff --git a/src/Absorber.php b/src/Absorber.php index 65b318f..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. @@ -111,8 +113,7 @@ public static function register( array $config ): void { * * @since 1.0.0 * - * @throws Config_Exception When no container has been set, or two sub-plugins were registered - * under one slug. + * @throws Config_Exception When no container has been set, or its binding is unusable. * * @return array */ @@ -181,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() ) ); } } @@ -216,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 17c5fa8..a5b27ef 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -11,8 +11,8 @@ use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; use Nexcess\PluginAbsorber\Conflict\Detector; use Nexcess\PluginAbsorber\Conflict\Gatekeeper; -use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Loader; +use Nexcess\PluginAbsorber\Traits\Reports_Errors; use StellarWP\ContainerContract\ContainerInterface; use Throwable; use WP_Hook; @@ -29,6 +29,8 @@ * @since 1.0.0 */ class Scheduler { + use Reports_Errors; + /** * plugins_loaded priority the load pass runs at. * @@ -122,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. @@ -219,19 +225,6 @@ private static function resolve_conflicts( ContainerInterface $container ): void // its own cannot drop one by omission -- and asking them first means a resolver is built // only on the request that goes on to use it. $container->get( Resolver_Interface::class )->resolve_all(); - } catch ( Config_Exception $exception ) { - // Reading the registry is where a duplicate slug surfaces, and this step reads it a - // priority ahead of the load pass that has always guarded the same read. Named separately - // from the catch below because it is the one failure here a developer can act on directly, - // and the message says which. - _doing_it_wrong( - self::class, - sprintf( - 'The registered sub-plugins could not be read, so no conflict was resolved: %s', - $exception->getMessage() - ), - '1.0.0' - ); } catch ( Throwable $thrown ) { // The backstop, and the promise the whole library rests on: plugins_loaded fires on every // request a site serves, so a throw out of a step is a white screen on all of them. What @@ -266,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. @@ -275,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/Conflict/Rewriter.php b/src/Conflict/Rewriter.php index cff4030..a0a6bb0 100644 --- a/src/Conflict/Rewriter.php +++ b/src/Conflict/Rewriter.php @@ -61,8 +61,7 @@ public function __construct( Reader $registry ) { * * @param string $markup Notice markup WordPress is about to print. * - * @throws Config_Exception When no hook prefix has been set, or two sub-plugins were registered - * under one slug. + * @throws Config_Exception When no hook prefix has been set. * * @return string */ @@ -156,8 +155,6 @@ public function rewrite( string $markup ): string { * * @param string $basename Standalone plugin basename named by the request. * - * @throws Config_Exception When two sub-plugins were registered under one slug. - * * @return Sub_Plugin|null */ private function find_by_standalone_basename( string $basename ): ?Sub_Plugin { diff --git a/src/Loader.php b/src/Loader.php index 28fee8a..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 @@ -67,9 +69,6 @@ public function __construct( /** * @since 1.0.0 * - * @throws Config_Exception From loading a sub-plugin, which reads the hook prefix the guard - * above has already established is set. - * * @return void */ public function load_all(): void { @@ -82,30 +81,11 @@ public function load_all(): void { // The reader rather than the registrar directly: it drains the registrations still buffered // on the facade before it reads, and a registrar asked on its own would miss anything - // registered since the last read. - try { - $sub_plugins = $this->registry->all(); - } catch ( Config_Exception $exception ) { - // The flush is where a duplicate slug is caught, and reading the registrar is where a - // missing container or an unusable binding is. All three are bootstrap mistakes, and - // all three arrive inside plugins_loaded: letting one out would fatal every request, - // front end and admin alike, and lock the developer out of the screen where the - // registration could be corrected. The hook this runs on exists to prevent a fatal, so - // it is the last place that may cause one -- the mistake is reported to the developer - // and the load is abandoned instead. - _doing_it_wrong( - self::class, - sprintf( - 'The registered sub-plugins could not be read, so none were loaded: %s', - $exception->getMessage() - ), - '1.0.0' - ); - - return; - } - - foreach ( $sub_plugins as $sub_plugin ) { + // registered since the last read. Unguarded, because the read answers with whatever the + // registrar legitimately holds: a duplicate slug is refused and reported where it is found, + // so the sub-plugins around it still reach this loop rather than a host's one mistaken + // registration costing the site every bundled plugin it has. + foreach ( $this->registry->all() as $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 @@ -116,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 ); } } @@ -170,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 31c0e55..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. * @@ -82,9 +85,12 @@ public static function buffer( Sub_Plugin $sub_plugin ): void { * registered since the last read, a host registering from its own `plugins_loaded` callback * included. * - * @since 1.0.0 + * A read always answers with what the registrar legitimately holds. A duplicate slug is refused + * and reported as it drains, never raised out of here: every caller is inside `plugins_loaded`, + * and one host bootstrap mistake about one sub-plugin must not stand down a pass that had every + * other sub-plugin to get on with. * - * @throws Config_Exception When two sub-plugins were registered under one slug. + * @since 1.0.0 * * @return array */ @@ -118,16 +124,25 @@ static function ( $sub_plugin ): bool { * while this object is being built, with the registrations still buffered for the read that comes * after the host has fixed its bindings. * - * That same emptying is why a duplicate slug is caught per entry rather than allowed to end the - * loop. The registrar refuses the collision, and letting the throw out of the loop would leave - * every sub-plugin registered *behind* the colliding one in no registrar and in no buffer — the - * host would get a report naming the two that collided and silently lose the rest, on both - * passes, for the rest of the process. Registering the whole batch and throwing afterwards costs - * the collision nothing: it still surfaces from the read, where both passes catch it. + * A collision the registrar refuses is reported here, with the discarded registration named, and + * goes no further. Throwing it on made one mistaken registration decide what a whole pass did: the + * first pass to read caught it and stood down — the load pass loading nothing at all on the front + * end, the conflict pass resolving nothing in wp-admin — while the registry it was standing down + * over was intact and readable the entire time. A slug registered twice is one sub-plugin's + * problem, and the sub-plugins around it still have to load. * - * @since 1.0.0 + * Reported as it is discovered, which is once per process and therefore once per request, since + * registration runs at plugin-file scope on every one: the host sees it in the log for as long as + * the duplicate exists, and the load pass does not repeat a sentence the conflict pass has + * already printed a priority earlier in the same request. A registration that arrives after a + * read — a host module registering from its own `plugins_loaded` callback — is checked when it + * drains, so a later collision still reports. * - * @throws Config_Exception When two sub-plugins were registered under one slug. + * Every collision is reported, not just the first. They are separate mistakes naming separate + * slugs, and hiding the second behind the first only means the host fixes one and gets the next + * on the following request. + * + * @since 1.0.0 * * @return void */ @@ -140,25 +155,30 @@ protected function flush(): void { self::$pending = []; - // The first collision, not the last, so that a buffer containing two of them reports the one - // the host wrote first and keeps reporting the same one until it is fixed. The exception is - // rethrown as the registrar raised it: it names the slug and both bundled files, which is the - // mistake the host has to go and correct, and what this method did with the rest of the batch - // is nothing they can act on. - $collision = null; - foreach ( $pending as $sub_plugin ) { try { $this->registrar->register( $sub_plugin ); } catch ( Config_Exception $exception ) { - if ( $collision === null ) { - $collision = $exception; - } + // The registrar's own sentence, unwrapped: it names the slug and both bundled files, + // which is the whole of what the host has to go and correct. One clause is added, + // because the registrar refuses a registration without saying what became of it, and + // 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. + // + // 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() + ), + $sub_plugin + ); } } - - if ( $collision !== null ) { - throw $collision; - } } } 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/README.md b/tests/README.md index a609f06..6c0dff5 100644 --- a/tests/README.md +++ b/tests/README.md @@ -584,13 +584,14 @@ sequenceDiagram ``` **A duplicate slug is reported, and what was registered behind it still loads.** -The collision is the registrar's exception and it is raised long after both +The collision is the registrar's refusal and it is found long after both `Absorber::register()` calls returned, from inside `plugins_loaded` — the hook -this library exists to keep a site off the floor on. Both passes guard the read, -so the conflict pass reports it and the load pass, finding the buffer already -drained, gets on with the load. The whole batch is registered before the -collision is rethrown, which is what keeps a host from silently losing every -sub-plugin it registered after the mistake. +this library exists to keep a site off the floor on. So it is reported where it +is found and nothing is raised out of the read: the conflict pass reports it and +resolves what it has, and the load pass behind it loads the registry that read +left standing. Only the colliding entry is refused, which is what keeps a host +from silently losing every sub-plugin it registered after the mistake — or, +back when the read still threw, every sub-plugin it registered at all. ```mermaid sequenceDiagram @@ -604,8 +605,8 @@ sequenceDiagram Note over R: first read — the conflict pass, priority 5 R->>Reg: A, then A again, then B Reg-->>R: the second A collides - R-->>R: whole batch registered, the first collision rethrown after it - Note over R: the pass reports it and abandons its own step + R-->>R: the second A refused and reported; A and B kept + Note over R: the pass carries on with the registry it has Note over R: second read — priority 6, buffer already drained R-->>L: A and B L->>L: both load @@ -707,9 +708,9 @@ sequenceDiagram **The request after a deactivation does not loop.** The failure mode a merge notice queued on every request would produce: a redirect loop, or a screen reporting the same deactivation for ever. Nothing is re-registered between the -two requests — a duplicate slug throws — because this is the next page view, not -a second bootstrap. The second request must *not* halt, and the helper fails the -test if it does. +two requests — a duplicate slug is refused — because this is the next page view, +not a second bootstrap. The second request must *not* halt, and the helper fails +the test if it does. ```mermaid sequenceDiagram diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index f54f68f..b3b8a7d 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -373,10 +373,13 @@ public function test_reading_twice_does_not_register_twice(): void { /** * Deferring registration moves the duplicate-slug report from the second register() call to the - * first read. It still names both bundled files, which is what the host needs to find them. + * first read, where it is reported rather than raised: what a host asks for here is the list of + * sub-plugins, and the second registration under a slug is no reason to hand back none of them. + * The report still names both bundled files, which is what the host needs to find them. */ public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { $this->set_up_container(); + $this->expect_incorrect_usage(); Absorber::register( $this->sub_plugin_config( 'give-recurring' ) ); Absorber::register( @@ -387,13 +390,22 @@ public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { ] ); - try { - Absorber::all(); - $this->fail( 'Expected a Config_Exception.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( 'give-recurring', $exception->getMessage() ); - $this->assertStringContainsString( '/tmp/other/other.php', $exception->getMessage() ); - } + $all = Absorber::all(); + + $this->assertSame( [ 'give-recurring' ], array_keys( $all ) ); + $this->assertSame( + '/tmp/give-recurring/give-recurring.php', + $all['give-recurring']->get_bundled_plugin_file(), + 'The registration that arrived first under a slug is the one that stands.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The collision is what failed, and the report has to say so rather than name some other gate.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + '/tmp/other/other.php', + 'The report has to name the registration that lost, or the host cannot find it.' + ); } public function test_all_is_empty_before_anything_is_registered(): void { @@ -580,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(); } @@ -656,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(); } @@ -747,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 a6274cf..44797b9 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -194,9 +194,9 @@ public function test_the_load_step_runs_early_in_plugins_loaded(): void { * something to do — a standalone in the way under the policy that only talks, and a bundled file to * require — so the step that did not throw is the one whose effect is still there afterwards. * - * The report is asserted by its wording, because the conflict step has two catch arms and both end - * "no conflict was resolved". Only "the conflict pass threw" belongs to the backstop under test - * here; the arm that names an unreadable registry is the duplicate-slug case further down. + * The report is asserted by its wording rather than by the fact of one, because every gate in this + * library reports through `_doing_it_wrong()`: "the conflict pass threw" is what only this backstop + * says, and a looser assertion would go on passing after the backstop stopped running at all. * * @dataProvider throwing_steps * @@ -444,16 +444,15 @@ public function test_a_user_who_cannot_activate_plugins_has_nothing_resolved_or_ /** * Reading the registry flushes the registration buffer, and the registrar refuses a slug it - * already holds. The conflict step reads a priority ahead of the load pass, so it — not the load - * pass that has always guarded this — is the first pass a duplicate slug reaches, and a throw - * here arrives inside plugins_loaded, where it takes wp-admin down and locks the developer out - * of the screen where the second registration could be undone. + * already holds. The conflict step reads a priority ahead of the load pass, so it is the first + * pass a duplicate slug reaches — and standing the step down over one refused registration left + * an active standalone in place, with the re-declaration fatal that pair is heading for still in + * front of the site, on the screens the mistake could have been corrected from. * - * The front end never reaches it, because the request gate turns away first. That is what makes - * this the worse failure rather than a lesser one: the only requests that fatal are the ones the - * mistake could have been corrected from. + * The registry the collision was refused from is intact, so the sub-plugin that did register is + * still in conflict and the step still resolves it. The refusal is reported alongside. */ - public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_conflict_step(): void { + public function test_a_duplicate_slug_does_not_stand_the_conflict_step_down(): void { set_current_screen( 'dashboard' ); $this->bind_active_standalone(); @@ -468,20 +467,10 @@ public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_confl do_action( 'plugins_loaded' ); - // Reaching this line at all is half of what is under test: the step has to return. - $this->assertSame( - [], + $this->assertArrayHasKey( + 'give-recurring:conflict', $this->queued_notices(), - 'A read that failed has no list to resolve from, so nothing may be resolved.' - ); - - // The arm under test, by the only words that separate it from the Throwable backstop beside it - // — which would catch the same exception and say "the conflict pass threw" instead. Without - // this the arm could be deleted outright and the test would go on passing. - $this->assert_the_library_reported_incorrect_usage_saying( - 'The registered sub-plugins could not be read, so no conflict was resolved', - 'An unreadable registry is the one failure here a developer can act on directly, and it is' - . ' reported as itself rather than as any throw.' + 'One mistaken registration must not cost the step the conflict it was there to resolve.' ); $this->assert_the_library_reported_incorrect_usage_saying( 'Two sub-plugins are registered under the slug "give-recurring"', @@ -724,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/DetectorTest.php b/tests/unit/Conflict/DetectorTest.php index 843c3c7..24f5c92 100644 --- a/tests/unit/Conflict/DetectorTest.php +++ b/tests/unit/Conflict/DetectorTest.php @@ -14,7 +14,6 @@ use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Conflict\Detector; use Nexcess\PluginAbsorber\Conflict_Policy; -use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Plugin\Contracts\Checker_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; @@ -22,6 +21,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Stub_Registry_Reader; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithNoticeQueue; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithSubPlugins; @@ -47,6 +47,7 @@ class DetectorTest extends WPTestCase { use UopzFunctions; use WithContainer; + use WithIncorrectUsage; use WithNoticeQueue; use WithSubPlugins; @@ -105,6 +106,7 @@ static function ( $plugins, $silent = false, $network_wide = null ) use ( &$deac } public function tearDown(): void { + $this->stop_expecting_incorrect_usage(); $this->clear_notices(); Absorber_State::reset(); Config_State::reset(); @@ -310,18 +312,27 @@ public function test_it_walks_past_a_sub_plugin_that_is_not_in_conflict(): void /** * The probe reads the registry and nothing else, which is what keeps it cheap enough to ask of - * every admin GET — and a duplicate slug is the one bootstrap mistake that read can still raise, - * because it is only found when the buffer reaches the registrar. The conflict step catches this - * exception type around the probe for exactly this case, so it has to arrive as this type. + * every admin GET — and a duplicate slug is the one bootstrap mistake that read can still meet, + * because it is only found when the buffer reaches the registrar. It is refused and reported as it + * drains, and the probe answers over the registry that is left: this step runs a priority ahead of + * the load pass, so a mistake that stood it down would leave an active standalone in place with + * nothing left in the request to deactivate it. */ - public function test_a_duplicate_slug_surfaces_from_the_probe(): void { + public function test_a_duplicate_slug_does_not_stop_the_probe_answering(): void { $this->standalone_is( true ); $this->register(); $this->register(); - $this->expectException( Config_Exception::class ); + $this->expect_incorrect_usage(); - $this->detector()->has_conflict(); + $this->assertTrue( + $this->detector()->has_conflict(), + 'The sub-plugin that did register is still in conflict, whatever the second registration did.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The refusal has to reach the developer, or a sub-plugin goes missing with nothing said.' + ); } /** 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 57eec04..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 { @@ -672,34 +680,39 @@ public function test_load_all_does_nothing_without_a_hook_prefix(): void { } /** - * The same guarantee for the read itself. Reading flushes the registration buffer, and the - * registrar refuses a slug it already holds — a throw that arrives inside plugins_loaded, where it - * would take down the front end and wp-admin together and lock the developer out of the screen - * where the duplicate registration could be undone. + * The read itself, and the failure this pass can least afford. Reading flushes the registration + * buffer, and the registrar refuses a slug it already holds — which the read used to raise as an + * exception, standing the whole pass down. + * + * The load pass is the first thing to read on every request that is not an interactive admin GET, + * because the conflict pass's gate turns those away before they reach it. So one duplicated + * registration meant the front end loaded none of the site's bundled plugins, on every request, + * for as long as the duplicate existed — while wp-admin, where the load pass reads second and + * found the buffer already drained, went on looking perfectly healthy. + * + * The refusal is reported and the pass gets on with the registry it has. */ - public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_request(): void { + public function test_a_duplicate_slug_is_reported_and_the_sub_plugins_around_it_still_load(): void { // Two registrations of the default slug, each with a bundled fixture of its own — one file // behind both would load once for the second registration and hide the skip under a dedupe. - $this->register(); - $this->register(); + $first = $this->register(); + $duplicate = $this->register(); + $behind = $this->register( [ 'slug' => 'give-fee-recovery' ] ); $this->expect_incorrect_usage(); $this->loader()->load_all(); - // Reaching this line at all is half of what is under test: load_all() has to return. - $this->assertSame( - 0, - $this->bundled_plugin_loads(), - 'A read that failed has no list to load from, so nothing may load.' - ); - $this->assert_the_library_reported_incorrect_usage_saying( - 'The registered sub-plugins could not be read, so none were loaded', - 'The read is what failed, and the report has to say so rather than name some other gate.' + $this->assertTrue( defined( $first ), 'The registration that stands is the one that loads.' ); + $this->assertFalse( defined( $duplicate ), 'The refused registration is not loaded in its place.' ); + $this->assertTrue( + defined( $behind ), + 'A sub-plugin registered behind the collision has nothing to do with it and still has to load.' ); + $this->assertSame( 2, $this->bundled_plugin_loads() ); $this->assert_the_library_reported_incorrect_usage_saying( - 'give-recurring', - 'The report has to name the slug, or it could have been raised for any other reason.' + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The collision is what failed, and the report has to say so rather than name some other gate.' ); } @@ -750,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 */ @@ -777,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 8aea606..17b9898 100644 --- a/tests/unit/Registry/ReaderTest.php +++ b/tests/unit/Registry/ReaderTest.php @@ -10,7 +10,6 @@ use Codeception\TestCase\WPTestCase; use Nexcess\PluginAbsorber\Absorber; use Nexcess\PluginAbsorber\Config; -use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Registry\Reader; use Nexcess\PluginAbsorber\Sub_Plugin; @@ -19,6 +18,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; use RuntimeException; use Throwable; @@ -35,6 +35,23 @@ */ class ReaderTest extends WPTestCase { use WithContainer; + use WithIncorrectUsage; + + /** + * Every `_doing_it_wrong()` message this test recorded for itself. + * + * `WithIncorrectUsage` asserts that a report was made and what it said; this counts how many times + * it was said, which is the difference between reporting a collision as it is discovered and + * reporting it again at every read. + * + * @var string[] + */ + private $reports = []; + + /** + * @var callable|null + */ + private $report_recorder = null; public function setUp(): void { parent::setUp(); @@ -45,6 +62,8 @@ public function setUp(): void { } public function tearDown(): void { + $this->stop_recording_reports(); + $this->stop_expecting_incorrect_usage(); Absorber_State::reset(); Config_State::reset(); $this->tear_down_container(); @@ -113,31 +132,89 @@ static function () use ( $registrar ): Registrar_Interface { } /** - * The one bootstrap mistake that can still arrive at read time, and the reason both passes catch - * `Config_Exception` around their read: a slug is only found to be a duplicate when the buffer - * reaches the registrar, which is a read after both `register()` calls have returned. + * The one bootstrap mistake that can still arrive at read time: a slug is only found to be a + * duplicate when the buffer reaches the registrar, which is a read after both `register()` calls + * have returned. * - * The container is not the other half of that any more. It is needed to *build* a reader, not to - * read from one — the registrar arrives as a constructor argument, so a reader that exists has - * one, and a container that could not supply it failed while this object was being built. + * It is reported, and it goes no further. Raised as an exception it decided what the whole pass + * did — the load pass loading nothing at all, the conflict pass resolving nothing — over a + * registry that was intact and readable the entire time. The read answers with what the registrar + * legitimately holds, and the registration that arrived first under the slug is the one that + * stands. */ - public function test_a_duplicate_slug_surfaces_from_the_read(): void { + public function test_a_duplicate_slug_is_reported_from_the_read_rather_than_thrown(): void { $this->set_up_container(); $this->register( 'give-recurring' ); + $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); + + $this->expect_incorrect_usage(); + + $all = $this->reader()->all(); + + $this->assertSame( [ 'give-recurring' ], array_keys( $all ) ); + $this->assertSame( + '/tmp/give-recurring.php', + $all['give-recurring']->get_bundled_plugin_file(), + 'The registration that arrived first under a slug is the one that stands.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The collision is what failed, and the report has to say so rather than name some other gate.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + '/tmp/give-recurring-again.php', + 'The report has to name the registration that lost, or the host cannot find it.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + '/tmp/give-recurring-again.php was discarded', + 'Naming both files says nothing about which of them the site is running; the report has' + . ' to say which registration was dropped.' + ); + } + + /** + * 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' ); - $reader = $this->reader(); + $this->expect_incorrect_usage(); - $this->expectException( Config_Exception::class ); + $announced = []; - $reader->all(); + 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 - * registrar never saw them, and the pass's own catch reports only the duplicate. A host left with a - * sub-plugin that is simply absent, named by nothing anywhere, is the worse of the two failures. + * registrar never saw them, and the read reports only the duplicate. A host left with a sub-plugin + * that is simply absent, named by nothing anywhere, is the worse of the two failures. */ public function test_a_duplicate_slug_does_not_discard_the_registrations_behind_it(): void { $this->set_up_container(); @@ -145,55 +222,90 @@ public function test_a_duplicate_slug_does_not_discard_the_registrations_behind_ $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); $this->register( 'give-fee-recovery' ); - $reader = $this->reader(); - - try { - $reader->all(); - $this->fail( 'Expected a Config_Exception naming the duplicated slug.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( 'give-recurring', $exception->getMessage() ); - $this->assertStringContainsString( - '/tmp/give-recurring-again.php', - $exception->getMessage(), - 'The report has to name the registration that lost, or the host cannot find it.' - ); - } + $this->expect_incorrect_usage(); $this->assertSame( [ 'give-recurring', 'give-fee-recovery' ], - array_keys( $reader->all() ), + array_keys( $this->reader()->all() ), 'Everything registered after the collision has to reach the registrar regardless.' ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The registrations behind the collision surviving must not cost the collision its report.' + ); } /** - * Two collisions in one buffer is one bootstrap mistake made twice, and the host reads the report - * from the top: the first is rethrown, and the second is what the next request reports once the - * first is fixed. Letting a later collision overwrite the first would move the report around - * between requests for no gain. + * The read that comes after the one that found the collision, which is every request in wp-admin: + * the conflict pass reads at `plugins_loaded` priority 5 and the load pass reads again at 6. A + * collision that emptied the registry, or that only the first reader could see, left the pass + * behind it with nothing to work on — the load pass loading none of the site's bundled plugins + * while the registry sat there readable. */ - public function test_it_reports_the_first_duplicate_when_more_than_one_collides(): void { + public function test_a_second_read_still_answers_with_the_whole_registry(): void { $this->set_up_container(); $this->register( 'give-recurring' ); $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); $this->register( 'give-fee-recovery' ); - $this->register( 'give-fee-recovery', '/tmp/give-fee-recovery-again.php' ); + + $this->expect_incorrect_usage(); $reader = $this->reader(); - try { - $reader->all(); - $this->fail( 'Expected a Config_Exception naming the duplicated slug.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( 'give-recurring', $exception->getMessage() ); - $this->assertStringNotContainsString( - 'give-fee-recovery', - $exception->getMessage(), - 'The first collision is the one reported; a later one must not overwrite it.' - ); - } + $this->assertSame( [ 'give-recurring', 'give-fee-recovery' ], array_keys( $reader->all() ) ); + $this->assertSame( + [ 'give-recurring', 'give-fee-recovery' ], + array_keys( $reader->all() ), + 'A pass reading behind the one that found the collision gets the same registry, not an empty one.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * Reported as it is discovered, and the buffer only drains once, so the two passes of one admin + * request print one sentence between them rather than each printing the same one. Reporting again + * at every read would cost the host a duplicate line per pass and per activation-error rewrite, + * for a mistake they have already been told about, and would buy nothing: registration runs at + * plugin-file scope, so the next request finds the collision and reports it again anyway. + */ + public function test_a_duplicate_is_reported_once_rather_than_at_every_read(): void { + $this->set_up_container(); + $this->register( 'give-recurring' ); + $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); + + $this->expect_incorrect_usage(); + $this->record_reports(); + + $reader = $this->reader(); - $all = $reader->all(); + $reader->all(); + + // The recorder catching the first report is what makes the assertion after the second read + // mean anything: a recorder that never attached would count nothing either way. + $this->assertCount( 1, $this->reports, 'The read that drains the buffer is the read that reports.' ); + + $reader->all(); + + $this->assertCount( 1, $this->reports, 'A read with nothing left to drain has nothing left to report.' ); + } + + /** + * Two collisions in one buffer is two mistakes, each naming a slug of its own, and each is + * reported. Rationing the report to the first was all a single rethrown exception could carry; + * nothing rations it now, and hiding the second would only mean the host fixes one duplicate and + * meets the next on the following request. + */ + public function test_it_reports_every_duplicate_when_more_than_one_collides(): void { + $this->set_up_container(); + $this->register( 'give-recurring' ); + $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); + $this->register( 'give-fee-recovery' ); + $this->register( 'give-fee-recovery', '/tmp/give-fee-recovery-again.php' ); + + $this->expect_incorrect_usage(); + $this->record_reports(); + + $all = $this->reader()->all(); $this->assertSame( [ 'give-recurring', 'give-fee-recovery' ], array_keys( $all ) ); $this->assertSame( @@ -201,6 +313,15 @@ public function test_it_reports_the_first_duplicate_when_more_than_one_collides( $all['give-fee-recovery']->get_bundled_plugin_file(), 'The registration that arrived first under a slug is the one that stands.' ); + $this->assertCount( 2, $this->reports, 'Each collision is a mistake of its own to correct.' ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-recurring"', + 'The first collision has to be reported.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'Two sub-plugins are registered under the slug "give-fee-recovery"', + 'And the second, which a report rationed to the first would have hidden.' + ); } /** @@ -249,6 +370,44 @@ static function (): Registrar_Interface { ); } + /** + * Count the library's reports for this test, so "reported once" can be told from "reported at + * every read". + * + * A recorder of this test's own rather than a reach into `WithIncorrectUsage`: that trait asserts + * that a report was made and what it said, which is a different question from how often. + * + * @return void + */ + private function record_reports(): void { + $reports = &$this->reports; + + // Static, and closing over a reference: a closure left on a hook outlives the test object, and + // `$this` inside one WordPress calls back is not this test. + $recorder = static function ( $function_name, $message = '' ) use ( &$reports ): void { + $reports[] = is_string( $message ) ? $message : ''; + }; + + $this->report_recorder = $recorder; + + add_action( 'doing_it_wrong_run', $recorder, 10, 2 ); + } + + /** + * Take the recorder back off, by identity: the rest of the suite is on this hook too. + * + * @return void + */ + private function stop_recording_reports(): void { + if ( $this->report_recorder !== null ) { + remove_action( 'doing_it_wrong_run', $this->report_recorder ); + + $this->report_recorder = null; + } + + $this->reports = []; + } + /** * The reader the container builds, which is the one every pass is handed. * diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index 7fa337e..f8334c1 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -218,7 +218,7 @@ public function test_the_merge_notice_renders_on_the_next_admin_screen_and_clear /** * The failure mode a merge notice queued on every request would produce: a redirect loop, or an * admin screen that reports the same deactivation for ever. Nothing is re-registered between the - * two requests — a duplicate slug throws — because this is the next page view, not a second + * two requests — a duplicate slug is refused — because this is the next page view, not a second * bootstrap. */ public function test_the_request_after_a_deactivation_does_not_loop(): void { diff --git a/tests/unit/Scenario/LoadTest.php b/tests/unit/Scenario/LoadTest.php index c8dfe8b..a7b75d0 100644 --- a/tests/unit/Scenario/LoadTest.php +++ b/tests/unit/Scenario/LoadTest.php @@ -196,17 +196,18 @@ function () use ( $second ): void { } /** - * Two registrations under one slug. The collision is the registrar's exception and it is raised - * long after both `Absorber::register()` calls returned — the buffer only reaches the registrar - * when something reads it, which is inside `plugins_loaded`, the hook this library exists to keep a - * site off the floor on. So the read is guarded at both passes: the conflict pass at priority 5 - * reads first and reports the mistake, and the load pass behind it finds the buffer already drained - * and gets on with the load. + * Two registrations under one slug. The collision is the registrar's refusal and it is found long + * after both `Absorber::register()` calls returned — the buffer only reaches the registrar when + * something reads it, which is inside `plugins_loaded`, the hook this library exists to keep a site + * off the floor on. So it is reported where it is found and nothing is raised out of the read: the + * conflict pass at priority 5 reads first and reports the mistake, and the load pass behind it + * loads the registry that read left standing. * * What the sub-plugin registered *after* the collision does is the part worth pinning. The whole - * batch is registered and the collision raised afterwards, so a host with a duplicate two entries - * up keeps everything it registered behind it — where a throw out of the middle of the flush would - * have left those in no registrar and in no buffer, silently, for the rest of the process. + * batch is offered to the registrar and only the colliding entry is refused, so a host with a + * duplicate two entries up keeps everything it registered behind it — where a throw out of the + * middle of the flush would have left those in no registrar and in no buffer, silently, for the + * rest of the process. */ public function test_a_duplicate_slug_is_reported_and_the_registration_behind_it_still_loads(): void { $this->expect_incorrect_usage();