diff --git a/docs/actions.md b/docs/actions.md index a38a337..a131251 100644 --- a/docs/actions.md +++ b/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 diff --git a/src/Loader.php b/src/Loader.php index f1def25..d39c3d9 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -115,11 +115,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 { @@ -217,6 +218,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 @@ -257,10 +294,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. * diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index e16e8bc..c34f6bc 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -461,6 +461,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 @@ -651,8 +740,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 ) { @@ -917,7 +1013,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: @@ -1112,6 +1208,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 $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 $overrides Config overrides. * @param string|null $constant Guard constant to use, or a fresh one. diff --git a/tests/unit/Scenario/LoadTest.php b/tests/unit/Scenario/LoadTest.php index c8dfe8b..58cc545 100644 --- a/tests/unit/Scenario/LoadTest.php +++ b/tests/unit/Scenario/LoadTest.php @@ -273,6 +273,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