diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 8f54b3d..ab9f404 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -99,7 +99,7 @@ public function __construct( /** * @since 1.0.0 * - * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * @throws Config_Exception When no hook prefix has been set. * * @return void */ @@ -168,7 +168,7 @@ public function resolve_all(): void { * * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. * - * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * @throws Config_Exception When no hook prefix has been set. * * @return bool Whether the standalone is gone -- not whether deactivating it was attempted. */ @@ -200,19 +200,7 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { return false; } - $this->deactivate( $sub_plugin ); - - // Asked again rather than assumed, because turning the standalone off is not the same - // event as the standalone being off. A site or mu-plugin filtering - // `option_active_plugins` puts it straight back, a host may have rebound - // `Plugin\Contracts\Deactivator_Interface` to something that does nothing, and a - // rebound `Plugin\Contracts\Checker_Interface` may mean by "active" something - // `deactivate_plugins()` never touches. Answering true on a standalone that is still - // running redirects to the screen the user asked for, where the next request detects - // the same conflict and redirects again -- until the browser gives up and the whole of - // wp-admin is out of reach, with the merge notice never drawn because every one of - // those requests exits before `all_admin_notices`. - return ! $this->detector->is_in_conflict( $sub_plugin ); + return $this->deactivate( $sub_plugin ); // NOTICE_ONLY, and anything is_valid() would accept that this switch has grown no // branch for. The default sits on the branch that only talks, never on the one that @@ -225,21 +213,46 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { } /** + * Turn the standalone off, and report the merge only if it really went off. + * + * The answer is also what decides the redirect. Sending the user back to the screen they asked for + * while the standalone is still running arrives at the same conflict, deactivates to no effect and + * redirects again -- until the browser gives up with the whole of wp-admin out of reach, and with + * every one of those requests exiting before `all_admin_notices` could draw anything. + * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. * - * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * @throws Config_Exception When no hook prefix has been set. * - * @return void + * @return bool Whether the standalone is gone -- not whether deactivating it was attempted. */ - protected function deactivate( Sub_Plugin $sub_plugin ): void { + protected function deactivate( Sub_Plugin $sub_plugin ): bool { $this->plugin_deactivator->deactivate( $sub_plugin->get_standalone_plugin_basename() ); - // Queued as the deactivation is made rather than once at the end, so the explanation is - // durable whether or not the request goes on to redirect — and so a site with two - // standalones gets one notice per plugin it lost. + // Asked again rather than assumed, because turning the standalone off is not the same event as + // the standalone being off. A site or mu-plugin filtering `option_active_plugins` puts it + // straight back, a host may have rebound `Plugin\Contracts\Deactivator_Interface` to something + // that does nothing, and a rebound `Plugin\Contracts\Checker_Interface` may mean by "active" + // something `deactivate_plugins()` never touches. + if ( $this->detector->is_in_conflict( $sub_plugin ) ) { + // Nothing is said and nothing is reported. The merge notice would tell the owner a plugin + // they can watch still running had been deactivated, and would say it again on every admin + // GET for as long as the site kept putting it back; and this is the DEFER outcome reached + // by another route, so there is no failure to announce either -- the standalone is running, + // which means its own guard constant stands the bundled copy down and nothing re-declares. + // Every way of arriving here is a site's own configuration rather than a mistake in the + // host's code, and `Plugin\Contracts\Deactivator_Interface` invites one of them by name. + return false; + } + + // Queued as each deactivation is confirmed rather than once at the end, so the explanation is + // durable whether or not the request goes on to redirect — and so a site with two standalones + // gets one notice per plugin it lost. $this->notices->queue_merge_notice( $sub_plugin ); + + return true; } /** diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index b365fee..f9ef081 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -511,8 +511,21 @@ public function test_it_deactivates_without_redirecting_once_the_headers_are_sen * Nothing exotic is required to get here: a site or mu-plugin filtering `option_active_plugins` * puts the standalone straight back into the active list, which is a pattern `learndash-core` * itself uses. + * + * And the merge notice is the other half. "We deactivated it for you" is a claim the owner can + * disprove by looking at the plugins list the notice is drawn above, and it is re-queued on every + * admin GET for as long as the standalone keeps coming back — so the one sentence this library + * says about a conflict has to wait until the re-check says there is a merge to report. + * + * Nothing is reported to the developer either, and the silence is deliberate: the standalone is + * running, so its own guard constant stands the bundled copy down and nothing re-declares. Every + * way to get here is a site's own configuration rather than a mistake in the host's code, and the + * conflict is re-detected on every admin GET — a report would be the same sentence on every screen + * of a debugging site, for a site behaving as its owner set it up to. That is covered without an + * assertion of its own: WPTestCase fails a test that receives a `_doing_it_wrong()` it never said + * to expect. */ - public function test_a_standalone_that_survives_deactivation_does_not_redirect(): void { + public function test_a_standalone_that_survives_deactivation_neither_redirects_nor_reports_a_merge(): void { $this->standalone_is( true ); $this->standalone_survives_deactivation( 'give-recurring/give-recurring.php' ); $this->register(); @@ -520,13 +533,14 @@ public function test_a_standalone_that_survives_deactivation_does_not_redirect() $this->resolve_all(); $this->assertCount( 1, $this->deactivations, 'The policy still runs: deactivation is asked for.' ); - $this->assertArrayHasKey( - 'give-recurring:merge', + $this->assertSame( + [], $this->queued_notices(), - 'The request goes on rendering, so the notice explaining the deactivation must survive to be drawn.' + 'A standalone the owner can watch still running must not be reported as deactivated.' ); } + /** * The same failure through the seam a host owns rather than through the site's own filters: a * bound `Deactivator_Interface` that records and does nothing leaves the standalone exactly where @@ -572,7 +586,11 @@ static function () use ( $deactivator ): Deactivator_Interface { $this->deactivations, 'The bound deactivator is what deactivates, so nothing was ever taken out of the active list.' ); - $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); + $this->assertSame( + [], + $this->queued_notices(), + 'A host is invited to bind deactivation away; being told its plugin was deactivated anyway is the lie.' + ); } /** @@ -663,7 +681,11 @@ public function test_it_still_redirects_when_one_of_two_standalones_really_went_ ); $queued = $this->queued_notices(); - $this->assertArrayHasKey( 'give-recurring:merge', $queued ); + $this->assertArrayNotHasKey( + 'give-recurring:merge', + $queued, + 'The stubborn one is still running, whatever the request managed for the other.' + ); $this->assertArrayHasKey( 'give-fee-recovery:merge', $queued ); } @@ -740,8 +762,8 @@ public function test_it_really_deactivates_a_network_active_standalone(): void { } /** - * The notice is queued after the deactivation, so it must not depend on the plugin still - * being active — and it is the only record the site owner gets. + * The notice is queued after the deactivation and after the re-check that confirms it, so it must + * not depend on the plugin still being active — and it is the only record the site owner gets. */ public function test_the_merge_notice_is_queued_before_the_redirect_halts_the_request(): void { $this->standalone_is( true ); diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index 7fa337e..e3f0a13 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -245,6 +245,56 @@ public function test_the_request_after_a_deactivation_does_not_loop(): void { $this->assertTrue( defined( $constant ) ); } + /** + * The deactivation that does not take. A site whose mu-plugin filters `option_active_plugins` puts + * the standalone straight back — a pattern `learndash-core` itself uses — so core's own + * `deactivate_plugins()` really runs, really writes the option, and the plugin is active again by + * the time the resolver looks. + * + * Nothing may be claimed about that. A merge notice would report a deactivation the owner can + * disprove by looking at the list it is drawn above, and it would be re-queued on every admin GET + * for as long as the site keeps putting the plugin back. The redirect goes for the same reason: + * the request has nothing to shed, so the one it redirects to would arrive at the same conflict. + */ + public function test_a_standalone_the_site_puts_back_is_not_reported_as_merged(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $this->add_tracked_filter( + 'option_active_plugins', + static function (): array { + return [ self::STANDALONE ]; + } + ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ] + ); + + $this->boot(); + + // run_request() fails the test on a redirect, which here is the loop itself rather than a + // symptom of one: every request after it would deactivate to no effect and redirect again. + $this->run_request(); + + $this->assertContains( + self::STANDALONE, + $this->active_plugins(), + 'The site put it back, and this library does not fight the site over it.' + ); + $this->assertSame( + [], + $this->queued_notices(), + 'A standalone the owner can watch still running must not be reported as deactivated.' + ); + + // The request really did reach this library, which is what makes the silence above mean + // something: a bootstrap that wired nothing at all would satisfy every assertion before it. + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + /** * DEFER hands the request to the standalone, and WordPress includes an active plugin from * wp-settings.php long before plugins_loaded — so by the time the resolver runs, the standalone