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

Core.

', Absorber::filter_activation_error_markup( '

Core.

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