From 83562cf34a4d7ff8e4ed7c94f4ea236caf7fbf03 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 3 Aug 2026 16:30:32 +0200 Subject: [PATCH 1/6] Add the conflict resolver and wire it at plugins_loaded priority 1 --- README.md | 18 ++ src/Conflict/Resolver.php | 135 +++++++++ src/Conflict/Resolver_Interface.php | 28 ++ src/Loader.php | 41 ++- tests/unit/Conflict/ResolverTest.php | 401 +++++++++++++++++++++++++++ tests/unit/LoaderBootTest.php | 22 ++ 6 files changed, 644 insertions(+), 1 deletion(-) create mode 100644 src/Conflict/Resolver.php create mode 100644 src/Conflict/Resolver_Interface.php create mode 100644 tests/unit/Conflict/ResolverTest.php diff --git a/README.md b/README.md index b51aa4a..1da9c54 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,23 @@ When a sub-plugin's standalone counterpart is still active: | `Conflict_Policy::DEFER` | Leave the standalone active; the load guard stands the bundled copy down. | | `Conflict_Policy::NOTICE_ONLY` | Leave it active and ask the user to deactivate it. | +A policy the library does not recognise is treated as `NOTICE_ONLY`, never as the default. A typo in +a stored policy should not deactivate a plugin the site owner deliberately turned on. + +### Per-sub-plugin policy override + +`conflict_policy` accepts a `callable( Sub_Plugin ): string`, so one sub-plugin can decide at +runtime without a container and without touching the library: + +```php +'conflict_policy' => static function ( Sub_Plugin $sub_plugin ) { + // Stand down if a newer standalone supersedes the bundled copy. + return my_standalone_version_at_least( $sub_plugin, '3.0.0' ) + ? Conflict_Policy::DEFER + : Conflict_Policy::DEACTIVATE; +}, +``` + ### Sub-plugin configuration | Key | Type | Required | Meaning | @@ -159,6 +176,7 @@ Config::set_container( $container ); |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | | `Contracts\Notices_Interface` | `Notices` | Notice queue and rendering. | +| `Conflict\Resolver_Interface` | `Conflict\Resolver` | Standalone detection, deactivation, redirect. | The default notices queue into the option `{prefix}_plugin_absorber_notices` — a network option on multisite — and render for users who can `activate_plugins`. Read `Notices::option_name()` if you diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php new file mode 100644 index 0000000..2437bf9 --- /dev/null +++ b/src/Conflict/Resolver.php @@ -0,0 +1,135 @@ +is_enabled() || ! $sub_plugin->is_standalone_plugin_active() ) { + continue; + } + + $this->resolve( $sub_plugin ); + } + } + + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @return void + */ + protected function resolve( Sub_Plugin $sub_plugin ): void { + $policy = $sub_plugin->get_conflict_policy(); + + // A host may persist a policy in an option and a filter may return anything. Falling + // through to deactivate() would turn off a plugin the site owner deliberately activated + // on the strength of a typo, so an unrecognised policy takes the conservative branch. + if ( ! Conflict_Policy::is_valid( $policy ) ) { + $policy = Conflict_Policy::NOTICE_ONLY; + } + + switch ( $policy ) { + case Conflict_Policy::DEFER: + // The standalone wins. Its own constant makes the load path skip the bundled copy. + return; + + case Conflict_Policy::NOTICE_ONLY: + Loader::notices()->queue_conflict_notice( $sub_plugin ); + + return; + + case Conflict_Policy::DEACTIVATE: + default: + $this->deactivate( $sub_plugin ); + } + } + + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @return void + */ + protected function deactivate( Sub_Plugin $sub_plugin ): void { + if ( ! function_exists( 'deactivate_plugins' ) ) { + require_once ABSPATH . 'wp-admin/includes/plugin.php'; + } + + // The network flag is evaluated before the call, while the plugin is still active. + // Omitting it makes deactivate_plugins() a silent no-op for a network-activated plugin, + // so the next request would deactivate nothing and redirect again, forever. + deactivate_plugins( + $sub_plugin->get_standalone_plugin_basename(), + false, + $sub_plugin->is_standalone_plugin_network_active() + ); + + // Queued after the deactivation but before the redirect, so the explanation is durable + // whether or not the request goes on to end here. + Loader::notices()->queue_merge_notice( $sub_plugin ); + + $destination = $this->redirect_destination( wp_get_referer() ); + + if ( $destination !== false ) { + wp_safe_redirect( $destination ); + + exit; + } + } + + /** + * Where to send the user after deactivating, or false to stay put. + * + * Never trap the user mid-update: an inline update on the plugins list must not be + * interrupted, and the update screens must not be reloaded. + * + * @since 1.0.0 + * + * @param string|false $referrer Result of wp_get_referer(). + * + * @return string|false + */ + protected function redirect_destination( $referrer ) { + if ( $referrer === false || $referrer === '' ) { + return admin_url( 'plugins.php' ); + } + + foreach ( [ admin_url( 'update.php' ), admin_url( 'update-core.php' ) ] as $update_url ) { + if ( strpos( $referrer, $update_url ) !== false ) { + return admin_url( 'plugins.php' ); + } + } + + if ( strpos( $referrer, admin_url( 'plugins.php' ) ) !== false ) { + return false; + } + + return $referrer; + } +} diff --git a/src/Conflict/Resolver_Interface.php b/src/Conflict/Resolver_Interface.php new file mode 100644 index 0000000..0933db6 --- /dev/null +++ b/src/Conflict/Resolver_Interface.php @@ -0,0 +1,28 @@ +resolve_all(); + } + /** * Whether it is already too late to wire the load hook. * diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php new file mode 100644 index 0000000..05e09b1 --- /dev/null +++ b/tests/unit/Conflict/ResolverTest.php @@ -0,0 +1,401 @@ +> + */ + private $deactivations = []; + + /** + * @var array + */ + private $redirects = []; + + public function setUp(): void { + parent::setUp(); + + Loader::reset(); + Config::reset(); + Config::set_hook_prefix( 'give' ); + $this->clear_notices(); + require_once ABSPATH . 'wp-admin/includes/plugin.php'; + + $this->deactivations = []; + $this->redirects = []; + + // uopz runs a replacement with no class scope, so $this and self:: are both fatal inside + // these closures. Bind references to the properties and resolve the constant to a local. + // See tests/README.md. + $deactivations = &$this->deactivations; + $redirects = &$this->redirects; + $halt_message = self::HALTED_AT_EXIT; + + $this->setFunctionReturn( + 'deactivate_plugins', + static function ( $plugins, $silent = false, $network_wide = null ) use ( &$deactivations ) { + $deactivations[] = [ + 'plugins' => $plugins, + 'silent' => $silent, + 'network_wide' => $network_wide, + ]; + }, + true + ); + + // Throwing here stops the resolver exactly where production calls exit, + // without mocking exit itself. See tests/README.md. + $this->setFunctionReturn( + 'wp_safe_redirect', + static function ( $location ) use ( &$redirects, $halt_message ) { + $redirects[] = $location; + + throw new TestException( $halt_message ); + }, + true + ); + } + + public function tearDown(): void { + $this->clear_notices(); + Loader::reset(); + Config::reset(); + parent::tearDown(); + } + + private function clear_notices(): void { + delete_option( 'give_plugin_absorber_notices' ); + delete_site_option( 'give_plugin_absorber_notices' ); + } + + /** + * @param array $overrides Config overrides. + */ + private function register( array $overrides = [] ): void { + Loader::register( + array_merge( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/give-recurring.php', + 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION_RESOLVER', + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', + ], + $overrides + ) + ); + } + + private function standalone_is( bool $active, bool $network_active = false ): void { + $this->setFunctionReturn( 'is_plugin_active', $active ); + $this->setFunctionReturn( 'is_plugin_active_for_network', $network_active ); + } + + /** + * Runs the resolver, absorbing the TestException that stands in for exit(). + * + * Paths that redirect halt inside wp_safe_redirect(); paths that do not run + * to completion. Either way the assertions afterwards see the same state + * production would have left behind. + * + * @return void + */ + private function resolve(): void { + try { + ( new Resolver() )->resolve_all(); + } catch ( TestException $e ) { + $this->assertSame( self::HALTED_AT_EXIT, $e->getMessage() ); + } + } + + /** + * @return array + */ + private function queued_notices(): array { + $queue = is_multisite() + ? get_site_option( 'give_plugin_absorber_notices', [] ) + : get_option( 'give_plugin_absorber_notices', [] ); + + return is_array( $queue ) ? $queue : []; + } + + public function test_the_loader_resolves_the_default_resolver(): void { + $this->assertInstanceOf( Resolver::class, Loader::resolver() ); + } + + public function test_the_default_resolver_satisfies_the_contract(): void { + $this->assertInstanceOf( Resolver_Interface::class, new Resolver() ); + } + + public function test_deactivate_deactivates_notifies_and_redirects(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::DEACTIVATE ] ); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve(); + + $this->assertCount( 1, $this->deactivations ); + $this->assertSame( 'give-recurring/give-recurring.php', $this->deactivations[0]['plugins'] ); + $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); + $this->assertCount( 1, $this->redirects ); + } + + public function test_deactivate_is_the_default_policy(): void { + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve(); + + $this->assertCount( 1, $this->deactivations ); + } + + public function test_it_passes_the_network_flag_for_a_network_active_standalone(): void { + $this->standalone_is( true, true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve(); + + $this->assertTrue( + $this->deactivations[0]['network_wide'], + 'Without $network_wide, deactivate_plugins() no-ops on a network-activated plugin and the redirect loops forever.' + ); + } + + public function test_it_omits_the_network_flag_for_a_normally_active_standalone(): void { + $this->standalone_is( true, false ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve(); + + $this->assertFalse( $this->deactivations[0]['network_wide'] ); + } + + /** + * 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. + */ + public function test_the_merge_notice_is_queued_before_the_redirect_halts_the_request(): void { + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve(); + + $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); + } + + public function test_defer_does_nothing_at_all(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::DEFER ] ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->redirects ); + $this->assertSame( [], $this->queued_notices() ); + } + + public function test_notice_only_notifies_without_deactivating(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::NOTICE_ONLY ] ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->redirects ); + $this->assertArrayHasKey( 'give-recurring:conflict', $this->queued_notices() ); + } + + /** + * A policy read from an option, or returned by someone else's filter, can be anything. + * Falling through to the destructive branch on a typo would turn off a plugin the site owner + * deliberately activated. + * + * @dataProvider unknown_policies + * + * @param string $policy Policy under test. + */ + public function test_an_unknown_policy_takes_the_conservative_branch( string $policy ): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => $policy ] ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations, 'An unrecognised policy must never deactivate.' ); + $this->assertArrayHasKey( 'give-recurring:conflict', $this->queued_notices() ); + } + + /** + * @return array + */ + public function unknown_policies(): array { + return [ + 'typo' => [ 'defered' ], + 'empty' => [ '' ], + 'wrong case' => [ 'DEACTIVATE' ], + ]; + } + + public function test_a_callable_policy_selects_the_branch(): void { + $this->standalone_is( true ); + $this->register( + [ + 'conflict_policy' => static function ( Sub_Plugin $sub_plugin ) { + return $sub_plugin->get_slug() === 'give-recurring' + ? Conflict_Policy::DEFER + : Conflict_Policy::DEACTIVATE; + }, + ] + ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations, 'The callable chose DEFER for this slug.' ); + } + + public function test_the_filter_can_override_the_policy(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::DEACTIVATE ] ); + + add_filter( + 'give/plugin_absorber/conflict_policy', + static function () { + return Conflict_Policy::DEFER; + } + ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_it_skips_a_disabled_sub_plugin(): void { + $this->standalone_is( true ); + $this->register( [ 'enabled' => false ] ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_it_skips_when_the_standalone_is_not_active(): void { + $this->standalone_is( false, false ); + $this->register(); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_it_skips_a_sub_plugin_with_no_standalone(): void { + $this->standalone_is( true ); + Loader::register( + [ + 'slug' => 'give-fee-recovery', + 'bundled_plugin_file' => '/tmp/give-fee-recovery.php', + 'plugin_loaded_constant' => 'GIVE_FEE_RECOVERY_VERSION_RESOLVER', + ] + ); + + $this->resolve(); + + $this->assertSame( [], $this->deactivations ); + } + + /** + * Exposes the protected redirect logic so it can be asserted directly. + * + * Defined once and reused — the four referrer cases differ only in their input. + */ + private function redirect_resolver(): Resolver { + return new class() extends Resolver { + /** + * @param string|false $referrer Referrer under test. + * + * @return string|false + */ + public function destination_for( $referrer ) { + return $this->redirect_destination( $referrer ); + } + }; + } + + public function test_it_redirects_to_the_plugins_page_without_a_referrer(): void { + $this->assertSame( admin_url( 'plugins.php' ), $this->redirect_resolver()->destination_for( false ) ); + } + + public function test_it_redirects_to_the_plugins_page_for_an_empty_referrer(): void { + $this->assertSame( admin_url( 'plugins.php' ), $this->redirect_resolver()->destination_for( '' ) ); + } + + public function test_it_redirects_to_the_plugins_page_from_an_update_screen(): void { + $resolver = $this->redirect_resolver(); + + $this->assertSame( admin_url( 'plugins.php' ), $resolver->destination_for( admin_url( 'update.php?action=x' ) ) ); + $this->assertSame( admin_url( 'plugins.php' ), $resolver->destination_for( admin_url( 'update-core.php' ) ) ); + } + + public function test_it_does_not_redirect_during_an_inline_update_on_the_plugins_page(): void { + $this->assertFalse( + $this->redirect_resolver()->destination_for( admin_url( 'plugins.php' ) ), + 'Redirecting here would interrupt an inline update.' + ); + } + + public function test_it_returns_any_other_referrer_unchanged(): void { + $this->assertSame( + admin_url( 'options-general.php' ), + $this->redirect_resolver()->destination_for( admin_url( 'options-general.php' ) ) + ); + } + + public function test_it_deactivates_without_redirecting_from_the_plugins_page(): void { + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', admin_url( 'plugins.php' ) ); + + $this->resolve(); + + $this->assertCount( 1, $this->deactivations ); + $this->assertSame( [], $this->redirects, 'Redirecting here would interrupt an inline update.' ); + } + + public function test_resolve_all_needs_a_hook_prefix(): void { + $this->standalone_is( true ); + $this->register(); + Config::reset(); + + $this->expectException( \Nexcess\PluginAbsorber\Exceptions\Config_Exception::class ); + + ( new Resolver() )->resolve_all(); + } +} diff --git a/tests/unit/LoaderBootTest.php b/tests/unit/LoaderBootTest.php index 687657b..6990025 100644 --- a/tests/unit/LoaderBootTest.php +++ b/tests/unit/LoaderBootTest.php @@ -52,6 +52,28 @@ public function test_it_wires_the_load_hook_at_priority_two(): void { $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); } + public function test_it_wires_conflict_resolution_at_priority_one(): void { + Loader::boot(); + + $this->assertSame( + 1, + has_action( 'plugins_loaded', [ Loader::class, 'run_conflict_resolution' ] ) + ); + } + + /** + * Resolution has to run before the load loop: a standalone that wins the conflict defines the + * guard constant, and the load loop reads it. + */ + public function test_conflict_resolution_is_wired_ahead_of_the_load(): void { + Loader::boot(); + + $this->assertLessThan( + has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ), + has_action( 'plugins_loaded', [ Loader::class, 'run_conflict_resolution' ] ) + ); + } + public function test_booting_twice_wires_the_hook_only_once(): void { Loader::boot(); Loader::boot(); From 97767f467b9a1667eb3c4053d127569086e10b6d Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 3 Aug 2026 16:43:25 +0200 Subject: [PATCH 2/6] Address review: stop the destructive path firing outside the admin Conflict resolution deactivates a plugin and ends the request, and it ran at plugins_loaded on every request type with no gate. Known issue B logged this as a logged-out visitor bouncing to the login screen, which is the mildest case. The same path turns a visitor's checkout POST into a 302 that silently drops the order, bounces a login POST back to a blank form, aborts wp-cron before its event loop, and ends a WP-CLI command with status 0 and no output, since header() does nothing under the CLI SAPI. Now gated on is_admin() plus not cron, AJAX or WP-CLI -- is_admin() alone is not enough, because admin-ajax.php and admin-post.php both define WP_ADMIN. deactivate_plugins() is now silent and passes no $network_wide. Verified against core: the default is null, not false, and core takes the network branch on 'false !== $network_wide' and the blog branch on 'true !== $network_wide', so null covers both. The infinite-redirect justification for computing the flag never existed, and passing true skips the blog branch, stranding an entry that needs a second request and a second deactivation hook to clear. Silent because the standalone's own deactivation callback would otherwise run at plugins_loaded, where a routine flush_rewrite_rules() rebuilds the rules before any post type is registered and every custom permalink 404s. Core's automatic deactivations are silent for the same reason. redirect_destination() matches the screen rather than a substring of an absolute URL. wp_get_referer() prefers the _wp_http_referer field that every nonce-bearing admin form carries, and that holds a bare path -- so the 'never interrupt an inline update' guard missed every admin form POST, the whole network admin, and any site behind a TLS-terminating proxy. The shared resolve() test helper caught the halt exception without asserting it arrived, so four tests passed whether or not the redirect happened at all. Two network-flag tests now run against real core instead of a stub, because that claim is the only thing the argument rested on. --- README.md | 7 ++ src/Conflict/Resolver.php | 41 ++++--- src/Loader.php | 33 +++++- src/Sub_Plugin.php | 4 +- tests/unit/Conflict/ResolverTest.php | 156 ++++++++++++++++++++++++--- 5 files changed, 208 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 1da9c54..32a9d47 100644 --- a/README.md +++ b/README.md @@ -110,6 +110,13 @@ When a sub-plugin's standalone counterpart is still active: A policy the library does not recognise is treated as `NOTICE_ONLY`, never as the default. A typo in a stored policy should not deactivate a plugin the site owner deliberately turned on. +`DEACTIVATE` deactivates the standalone and then **ends the request with a redirect**, so it only +runs on ordinary admin page loads — never on the front end, and never during cron, AJAX, or WP-CLI. +Ending a checkout POST or a cron run to deactivate a plugin would cost far more than the conflict +does. The deactivation is silent: the standalone's own deactivation hook is not fired, because at +`plugins_loaded` a routine `flush_rewrite_rules()` in that callback would rebuild the rules before a +single post type is registered. + ### Per-sub-plugin policy override `conflict_policy` accepts a `callable( Sub_Plugin ): string`, so one sub-plugin can decide at diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 2437bf9..1595da5 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -81,14 +81,21 @@ protected function deactivate( Sub_Plugin $sub_plugin ): void { require_once ABSPATH . 'wp-admin/includes/plugin.php'; } - // The network flag is evaluated before the call, while the plugin is still active. - // Omitting it makes deactivate_plugins() a silent no-op for a network-activated plugin, - // so the next request would deactivate nothing and redirect again, forever. - deactivate_plugins( - $sub_plugin->get_standalone_plugin_basename(), - false, - $sub_plugin->is_standalone_plugin_network_active() - ); + // Silent, and with no $network_wide argument. + // + // Silent because this is an unattended deactivation, and the standalone's own deactivation + // hook has already been registered this request. Running it at plugins_loaded means a + // routine flush_rewrite_rules() in that callback regenerates the rules before init has + // registered a single post type, and every custom permalink on the site starts 404ing. + // Core makes the same call: its interactive paths are noisy, its automatic ones -- + // validate_active_plugins(), the plugin upgrader -- are silent. + // + // The $network_wide default is null, not false, and null is the value that handles both + // scopes. Core enters the network branch on `false !== $network_wide` and the blog branch + // on `true !== $network_wide`, so null takes both. Passing a computed true would skip the + // blog branch, stranding an entry for a plugin that is active in both, which then takes a + // second request and a second deactivation hook to clear. + deactivate_plugins( $sub_plugin->get_standalone_plugin_basename(), true ); // Queued after the deactivation but before the redirect, so the explanation is durable // whether or not the request goes on to end here. @@ -116,17 +123,23 @@ protected function deactivate( Sub_Plugin $sub_plugin ): void { * @return string|false */ protected function redirect_destination( $referrer ) { - if ( $referrer === false || $referrer === '' ) { + if ( ! is_string( $referrer ) || $referrer === '' ) { return admin_url( 'plugins.php' ); } - foreach ( [ admin_url( 'update.php' ), admin_url( 'update-core.php' ) ] as $update_url ) { - if ( strpos( $referrer, $update_url ) !== false ) { - return admin_url( 'plugins.php' ); - } + // Match on the screen, not on a substring of an absolute URL. wp_get_referer() prefers + // the _wp_http_referer field that every nonce-bearing admin form carries, and that field + // holds a bare path -- so comparing against admin_url() misses every admin form POST, + // misses the network admin entirely, and misses any site behind a TLS-terminating proxy + // where admin_url() says http and the referrer says https. + $screen = basename( (string) wp_parse_url( $referrer, PHP_URL_PATH ) ); + + if ( $screen === 'update.php' || $screen === 'update-core.php' ) { + return admin_url( 'plugins.php' ); } - if ( strpos( $referrer, admin_url( 'plugins.php' ) ) !== false ) { + // Staying put: a redirect here would drop a bulk action or interrupt an inline update. + if ( $screen === 'plugins.php' ) { return false; } diff --git a/src/Loader.php b/src/Loader.php index 310d0a9..811dd90 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -197,6 +197,10 @@ public static function reset(): void { * container optional. Each trampoline delegates to the resolved collaborator, so rebinding * still takes effect. * + * Called too late, this runs the work inline instead of wiring it — and conflict resolution + * can end the request, so on an admin page load this call may not return. Boot at + * plugins_loaded priority 0, as documented, and it always returns. + * * @since 1.0.0 * * @return void @@ -245,13 +249,40 @@ public static function boot(): void { * @return void */ public static function run_conflict_resolution(): void { - if ( ! self::has_hook_prefix() ) { + if ( ! self::is_interactive_admin_request() || ! self::has_hook_prefix() ) { return; } self::resolver()->resolve_all(); } + /** + * Whether this request is one a person is watching in wp-admin. + * + * Conflict resolution deactivates a plugin and ends the request, so it must only run where + * someone is there to see the result. Unguarded it fires at plugins_loaded on every request: + * a visitor's checkout POST becomes a 302 that drops the order, a login POST bounces back to + * a blank form, wp-cron never reaches its event loop, and a WP-CLI command exits 0 having + * printed nothing, because header() is a no-op under the CLI SAPI. + * + * is_admin() alone is not enough: admin-ajax.php and admin-post.php both define WP_ADMIN. + * + * @since 1.0.0 + * + * @return bool + */ + private static function is_interactive_admin_request(): bool { + if ( defined( 'WP_CLI' ) && WP_CLI ) { + return false; + } + + if ( wp_doing_cron() || wp_doing_ajax() ) { + return false; + } + + return is_admin(); + } + /** * Whether it is already too late to wire the load hook. * diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index e4a8234..c45fb1f 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -196,8 +196,8 @@ public function is_standalone_plugin_active(): bool { /** * Whether the standalone is network-activated. * - * Deactivating it requires passing $network_wide to deactivate_plugins(); without that the - * call silently no-ops and the resolver redirects forever. + * Informational. Deactivating does not need it: core's `$network_wide` default of null already + * covers both scopes, and passing a computed value is worse than omitting it. * * @since 1.0.0 * diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index 05e09b1..5cf283c 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -84,6 +84,7 @@ static function ( $location ) use ( &$redirects, $halt_message ) { } public function tearDown(): void { + set_current_screen( 'front' ); $this->clear_notices(); Loader::reset(); Config::reset(); @@ -120,18 +121,35 @@ private function standalone_is( bool $active, bool $network_active = false ): vo /** * Runs the resolver, absorbing the TestException that stands in for exit(). * - * Paths that redirect halt inside wp_safe_redirect(); paths that do not run - * to completion. Either way the assertions afterwards see the same state - * production would have left behind. + * Paths that redirect halt inside wp_safe_redirect(); paths that do not run to completion. + * Either way the assertions afterwards see the same state production would have left behind. + * + * $expects_halt is not optional decoration. Catching the exception without asserting that it + * arrived turns "the resolver never redirected at all" into a silent pass — the exact failure + * tests/README.md opens by warning about. + * + * @param bool $expects_halt Whether the resolver must stop where production calls exit(). * * @return void */ - private function resolve(): void { + private function resolve( bool $expects_halt = false ): void { + $halted = false; + try { ( new Resolver() )->resolve_all(); } catch ( TestException $e ) { + $halted = true; + $this->assertSame( self::HALTED_AT_EXIT, $e->getMessage() ); } + + $this->assertSame( + $expects_halt, + $halted, + $expects_halt + ? 'The resolver must stop where production calls exit().' + : 'The resolver must not end the request on this path.' + ); } /** @@ -158,7 +176,7 @@ public function test_deactivate_deactivates_notifies_and_redirects(): void { $this->register( [ 'conflict_policy' => Conflict_Policy::DEACTIVATE ] ); $this->setFunctionReturn( 'wp_get_referer', false ); - $this->resolve(); + $this->resolve( true ); $this->assertCount( 1, $this->deactivations ); $this->assertSame( 'give-recurring/give-recurring.php', $this->deactivations[0]['plugins'] ); @@ -171,32 +189,71 @@ public function test_deactivate_is_the_default_policy(): void { $this->register(); $this->setFunctionReturn( 'wp_get_referer', false ); - $this->resolve(); + $this->resolve( true ); $this->assertCount( 1, $this->deactivations ); } - public function test_it_passes_the_network_flag_for_a_network_active_standalone(): void { + /** + * Silent, and with no $network_wide argument — core's default of null is what handles both + * scopes, and the standalone's deactivation hook must not run at plugins_loaded. + */ + public function test_it_deactivates_silently_and_lets_core_decide_the_scope(): void { $this->standalone_is( true, true ); $this->register(); $this->setFunctionReturn( 'wp_get_referer', false ); - $this->resolve(); + $this->resolve( true ); - $this->assertTrue( + $this->assertTrue( $this->deactivations[0]['silent'], 'An unattended deactivation must be silent.' ); + $this->assertNull( $this->deactivations[0]['network_wide'], - 'Without $network_wide, deactivate_plugins() no-ops on a network-activated plugin and the redirect loops forever.' + 'Core enters the network branch on false !== $network_wide and the blog branch on true !== $network_wide, so null takes both.' ); } - public function test_it_omits_the_network_flag_for_a_normally_active_standalone(): void { - $this->standalone_is( true, false ); - $this->register(); + /** + * Against real core rather than a stub, because the whole reason the scope argument was + * dropped is a claim about what core does with the default. + */ + public function test_it_really_deactivates_a_site_active_standalone(): void { + $this->unsetFunctionReturn( 'deactivate_plugins' ); + + $basename = 'absorber-fixture/absorber-fixture.php'; + update_option( 'active_plugins', [ $basename ] ); + + $this->register( [ 'standalone_plugin_basename' => $basename ] ); $this->setFunctionReturn( 'wp_get_referer', false ); - $this->resolve(); + $this->resolve( true ); + + $this->assertNotContains( $basename, (array) get_option( 'active_plugins', [] ) ); - $this->assertFalse( $this->deactivations[0]['network_wide'] ); + delete_option( 'active_plugins' ); + } + + public function test_it_really_deactivates_a_network_active_standalone(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Network activation only exists on multisite.' ); + } + + $this->unsetFunctionReturn( 'deactivate_plugins' ); + + $basename = 'absorber-fixture/absorber-fixture.php'; + update_site_option( 'active_sitewide_plugins', [ $basename => time() ] ); + + $this->register( [ 'standalone_plugin_basename' => $basename ] ); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->resolve( true ); + + $this->assertArrayNotHasKey( + $basename, + (array) get_site_option( 'active_sitewide_plugins', [] ), + 'Omitting $network_wide must still clear a network activation.' + ); + + delete_site_option( 'active_sitewide_plugins' ); } /** @@ -208,7 +265,7 @@ public function test_the_merge_notice_is_queued_before_the_redirect_halts_the_re $this->register(); $this->setFunctionReturn( 'wp_get_referer', false ); - $this->resolve(); + $this->resolve( true ); $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); } @@ -389,6 +446,73 @@ public function test_it_deactivates_without_redirecting_from_the_plugins_page(): $this->assertSame( [], $this->redirects, 'Redirecting here would interrupt an inline update.' ); } + /** + * The trampoline gates the destructive path on the request being one a person is watching. + * Unguarded it fires on every request: a visitor's checkout POST becomes a 302 that drops the + * order, wp-cron never reaches its event loop, and a WP-CLI command exits having printed + * nothing, because header() does nothing under the CLI SAPI. + */ + public function test_the_trampoline_does_not_resolve_on_a_front_end_request(): void { + set_current_screen( 'front' ); + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + Loader::run_conflict_resolution(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->redirects ); + } + + public function test_the_trampoline_resolves_on_an_admin_request(): void { + set_current_screen( 'dashboard' ); + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + try { + Loader::run_conflict_resolution(); + $this->fail( 'Expected the resolver to stop where production calls exit().' ); + } catch ( TestException $e ) { + $this->assertSame( self::HALTED_AT_EXIT, $e->getMessage() ); + } + + $this->assertCount( 1, $this->deactivations ); + } + + public function test_the_trampoline_does_not_resolve_during_cron(): void { + set_current_screen( 'dashboard' ); + $this->setConstant( 'DOING_CRON', true ); + $this->standalone_is( true ); + $this->register(); + + Loader::run_conflict_resolution(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_the_trampoline_does_not_resolve_during_ajax(): void { + set_current_screen( 'dashboard' ); + $this->setConstant( 'DOING_AJAX', true ); + $this->standalone_is( true ); + $this->register(); + + Loader::run_conflict_resolution(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_the_trampoline_does_not_resolve_under_wp_cli(): void { + set_current_screen( 'dashboard' ); + $this->setConstant( 'WP_CLI', true ); + $this->standalone_is( true ); + $this->register(); + + Loader::run_conflict_resolution(); + + $this->assertSame( [], $this->deactivations ); + } + public function test_resolve_all_needs_a_hook_prefix(): void { $this->standalone_is( true ); $this->register(); From 4f731e0e951527ab7db7c11642ad0704f33504c9 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 3 Aug 2026 16:43:25 +0200 Subject: [PATCH 3/6] Plan: correct the network_wide reasoning and un-defer issue B --- .../plans/2026-07-31-plugin-absorber.md | 41 ++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index 7cb2be7..d72fbfa 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -3850,8 +3850,34 @@ loading anything.' - Create: `src/Conflict/Resolver_Interface.php`, `src/Conflict/Resolver.php`, `tests/unit/Conflict/ResolverTest.php` - Modify: `src/Loader.php` (add `resolver()` and the @1 hook), `README.md` +> **Deviations, deliberate (added 2026-08-03, from the PR 12 review):** +> +> 1. **`deactivate_plugins()` is called with no `$network_wide` argument, and the plan's reasoning +> for passing one is factually wrong.** The plan says omitting it makes the call "a silent no-op +> for a network-activated plugin", producing an endless redirect. Verified against core: the +> default is `null`, not `false`. Core enters the network branch on `false !== $network_wide` and +> the blog branch on `true !== $network_wide`, so `null` takes **both**. The infinite loop never +> existed. Worse, passing a computed `true` *skips* the blog branch, so a plugin that is both +> network-active and listed in a blog's `active_plugins` keeps that entry and needs a second +> request — and a second deactivation hook — to clear. Two tests now exercise real core rather +> than a stub, since this claim is the only thing the argument rested on. +> 2. **The deactivation is silent.** With `$silent = false` core fires `deactivate_plugin` and the +> standalone's own `register_deactivation_hook()` callback — at `plugins_loaded`, before `init`. +> A routine `flush_rewrite_rules()` there rebuilds the rules with no post type or taxonomy +> registered, and every custom permalink on the site 404s. Core's own automatic deactivations, +> `validate_active_plugins()` and the plugin upgrader, both pass `true`; only its interactive +> admin paths are noisy. +> 3. **`redirect_destination()` matches the screen, not a substring of an absolute URL.** +> `wp_get_referer()` prefers the `_wp_http_referer` field that every nonce-bearing admin form +> carries, and that field holds a bare *path* — so comparing against `admin_url()` missed every +> admin form POST, the whole network admin, and any site behind a TLS-terminating proxy where +> `admin_url()` says http and the referrer says https. The "never interrupt an inline update" +> guard the plan is proudest of did not fire for a plugins.php bulk action. +> 4. **The destructive path is gated on the request context** — see the correction to deferred +> issue B. + **Interfaces:** -- Consumes: `Loader::all()` (Task 9), `Loader::notices()` (Task 10), `Sub_Plugin::is_standalone_plugin_active()` / `is_standalone_plugin_network_active()` / `get_conflict_policy()` (Task 7), `Conflict_Policy::*` (Task 6). +- Consumes: `Loader::all()` (Task 9), `Loader::notices()` (Task 10), `Sub_Plugin::is_standalone_plugin_active()` / `get_conflict_policy()` (Task 7), `Conflict_Policy::*` (Task 6). - Produces: - `Conflict\Resolver_Interface` with `resolve_all(): void` - `Conflict\Resolver::redirect_destination( $referrer )` — `protected`, returns `string|false` @@ -5789,10 +5815,15 @@ Submit `https://github.com/stellarwp/plugin-absorber` at Date: Thu, 6 Aug 2026 09:36:29 +0200 Subject: [PATCH 4/6] Address review: gate resolution on a GET and widen the too-late guard The context gate let every admin form submission through: admin-post.php and options.php define WP_ADMIN and never define DOING_AJAX, so a POST to either was deactivated and 302'd, and the browser followed with a GET that had no body. Resolution now requires a GET and waits for the next page view. boot()'s too-late guard still measured against the load priority, so booting at plugins_loaded priority 1 wired both hooks, ran the load loop, and dropped conflict resolution without reporting anything. --- README.md | 14 ++++--- .../plans/2026-07-31-plugin-absorber.md | 12 +++++- src/Conflict/Resolver.php | 9 +++-- src/Loader.php | 26 +++++++++---- tests/unit/Conflict/ResolverTest.php | 39 ++++++++++++++++++- tests/unit/LoaderBootTest.php | 38 ++++++++++++++++++ 6 files changed, 119 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 32a9d47..0a55e75 100644 --- a/README.md +++ b/README.md @@ -64,9 +64,9 @@ last, so it is only consulted for a sub-plugin that would otherwise have loaded Keep the `, 0` on the bootstrap hook. `boot()` wires its work at `plugins_loaded` priorities 1 and 2, and WordPress silently ignores a callback added at or past the priority the running dispatch has -reached — including the priority it is running right now. Booting that late is detected and reported -through `_doing_it_wrong()`, and the load runs inline instead, but the ordering guarantees are -weaker. +reached — including the priority it is running right now, so priority 1 is already too late. Booting +that late is detected and reported through `_doing_it_wrong()`, and both the conflict resolution and +the load run inline instead, but the ordering guarantees are weaker. ### The bundled file is included from a function, not from global scope @@ -110,10 +110,12 @@ When a sub-plugin's standalone counterpart is still active: A policy the library does not recognise is treated as `NOTICE_ONLY`, never as the default. A typo in a stored policy should not deactivate a plugin the site owner deliberately turned on. -`DEACTIVATE` deactivates the standalone and then **ends the request with a redirect**, so it only -runs on ordinary admin page loads — never on the front end, and never during cron, AJAX, or WP-CLI. +`DEACTIVATE` deactivates the standalone and then **ends the request with a redirect**, so conflict +resolution only runs on an admin page view: a `GET`, in `wp-admin`, outside cron, AJAX, and WP-CLI. Ending a checkout POST or a cron run to deactivate a plugin would cost far more than the conflict -does. The deactivation is silent: the standalone's own deactivation hook is not fired, because at +does, and a redirect turns any submitted form — including one posted to `admin-post.php`, which is +`is_admin()` — into a `GET` that arrives without its body. Whatever is skipped is picked up on the +next page view; the standalone is still there to find. The deactivation is silent: the standalone's own deactivation hook is not fired, because at `plugins_loaded` a routine `flush_rewrite_rules()` in that callback would rebuild the rules before a single post type is registered. diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index d72fbfa..2e44d36 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -3874,7 +3874,17 @@ loading anything.' > `admin_url()` says http and the referrer says https. The "never interrupt an inline update" > guard the plan is proudest of did not fire for a plugins.php bulk action. > 4. **The destructive path is gated on the request context** — see the correction to deferred -> issue B. +> issue B. The gate is `GET`, in the admin, outside cron/AJAX/WP-CLI. `is_admin()` and +> `wp_doing_ajax()` between them do not cover `admin-post.php` or `options.php`: both define +> `WP_ADMIN`, neither defines `DOING_AJAX`, and a redirect turns the form POST into a bodyless +> `GET`. Requiring a `GET` covers every admin form submission at once, which is also what core +> does in `wp_cron()`. Nothing is lost — the standalone is still active on the next page view. +> 5. **`boot()`'s too-late guard measures against priority 1, not 2.** Task 11 fixed the comparison +> to be inclusive; adding the @1 hook here moved the boundary with it. Left at 2, booting from +> `plugins_loaded` at priority 1 would wire both hooks, run the load loop, and silently drop +> conflict resolution — the standalone stays active, the bundled copy stands down behind its +> guard constant, and nothing is reported. Wiring is all-or-nothing, so the guard now trips at +> the earlier of the two priorities and does both inline. **Interfaces:** - Consumes: `Loader::all()` (Task 9), `Loader::notices()` (Task 10), `Sub_Plugin::is_standalone_plugin_active()` / `get_conflict_policy()` (Task 7), `Conflict_Policy::*` (Task 6). diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 1595da5..d357a72 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -113,8 +113,10 @@ protected function deactivate( Sub_Plugin $sub_plugin ): void { /** * Where to send the user after deactivating, or false to stay put. * - * Never trap the user mid-update: an inline update on the plugins list must not be - * interrupted, and the update screens must not be reloaded. + * The point of the redirect is to re-render whatever the user was looking at now that the + * standalone is gone. Two referrers are handled specially: the update screens, where reloading + * would re-run an update, and the plugins list, which already reads plugin state fresh — so + * sending them back there would only cost a round trip. * * @since 1.0.0 * @@ -138,7 +140,8 @@ protected function redirect_destination( $referrer ) { return admin_url( 'plugins.php' ); } - // Staying put: a redirect here would drop a bulk action or interrupt an inline update. + // Staying put. Core lands here after a bulk action, and the list is about to render the + // deactivation we just made anyway. if ( $screen === 'plugins.php' ) { return false; } diff --git a/src/Loader.php b/src/Loader.php index 811dd90..9d28a95 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -224,10 +224,10 @@ public static function boot(): void { // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with // no warning and a site that looks entirely healthy. - if ( self::load_priority_has_passed() ) { + if ( self::wiring_window_has_closed() ) { _doing_it_wrong( __METHOD__, - 'Loader::boot() must run before plugins_loaded priority 2. Resolving and loading inline instead.', + 'Loader::boot() must run before plugins_loaded priority 1. Resolving and loading inline instead.', '1.0.0' ); @@ -280,23 +280,35 @@ private static function is_interactive_admin_request(): bool { return false; } + // Only a GET. A redirect discards the request, and the browser follows it with a GET, so + // anything submitted is gone -- which is exactly what would happen to a form posted to + // admin-post.php or options.php, both of which define WP_ADMIN and neither of which + // wp_doing_ajax() catches. Core draws the same line in wp_cron(). Deferring resolution to + // the next page view costs nothing: the standalone is still there to detect. + if ( ( $_SERVER['REQUEST_METHOD'] ?? 'GET' ) !== 'GET' ) { + return false; + } + return is_admin(); } /** - * Whether it is already too late to wire the load hook. + * Whether it is already too late to wire either hook. + * + * Measured against the earlier of the two priorities. Wiring is all-or-nothing: reaching + * priority 1 loses conflict resolution while leaving the load loop at 2 to run, which is worse + * than doing both inline — the standalone stays active, the bundled copy stands down behind its + * guard constant, and nothing is reported. * * The comparison is inclusive. A callback added to the priority currently being dispatched is * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` * with a by-value foreach, so the append lands on an array the running loop has already copied. - * Booting from plugins_loaded at priority 2 is the case a host is likeliest to hit by accident, - * and an exclusive comparison would let exactly that one through unreported. * * @since 1.0.0 * * @return bool */ - private static function load_priority_has_passed(): bool { + private static function wiring_window_has_closed(): bool { if ( ! did_action( 'plugins_loaded' ) ) { return false; } @@ -307,7 +319,7 @@ private static function load_priority_has_passed(): bool { $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; - return $hook instanceof \WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + return $hook instanceof \WP_Hook && $hook->current_priority() >= self::RESOLVE_PRIORITY; } /** diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index 5cf283c..d377e0b 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -39,6 +39,11 @@ class ResolverTest extends WPTestCase { */ private $redirects = []; + /** + * @var string|null + */ + private $request_method; + public function setUp(): void { parent::setUp(); @@ -51,6 +56,11 @@ public function setUp(): void { $this->deactivations = []; $this->redirects = []; + // Set explicitly rather than inherited from the harness: resolution is gated on the request + // being a GET, so every test that expects it to run depends on this value. + $this->request_method = $_SERVER['REQUEST_METHOD'] ?? null; + $_SERVER['REQUEST_METHOD'] = 'GET'; + // uopz runs a replacement with no class scope, so $this and self:: are both fatal inside // these closures. Bind references to the properties and resolve the constant to a local. // See tests/README.md. @@ -84,6 +94,12 @@ static function ( $location ) use ( &$redirects, $halt_message ) { } public function tearDown(): void { + if ( $this->request_method === null ) { + unset( $_SERVER['REQUEST_METHOD'] ); + } else { + $_SERVER['REQUEST_METHOD'] = $this->request_method; + } + set_current_screen( 'front' ); $this->clear_notices(); Loader::reset(); @@ -424,7 +440,7 @@ public function test_it_redirects_to_the_plugins_page_from_an_update_screen(): v public function test_it_does_not_redirect_during_an_inline_update_on_the_plugins_page(): void { $this->assertFalse( $this->redirect_resolver()->destination_for( admin_url( 'plugins.php' ) ), - 'Redirecting here would interrupt an inline update.' + 'Coming from the plugins list, there is nothing to send the user back to.' ); } @@ -443,7 +459,7 @@ public function test_it_deactivates_without_redirecting_from_the_plugins_page(): $this->resolve(); $this->assertCount( 1, $this->deactivations ); - $this->assertSame( [], $this->redirects, 'Redirecting here would interrupt an inline update.' ); + $this->assertSame( [], $this->redirects, 'Coming from the plugins list, there is nothing to send the user back to.' ); } /** @@ -480,6 +496,25 @@ public function test_the_trampoline_resolves_on_an_admin_request(): void { $this->assertCount( 1, $this->deactivations ); } + /** + * admin-post.php and options.php define WP_ADMIN and never define DOING_AJAX, so is_admin() is + * true and wp_doing_ajax() is false. Deactivating and redirecting there turns a submitted form + * into a 302 the browser follows with a GET, and the submission is gone — the same data loss the + * gate exists to prevent, one layer in. Nothing is lost by waiting for the next page view. + */ + public function test_the_trampoline_does_not_resolve_on_an_admin_form_submission(): void { + set_current_screen( 'dashboard' ); + $_SERVER['REQUEST_METHOD'] = 'POST'; + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + Loader::run_conflict_resolution(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->redirects ); + } + public function test_the_trampoline_does_not_resolve_during_cron(): void { set_current_screen( 'dashboard' ); $this->setConstant( 'DOING_CRON', true ); diff --git a/tests/unit/LoaderBootTest.php b/tests/unit/LoaderBootTest.php index 6990025..91b3131 100644 --- a/tests/unit/LoaderBootTest.php +++ b/tests/unit/LoaderBootTest.php @@ -7,7 +7,9 @@ use Codeception\TestCase\WPTestCase; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Conflict\Resolver_Interface; use Nexcess\PluginAbsorber\Loader; +use Nexcess\PluginAbsorber\Tests\Support\Test_Container; /** * @since 1.0.0 @@ -34,6 +36,7 @@ public function setUp(): void { } public function tearDown(): void { + unset( $_SERVER['REQUEST_METHOD'] ); $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; // In tearDown rather than at the end of the test body: a failing assertion would otherwise // leak an admin screen into every test that runs after it, since is_admin() checks the @@ -153,12 +156,47 @@ static function () use ( $path, $priority ) { */ public function late_boot_priorities(): array { return [ + 'at the resolve priority' => [ 1 ], 'at the load priority' => [ 2 ], 'one past it' => [ 3 ], 'the default a host omits' => [ 10 ], ]; } + /** + * Booting at priority 1 still leaves priority 2 reachable, so the load happens and the site + * looks fine — but the conflict-resolution hook lands on the bucket being dispatched and never + * fires. A standalone would stay active with nothing reported. + */ + public function test_booting_at_the_resolve_priority_still_resolves_conflicts(): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); + + // Resolution runs only on an admin page view, and this test is about when it is wired. + set_current_screen( 'dashboard' ); + $_SERVER['REQUEST_METHOD'] = 'GET'; + + $resolver = new class() implements Resolver_Interface { + /** + * @var int + */ + public $calls = 0; + + public function resolve_all(): void { + ++$this->calls; + } + }; + + $container = new Test_Container(); + $container->singleton( Resolver_Interface::class, static fn() => $resolver ); + Config::set_container( $container ); + + add_action( 'plugins_loaded', [ Loader::class, 'boot' ], 1 ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $resolver->calls, 'A boot at priority 1 must still resolve conflicts.' ); + } + public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); From 47c380887030c2bf3dd3fb9c07a002fe3c0c8ae8 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Fri, 7 Aug 2026 14:45:38 +0200 Subject: [PATCH 5/6] Address review: move the conflict default and the WP calls off Sub_Plugin Sub_Plugin named DEACTIVATE as its own fallback, which put "which policy applies when none is configured" in the object that only holds one sub-plugin's config. Conflict_Policy::default() states it now. The two fallbacks stay different, and say why: unconfigured means the sub-plugin accepted the default, whereas an unrecognised policy is a value nobody chose, and reading a typo as consent to deactivate is the outcome worth refusing. Plugin_State_Interface becomes the library's only route to WordPress's plugin functions. Sub_Plugin was a config value object that also queried global plugin state and required wp-admin/includes/plugin.php; Resolver required the same file guarded on a different function. One gateway, one include, one guard -- on deactivate_plugins, a function the library still calls, and still not is_plugin_active, whose third-party shims would short-circuit the require. is_standalone_plugin_active() is gone rather than delegated: its only caller already reaches collaborators through Loader, so forwarding would have bought Sub_Plugin a dependency on Loader to answer a question that was never about its configuration. is_standalone_plugin_network_active() had no production callers at all and is deleted. Sub_Plugin now makes no global WordPress calls beyond the defined() that is intrinsic to it, and its tests no longer stub is_plugin_active. --- README.md | 8 +- src/Conflict/Resolver.php | 26 +--- src/Conflict_Policy.php | 21 ++- src/Contracts/Plugin_State_Interface.php | 47 ++++++ src/Loader.php | 12 ++ src/Plugin_State.php | 78 ++++++++++ src/Sub_Plugin.php | 65 +------- tests/unit/ConflictPolicyTest.php | 12 ++ tests/unit/PluginStateTest.php | 184 +++++++++++++++++++++++ tests/unit/SubPluginTest.php | 91 ++--------- 10 files changed, 381 insertions(+), 163 deletions(-) create mode 100644 src/Contracts/Plugin_State_Interface.php create mode 100644 src/Plugin_State.php create mode 100644 tests/unit/PluginStateTest.php diff --git a/README.md b/README.md index 0a55e75..150cb5e 100644 --- a/README.md +++ b/README.md @@ -185,7 +185,13 @@ Config::set_container( $container ); |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | | `Contracts\Notices_Interface` | `Notices` | Notice queue and rendering. | -| `Conflict\Resolver_Interface` | `Conflict\Resolver` | Standalone detection, deactivation, redirect. | +| `Contracts\Plugin_State_Interface` | `Plugin_State` | Whether a plugin is active, and turning one off. | +| `Conflict\Resolver_Interface` | `Conflict\Resolver` | Which sub-plugins conflict, the policy branch, the redirect. | + +`Plugin_State` is the only place the library calls WordPress's plugin functions, so binding it is +how you keep the absorber away from `is_plugin_active()` and `deactivate_plugins()` entirely — +useful where plugin state is managed outside WordPress, and where a test wants the decisions +without the side effects. The default notices queue into the option `{prefix}_plugin_absorber_notices` — a network option on multisite — and render for users who can `activate_plugins`. Read `Notices::option_name()` if you diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index d357a72..6a6f332 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -28,7 +28,11 @@ public function resolve_all(): void { continue; } - if ( ! $sub_plugin->is_enabled() || ! $sub_plugin->is_standalone_plugin_active() ) { + if ( ! $sub_plugin->is_enabled() || ! $sub_plugin->has_standalone_plugin() ) { + continue; + } + + if ( ! Loader::plugin_state()->is_active( $sub_plugin->get_standalone_plugin_basename() ) ) { continue; } @@ -77,25 +81,7 @@ protected function resolve( Sub_Plugin $sub_plugin ): void { * @return void */ protected function deactivate( Sub_Plugin $sub_plugin ): void { - if ( ! function_exists( 'deactivate_plugins' ) ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - } - - // Silent, and with no $network_wide argument. - // - // Silent because this is an unattended deactivation, and the standalone's own deactivation - // hook has already been registered this request. Running it at plugins_loaded means a - // routine flush_rewrite_rules() in that callback regenerates the rules before init has - // registered a single post type, and every custom permalink on the site starts 404ing. - // Core makes the same call: its interactive paths are noisy, its automatic ones -- - // validate_active_plugins(), the plugin upgrader -- are silent. - // - // The $network_wide default is null, not false, and null is the value that handles both - // scopes. Core enters the network branch on `false !== $network_wide` and the blog branch - // on `true !== $network_wide`, so null takes both. Passing a computed true would skip the - // blog branch, stranding an entry for a plugin that is active in both, which then takes a - // second request and a second deactivation hook to clear. - deactivate_plugins( $sub_plugin->get_standalone_plugin_basename(), true ); + Loader::plugin_state()->deactivate( $sub_plugin->get_standalone_plugin_basename() ); // Queued after the deactivation but before the redirect, so the explanation is durable // whether or not the request goes on to end here. diff --git a/src/Conflict_Policy.php b/src/Conflict_Policy.php index 713aceb..a345ee3 100644 --- a/src/Conflict_Policy.php +++ b/src/Conflict_Policy.php @@ -15,8 +15,6 @@ final class Conflict_Policy { * Deactivate the standalone, notify, and redirect. The bundled copy loads on the next * request, since the standalone has already defined the guard constant on this one. * - * The default. - * * @since 1.0.0 * * @var string @@ -41,6 +39,25 @@ final class Conflict_Policy { */ public const NOTICE_ONLY = 'notice_only'; + /** + * The policy that applies when a sub-plugin configures none. + * + * Deactivating is the default because two copies of the same plugin are the failure this + * library exists to prevent, and a sub-plugin that has not thought about the question wants + * the outcome where its bundled copy ends up running. + * + * Distinct from the branch a caller takes for a policy it does not recognise: not configuring + * one is a choice to accept the default, whereas an unrecognised value is a value nobody + * chose, and reading it as consent to deactivate would act on a typo. + * + * @since 1.0.0 + * + * @return string + */ + public static function default(): string { + return self::DEACTIVATE; + } + /** * Every policy this library understands. * diff --git a/src/Contracts/Plugin_State_Interface.php b/src/Contracts/Plugin_State_Interface.php new file mode 100644 index 0000000..23f272d --- /dev/null +++ b/src/Contracts/Plugin_State_Interface.php @@ -0,0 +1,47 @@ +load_plugin_functions(); + + // WordPress's own is_plugin_active() already ORs in the network check, so asking + // is_plugin_active_for_network() as well would only buy a second get_site_option() per + // sub-plugin per request. + return is_plugin_active( $basename ); + } + + /** + * @since 1.0.0 + * + * @param string $basename Plugin basename. + * + * @return void + */ + public function deactivate( string $basename ): void { + $this->load_plugin_functions(); + + // Silent, and with no $network_wide argument. + // + // Silent because this is an unattended deactivation, and the standalone's own deactivation + // hook has already been registered this request. Running it at plugins_loaded means a + // routine flush_rewrite_rules() in that callback regenerates the rules before init has + // registered a single post type, and every custom permalink on the site starts 404ing. + // Core makes the same call: its interactive paths are noisy, its automatic ones -- + // validate_active_plugins(), the plugin upgrader -- are silent. + // + // The $network_wide default is null, not false, and null is the value that handles both + // scopes. Core enters the network branch on `false !== $network_wide` and the blog branch + // on `true !== $network_wide`, so null takes both. Passing a computed true would skip the + // blog branch, stranding an entry for a plugin that is active in both, which then takes a + // second request and a second deactivation hook to clear. + deactivate_plugins( $basename, true ); + } + + /** + * WordPress only loads these in the admin, and we run at plugins_loaded on every request. + * + * Guarded on deactivate_plugins() rather than is_plugin_active(), because the latter is a + * common third-party shim: something else defining it would short-circuit this and leave the + * rest of the file unloaded, so the first call that needs a function nobody shimmed fatals. + * + * @since 1.0.0 + * + * @return void + */ + private function load_plugin_functions(): void { + if ( ! function_exists( 'deactivate_plugins' ) ) { + require_once ABSPATH . 'wp-admin/includes/plugin.php'; + } + } +} diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index c45fb1f..c6878d7 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -8,7 +8,11 @@ use Nexcess\PluginAbsorber\Exceptions\Config_Exception; /** - * One registered sub-plugin: its configuration and every decision about it. + * One registered sub-plugin: its configuration, and the answers that configuration alone decides. + * + * Deliberately not a window onto WordPress. Asking whether the standalone counterpart is active is + * a question about the site rather than about this configuration, and it belongs to + * Plugin_State_Interface; this object only names the plugin to ask about. * * @since 1.0.0 */ @@ -118,7 +122,7 @@ public function get_plugin_loaded_constant(): string { * @return string */ public function get_conflict_policy(): string { - $policy = $this->config['conflict_policy'] ?? Conflict_Policy::DEACTIVATE; + $policy = $this->config['conflict_policy'] ?? Conflict_Policy::default(); $policy = apply_filters( Config::get_hook_prefix() . '/plugin_absorber/conflict_policy', @@ -173,46 +177,6 @@ public function get_standalone_plugin_basename(): string { return (string) ( $this->config['standalone_plugin_basename'] ?? '' ); } - /** - * Whether the standalone is active, site-wide or network-wide. - * - * WordPress's own is_plugin_active() already ORs in the network check, so asking it again - * here would only buy a second get_site_option() per sub-plugin per request. - * - * @since 1.0.0 - * - * @return bool - */ - public function is_standalone_plugin_active(): bool { - if ( ! $this->has_standalone_plugin() ) { - return false; - } - - $this->load_plugin_functions(); - - return is_plugin_active( $this->get_standalone_plugin_basename() ); - } - - /** - * Whether the standalone is network-activated. - * - * Informational. Deactivating does not need it: core's `$network_wide` default of null already - * covers both scopes, and passing a computed value is worse than omitting it. - * - * @since 1.0.0 - * - * @return bool - */ - public function is_standalone_plugin_network_active(): bool { - if ( ! $this->has_standalone_plugin() ) { - return false; - } - - $this->load_plugin_functions(); - - return is_plugin_active_for_network( $this->get_standalone_plugin_basename() ); - } - /** * @since 1.0.0 * @@ -313,21 +277,4 @@ private function resolve_callable( $value ) { return $value( $this ); } - - /** - * WordPress only loads these in the admin, and we run at plugins_loaded on every request. - * - * Guarded on is_plugin_active_for_network() rather than is_plugin_active(), because the - * latter is a common third-party shim: something else defining it would short-circuit this - * and leave the network predicate calling a function that was never loaded. - * - * @since 1.0.0 - * - * @return void - */ - private function load_plugin_functions(): void { - if ( ! function_exists( 'is_plugin_active_for_network' ) ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - } - } } diff --git a/tests/unit/ConflictPolicyTest.php b/tests/unit/ConflictPolicyTest.php index cb8d3b7..80d7f43 100644 --- a/tests/unit/ConflictPolicyTest.php +++ b/tests/unit/ConflictPolicyTest.php @@ -36,6 +36,18 @@ public function test_no_policy_is_added_or_removed_unnoticed(): void { ); } + public function test_the_default_is_to_deactivate(): void { + $this->assertSame( Conflict_Policy::DEACTIVATE, Conflict_Policy::default() ); + } + + /** + * A default nobody understands would send every unconfigured sub-plugin down the branch a + * caller takes for garbage, which is not what "no policy configured" means. + */ + public function test_the_default_is_a_policy_the_library_understands(): void { + $this->assertTrue( Conflict_Policy::is_valid( Conflict_Policy::default() ) ); + } + public function test_all_returns_every_policy(): void { $this->assertSame( [ 'deactivate', 'defer', 'notice_only' ], diff --git a/tests/unit/PluginStateTest.php b/tests/unit/PluginStateTest.php new file mode 100644 index 0000000..0eb90c9 --- /dev/null +++ b/tests/unit/PluginStateTest.php @@ -0,0 +1,184 @@ +plugin_state = new Plugin_State(); + } + + public function tearDown(): void { + Loader::reset(); + Config::reset(); + parent::tearDown(); + } + + public function test_it_implements_the_contract(): void { + $this->assertInstanceOf( Plugin_State_Interface::class, $this->plugin_state ); + } + + public function test_the_loader_resolves_the_default_plugin_state(): void { + $this->assertInstanceOf( Plugin_State::class, Loader::plugin_state() ); + } + + /** + * The point of the seam: a host can answer from somewhere other than the active-plugins + * option, or make deactivation a no-op, without this library reaching WordPress at all. + */ + public function test_a_bound_plugin_state_replaces_the_default_everywhere(): void { + $bound = new class() implements Plugin_State_Interface { + /** + * @var string[] + */ + public $deactivated = []; + + public function is_active( string $basename ): bool { + return true; + } + + public function deactivate( string $basename ): void { + $this->deactivated[] = $basename; + } + }; + + $container = new Test_Container(); + $container->singleton( Plugin_State_Interface::class, $bound ); + Config::set_container( $container ); + + $this->setFunctionReturn( 'is_plugin_active', false ); + $this->setFunctionReturn( + 'deactivate_plugins', + static function () { + throw new \LogicException( 'A bound plugin state must keep WordPress out of it.' ); + }, + true + ); + + $this->assertTrue( Loader::plugin_state()->is_active( 'give-recurring/give-recurring.php' ) ); + + Loader::plugin_state()->deactivate( 'give-recurring/give-recurring.php' ); + + $this->assertSame( [ 'give-recurring/give-recurring.php' ], $bound->deactivated ); + } + + /** + * Loader::resolve() builds every unbound collaborator with a bare `new`, so a constructor + * that grew a required argument would fatal at plugins_loaded rather than here. + */ + public function test_it_constructs_without_arguments(): void { + $this->assertInstanceOf( Plugin_State::class, new Plugin_State() ); + } + + public function test_it_reports_an_active_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', true ); + + $this->assertTrue( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); + } + + public function test_it_reports_an_inactive_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', false ); + + $this->assertFalse( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); + } + + /** + * The basename is what reaches deactivate_plugins(), so asserting only the return value would + * let the wrong plugin be turned off unnoticed. + */ + public function test_it_passes_the_basename_through_to_wordpress(): void { + $received = null; + + $this->setFunctionReturn( + 'is_plugin_active', + static function ( $basename ) use ( &$received ) { + $received = $basename; + + return true; + }, + true + ); + + $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 'give-recurring/give-recurring.php', $received ); + } + + /** + * is_plugin_active() already ORs in the network check, so one call answers both scopes. A + * second get_site_option() per sub-plugin per request would buy nothing. + */ + public function test_the_active_check_costs_one_call(): void { + $calls = 0; + + $this->setFunctionReturn( + 'is_plugin_active', + static function () use ( &$calls ) { + ++$calls; + + return true; + }, + true + ); + $this->setFunctionReturn( + 'is_plugin_active_for_network', + static function () { + throw new \LogicException( 'The network check is redundant and must not be called.' ); + }, + true + ); + + $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 1, $calls ); + } + + /** + * Silent, and with no third argument. A noisy deactivation runs the standalone's own + * deactivation hook at plugins_loaded, and a computed $network_wide would skip one of the two + * scopes core's null default covers. + */ + public function test_it_deactivates_silently_in_every_scope(): void { + $received = []; + + $this->setFunctionReturn( + 'deactivate_plugins', + static function ( ...$arguments ) use ( &$received ) { + $received = $arguments; + }, + true + ); + + $this->plugin_state->deactivate( 'give-recurring/give-recurring.php' ); + + $this->assertSame( [ 'give-recurring/give-recurring.php', true ], $received ); + } +} diff --git a/tests/unit/SubPluginTest.php b/tests/unit/SubPluginTest.php index 15d6aeb..e35eda6 100644 --- a/tests/unit/SubPluginTest.php +++ b/tests/unit/SubPluginTest.php @@ -177,18 +177,6 @@ public function test_it_reports_no_standalone_when_the_basename_is_absent(): voi $this->assertFalse( $sub_plugin->has_standalone_plugin() ); $this->assertSame( '', $sub_plugin->get_standalone_plugin_basename() ); - $this->assertFalse( $sub_plugin->is_standalone_plugin_active() ); - $this->assertFalse( $sub_plugin->is_standalone_plugin_network_active() ); - } - - public function test_it_never_calls_wordpress_without_a_standalone(): void { - $this->setFunctionReturn( 'is_plugin_active', true ); - $this->setFunctionReturn( 'is_plugin_active_for_network', true ); - - $this->assertFalse( - $this->make_sub_plugin()->is_standalone_plugin_active(), - 'Absent a standalone basename the predicate must short-circuit.' - ); } public function test_it_reports_a_configured_standalone(): void { @@ -199,77 +187,14 @@ public function test_it_reports_a_configured_standalone(): void { } /** - * The basename is what later reaches deactivate_plugins() and the activation-error rewrite, - * so asserting only the return value would let the wrong string be passed unnoticed. - */ - public function test_it_passes_the_standalone_basename_to_wordpress(): void { - $received = []; - - $this->setFunctionReturn( - 'is_plugin_active', - static function ( $basename ) use ( &$received ) { - $received['is_plugin_active'] = $basename; - - return true; - }, - true - ); - $this->setFunctionReturn( - 'is_plugin_active_for_network', - static function ( $basename ) use ( &$received ) { - $received['is_plugin_active_for_network'] = $basename; - - return true; - }, - true - ); - - $sub_plugin = $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ); - - $sub_plugin->is_standalone_plugin_active(); - $sub_plugin->is_standalone_plugin_network_active(); - - $this->assertSame( - [ - 'is_plugin_active' => 'give-recurring/give-recurring.php', - 'is_plugin_active_for_network' => 'give-recurring/give-recurring.php', - ], - $received - ); - } - - public function test_it_delegates_the_active_check_to_wordpress(): void { - $this->setFunctionReturn( 'is_plugin_active', true ); - $this->setFunctionReturn( 'is_plugin_active_for_network', false ); - - $sub_plugin = $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ); - - $this->assertTrue( $sub_plugin->is_standalone_plugin_active() ); - $this->assertFalse( $sub_plugin->is_standalone_plugin_network_active() ); - } - - /** - * WordPress's own is_plugin_active() ORs in the network check, so a network-active plugin - * reports true from both. Stubbing is_plugin_active false here would describe a state - * WordPress cannot produce. + * Whether the standalone is active is a question about the site, and this object answers only + * from its own configuration. Stubbing WordPress into saying yes must change nothing here. */ - public function test_it_detects_a_network_active_standalone(): void { + public function test_it_asks_wordpress_nothing_about_the_standalone(): void { $this->setFunctionReturn( 'is_plugin_active', true ); $this->setFunctionReturn( 'is_plugin_active_for_network', true ); - $sub_plugin = $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ); - - $this->assertTrue( $sub_plugin->is_standalone_plugin_active() ); - $this->assertTrue( $sub_plugin->is_standalone_plugin_network_active() ); - } - - public function test_it_detects_an_inactive_standalone(): void { - $this->setFunctionReturn( 'is_plugin_active', false ); - $this->setFunctionReturn( 'is_plugin_active_for_network', false ); - - $sub_plugin = $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ); - - $this->assertFalse( $sub_plugin->is_standalone_plugin_active() ); + $this->assertFalse( $this->make_sub_plugin()->has_standalone_plugin() ); } public function test_dependencies_are_met_without_a_check(): void { @@ -285,8 +210,12 @@ public function test_it_honours_the_dependency_check(): void { ); } - public function test_the_conflict_policy_defaults_to_deactivate(): void { - $this->assertSame( Conflict_Policy::DEACTIVATE, $this->make_sub_plugin()->get_conflict_policy() ); + /** + * Asserted against Conflict_Policy::default() rather than a named policy: which policy is the + * default is that class's to state, and this only proves an unconfigured sub-plugin asks it. + */ + public function test_an_unconfigured_conflict_policy_falls_back_to_the_library_default(): void { + $this->assertSame( Conflict_Policy::default(), $this->make_sub_plugin()->get_conflict_policy() ); } public function test_it_resolves_a_string_conflict_policy(): void { From 0e4b982eded59e0f4ae2e607ef2c7256e866d29f Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Fri, 7 Aug 2026 14:45:44 +0200 Subject: [PATCH 6/6] Plan: record the PR 7 follow-up deviations Also marks two PR 7 deviations superseded: the network-active predicate has moved off Sub_Plugin, and the include guard names deactivate_plugins now that the old guard function is one the library no longer calls. --- .../plans/2026-07-31-plugin-absorber.md | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index 2e44d36..216d7ef 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -1557,10 +1557,13 @@ git checkout 06-conflict-policy && git checkout -b 07-sub-plugin > 3. **`is_standalone_plugin_active()` no longer ORs in `is_plugin_active_for_network()`.** Verified > against core: `is_plugin_active()` already does, so the OR was dead code costing a second > `get_site_option()` per sub-plugin per request. The test that pinned it described a state -> WordPress cannot produce. +> WordPress cannot produce. *(The reasoning stands; the method has since moved off `Sub_Plugin` +> onto `Plugin_State::is_active()` — see the PR 7 follow-up block under Task 12.)* > 4. **`load_plugin_functions()` guards on `is_plugin_active_for_network`,** not `is_plugin_active`. > Both live in the same file, but a third party defining an `is_plugin_active` shim — a known WP > idiom — would short-circuit the require and leave the network predicate undefined. +> *(Superseded: the guard is now `deactivate_plugins`, and the include lives on `Plugin_State` — +> same reasoning, a function the library still calls. See the PR 7 follow-up block under Task 12.)* > 5. **`get_conflict_notice_message()` takes a `$default`.** Task 14 has no fallback of its own, so > an unconfigured host would have been shown WordPress's raw fatal-error screen. > 6. **The filter result is `is_scalar()`-guarded.** A filter returning `WP_Error` would otherwise @@ -3886,12 +3889,41 @@ loading anything.' > guard constant, and nothing is reported. Wiring is all-or-nothing, so the guard now trips at > the earlier of the two priorities and does both inline. +> **Deviations, deliberate (added 2026-08-07, from the PR 7 follow-up review):** +> +> 1. **`Conflict_Policy` owns the default policy.** `Sub_Plugin::get_conflict_policy()` used to +> name `Conflict_Policy::DEACTIVATE` as its fallback, which put "which policy applies when none +> is configured" in the object that merely holds one sub-plugin's config. It now asks +> `Conflict_Policy::default()`. The two fallbacks are deliberately different and now documented +> as such: *unconfigured* means the sub-plugin accepted the default, whereas *unrecognised* — +> still `NOTICE_ONLY`, still decided in `Resolver::resolve()` — is a value nobody chose, and +> reading a typo as consent to deactivate is the one outcome worth refusing. +> 2. **`Plugin_State_Interface` is now the library's only route to WordPress's plugin functions.** +> `Sub_Plugin` was a config value object that also queried global plugin state and +> `require_once`'d `wp-admin/includes/plugin.php`; `Resolver` did its own `require_once` of the +> same file guarded on a *different* function. One gateway, one include, one guard. It is the +> fourth container-bindable collaborator, resolved through the existing `Loader::resolve()`. +> 3. **`Sub_Plugin::is_standalone_plugin_active()` is gone, not delegated.** Its only production +> caller was `Resolver::resolve_all()`, which already reaches collaborators through `Loader`, so +> delegating would have bought `Sub_Plugin` a dependency on `Loader` — a cycle — to answer a +> question that was never about its configuration. The resolver now pairs +> `has_standalone_plugin()` with `Loader::plugin_state()->is_active( … )`. +> 4. **`is_standalone_plugin_network_active()` is deleted rather than ported.** Zero production +> callers; its own docblock conceded it was informational. Deviation 1 of the PR 12 block is why +> nothing needs it — core's `null` default already covers both scopes. +> 5. **The include guards on `deactivate_plugins`,** superseding deviation 4 of the PR 7 block, +> which named `is_plugin_active_for_network` — a function the library no longer calls. The +> reasoning is unchanged and still applies: not `is_plugin_active`, because a third-party shim +> of it is a known WP idiom and would short-circuit the require. + **Interfaces:** -- Consumes: `Loader::all()` (Task 9), `Loader::notices()` (Task 10), `Sub_Plugin::is_standalone_plugin_active()` / `get_conflict_policy()` (Task 7), `Conflict_Policy::*` (Task 6). +- Consumes: `Loader::all()` (Task 9), `Loader::notices()` (Task 10), `Loader::plugin_state()`, `Sub_Plugin::has_standalone_plugin()` / `get_standalone_plugin_basename()` / `get_conflict_policy()` (Task 7), `Conflict_Policy::*` (Task 6). - Produces: - `Conflict\Resolver_Interface` with `resolve_all(): void` - `Conflict\Resolver::redirect_destination( $referrer )` — `protected`, returns `string|false` + - `Contracts\Plugin_State_Interface` with `is_active( string ): bool` and `deactivate( string ): void` - `Loader::resolver(): Resolver_Interface` + - `Loader::plugin_state(): Plugin_State_Interface` - `Loader::run_conflict_resolution(): void` - [ ] **Step 1: Cut the branch**