9A. Report a require that left the guard constant undefined

docs/actions.md

@@ -63,19 +63,24 @@ redirects before the load pass runs at all, so on that request no sub-plugin ann
 ## 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.
+one — a duplicate slug, a broken bundled file, a bundled file that never defined its guard constant,
+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 );
 ```
 
-Two things to know:
+Three things to know:
 
+- **A guard constant that never arrived is an `error`, and the sub-plugin still `loaded`.** The
+  bundled file is checked once it has been required: if it did not define the constant named by
+  `plugin_loaded_constant`, nothing stands a standalone copy down and a re-declaration fatal is one
+  activation away. The code is in memory all the same, so the activation callback runs and `loaded`
+  fires — the fix is to define the constant at file scope, or to correct the key.
 - **`file_unreadable` fires both.** A bundled file that is not there is a build to fix *and* a
   sub-plugin that will not be present, so it is announced as an `error` and as a `skipped`. Listen
   to both and you will see it twice.

src/Loader.php

@@ -95,11 +95,12 @@ public function load_all(): void {
 			// loop carries on with the next.
 			//
 			// 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.
+			// a Throwable -- which is what the guard constant, checked before any of this and checked
+			// again once the require has happened, is for.
 			//
 			// The loaded and skipped actions run host code too, but they catch their own throws
 			// rather than falling to this one: by the time `loaded` fires the require has happened,
-			// the guard constant is defined and the activation callback has run, so a listener's
+			// the guard constant has been checked and the activation callback has run, so a listener's
 			// throw arriving here would report a sub-plugin that is loaded and healthy as one that
 			// was abandoned -- on the channel a host built its log line and its health check on.
 			// Reporting a failure cannot add one of its own either way: report_error() swallows
@@ -198,6 +199,41 @@ private function load( Sub_Plugin $sub_plugin ): void {
 		// wrapping here can hand a required file the global scope it would have had.
 		require_once $file;
 
+		// The same defined() the second gate asked, on the other side of the require -- asked of the
+		// constant table here rather than through `Sub_Plugin::is_already_loaded()`, because it is no
+		// longer the same question. In front of the require that predicate means "the code is already
+		// here, from whichever copy"; behind it, the only thing worth asking is whether this require
+		// did what the guard promises. `Sub_Plugin` still owns the name, so nothing here spells a guard
+		// constant for Strauss's constant_prefix to rewrite at build time.
+		//
+		// Nothing else checks: a typo in `plugin_loaded_constant`, or a bundled plugin that defines its
+		// constant from its own plugins_loaded callback rather than at file scope, leaves the code in
+		// memory with nothing standing a standalone copy down -- and a re-declaration fatal, which PHP
+		// does not raise as a Throwable and nothing here can catch, is then one activation away while
+		// every counter and every action says the load went perfectly.
+		//
+		// Reported rather than skipped, and the load carries on. The require happened and cannot be
+		// undone: the file's code is in memory whatever the guard says, so the activation callback
+		// behind this still has to run and `loaded` still has to fire -- announcing a skip would tell
+		// a host that code which is running is not there, and withholding the callback would leave
+		// the sub-plugin loaded with the tables it expects never created. What is broken is the
+		// host's build, which is who `error` addresses.
+		$guard_constant = $sub_plugin->get_plugin_loaded_constant();
+
+		if ( ! defined( $guard_constant ) ) {
+			self::report_error(
+				self::class,
+				sprintf(
+					'The bundled plugin "%s" was required and left %s undefined, so nothing stands a'
+						. ' standalone copy down. Define the guard constant at file scope, or correct'
+						. ' plugin_loaded_constant.',
+					$sub_plugin->get_slug(),
+					$guard_constant
+				),
+				$sub_plugin
+			);
+		}
+
 		// Only after a require that actually happened. A bundled plugin is included rather than
 		// activated, so register_activation_hook() never fires for it and whatever that hook would
 		// have done -- creating a table, seeding options -- would never happen at all. Running it
@@ -238,9 +274,9 @@ private function announce_skip( Sub_Plugin $sub_plugin, string $reason ): void {
 	 *
 	 * The throw is caught here rather than a frame up, because `load_all()`'s per-sub-plugin catch
 	 * has exactly one sentence and it is "threw while loading, so it was abandoned". For a listener
-	 * on `loaded` that sentence is false in both halves: the require happened, the guard constant is
-	 * defined and the activation callback has already run, so the sub-plugin is loaded and nothing
-	 * about it was abandoned. A host listening to both channels would get `loaded` and `error` for
+	 * on `loaded` that sentence is false in both halves: the require happened, the guard constant has
+	 * been checked and the activation callback has already run, so the sub-plugin is loaded and
+	 * nothing about it was abandoned. A host listening to both channels would get `loaded` and `error` for
 	 * the same sub-plugin in the same pass, and the log line, health check and support tool that
 	 * `error` exists for would each read a successful load as a failed one. The same is true of a
 	 * listener on `skipped`, which reports a skip that really did happen.

tests/unit/LoaderTest.php

@@ -469,6 +469,91 @@ public function test_the_should_load_filter_is_not_consulted_when_dependencies_a
 		$this->assert_the_should_load_recorder_works();
 	}
 
+	/**
+	 * The guard constant carries the whole re-declaration guarantee, and a require is the only thing
+	 * that can deliver one. A typo in `plugin_loaded_constant`, or a bundled plugin that defines its
+	 * constant from its own `plugins_loaded` callback rather than at file scope, leaves the code in
+	 * memory with nothing standing a standalone copy down — and the fatal this library exists to
+	 * prevent is then one activation away, with nothing anywhere having said so.
+	 */
+	public function test_a_require_that_defined_no_guard_constant_is_reported(): void {
+		$this->expect_incorrect_usage();
+
+		$expected = $this->register_with_a_guard_nothing_defines();
+
+		$this->loader()->load_all();
+
+		$this->assertSame( 1, $this->bundled_plugin_loads(), 'The require happened and cannot be undone.' );
+		$this->assertFalse( defined( $expected ) );
+
+		// The guard's own sentence, naming the constant that never arrived: every other gate reports
+		// too, and a looser assertion would go on passing after this check stopped running at all.
+		$this->assert_the_library_reported_incorrect_usage_saying(
+			sprintf( '"give-recurring" was required and left %s undefined', $expected ),
+			'The report has to name the sub-plugin and the constant nothing defined.'
+		);
+	}
+
+	/**
+	 * A report, not a skip. The require happened, so the file's code is in memory whatever the guard
+	 * says: the setup that stands in for `register_activation_hook()` still has to run, and a host
+	 * listening on `loaded` still has to be told, or the one channel that answers "is this sub-plugin
+	 * here?" would answer no about code that is running.
+	 */
+	public function test_a_sub_plugin_whose_guard_never_arrived_still_counts_as_loaded(): void {
+		$this->expect_incorrect_usage();
+		$this->record_lifecycle_actions();
+
+		$activated = [];
+
+		$this->register_with_a_guard_nothing_defines(
+			[
+				'activation_callback' => static function ( Sub_Plugin $sub_plugin ) use ( &$activated ): void {
+					$activated[] = $sub_plugin->get_slug();
+				},
+			]
+		);
+
+		$this->loader()->load_all();
+
+		$this->assertSame( [ 'give-recurring' ], $activated, 'The code is in memory, so its setup still runs.' );
+		$this->assertSame( [ 'give-recurring' => true ], $this->activation_record() );
+		$this->assertCount( 1, $this->loaded_calls );
+		$this->assertSame( [], $this->skipped_calls, 'A load that happened is not a skip, whatever it left undefined.' );
+	}
+
+	/**
+	 * And the ordinary load says nothing at all. The check is one `defined()` on the far side of the
+	 * require, so a guard that arrived has to leave the channel a host built a health check on
+	 * exactly as quiet as it was before.
+	 */
+	public function test_a_require_that_defined_its_guard_constant_reports_nothing(): void {
+		$this->record_error_action();
+
+		$this->register();
+
+		$this->loader()->load_all();
+
+		$this->assertSame( 1, $this->bundled_plugin_loads() );
+		$this->assertSame( [], $this->error_calls, 'A guard that arrived is the ordinary success case.' );
+
+		// The recorder has to be shown to work. A listener that never attached leaves the same empty
+		// list, for a reason that has nothing to do with the guard being where it belongs.
+		$this->expect_incorrect_usage();
+
+		Absorber::register(
+			[
+				'slug'                   => 'give-fee-recovery',
+				'bundled_plugin_file'    => $this->missing_bundled_plugin_file(),
+				'plugin_loaded_constant' => $this->make_guard_constant(),
+			]
+		);
+
+		$this->loader()->load_all();
+
+		$this->assertCount( 1, $this->error_calls, 'The recorder must catch an error that really happened.' );
+	}
+
 	/**
 	 * The activation callback stands in for the register_activation_hook() a bundled plugin never
 	 * gets, so it has to run with the plugin's own code already in memory: a migration that calls a
@@ -659,8 +744,15 @@ static function () use ( $registrar ): Registrar_Interface {
 	/**
 	 * require_once dedupes by resolved path, so one file behind two registrations executes once even
 	 * when the second one's guard constant never gets defined.
+	 *
+	 * Neither guard gets defined here, in fact — the shared file defines a constant of its own and
+	 * each registration names another — so both loads are reported for a guard that never arrived.
+	 * That is the check doing its job on the very shape it exists for: two registrations sharing one
+	 * file is two sub-plugins with no working load guard between them.
 	 */
 	public function test_one_bundled_file_behind_two_registrations_loads_once(): void {
+		$this->expect_incorrect_usage();
+
 		$path = $this->make_bundled_plugin_file( $this->make_guard_constant() );
 
 		foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $slug ) {
@@ -930,7 +1022,7 @@ public function test_it_announces_a_skip_for_a_load_the_filter_vetoed(): void {
 	 * inside `plugins_loaded`. A listener that throws costs its own sub-plugin and nothing behind it.
 	 *
 	 * And it costs its own sub-plugin nothing either, which is the half worth pinning: by the time
-	 * `loaded` fires the require has happened, the guard constant is defined and the activation
+	 * `loaded` fires the require has happened, the guard constant has been checked and the activation
 	 * callback has run. Left to the per-sub-plugin catch in `load_all()`, the throw would be reported
 	 * as "threw while loading, so it was abandoned" — a sentence that is false in both halves, on the
 	 * one channel a host is expected to build a log line and a health check on. It is announced as
@@ -1265,6 +1357,35 @@ private function define_guard( string $constant ): string {
 		return $constant;
 	}
 
+	/**
+	 * Register a sub-plugin whose bundled file defines some constant other than the configured one.
+	 *
+	 * The state a mistyped `plugin_loaded_constant` leaves behind, and the state a bundled plugin that
+	 * defines its guard from its own `plugins_loaded` callback leaves behind at the moment the require
+	 * returns. A fixture that defines nothing at all would be the same test with a file no bundled
+	 * plugin resembles.
+	 *
+	 * @param array<string,mixed> $overrides Config overrides.
+	 *
+	 * @return string The guard constant the config names, which nothing defines.
+	 */
+	private function register_with_a_guard_nothing_defines( array $overrides = [] ): string {
+		$expected = $this->make_guard_constant();
+
+		Absorber::register(
+			array_merge(
+				[
+					'slug'                   => 'give-recurring',
+					'bundled_plugin_file'    => $this->make_bundled_plugin_file( $this->make_guard_constant() ),
+					'plugin_loaded_constant' => $expected,
+				],
+				$overrides
+			)
+		);
+
+		return $expected;
+	}
+
 	/**
 	 * @param array<string,mixed> $overrides Config overrides.
 	 * @param string|null         $constant  Guard constant to use, or a fresh one.

tests/unit/Scenario/LoadTest.php

@@ -274,6 +274,52 @@ public function test_a_missing_bundled_file_is_reported_to_the_developer_and_not
 		$this->assertStringNotContainsString( self::SLUG, $rendered );
 	}
 
+	/**
+	 * The require is the only thing that can deliver a guard constant, and here it does not: the file
+	 * loads and the constant the registration named never arrives. That is what a typo in
+	 * `plugin_loaded_constant` looks like from here, and what a bundled plugin that defines its guard
+	 * from its own `plugins_loaded` callback looks like at the moment the require returns — and with
+	 * nothing checking, the site keeps the re-declaration fatal this library exists to prevent while
+	 * every counter and every action says the load went perfectly.
+	 *
+	 * The sub-plugin is loaded all the same. Its code is in memory, so its activation callback runs
+	 * and `loaded` is fired; what is broken is the host's build, which is a developer's to fix and
+	 * nothing the owner's screen can help with.
+	 */
+	public function test_a_bundled_file_that_defines_no_guard_is_loaded_and_reported(): void {
+		$this->expect_incorrect_usage();
+
+		$activated = [];
+
+		// The registration names one guard constant and the fixture defines another, which is the
+		// state a mistyped key leaves behind without a fixture no bundled plugin resembles.
+		$expected = $this->register(
+			[
+				'bundled_plugin_file' => $this->make_bundled_plugin_file( $this->make_guard_constant() ),
+				'activation_callback' => static function ( Sub_Plugin $sub_plugin ) use ( &$activated ): void {
+					$activated[] = $sub_plugin->get_slug();
+				},
+			]
+		);
+
+		$this->boot();
+		$this->run_request();
+
+		$this->assertSame( 1, $this->bundled_plugin_loads() );
+		$this->assertFalse( defined( $expected ), 'Nothing defined the guard the registration named.' );
+		$this->assertSame( [ self::SLUG ], $activated, 'The code is in memory, so the setup it needs still runs.' );
+		$this->assertSame( [ self::SLUG => true ], $this->activation_record() );
+		$this->assert_the_library_reported_incorrect_usage_saying(
+			sprintf( '"%s" was required and left %s undefined', self::SLUG, $expected ),
+			'The report has to name the sub-plugin and the constant nothing defined.'
+		);
+
+		$rendered = $this->render_admin_notices();
+
+		$this->assertSame( [], $this->queued_notices(), 'A build the owner cannot fix is nothing to tell them about.' );
+		$this->assertStringNotContainsString( self::SLUG, $rendered );
+	}
+
 	/**
 	 * All the way to the screen: the load is skipped, the host's own explanation is queued, the render
 	 * draws it as an error, and the render consumes the queue so the owner is told once rather than on