Merge pull request #74 from stellarwp/52-registration-after-the-pass

9 [2/4]. Report a registration that arrives after the load pass

Author: nikolaystrikhar

src/Boot/Scheduler.php

@@ -142,6 +142,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.
 	 *
@@ -294,9 +334,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;
 	}
 }

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;
@@ -19,8 +20,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
@@ -64,6 +65,16 @@ 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 leaves next to nothing behind to go on: no
+	 * notice, no skip, no missing file — a sub-plugin that simply is not there. The report is a
+	 * `_doing_it_wrong()`, which is the reach every other report in this library has and no further:
+	 * it prints where a site is debugging, and fires core's `doing_it_wrong_run` wherever it is not,
+	 * for a host that listens. `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.
@@ -72,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.
+		_doing_it_wrong(
+			self::class . '::buffer',
+			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()
+			),
+			'1.0.0'
+		);
 	}
 
 	/**

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<int,array{0:callable,1:int}>
+	 */
+	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();
@@ -332,6 +354,166 @@ 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.'
+		);
+	}
+
+	/**
+	 * 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<string,array{0:int}>
+	 */
+	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".