From 6c4ef09e9c9c2146827bc54f1e5e4258e16abdc1 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:23:18 +0200 Subject: [PATCH 1/2] Report a registration that arrives after the load pass has gone by --- docs/actions.md | 6 +- src/Boot/Scheduler.php | 67 ++++++++- src/Registry/Reader.php | 35 +++++ tests/unit/Registry/ReaderTest.php | 216 +++++++++++++++++++++++++++++ 4 files changed, 319 insertions(+), 5 deletions(-) diff --git a/docs/actions.md b/docs/actions.md index 7cff5d6..020da0d 100644 --- a/docs/actions.md +++ b/docs/actions.md @@ -64,9 +64,9 @@ redirects before the load pass runs at all, so on that request no sub-plugin ann `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. +could not be resolved, a registration made after the load pass had gone by. 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 ) { diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index a5b27ef..725356e 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -149,6 +149,46 @@ public function wire(): void { } } + /** + * Whether plugins_loaded has already carried the dispatch past the load pass, so that a + * registration made now is one the load pass will not see. + * + * Here rather than in `Registry\Reader`, which is what asks. What the answer turns on is this + * library's own priorities and how far the hook it lives on has got — the two facts + * `wiring_window_has_closed()` weighs a few lines below, off the same measurement. A registry + * that read the hook for itself would hold a second copy of a rule that moves every time a + * priority here does, and the copy that was not updated would be the one a host heard from. + * + * Static, because registration is. `Absorber::register()` resolves nothing, so the question it + * asks on the way past cannot need a container answered first. + * + * Measured against the load pass because that is the last step in the sequence and the last read + * of the registry there is; the wiring window is measured against the first. A step added behind + * the load pass is the one change that would make this number the wrong one. + * + * The comparison is exclusive where the wiring window's is inclusive, because the two are not + * the same question. A callback appended to the priority being dispatched lands on an array the + * running loop already copied, so it can never fire whatever else sits in that priority. A + * registration is read by a callback already in that priority — the load pass — and whether it + * has run yet is its position within the priority, which nothing exposes. Where the answer + * cannot be known, this says nothing rather than warning about a sub-plugin that loaded. + * + * It says nothing outside the dispatch either, and that is the deliberate limit of it. Before + * plugins_loaded every registration is early. After it, a host that has not booted yet is not + * late — `wire()` finds the window shut and runs the whole sequence inline, and that pass reads + * the buffer like any other — and nothing here can tell that host from one whose load pass ran + * five priorities ago. + * + * @since 1.0.0 + * + * @return bool + */ + public static function registration_window_has_closed(): bool { + $position = self::plugins_loaded_position(); + + return $position !== null && $position > self::LOAD_PRIORITY; + } + /** * The plugins_loaded steps, in run order, as priority and callback. * @@ -309,9 +349,32 @@ private function wiring_window_has_closed(): bool { return true; } + $position = self::plugins_loaded_position(); + + return $position !== null && $position >= min( array_column( $this->sequence(), 'priority' ) ); + } + + /** + * The plugins_loaded priority being dispatched, or null when the hook is not dispatching at all. + * + * The one place this library reads how far the hook has got, so that the two windows either side + * of it differ in the priority they measure and in the comparison they make, and in nothing + * else. Both used to reach into `$GLOBALS['wp_filter']` for themselves, which is a second + * dialect of the same reading. + * + * `WP_Hook::current_priority()` answers `false` while the hook is not iterating, and that + * covers both "not yet" and "over" — a caller that has to tell those two apart asks + * `did_action()` as well. + * + * @since 1.0.0 + * + * @return int|null + */ + private static function plugins_loaded_position(): ?int { $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; - return $hook instanceof WP_Hook - && $hook->current_priority() >= min( array_column( $this->sequence(), 'priority' ) ); + $priority = $hook instanceof WP_Hook ? $hook->current_priority() : false; + + return is_int( $priority ) ? $priority : null; } } diff --git a/src/Registry/Reader.php b/src/Registry/Reader.php index 332ddcb..ffc03f7 100644 --- a/src/Registry/Reader.php +++ b/src/Registry/Reader.php @@ -7,6 +7,7 @@ namespace Nexcess\PluginAbsorber\Registry; +use Nexcess\PluginAbsorber\Boot\Scheduler; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; @@ -67,6 +68,13 @@ public function __construct( Registrar_Interface $registrar ) { * a registration that reached a registrar before that point would go into the container being * thrown away. Buffering is what lets the container arrive at any point before boot. * + * A registration that arrives after the load pass has gone by is buffered like any other and + * reported, because a buffer nothing reads again is the one failure in this library with no + * symptom at all: no notice, no skip, no missing file — a sub-plugin that simply is not there. + * `Absorber::boot()` has had a barrier for the same mistake since it was written, and boot is the + * call a host is *less* likely to misplace: registration is what a service provider tends to + * carry, and a provider runs whenever the host's bootstrap happens to run it. + * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin to hold. @@ -75,6 +83,33 @@ public function __construct( Registrar_Interface $registrar ) { */ public static function buffer( Sub_Plugin $sub_plugin ): void { self::$pending[] = $sub_plugin; + + if ( ! Scheduler::registration_window_has_closed() ) { + return; + } + + // Reported, and the report is the whole of the remedy. `boot()` can offer an inline fallback + // because what it was late for had not happened yet: the sequence was still there to be run + // by hand. Nothing is left to run here. The load pass has been and gone, this library has + // nothing further on `plugins_loaded`, and requiring the file from a registration instead + // would be a load pass of one that skipped every gate the real one applies and ran behind the + // conflict step that decides whether a bundled copy may load at all. It would land on top of + // a standalone nobody stood down, which is the re-declaration fatal this library exists to + // prevent. + // + // Buffered first, and buffered regardless: this is a report, not a refusal. `Absorber::all()` + // still answers with the registration, and a host whose own `boot()` is late enough to run + // the sequence inline reads it from there -- with a report of its own about the boot. + self::report_error( + self::class, + sprintf( + 'Absorber::register() ran after plugins_loaded had gone past the load pass, so "%s"' + . ' arrived too late to be read. Register at plugin-file scope, or no later than' + . ' plugins_loaded priority 5.', + $sub_plugin->get_slug() + ), + $sub_plugin + ); } /** diff --git a/tests/unit/Registry/ReaderTest.php b/tests/unit/Registry/ReaderTest.php index 17b9898..9741dc3 100644 --- a/tests/unit/Registry/ReaderTest.php +++ b/tests/unit/Registry/ReaderTest.php @@ -8,7 +8,10 @@ namespace Nexcess\PluginAbsorber\Tests\Unit\Registry; use Codeception\TestCase\WPTestCase; +use Generator; +use LogicException; use Nexcess\PluginAbsorber\Absorber; +use Nexcess\PluginAbsorber\Boot\Scheduler; use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Registry\Reader; @@ -19,6 +22,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; +use ReflectionClass; use RuntimeException; use Throwable; @@ -53,6 +57,17 @@ class ReaderTest extends WPTestCase { */ private $report_recorder = null; + /** + * plugins_loaded callbacks these tests added, as [ callback, priority ] pairs. + * + * Tracked so tearDown can take back exactly what a test put there. `remove_all_actions()` would + * strip the hook bare instead, discarding every callback WordPress and the rest of the suite have + * on it for the remainder of the process. + * + * @var array + */ + private $added_actions = []; + public function setUp(): void { parent::setUp(); @@ -62,6 +77,13 @@ public function setUp(): void { } public function tearDown(): void { + // In tearDown rather than at the end of a test body: a failed assertion would otherwise leave + // a callback that registers a sub-plugin on plugins_loaded for the rest of the process. + foreach ( $this->added_actions as [ $callback, $priority ] ) { + remove_action( 'plugins_loaded', $callback, $priority ); + } + $this->added_actions = []; + $this->stop_recording_reports(); $this->stop_expecting_incorrect_usage(); Absorber_State::reset(); @@ -370,6 +392,200 @@ static function (): Registrar_Interface { ); } + /** + * The mistake with no symptom: a registration made after the load pass has gone by is read by + * nothing, so the sub-plugin is simply absent — no notice, no skip, no missing file, and nothing + * for a support engineer to pull on. + * + * `Absorber::boot()` has had a barrier for this since it was written. Registration is the call a + * host is likelier to misplace, because a service provider is where a WordPress plugin usually + * puts it and a provider runs whenever the host's bootstrap happens to run it. + */ + public function test_a_registration_past_the_load_pass_is_reported(): void { + $this->set_up_container(); + $this->expect_incorrect_usage(); + + $this->register_from_plugins_loaded( self::load_priority() + 1 ); + + $this->assert_the_library_reported_incorrect_usage_saying( + '"give-recurring"', + 'The report has to name the sub-plugin that will not load, or the host cannot find it.' + ); + $this->assert_the_library_reported_incorrect_usage_saying( + 'after plugins_loaded had gone past the load pass', + 'A registration read by nothing is what failed, and the report has to say so rather than' + . ' name some other gate.' + ); + } + + /** + * `_doing_it_wrong()` prints nothing on a production site, which is exactly where a sub-plugin + * quietly missing is hardest to find, so the registration goes down the other channel too. + */ + public function test_a_registration_past_the_load_pass_announces_the_sub_plugin(): void { + $this->set_up_container(); + $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->register_from_plugins_loaded( self::load_priority() + 1 ); + + $this->assertCount( 1, $announced ); + + $late = $announced[0]; + + $this->assertInstanceOf( Sub_Plugin::class, $late ); + $this->assertSame( + 'give-recurring', + $late->get_slug(), + 'The announcement has to carry the registration that arrived too late, or a listener' + . ' cannot tell which one it was.' + ); + } + + /** + * Reported, not refused. The registration is buffered like any other, so a host reading + * `Absorber::all()` still sees what it registered — and a `boot()` late enough to run the + * sequence inline still has something to load. + */ + public function test_a_registration_past_the_load_pass_is_still_buffered(): void { + $this->set_up_container(); + $this->expect_incorrect_usage(); + + $this->register_from_plugins_loaded( self::load_priority() + 1 ); + + $this->assertSame( + [ 'give-recurring' ], + array_keys( $this->reader()->all() ), + 'The guard reports a registration; it must not throw one away.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The other side of the barrier, and the reason it is measured where it is. A host module + * registering from its own `plugins_loaded` callback at the conflict pass's priority is a + * documented shape — the load pass reads a priority later and loads it — and so is a + * registration in the load pass's own priority, where whether the pass has run yet is the + * position within that priority and nothing exposes it. + * + * @dataProvider priorities_the_load_pass_may_still_read + * + * @param int $priority plugins_loaded priority the host registers from. + */ + public function test_a_registration_the_load_pass_may_still_read_is_left_alone( int $priority ): void { + $this->set_up_container(); + $this->expect_incorrect_usage(); + $this->record_reports(); + + $this->register_from_plugins_loaded( $priority ); + + $this->assertSame( [], $this->reports, 'A registration this early is not a mistake to report.' ); + + // The recorder has to be shown to work, or a guard that never ran at all satisfies the + // assertion above however it had behaved. + $this->register_from_plugins_loaded( self::load_priority() + 1, 'give-fee-recovery' ); + + $this->assertCount( + 1, + $this->reports, + 'The recorder must catch a registration that really did arrive too late.' + ); + } + + /** + * @return Generator + */ + public static function priorities_the_load_pass_may_still_read(): Generator { + yield 'while the conflict pass is dispatching' => [ self::load_priority() - 1 ]; + yield 'in the load pass own priority' => [ self::load_priority() ]; + } + + /** + * The deliberate limit of the guard: outside a `plugins_loaded` dispatch it says nothing. + * + * Not an oversight, and not for want of knowing the hook is over. A host that has not booted yet + * is not late — `Absorber::boot()` finds the wiring window shut and runs the whole sequence + * inline, and that pass reads the buffer like any other — and from a static call that resolves + * nothing there is no telling that host from one whose load pass ran already. A report that + * fired on both would be wrong on the shape this library documents a rescue for. + */ + public function test_a_registration_made_outside_the_dispatch_is_left_alone(): void { + $this->set_up_container(); + $this->expect_incorrect_usage(); + $this->record_reports(); + + $this->register( 'give-recurring' ); + + $this->assertSame( + [], + $this->reports, + 'Outside the dispatch a late boot can still rescue the registration, so nothing is said.' + ); + + $this->register_from_plugins_loaded( self::load_priority() + 1, 'give-fee-recovery' ); + + $this->assertCount( + 1, + $this->reports, + 'The recorder must catch a registration that really did arrive too late.' + ); + } + + /** + * The priority the load pass is wired at, read from the scheduler rather than restated, so that + * "one past it" goes on meaning that if the number ever moves. + * + * @throws LogicException When the constant is missing or not an int, rather than registering at + * priority zero and passing for the wrong reason. + * + * @return int + */ + private static function load_priority(): int { + $priority = ( new ReflectionClass( Scheduler::class ) )->getConstant( 'LOAD_PRIORITY' ); + + if ( ! is_int( $priority ) ) { + throw new LogicException( 'Boot\Scheduler::LOAD_PRIORITY must be an int.' ); + } + + return $priority; + } + + /** + * Register one sub-plugin from a `plugins_loaded` callback at the given priority, and dispatch. + * + * The callback comes back off the hook as soon as the dispatch is over: a test that dispatches + * twice would otherwise register the same sub-plugin again on the second pass, from a priority + * it is no longer about. + * + * @param int $priority plugins_loaded priority to register from. + * @param string $slug Slug to register under. + * + * @return void + */ + private function register_from_plugins_loaded( int $priority, string $slug = 'give-recurring' ): void { + $callback = function () use ( $slug ): void { + $this->register( $slug ); + }; + + $this->added_actions[] = [ $callback, $priority ]; + + add_action( 'plugins_loaded', $callback, $priority ); + + do_action( 'plugins_loaded' ); + + remove_action( 'plugins_loaded', $callback, $priority ); + } + /** * Count the library's reports for this test, so "reported once" can be told from "reported at * every read". From de436d9c4ce3c11f7fa230fe2cfc43dd6860a93a Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:40:04 +0200 Subject: [PATCH 2/2] Scope the naming rule to what it is really about Boot\Scheduler names Absorber::class in executable code, and has to: a named static callback is what remove_filter() needs. What the rule is about is the registry -- no collaborator reaches it through the facade. --- src/Registry/Reader.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Registry/Reader.php b/src/Registry/Reader.php index ffc03f7..9bc72a4 100644 --- a/src/Registry/Reader.php +++ b/src/Registry/Reader.php @@ -21,8 +21,8 @@ * container to resolve a registrar from. What has to be decided is which class that costs — and it * is this one, not the facade. Everything that reads the registry (`Conflict\Detector`, * `Conflict\Resolver`, `Loader`, and `Conflict\Rewriter`) declares this - * object in its constructor, so nothing but `Absorber` itself names `Absorber`, and the dependency - * between the facade and the collaborators runs one way. + * object in its constructor, so no collaborator reaches the registry through `Absorber`, and the + * dependency between the facade and the collaborators runs one way. * * The buffer is deliberately shared across instances. It is one process's registrations, and a second * reader holding a second, emptier list is the bug `Provider` binds every collaborator as a singleton