Merge pull request #57 from stellarwp/39-registry-survives-a-collision

9 [1/4]. Keep the registry readable when one slug is registered twice

Author: nikolaystrikhar

AGENTS.md

@@ -242,12 +242,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
@@ -296,12 +298,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

docs/configuration.md

@@ -61,8 +61,10 @@ container that was never taught about this library. Set the container once, befo
 Sub-plugins load in **registration order**, so register a dependency before anything that
 extends it at include time, and register each slug exactly once. A config array the library
 cannot use throws `Config_Exception` on the spot, in the call you can see in your own stack
-trace; a duplicate slug is the exception that surfaces later, on `plugins_loaded`, since
-registrations are buffered until the first read.
+trace. A duplicate slug is found later, at the first read of the registry — normally on
+`plugins_loaded` — since registrations are buffered until then: it is refused there and reported
+through `_doing_it_wrong()`, the first registration under the slug stands, and the second is
+discarded.
 
 Register unconditionally and put anything you cannot decide up front — a licence, a setting the
 site owner can change — in `enabled`, which is re-evaluated on every load. See

docs/recipes.md

@@ -65,9 +65,11 @@ foreach ( $sub_plugins as $slug => $constant ) {
 ```
 
 An entry the library cannot use throws `Config_Exception` out of the `Absorber::register()` call it
-is in, so a typo names itself in a stack trace pointing at your loop. A duplicate `slug` surfaces
-later: registrations are buffered, and the collision is raised at the first read on
-`plugins_loaded`.
+is in, so a typo names itself in a stack trace pointing at your loop. A duplicate `slug` is found
+later, at the first read of the registry — normally on `plugins_loaded` — because registrations are
+buffered until then. It is refused there and reported through `_doing_it_wrong()`: the first
+registration under the slug stands, the second is discarded, and every other sub-plugin loads as
+normal.
 
 ## Choose a policy, and know what the site owner sees
 

src/Absorber.php

@@ -139,8 +139,7 @@ public static function register( array $config ): void {
 	 *
 	 * @since 1.0.0
 	 *
-	 * @throws Config_Exception When no container has been set, or two sub-plugins were registered
-	 *                          under one slug.
+	 * @throws Config_Exception When no container has been set, or its binding is unusable.
 	 *
 	 * @return array<string,Sub_Plugin>
 	 */

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

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 {

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

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<string,Sub_Plugin>
 	 */
@@ -124,16 +127,25 @@ static function ( $sub_plugin ): bool {
 	 * while this object is being built, with the registrations still buffered for the read that comes
 	 * after the host has fixed its bindings.
 	 *
-	 * That same emptying is why a duplicate slug is caught per entry rather than allowed to end the
-	 * loop. The registrar refuses the collision, and letting the throw out of the loop would leave
-	 * every sub-plugin registered *behind* the colliding one in no registrar and in no buffer — the
-	 * host would get a report naming the two that collided and silently lose the rest, on both
-	 * passes, for the rest of the process. Registering the whole batch and throwing afterwards costs
-	 * the collision nothing: it still surfaces from the read, where both passes catch it.
+	 * A collision the registrar refuses is reported here, with the discarded registration named, and
+	 * goes no further. Throwing it on made one mistaken registration decide what a whole pass did: the
+	 * first pass to read caught it and stood down — the load pass loading nothing at all on the front
+	 * end, the conflict pass resolving nothing in wp-admin — while the registry it was standing down
+	 * over was intact and readable the entire time. A slug registered twice is one sub-plugin's
+	 * problem, and the sub-plugins around it still have to load.
 	 *
-	 * @since 1.0.0
+	 * Reported as it is discovered, which is once per process and therefore once per request, since
+	 * registration runs at plugin-file scope on every one: the host sees it in the log for as long as
+	 * the duplicate exists, and the load pass does not repeat a sentence the conflict pass has
+	 * already printed a priority earlier in the same request. A registration that arrives after a
+	 * read — a host module registering from its own `plugins_loaded` callback — is checked when it
+	 * drains, so a later collision still reports.
+	 *
+	 * Every collision is reported, not just the first. They are separate mistakes naming separate
+	 * slugs, and hiding the second behind the first only means the host fixes one and gets the next
+	 * on the following request.
 	 *
-	 * @throws Config_Exception When two sub-plugins were registered under one slug.
+	 * @since 1.0.0
 	 *
 	 * @return void
 	 */
@@ -146,25 +158,28 @@ public function flush(): void {
 
 		self::$pending = [];
 
-		// The first collision, not the last, so that a buffer containing two of them reports the one
-		// the host wrote first and keeps reporting the same one until it is fixed. The exception is
-		// rethrown as the registrar raised it: it names the slug and both bundled files, which is the
-		// mistake the host has to go and correct, and what this method did with the rest of the batch
-		// is nothing they can act on.
-		$collision = null;
-
 		foreach ( $pending as $sub_plugin ) {
 			try {
 				$this->registrar->register( $sub_plugin );
 			} catch ( Config_Exception $exception ) {
-				if ( $collision === null ) {
-					$collision = $exception;
-				}
+				// The registrar's own sentence, unwrapped: it names the slug and both bundled files,
+				// which is the whole of what the host has to go and correct. One clause is added,
+				// because the registrar refuses a registration without saying what became of it, and
+				// what became of it is now the consequence -- the site runs one of those two files
+				// and silently does not run the other. Every other report in this library says what
+				// the outcome was; this one has to as well. The clause names the loser as "the
+				// duplicate" rather than by path alone, because the two registrations may well name
+				// the same file, and a bare path then reads as if the surviving one went too.
+				_doing_it_wrong(
+					self::class,
+					sprintf(
+						'%1$s The original registration was kept; the duplicate %2$s was discarded.',
+						$exception->getMessage(),
+						$sub_plugin->get_bundled_plugin_file()
+					),
+					'1.0.0'
+				);
 			}
 		}
-
-		if ( $collision !== null ) {
-			throw $collision;
-		}
 	}
 }

tests/README.md

@@ -644,13 +644,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
@@ -664,8 +665,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
@@ -767,9 +768,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

tests/unit/AbsorberTest.php

@@ -445,10 +445,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(
@@ -459,13 +462,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 {