From 46a3d4ac193ee27432cb1888d2ee596aa4b9af32 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 21 Aug 2026 10:01:19 -0400 Subject: [PATCH] Decline a deactivation that would strand sites the host cannot reach --- CLAUDE.md | 19 +++-- docs/conflict-handling.md | 22 +++++- src/Conflict/Detector.php | 35 +++++++++ src/Conflict/Resolver.php | 12 ++++ tests/unit/Conflict/DetectorTest.php | 90 +++++++++++++++++++++++ tests/unit/Conflict/ResolverTest.php | 68 ++++++++++++++++++ tests/unit/Scenario/ConflictTest.php | 102 +++++++++++++++++++++++++++ 7 files changed, 342 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1eaace3..e476cb4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -298,9 +298,11 @@ registrar now fails like every other binding rather than being the one collabora binding surfaced late and politely. `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and -`DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. It is +`DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects — unless +the multisite stranding guard, `Conflict\Detector::deactivation_would_strand_sites()`, declines the +deactivation, in which case it queues a stranding notice and leaves the standalone alone. It is the worked example of required injection — `Conflict\Detector` to say which sub-plugins are in -conflict, `Plugin\Contracts\Deactivator_Interface` to turn the standalone off, `Writer_Interface` for the notice +conflict and whether deactivating one would strand sites, `Plugin\Contracts\Deactivator_Interface` to turn the standalone off, `Writer_Interface` for the notice and `Conflict\Redirector` for the destination, all four constructor arguments with no default — so the object a test builds is the object the provider builds, and a host's rebinding of either plugin seam reaches it, the deactivator directly and the checker through the detector, without the resolver @@ -552,9 +554,16 @@ against real WordPress state. `Bootstrap_Test_Case.php` is the abstract parent o and must not treat an unrecognised value as consent to deactivate. - **Filters run last** — after the configured value and any fallback — which is what makes deferred translation work. A non-scalar filter return becomes `''`, never a fatal cast. -- **`deactivate_plugins()` is called silent, with no `$network_wide` argument.** Silent because a - `flush_rewrite_rules()` in the standalone's deactivation hook at `plugins_loaded` 404s the site. - The `null` default takes both the network and blog branches; a computed `true` strands an entry. +- **`deactivate_plugins()` is called silent, with no `$network_wide` argument — and, under + `DEACTIVATE`, only after the stranding guard clears.** Silent because a `flush_rewrite_rules()` in + the standalone's deactivation hook at `plugins_loaded` 404s the site. The `null` default takes both + the network and blog branches; a computed `true` strands an entry. The one topology the `null` + default over-reaches is a **network-active standalone whose host is not network-active**: pulling it + network-wide removes it from the sites the host never loads on, where nothing stands in for it. + There `Conflict\Detector::deactivation_would_strand_sites()` reports the danger and `Conflict\Resolver` + declines — the standalone stays, the load guard defers the bundled copy network-wide, and a stranding + notice explains it. The guard is opt-in via `Config::set_host_plugin_basename()`; unset, behaviour is + exactly the `null` default in every topology, so no host regresses. - **`Plugin\Loads_Plugin_Functions` guards on `deactivate_plugins()`**, not `is_plugin_active()` — the latter is a common third-party shim. - **Strauss must not rewrite `plugin_loaded_constant` values.** They are shared runtime constants; diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index d4e3cb8..e634ff6 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -62,7 +62,8 @@ notice is neither shown nor cleared for a user without the same capability. The deactivation itself is silent, and covers both scopes on multisite. Silent because the standalone's own deactivation hook would otherwise run this early: a routine `flush_rewrite_rules()` in it would regenerate the rules before `init` declared a single post type, and every custom -permalink on the site would start 404ing. +permalink on the site would start 404ing. On multisite it can also decline outright — see +[the stranding guard](#the-multisite-stranding-guard). ## The redirect @@ -80,6 +81,25 @@ and the redirect is skipped entirely once headers have been sent — the request rendering instead of dying blank. The [merge notice](notices.md) is queued first either way, so the explanation survives whether or not the request ends in a redirect. +## The multisite stranding guard + +`deactivate_plugins()` runs with no `$network_wide` argument, so a network-active standalone is taken +out of *every* site's plugins. But the bundled copy only loads where the host plugin runs, so on a +network where the host is active on only some sites, deactivating a network-active standalone would +remove it from the sites the host never reached — leaving them with no copy of it at all, bundled or +standalone. + +When the host names itself with `Config::set_host_plugin_basename( plugin_basename( __FILE__ ) )`, +the `DEACTIVATE` policy checks for exactly that case — a network-active standalone whose host is not +itself network-activated — and declines: the standalone is left active, [the load +guard](#the-load-guard) stands the bundled copy down network-wide as under `DEFER`, and a [stranding +notice](notices.md) tells a network administrator why and how to finish, by network-activating the +host or removing the standalone from the Network Admin. It recurs until one of those is done. + +The guard is **opt-in and single-site-safe**: with no host basename set it never fires, and off a +network it never fires, so in every other topology — both network-active, both per-site, or a +per-site standalone — deactivation behaves exactly as it always has. + ## The load guard Before loading a bundled plugin, the library checks whether `plugin_loaded_constant` is already diff --git a/src/Conflict/Detector.php b/src/Conflict/Detector.php index 2d6bf71..d51027c 100644 --- a/src/Conflict/Detector.php +++ b/src/Conflict/Detector.php @@ -7,6 +7,7 @@ namespace Nexcess\PluginAbsorber\Conflict; +use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Plugin\Contracts\Checker_Interface; use Nexcess\PluginAbsorber\Registry\Reader; @@ -100,4 +101,38 @@ public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { return $this->plugin_checker->is_active( $sub_plugin->get_standalone_plugin_basename() ); } + + /** + * Whether deactivating this standalone would strand sites the bundled copy will never reach. + * + * `deactivate_plugins()` takes a network-active standalone out of *every* site's plugins, but the + * bundled copy only loads where the host plugin runs. So on the one topology of a network-active + * standalone whose host is not itself network-active, a network-wide deactivation removes it from + * the sites the host never loads on, where nothing stands in for it. The resolver reads this and + * declines, leaving the load guard to defer the bundled copy network-wide instead. + * + * Opt-in, and cheap in the common case: with no host basename configured the guard stands down on + * a single string compare, before any option is read. It needs no `is_multisite()` test either -- + * `Checker_Interface::is_network_active()` is `false` off a network, so the whole predicate is + * `false` on a single site. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @return bool + */ + public function deactivation_would_strand_sites( Sub_Plugin $sub_plugin ): bool { + $host_basename = Config::get_host_plugin_basename(); + + if ( $host_basename === '' ) { + return false; + } + + if ( ! $this->plugin_checker->is_network_active( $sub_plugin->get_standalone_plugin_basename() ) ) { + return false; + } + + return ! $this->plugin_checker->is_network_active( $host_basename ); + } } diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 8a3f80b..8f54b3d 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -188,6 +188,18 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { return false; case Conflict_Policy::DEACTIVATE: + // A network-active standalone whose bundled replacement ships in a host that is not + // itself network-active: deactivating it network-wide would pull it from the sites the + // host never reached, where nothing loads the bundled copy. Leave it -- the load guard + // defers the bundled copy network-wide instead -- and say why. Opt-in and + // single-site-safe: false whenever no host basename is configured, and false off a + // network, so this is the ordinary deactivation in every other case. + if ( $this->detector->deactivation_would_strand_sites( $sub_plugin ) ) { + $this->notices->queue_stranding_notice( $sub_plugin ); + + return false; + } + $this->deactivate( $sub_plugin ); // Asked again rather than assumed, because turning the standalone off is not the same diff --git a/tests/unit/Conflict/DetectorTest.php b/tests/unit/Conflict/DetectorTest.php index fa9841d..843c3c7 100644 --- a/tests/unit/Conflict/DetectorTest.php +++ b/tests/unit/Conflict/DetectorTest.php @@ -424,6 +424,52 @@ private function assert_the_checker_is_really_recording( Detector $detector ): v ); } + /** + * The multisite stranding predicate: deactivating a network-active standalone is only unsafe when + * the host that ships its bundled replacement is not itself network-active -- then a network-wide + * deactivation pulls the standalone from the sites the host never loads on, with nothing to + * replace it. Opt-in: false whenever no host basename is configured. is_multisite() is never + * asked and never stubbed -- is_network_active() is false off a network, so the predicate is + * false on single site of its own accord. + * + * @dataProvider stranding_topologies + * + * @param string $host_basename Host basename to configure, or '' for none. + * @param string[] $network_active Basenames the checker reports network-active. + * @param bool $expected Whether deactivation would strand sites. + */ + public function test_it_reports_whether_deactivation_would_strand_sites( + string $host_basename, + array $network_active, + bool $expected + ): void { + Config::set_host_plugin_basename( $host_basename ); + $this->install_checker( $this->checker_network_active_for( $network_active ) ); + + $sub_plugin = $this->make_sub_plugin( + [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] + ); + + $this->assertSame( + $expected, + $this->detector()->deactivation_would_strand_sites( $sub_plugin ) + ); + } + + /** + * @return Generator + */ + public static function stranding_topologies(): Generator { + $standalone = 'give-recurring/give-recurring.php'; + $host = 'give/give.php'; + + yield 'network standalone, non-network host' => [ $host, [ $standalone ], true ]; + yield 'network standalone, network host' => [ $host, [ $standalone, $host ], false ]; + yield 'site-only standalone, non-network host' => [ $host, [], false ]; + yield 'site-only standalone, network host' => [ $host, [ $host ], false ]; + yield 'no host basename configured' => [ '', [ $standalone ], false ]; + } + /** * The detector the container builds, which is the one the conflict step reaches. * @@ -552,6 +598,50 @@ public function is_network_active( string $basename ): bool { }; } + /** + * A checker that reports the named basenames network-active, for the stranding predicate. + * + * is_active() is not the axis these tests turn on -- the predicate never calls it -- so it + * answers a constant true. + * + * @param string[] $network_active Basenames reported network-active. + * + * @return Checker_Interface + */ + private function checker_network_active_for( array $network_active ): Checker_Interface { + return new class( $network_active ) implements Checker_Interface { + /** + * @var string[] + */ + private $network_active; + + /** + * @param string[] $network_active Basenames reported network-active. + */ + public function __construct( array $network_active ) { + $this->network_active = $network_active; + } + + /** + * @param string $basename Plugin basename. + * + * @return bool + */ + public function is_active( string $basename ): bool { + return true; + } + + /** + * @param string $basename Plugin basename. + * + * @return bool + */ + public function is_network_active( string $basename ): bool { + return in_array( $basename, $this->network_active, true ); + } + }; + } + /** * Bind the recording checker in place of the default one. * diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index ee598d2..b365fee 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -575,6 +575,74 @@ static function () use ( $deactivator ): Deactivator_Interface { $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); } + /** + * The multisite stranding guard: when the detector reports that deactivating this standalone would + * strand sites -- a network-active standalone whose host is not itself network-active -- the + * DEACTIVATE policy declines. Nothing is deactivated, so there is no merge notice and no redirect; + * a stranding notice explains why the standalone is still there. Whether that condition holds is + * `DetectorTest`'s; that the resolver obeys it is this test's, so the detector is a double. + */ + public function test_it_declines_to_deactivate_when_that_would_strand_sites(): void { + $detector = new class() extends Detector { + /** + * No plugin checker and no parent constructor: this stands in for the two answers the + * resolver reads, not for how a real detector reaches them. + */ + public function __construct() { + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin to test. + * + * @return bool + */ + public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { + return true; + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @return bool + */ + public function deactivation_would_strand_sites( Sub_Plugin $sub_plugin ): bool { + return true; + } + }; + + $container = new Test_Container(); + $this->set_up_container( $container ); + + // A concrete class, so it is bound after the provider, which would otherwise replace it. + $container->singleton( + Detector::class, + static function () use ( $detector ): Detector { + return $detector; + } + ); + + $this->register(); + $this->resolve_all(); + + $this->assertSame( + [], + $this->deactivations, + 'A standalone whose deactivation would strand sites must be left active.' + ); + + $queued = $this->queued_notices(); + $this->assertArrayHasKey( + 'give-recurring:stranding', + $queued, + 'The superadmin has to be told why the standalone was left active.' + ); + $this->assertArrayNotHasKey( + 'give-recurring:merge', + $queued, + 'Nothing was deactivated, so there is no merge to report.' + ); + } + /** * One stubborn standalone must not cost the site the redirect the other one earned. The request * still has a plugin's code in memory that a fresh one would shed, so it is still worth taking — diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index 254d346..7fa337e 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -9,6 +9,7 @@ namespace Nexcess\PluginAbsorber\Tests\Unit\Scenario; +use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Conflict_Policy; /** @@ -45,6 +46,15 @@ class ConflictTest extends Bootstrap_Test_Case { */ private const SECOND_SLUG = 'absorber-fee-recovery'; + /** + * A host plugin basename, for the multisite stranding scenarios. Only ever an entry in + * `active_sitewide_plugins` and the value handed to `Config::set_host_plugin_basename()` — no + * fixture file stands behind it, because the guard reads its activation state and nothing more. + * + * @var string + */ + private const HOST = 'absorber-host/absorber-host.php'; + /** * Here rather than in the parent because only this file builds a request that carries query args: * an activation error, and the action arg the request gate refuses. A `$_GET` left standing would @@ -88,6 +98,98 @@ public function test_deactivate_deactivates_notifies_and_redirects(): void { $this->assertSame( 0, $this->bundled_plugin_loads() ); } + /** + * The multisite stranding guard, end to end: a network-active standalone whose bundled copy ships + * in a host plugin that is not itself network-active is left active rather than deactivated. A + * network-wide deactivation would strip it from the sites the host never loads on, with nothing to + * replace it — so the standalone stays, a stranding notice explains why, and, as under DEFER, its + * own guard constant stands the bundled copy down. + */ + public function test_a_network_active_standalone_is_left_when_the_host_is_not_network_active(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Network activation only exists on multisite.' ); + } + + update_site_option( 'active_sitewide_plugins', [ self::STANDALONE => time() ] ); + + // A host basename that is not itself network-active: the bundled copy would not load on the + // sites the standalone is being removed from, which is the whole reason to leave it. + Config::set_host_plugin_basename( self::HOST ); + + $constant = $this->define_guard( 'ABSORBER_E2E_STRANDING_GUARD' ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ], + $constant + ); + + $this->boot(); + + // Must not halt: declining to deactivate leaves nothing to shed and nowhere to redirect. + $this->run_request(); + + $this->assertArrayHasKey( + self::STANDALONE, + (array) get_site_option( 'active_sitewide_plugins', [] ), + 'A standalone whose deactivation would strand sites stays network-active.' + ); + $this->assertArrayHasKey( self::SLUG . ':stranding', $this->queued_notices() ); + + $rendered = $this->render_admin_notices(); + + $this->assertStringContainsString( 'notice-warning', $rendered ); + $this->assertStringContainsString( 'network-activate', $rendered ); + + $this->assertSame( + 0, + $this->bundled_plugin_loads(), + 'The standalone stayed, so its guard constant stands the bundled copy down.' + ); + } + + /** + * The other half: when the host plugin is itself network-active, its bundled copy loads on every + * site the standalone is removed from, so the network-wide deactivation strands nothing and the + * resolver takes it — exactly as it does with no host basename configured at all. + */ + public function test_a_network_active_standalone_is_deactivated_when_the_host_is_network_active(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Network activation only exists on multisite.' ); + } + + update_site_option( + 'active_sitewide_plugins', + [ + self::STANDALONE => time(), + self::HOST => time(), + ] + ); + + Config::set_host_plugin_basename( self::HOST ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ] + ); + + $this->boot(); + + $location = $this->run_halted_request(); + + $this->assertArrayNotHasKey( + self::STANDALONE, + (array) get_site_option( 'active_sitewide_plugins', [] ), + 'With the host network-active too, the standalone is safe to deactivate network-wide.' + ); + $this->assertArrayHasKey( self::SLUG . ':merge', $this->queued_notices() ); + $this->assertSame( admin_url( 'plugins.php' ), $location ); + } + /** * All the way to the screen. The merge notice is the one this library raises exactly once and * never re-queues, so the admin page load after the deactivation has to draw it — and consume it,