diff --git a/AGENTS.md b/AGENTS.md index 653b680..904da34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 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/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..60f28cc 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -111,8 +111,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 */ diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index 17c5fa8..4320b20 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -11,7 +11,6 @@ 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 StellarWP\ContainerContract\ContainerInterface; use Throwable; @@ -219,19 +218,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 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..e26ef2f 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -67,9 +67,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 +79,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 diff --git a/src/Registry/Reader.php b/src/Registry/Reader.php index 31c0e55..4ff10dd 100644 --- a/src/Registry/Reader.php +++ b/src/Registry/Reader.php @@ -82,9 +82,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 +121,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. + * + * 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. * - * @throws Config_Exception When two sub-plugins were registered under one slug. + * @since 1.0.0 * * @return void */ @@ -140,25 +152,26 @@ 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. + _doing_it_wrong( + 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' + ); } } - - if ( $collision !== null ) { - throw $collision; - } } } 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..5b4a4b2 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 { diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index a6274cf..cfb1286 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"', 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/LoaderTest.php b/tests/unit/LoaderTest.php index 57eec04..fb7d8f6 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -672,34 +672,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.' ); } diff --git a/tests/unit/Registry/ReaderTest.php b/tests/unit/Registry/ReaderTest.php index 8aea606..119d092 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,51 @@ 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' ); + $this->register( 'give-recurring', '/tmp/give-recurring-again.php' ); - $reader = $this->reader(); + $this->expect_incorrect_usage(); - $this->expectException( Config_Exception::class ); + $all = $this->reader()->all(); - $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.' + ); } /** * 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 +184,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(); + } - $all = $reader->all(); + /** + * 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(); + + $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 +275,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 +332,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();