From dd871968260b0530cee739f6b80918a15d857f88 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:15:53 +0200 Subject: [PATCH 1/2] Let the conflict pass see the gate the load pass reads --- docs/conflict-handling.md | 6 +- docs/filters.md | 11 +- src/Conflict/Detector.php | 27 ++++- tests/unit/Conflict/DetectorTest.php | 150 ++++++++++++++++++++++++++- tests/unit/Scenario/ConflictTest.php | 53 ++++++++++ 5 files changed, 242 insertions(+), 5 deletions(-) diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index fd284ea..aa595b0 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -2,8 +2,10 @@ ## Policies -A policy is only reached for a sub-plugin that is enabled, names a `standalone_plugin_basename`, and -whose standalone is active right now. Everything else is skipped before any policy is read. +A policy is only reached for a sub-plugin that is enabled, names a `standalone_plugin_basename`, +whose standalone is active right now, and that the [`should_load` filter](filters.md#the-load-gate) +has not vetoed — a bundled copy nothing is going to load is worth no standalone's deactivation. +Everything else is skipped before any policy is read. | Policy | Behavior | |---|---| diff --git a/docs/filters.md b/docs/filters.md index 3178dc8..24c19cf 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -20,7 +20,7 @@ deactivate](conflict-handling.md#policies). | Filter | Arguments | Purpose | |---|---|---| -| `{prefix}/plugin_absorber/should_load` | `bool $should_load`, `Sub_Plugin $sub_plugin` | Last word before `require_once`. | +| `{prefix}/plugin_absorber/should_load` | `bool $should_load`, `Sub_Plugin $sub_plugin` | Last word before `require_once`, and before a conflict is resolved. | ```php add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_plugin ) { @@ -33,6 +33,15 @@ the guard constant, the dependency check and the file check. Returning `true` ca past the guard constant; anything other than a truthy return skips the load, which is the safe direction. +**Conflict handling reads it too**, at `plugins_loaded` priority 5, one ahead of the load. A +sub-plugin you veto is invisible to it: no standalone is deactivated to make room for a bundled copy +that is not going to load. So the filter may be asked more than once in a request, and it has to +*decide* rather than do — no logging, no counters, no writes. + +A filter added later than priority 5 is too late for the conflict pass, which has already run. For a +sub-plugin with a `standalone_plugin_basename`, put the toggle in the `enabled` +[config key](configuration.md) instead: both passes read it, whenever it is set. + **Watch the polarity when you wire an existing gate to this one.** `should_load` is true means *do load*. A host filter named for the opposite — LearnDash's `learndash_module_{x}_disabled`, where true means *do not load* — inverts the gate if passed through unnegated, and the failure is silent diff --git a/src/Conflict/Detector.php b/src/Conflict/Detector.php index d51027c..6124ed1 100644 --- a/src/Conflict/Detector.php +++ b/src/Conflict/Detector.php @@ -88,10 +88,27 @@ public function has_conflict(): bool { * Policy is not consulted: a sub-plugin set to defer is still in conflict, and the resolver is * where the decision to leave it alone belongs. * + * The `should_load` filter is, and it is the one piece of host code this class reads. It decides + * whether the bundled copy will be in memory at all, one priority behind this — so a sub-plugin + * the host vetoes there is in conflict with nothing: deactivating its standalone would take away + * the only copy of that code the site has, and the merge notice would tell the owner the bundled + * copy had taken over. Invisible here exactly as a disabled sub-plugin already is. + * + * Asked last, behind three checks that cost nothing. A filter is arbitrary host code and this + * runs on every admin GET a site serves, so the sub-plugin has to be enabled, name a standalone, + * and have that standalone actually running before any of it executes — which is also the order + * the load pass asks it in, last of its own gates. + * + * Nothing here catches: `Boot\Scheduler` wraps the whole conflict step in `catch ( Throwable )` + * and `Conflict\Resolver` catches per sub-plugin behind that, so a host filter that throws is + * already reported and already survivable from both callers. + * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin to test. * + * @throws Config_Exception When no hook prefix has been set. + * * @return bool */ public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { @@ -99,7 +116,15 @@ public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { return false; } - return $this->plugin_checker->is_active( $sub_plugin->get_standalone_plugin_basename() ); + if ( ! $this->plugin_checker->is_active( $sub_plugin->get_standalone_plugin_basename() ) ) { + return false; + } + + // The same two arguments the load pass passes, so a host wires one filter and sees one + // signature wherever it is asked from. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); + + return (bool) $should_load; } /** diff --git a/tests/unit/Conflict/DetectorTest.php b/tests/unit/Conflict/DetectorTest.php index 24f5c92..18ec927 100644 --- a/tests/unit/Conflict/DetectorTest.php +++ b/tests/unit/Conflict/DetectorTest.php @@ -34,7 +34,9 @@ * anything it changed would be a change made on behalf of an anonymous visitor, and anything * expensive it read would be read on every admin GET a site ever serves. Policy is the expensive * thing it must not read — a host's policy callable and the filter behind it can do anything at all, - * and neither says whether a standalone is running. + * and neither says whether a standalone is running. The `should_load` filter is the one piece of host + * code it does read, because that one decides whether the bundled copy will be there at all — and it + * is read last, of a sub-plugin every cheap check has already agreed is in conflict. * * `has_conflict()` reads the registry through the reader it was built with, so most of the tests for * it register through the facade the default reader is behind; `is_in_conflict()` is handed the @@ -62,6 +64,16 @@ class DetectorTest extends WPTestCase { */ private $asked = []; + /** + * The `should_load` filter a test installed, taken back off in tearDown. + * + * By identity rather than with `remove_all_filters()`, which would strip the hook bare for the + * rest of the process — including whatever the library itself has on it. + * + * @var callable|null + */ + private $load_gate = null; + /** * Every deactivate_plugins() call, as the arguments it was made with. * @@ -106,6 +118,9 @@ static function ( $plugins, $silent = false, $network_wide = null ) use ( &$deac } public function tearDown(): void { + // Before Config_State::reset(), which takes away the prefix the hook name is built from. + $this->remove_the_load_gate(); + $this->stop_expecting_incorrect_usage(); $this->clear_notices(); Absorber_State::reset(); @@ -179,6 +194,23 @@ public function test_it_ignores_a_sub_plugin_with_no_standalone(): void { $this->assertFalse( $this->detector()->has_conflict() ); } + /** + * The gate the load pass reads one priority later, read here as well. A host that vetoes a + * sub-plugin through `should_load` is getting no bundled copy at priority 6, so a probe that + * answered yes at priority 5 would send the step on to deactivate the only copy of that code the + * site has — and the merge notice would tell the owner the bundled one had taken over. + */ + public function test_it_ignores_a_sub_plugin_the_load_gate_vetoes(): void { + $this->standalone_is( true ); + $this->register(); + $this->gate_the_load_with( static fn( $should_load ) => false ); + + $this->assertFalse( + $this->detector()->has_conflict(), + 'A sub-plugin that is not going to load is no more in conflict than a disabled one.' + ); + } + public function test_it_is_false_with_nothing_registered(): void { $this->standalone_is( true ); @@ -414,6 +446,92 @@ public function test_is_in_conflict_asks_about_the_configured_basename(): void { $this->assertSame( [ 'give-fee-recovery/give-fee-recovery.php' ], $this->asked ); } + public function test_is_in_conflict_is_false_when_the_load_gate_vetoes_the_sub_plugin(): void { + $detector = new Detector( new Stub_Registry_Reader(), $this->recording_checker( true ) ); + + $this->gate_the_load_with( static fn( $should_load ) => false ); + + $this->assertFalse( + $detector->is_in_conflict( + $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ) + ), + 'The standalone is the only copy of that code the site is going to run.' + ); + } + + /** + * The same two arguments the load pass passes, in the same order. A host wires one filter and it + * is asked from two places now, so a signature that differed between them would make the second + * call the one that broke it. + */ + public function test_is_in_conflict_asks_the_load_gate_the_way_the_load_pass_does(): void { + $seen = []; + $sub_plugin = $this->make_sub_plugin( + [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] + ); + + $this->gate_the_load_with( + static function ( $should_load, $filtered ) use ( &$seen ) { + $seen[] = [ $should_load, $filtered ]; + + return $should_load; + } + ); + + $detector = new Detector( new Stub_Registry_Reader(), $this->recording_checker( true ) ); + + $this->assertTrue( $detector->is_in_conflict( $sub_plugin ) ); + $this->assertSame( + [ [ true, $sub_plugin ] ], + $seen, + 'The default is true and the sub-plugin is the one being asked about, exactly as at the load.' + ); + } + + /** + * Last of the four, behind every check that costs nothing. The gate is host code that may do + * anything at all, and this runs on every admin GET a site serves — so a sub-plugin that is + * disabled, has no standalone, or has no standalone *running* is turned away before any of it. + */ + public function test_is_in_conflict_asks_the_load_gate_last(): void { + $asked = []; + + $this->gate_the_load_with( + static function ( $should_load, $filtered ) use ( &$asked ) { + $asked[] = $filtered instanceof Sub_Plugin ? $filtered->get_slug() : ''; + + return $should_load; + } + ); + + $standalone = new Detector( new Stub_Registry_Reader(), $this->recording_checker( false ) ); + + $standalone->is_in_conflict( + $this->make_sub_plugin( + [ + 'enabled' => false, + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', + ] + ) + ); + $standalone->is_in_conflict( $this->make_sub_plugin() ); + $standalone->is_in_conflict( + $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ) + ); + + $this->assertSame( [], $asked, 'None of those three is a conflict, so none of them may run host code.' ); + + // A filter that failed to install leaves the same empty log, so it is shown working once the + // cheap checks have all passed. + $conflicted = new Detector( new Stub_Registry_Reader(), $this->recording_checker( true ) ); + + $conflicted->is_in_conflict( + $this->make_sub_plugin( [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] ) + ); + + $this->assertSame( [ 'give-recurring' ], $asked, 'The gate really is on the hook and really is read.' ); + } + /** * A checker that was never reached and a checker whose recorder was never installed leave the * same empty log, so every test asserting the checker was *not* asked shows it working once @@ -748,4 +866,34 @@ private function register_fee_recovery( array $overrides = [] ): void { private function standalone_is( bool $active ): void { $this->setFunctionReturn( 'is_plugin_active', $active ); } + + /** + * Put a host's `should_load` filter on the hook, as a host would. + * + * The real hook and the real name `Config` builds, not a double: what these tests are about is + * that the detector reads the same gate the load pass reads, and a filter installed under any + * other name would prove the opposite. + * + * @param callable $callback Filter callback. + * + * @return void + */ + private function gate_the_load_with( callable $callback ): void { + $this->load_gate = $callback; + + add_filter( Config::get_hook_name( 'should_load' ), $callback, 10, 2 ); + } + + /** + * @return void + */ + private function remove_the_load_gate(): void { + if ( $this->load_gate === null ) { + return; + } + + remove_filter( Config::get_hook_name( 'should_load' ), $this->load_gate ); + + $this->load_gate = null; + } } diff --git a/tests/unit/Scenario/ConflictTest.php b/tests/unit/Scenario/ConflictTest.php index f8334c1..43bd7d7 100644 --- a/tests/unit/Scenario/ConflictTest.php +++ b/tests/unit/Scenario/ConflictTest.php @@ -11,6 +11,7 @@ use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Conflict_Policy; +use Nexcess\PluginAbsorber\Sub_Plugin; /** * Every policy branch, driven end to end against a real WordPress. @@ -190,6 +191,58 @@ public function test_a_network_active_standalone_is_deactivated_when_the_host_is $this->assertSame( admin_url( 'plugins.php' ), $location ); } + /** + * The combination that costs a site its feature outright if the two passes disagree: a standalone + * still active, the default policy, and a host vetoing the bundled copy through `should_load`. + * Deactivating here would take away the only copy of that code the request is going to have — + * priority 6 is not going to require anything — and the merge notice would tell the owner the + * opposite, that the bundled copy is now loaded automatically. + * + * The second half is what makes the first mean anything: lift the veto and the same site, on the + * next page view, deactivates and merges. So the veto is what stood the conflict pass down, not a + * gate somewhere else that would have refused this request anyway. + */ + public function test_a_vetoed_sub_plugin_leaves_its_standalone_alone(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $constant = $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ] + ); + + $veto = static function ( $should_load, $sub_plugin ) { + return $sub_plugin instanceof Sub_Plugin && $sub_plugin->get_slug() === self::SLUG + ? false + : $should_load; + }; + + $this->add_tracked_filter( Config::get_hook_name( 'should_load' ), $veto, 10, 2 ); + + $this->boot(); + + // run_request() fails the test if anything redirects, which is the third of the three things + // a resolution would have done. + $this->run_request(); + + $this->assertContains( + self::STANDALONE, + $this->active_plugins(), + 'Nothing may deactivate the standalone while the bundled copy is vetoed.' + ); + $this->assertSame( [], $this->queued_notices(), 'And nothing may claim a merge that did not happen.' ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertFalse( defined( $constant ) ); + + remove_filter( Config::get_hook_name( 'should_load' ), $veto, 10 ); + + $this->run_halted_request(); + + $this->assertNotContains( self::STANDALONE, $this->active_plugins() ); + $this->assertArrayHasKey( self::SLUG . ':merge', $this->queued_notices() ); + } + /** * 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, From abe252d2ecb1639a2041204b9ec8bc3f69f06cb5 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:39:26 +0200 Subject: [PATCH 2/2] Say what the detector can still throw, and how many methods it has The container clause outlived the registry refactor: a reader holds its own registrar, so a container that cannot supply one fails while the pass is built. What can still throw is the hook name the load gate is read under. "Both methods" predates the stranding guard, which made three. --- src/Conflict/Detector.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Conflict/Detector.php b/src/Conflict/Detector.php index 6124ed1..247c18c 100644 --- a/src/Conflict/Detector.php +++ b/src/Conflict/Detector.php @@ -23,7 +23,7 @@ * every host that binds its own resolver, and give the resolver two reasons to change: how a * conflict is found, and what to do about one. * - * Both methods leave the request exactly as they found it. Nothing here resolves a user, + * Nothing here leaves a mark on the request. Nothing resolves a user, * deactivates a plugin or queues a notice — an answer is all a caller gets, and the acting is the * resolver's. * @@ -65,7 +65,7 @@ public function __construct( Reader $registry, Checker_Interface $plugin_checker * * @since 1.0.0 * - * @throws Config_Exception When no container has been set, or a container binding is unusable. + * @throws Config_Exception When no hook prefix has been set. * * @return bool */