10 [2/2]. Report a require that left the guard constant undefined

docs/actions.md

@@ -61,8 +61,8 @@ load pass runs at all, so on that request no sub-plugin announces anything.
 ## Your listener cannot take the site down
 
 These fire from inside `plugins_loaded`, so a listener that throws is caught rather than allowed
-out. It costs nothing: by the time `loaded` fires the require has happened, the guard constant is
-defined and the activation callback has run, and a `skipped` announcement is the last thing that
+out. It costs nothing: by the time `loaded` fires the require has happened, the guard constant has
+been checked and the activation callback has run, and a `skipped` announcement is the last thing that
 happens to that sub-plugin either way. The throw is reported through `_doing_it_wrong()` as what it
 is — a listener, named by the hook it is on — rather than as the sub-plugin having failed, so a host
 reading its log does not mistake its own bug for a load that broke. That is a backstop, not a

src/Loader.php

@@ -93,11 +93,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 on.
 			try {
@@ -195,6 +196,42 @@ 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 a developer's to fix and nothing a site owner's screen can help with --
+		// so it goes to _doing_it_wrong() and to no notice.
+		$guard_constant = $sub_plugin->get_plugin_loaded_constant();
+
+		if ( ! defined( $guard_constant ) ) {
+			_doing_it_wrong(
+				self::class . '::load',
+				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
+				),
+				'1.0.0'
+			);
+		}
+
 		// 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
@@ -235,10 +272,10 @@ 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 would get the `loaded` announcement and a report of a failed
-	 * load for the same sub-plugin in the same pass, and the log line it keeps would read a
+	 * 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 would get the `loaded` announcement and a report of a
+	 * failed load for the same sub-plugin in the same pass, and the log line it keeps would read a
 	 * successful load as a broken one. The same is true of a listener on `skipped`, which reports a
 	 * skip that really did happen.
 	 *

tests/unit/LoaderTest.php

@@ -471,6 +471,95 @@ 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 a developer's log exactly as quiet as it was
+	 * before.
+	 */
+	public function test_a_require_that_defined_its_guard_constant_reports_nothing(): void {
+		// On before the load that must stay quiet, not after it: the listener records every report the
+		// library makes, so a guard check that fired here would be caught rather than missed.
+		$this->expect_incorrect_usage();
+
+		$this->register();
+
+		$this->loader()->load_all();
+
+		$this->assertSame( 1, $this->bundled_plugin_loads() );
+		$this->assertSame( [], $this->incorrect_usage_messages, '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.
+		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->incorrect_usage_messages,
+			'The recorder must catch a report 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
@@ -661,8 +750,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 ) {
@@ -932,7 +1028,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 on. It is reported as what it is instead:
@@ -1127,6 +1223,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