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..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 */ @@ -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,