diff --git a/CLAUDE.md b/CLAUDE.md index 933596f..faf9e31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -69,7 +69,7 @@ seams a host may rebind: |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | | `Notices\Contracts\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | -| `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | standalone detection, deactivation, redirect | +| `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | one method: which policy branch a conflict takes | | `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | deactivates the standalone, network-aware | | `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | answers whether a plugin is active | | `Contracts\Activator_Interface` | `Activator` | run-once activation-callback tracking | @@ -118,15 +118,15 @@ it is di52-only: `stellarwp/container-contract` declares `bind`, `get`, `has` an nothing else. `[ $resolved_object, 'method' ]` is the other wrong answer — it forces every collaborator to be built at boot. -`Absorber` keeps the public surface. `registrar()`, `notices()` and `all()` are one-line delegations -to `$container->get()`, so what a host calls is unchanged; what changed is that a *collaborator* now +`Absorber` keeps the public surface. `registrar()`, `notices()`, `resolver()` and `all()` are one-line +delegations to `$container->get()`, so what a host calls is unchanged; what changed is that a *collaborator* now depends on the peer it was handed rather than on the facade. **Nothing but `Absorber` names `Absorber`.** The registration buffer belongs to `Registry_Reader`, which is also what reads it back out: `Absorber::register()` pushes a `Sub_Plugin` into it and -`Absorber::all()` delegates to it, while `Conflict\Detector` and `Loader` are each handed one. The -buffer is static because it must be — `register()` is a static call a host makes at plugin-file -scope, before there is a container to resolve a registrar from — and what is decided +`Absorber::all()` delegates to it, while `Conflict\Detector`, `Conflict\Resolver` and `Loader` are +each handed one. The buffer is static because it must be — `register()` is a static call a host makes +at plugin-file scope, before there is a container to resolve a registrar from — and what is decided is only which class pays for that. Leaving it on the facade left an edge pointing back up: the passes the facade boots read the registry by calling the facade, so `Absorber` sat both above and below its own collaborators, and a pass could not be handed a registry to work on. The arrows run one way now, @@ -143,8 +143,7 @@ the plugin to ask about, and the collaborator does the asking. ### What exists today -`src/Conflict/` — `Resolver`, `Gatekeeper`, `Redirector` — and `Activator` are not built yet. -Currently: +`Activator` is not built yet. Currently: | Path | What | |---|---| @@ -158,7 +157,7 @@ Currently: | `src/Plugin_Deactivator.php`, `src/Plugin_Checker.php` | The only files that touch WordPress plugin functions, through `Traits\Loads_Plugin_Functions`. | | `src/Registrar.php` | Holds registered `Sub_Plugin` objects. | | `src/Registry_Reader.php` | The registration buffer, drained into the registrar on the way past; the object every pass reads the registry through. | -| `src/Conflict/` | `Detector` (whether a standalone is in the way), `Gatekeeper` (which requests, and which users, may have one resolved), `Redirector` (where the user lands afterwards). | +| `src/Conflict/` | `Detector` (whether a standalone is in the way), `Resolver` (which policy branch to take), `Gatekeeper` (which requests, and which users, may have one resolved), `Redirector` (where the user lands afterwards), `Contracts\Resolver_Interface`. | | `src/Traits/` | `Loads_Plugin_Functions` (pulls in `wp-admin/includes/plugin.php`), `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing). | | `src/Notices/` | `Queue` (what a notice says, who may consume it), `Store` (keeps it), `Renderer` (draws it), `Contracts\Queue_Interface`. | | `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Config_Exception`. | @@ -213,11 +212,31 @@ registrar now fails like every other binding rather than being the one collabora binding surfaced late and politely. `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and -`DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. -`Conflict\Redirector` decides where to; it returns `false` when the referrer is already `plugins.php`, -so an inline update is never interrupted. It decides and never navigates — `wp_safe_redirect()` and -`exit` stay in the resolver, so the policy action and the admin-URL knowledge change for separate -reasons. +`DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. It is +the worked example of required injection — `Conflict\Detector` to say which sub-plugins are in +conflict, `Plugin_Deactivator_Interface` to turn the standalone off, `Queue_Interface` for the notice +and `Conflict\Redirector` for the destination, all four constructor arguments with no default — so +the object a test builds is the object the provider builds, and a host's rebinding of either plugin +seam reaches it, the deactivator directly and the checker through the detector, without the resolver +knowing a container exists. + +`Conflict\Redirector::after_deactivation( $request_uri )` decides where the user lands and never goes +there: `wp_safe_redirect()` and the `exit` after it stay in the resolver, so the policy action and +the admin-URL knowledge change for separate reasons, and every destination is assertable without a +test standing in for the end of a request. It reads the *current* request, not the referrer, because +the point of the redirect is to re-render what the user asked for now that the standalone's code is +out of memory — a referrer names the page before, which for an admin arriving from a bookmark or a +front-end link is somewhere they never asked to go. There is no "stay put" answer for the same +reason: re-requesting the screen already on display is the point, `plugins.php` included, and it +cannot loop, since the next request finds no active standalone. `update.php` and `update-core.php` +are the exception and go to `plugins.php`, because reloading either re-runs an update; so does a URI +naming no admin screen at all. It matches on the screen basename, not on a substring of an absolute +URL: the request URI is a bare path, on a site that may sit in a subdirectory, behind a +TLS-terminating proxy whose scheme disagrees with `admin_url()`, or under the network or user admin, +so nothing built from `admin_url()` would recognise it. The basename is also what keeps a crafted URI +out of the destination — only a validated screen name and a re-encoded query leave the class, and +`admin_url()`, or `network_admin_url()`/`user_admin_url()` in the other two admins, supplies +everything in front of them. **Who may have a conflict resolved is `Conflict\Gatekeeper`'s business, not the resolver's.** It gates on an interactive admin `GET` (`plugins_loaded` fires on every request) *and* on @@ -228,9 +247,11 @@ capability gate covers every policy, not just the destructive one, and that is f branches only queue a notice, and `Notices\Queue::render()` refuses to render *or clear* for a user without the same capability, so queuing earlier would only park it until a capable admin arrives. -An unknown policy must be handled as its own case via `Conflict_Policy::is_valid()`, never left to -a `default:` fallthrough — a typo like `'defered'` would otherwise deactivate a plugin the site -owner deliberately turned on. +An unknown policy is normalised to `NOTICE_ONLY` through `Conflict_Policy::is_valid()` before the +switch, never decided by wherever a `switch` happens to fall through — a typo like `'defered'` would +otherwise deactivate a plugin the site owner deliberately turned on. The `default:` branch that +remains is the one that only queues a notice, so a policy nobody wrote is never read as consent to +turn a plugin off. ### Keys @@ -320,8 +341,8 @@ treatment. Any older sketch showing `Config::reset()` or `Absorber::reset()` mea anything is hooked. Past that point this library is code on somebody's live site, and a white screen is never the better answer — so every entry point it puts on a hook catches `Throwable`, reports with `_doing_it_wrong()` and abandons that step alone: the `plugins_loaded` step in - `Boot\Scheduler`, and `Absorber::render_notices()` on `all_admin_notices`. `Loader::load_all()` - catches *per sub-plugin* as well, because one sub-plugin's throw + `Boot\Scheduler`, and `Absorber::render_notices()` on `all_admin_notices`. `Loader::load_all()` and + `Conflict\Resolver::resolve_all()` catch *per sub-plugin* as well, because one sub-plugin's throw must not take the ones behind it in the registration order with it. Everything past those catches is somebody else's code — `enabled`, `dependency_check`, `activation_callback`, `conflict_policy`, the notice messages, the `should_load` filter, the bundled file a `require` runs top to bottom, and the diff --git a/docs/configuration.md b/docs/configuration.md index bac9676..fe66d18 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -47,6 +47,7 @@ $container->singleton( Registrar_Interface::class, My_Registrar::class ); | `Notices\Contracts\Queue_Interface` | `Notices\Queue` | Queues and renders the admin notices. | | `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | Deactivates the standalone. | | `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | Answers whether a plugin is active. | +| `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | Detects the active standalone and applies the policy. | `Plugin_Checker_Interface` is the seam to rebind when your plugin filters `option_active_plugins` or `site_option_active_sitewide_plugins` — LearnDash injects and then strips a synthetic path — because diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index 43cd75b..f22e364 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -10,8 +10,28 @@ 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. | -Set one per sub-plugin with the `conflict_policy` key, or decide it at runtime with the -`conflict_policy` [filter](filters.md), which has the final say. +Set one per sub-plugin with the `conflict_policy` key — a constant, or a `callable( Sub_Plugin ): +string`. The `conflict_policy` [filter](filters.md) runs after that and has the final say: + +```php +// In the config: stand down when a newer standalone supersedes the bundled copy. +'conflict_policy' => static fn( Sub_Plugin $sub ) => give_standalone_is_newer( $sub ) + ? Conflict_Policy::DEFER + : Conflict_Policy::DEACTIVATE, + +// Anywhere, and last: +add_filter( 'give/plugin_absorber/conflict_policy', static function ( $policy, $sub ) { + return $sub->get_slug() === 'give-recurring' ? Conflict_Policy::NOTICE_ONLY : $policy; +}, 10, 2 ); +``` + +**An unrecognised policy is treated as `NOTICE_ONLY`**, never as consent to deactivate. +`Conflict_Policy::is_valid()` decides, so a typo like `'defered'` — in a policy a host persisted in +an option, or in whatever that filter returned — only produces a notice. A value nobody chose must +not turn off a plugin somebody chose. + +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. ## The load guard diff --git a/docs/filters.md b/docs/filters.md index 4063f16..14e8da2 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -12,7 +12,9 @@ Each runs last, after the configured value and any fallback. Because they fire w asked for rather than when the sub-plugin is registered, they are also the place to call `__()` — by then the textdomain is loaded. -A filter returning a non-scalar yields an empty string rather than a fatal cast. +A filter returning a non-scalar yields an empty string rather than a fatal cast. A `conflict_policy` +return that is not one of the three constants is treated as [`NOTICE_ONLY`, never as consent to +deactivate](conflict-handling.md#policies). ## The load gate diff --git a/src/Absorber.php b/src/Absorber.php index 75f1d65..44069e0 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -6,6 +6,7 @@ namespace Nexcess\PluginAbsorber; use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; @@ -58,6 +59,17 @@ public static function notices(): Queue_Interface { return self::collaborator( Queue_Interface::class ); } + /** + * @since 1.0.0 + * + * @throws Config_Exception When no container has been set, or its binding is unusable. + * + * @return Resolver_Interface + */ + public static function resolver(): Resolver_Interface { + return self::collaborator( Resolver_Interface::class ); + } + /** * Register one bundled sub-plugin. Call once per sub-plugin, before boot(). * diff --git a/src/Conflict/Contracts/Resolver_Interface.php b/src/Conflict/Contracts/Resolver_Interface.php new file mode 100644 index 0000000..3b3d95a --- /dev/null +++ b/src/Conflict/Contracts/Resolver_Interface.php @@ -0,0 +1,37 @@ +registry = $registry; + $this->detector = $detector; + $this->plugin_deactivator = $plugin_deactivator; + $this->notices = $notices; + $this->redirector = $redirector; + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * + * @return void + */ + public function resolve_all(): void { + // Every notice this pass raises is keyed and filtered by the host's prefix, so without one + // there is no way to tell the site owner why a plugin of theirs went off. Standing down whole + // is the honest answer -- deactivating first and failing to explain it is worse than leaving + // the conflict for the request after the host fixes its bootstrap -- and it is what the load + // pass at the next priority does with the same missing prefix. + if ( ! self::has_hook_prefix() ) { + return; + } + + $deactivated = false; + + // The reader rather than a registrar of our own: it drains the registrations still buffered + // on the facade before it reads, and a registrar asked directly would miss anything + // registered since the last read. + foreach ( $this->registry->all() as $sub_plugin ) { + // The policy may be a host callable behind a filter any plugin on the site may have + // hooked, the notice message is another callable, and deactivate_plugins() runs the + // standalone's own deactivation hook -- so this loop calls arbitrary code, from inside + // plugins_loaded, on an admin page view. A throw out of here would take away the screen + // the site owner would have used to undo whatever caused it, and would leave a second + // standalone running with nothing said about it. Reported per sub-plugin, and the next + // one is still resolved. + try { + if ( ! $this->detector->is_in_conflict( $sub_plugin ) ) { + continue; + } + + if ( $this->resolve( $sub_plugin ) ) { + $deactivated = true; + } + } catch ( Throwable $thrown ) { + _doing_it_wrong( + self::class, + sprintf( + 'The conflict for "%s" threw while being resolved, so it was abandoned: %s', + $sub_plugin->get_slug(), + $thrown->getMessage() + ), + '1.0.0' + ); + } + } + + // After the loop, never inside it. A site bundling two sub-plugins can have both standalones + // active, and an `exit` on the first would leave the second's standalone running with no + // notice raised about it — and would take the load pass at the next priority with it, so + // nothing bundled loaded on the request that was supposed to fix the conflict. + if ( $deactivated ) { + $this->redirect(); + } + } + + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * + * @return bool Whether the standalone was deactivated. + */ + protected function resolve( Sub_Plugin $sub_plugin ): bool { + $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 false; + + case Conflict_Policy::DEACTIVATE: + $this->deactivate( $sub_plugin ); + + return true; + + // NOTICE_ONLY, and anything is_valid() would accept that this switch has grown no + // branch for. The default sits on the branch that only talks, never on the one that + // deactivates: a policy nobody wrote must not be read as consent to turn a plugin off. + default: + $this->notices->queue_conflict_notice( $sub_plugin ); + + return false; + } + } + + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin whose standalone is active. + * + * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * + * @return void + */ + protected function deactivate( Sub_Plugin $sub_plugin ): void { + $this->plugin_deactivator->deactivate( $sub_plugin->get_standalone_plugin_basename() ); + + // Queued as the deactivation is made rather than once at the end, so the explanation is + // durable whether or not the request goes on to redirect — and so a site with two + // standalones gets one notice per plugin it lost. + $this->notices->queue_merge_notice( $sub_plugin ); + } + + /** + * Re-request the current screen, now that the standalone's code is out of the way. + * + * @since 1.0.0 + * + * @return void + */ + protected function redirect(): void { + // Boot\Scheduler runs this whole sequence inline when a host boots too late, immediately + // after a _doing_it_wrong() that prints on a debugging site — so the headers are gone before + // we get here, wp_safe_redirect() would warn and set no Location, and the exit behind it + // would end the request on a blank page. Falling through instead lets the page finish + // rendering, which is where the merge notice queued above is waiting to be read. + if ( headers_sent() ) { + return; + } + + $request_uri = isset( $_SERVER['REQUEST_URI'] ) ? wp_unslash( $_SERVER['REQUEST_URI'] ) : ''; + + // $_SERVER carries whatever the SAPI put there and wp_unslash() hands back the shape it was + // given, so the string the redirector is promised is made one here rather than assumed. + if ( ! is_string( $request_uri ) ) { + $request_uri = ''; + } + + wp_safe_redirect( $this->redirector->after_deactivation( $request_uri ) ); + + exit; + } +} diff --git a/src/Provider.php b/src/Provider.php index 003a3e2..e7e5fb1 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -6,9 +6,11 @@ namespace Nexcess\PluginAbsorber; use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; use Nexcess\PluginAbsorber\Conflict\Detector; use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Conflict\Redirector; +use Nexcess\PluginAbsorber\Conflict\Resolver; use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; @@ -97,6 +99,19 @@ static function () use ( $container ): Detector { } ); + $this->bind_once( + Resolver_Interface::class, + static function () use ( $container ): Resolver { + return new Resolver( + $container->get( Registry_Reader::class ), + $container->get( Detector::class ), + $container->get( Plugin_Deactivator_Interface::class ), + $container->get( Queue_Interface::class ), + $container->get( Redirector::class ) + ); + } + ); + $this->bind_once( Loader::class, static function () use ( $container ): Loader { diff --git a/tests/_support/Spy_Resolver.php b/tests/_support/Spy_Resolver.php new file mode 100644 index 0000000..6d37e3d --- /dev/null +++ b/tests/_support/Spy_Resolver.php @@ -0,0 +1,38 @@ +resolve_calls` off a value typed + * as `Resolver_Interface` is reading a property the interface does not declare, and static analysis + * rightly rejects it. Named, the spy's own type carries the counter. + * + * It resolves nothing, which is the point — a test that binds this one proves the conflict step + * reached a resolver at all without deactivating anything or ending the request. + * + * @since 1.0.0 + */ +class Spy_Resolver implements Resolver_Interface { + /** + * How many times resolve_all() was called. + * + * @var int + */ + public $resolve_calls = 0; + + /** + * @return void + */ + public function resolve_all(): void { + ++$this->resolve_calls; + } +} diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 9343a33..91bab41 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -9,6 +9,7 @@ use Generator; use Nexcess\PluginAbsorber\Absorber; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Conflict\Resolver; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; @@ -152,8 +153,9 @@ public function test_a_missing_container_is_not_reported_as_a_failed_binding(): * @return Generator */ public static function collaborator_accessors(): Generator { - yield 'the registrar' => [ 'registrar', Registrar::class ]; - yield 'the notice queue' => [ 'notices', Queue::class ]; + yield 'the registrar' => [ 'registrar', Registrar::class ]; + yield 'the notice queue' => [ 'notices', Queue::class ]; + yield 'the conflict resolver' => [ 'resolver', Resolver::class ]; } /** @@ -175,8 +177,9 @@ public function test_an_accessor_without_a_container_is_a_configuration_error( s * @return Generator */ public static function accessor_names(): Generator { - yield 'the registrar' => [ 'registrar' ]; - yield 'the notice queue' => [ 'notices' ]; + yield 'the registrar' => [ 'registrar' ]; + yield 'the notice queue' => [ 'notices' ]; + yield 'the conflict resolver' => [ 'resolver' ]; } /** diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php new file mode 100644 index 0000000..881c4bc --- /dev/null +++ b/tests/unit/Conflict/ResolverTest.php @@ -0,0 +1,857 @@ +> + */ + private $deactivations = []; + + /** + * @var string|null + */ + private $request_uri; + + public function setUp(): void { + parent::setUp(); + + Absorber_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + $this->set_up_container(); + $this->clear_notices(); + + // uopz cannot stub a function that does not exist yet. + require_once ABSPATH . 'wp-admin/includes/plugin.php'; + + $this->deactivations = []; + + // The request the whole file describes: an admin GET of an ordinary screen. Both keys are set + // rather than inherited, because the redirect reads REQUEST_URI for the screen to re-request and + // $_SERVER outlives whichever test wrote to it last. The method itself gates nothing here — that + // is the gatekeeper's business — but a request with a destination and no method is not one. + $this->set_request_method( 'GET' ); + $this->request_uri = $_SERVER['REQUEST_URI'] ?? null; + $_SERVER['REQUEST_URI'] = '/wp-admin/options-general.php'; + + // Stated rather than inherited, because under CLI the real answer is whatever the test runner + // has printed so far: PHP records headers as sent the moment anything reaches the output layer, + // so an unstubbed headers_sent() would make every redirect assertion in this file depend on + // which printer PHPUnit was configured with. The test that is about the sent-headers branch says + // so itself. + $this->setFunctionReturn( 'headers_sent', false ); + + // uopz runs a replacement with no class scope, so $this and self:: are both fatal inside this + // closure. Bind a reference to the property instead. See tests/README.md. + $deactivations = &$this->deactivations; + + $this->setFunctionReturn( + 'deactivate_plugins', + static function ( $plugins, $silent = false, $network_wide = null ) use ( &$deactivations ) { + $deactivations[] = [ + 'plugins' => $plugins, + 'silent' => $silent, + 'network_wide' => $network_wide, + ]; + }, + true + ); + } + + public function tearDown(): void { + if ( $this->request_uri === null ) { + unset( $_SERVER['REQUEST_URI'] ); + } else { + $_SERVER['REQUEST_URI'] = $this->request_uri; + } + + $this->stop_expecting_incorrect_usage(); + $this->restore_request_method(); + $this->clear_notices(); + Absorber_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + public function test_the_loader_resolves_the_default_resolver(): void { + $this->assertInstanceOf( Resolver::class, Absorber::resolver() ); + } + + public function test_the_default_resolver_satisfies_the_contract(): void { + $this->assertInstanceOf( Resolver_Interface::class, $this->resolver() ); + } + + /** + * The point of the required constructor arguments. Every peer arrives from the container, so a host + * that rebinds one has it reached — and nothing in the run touches the option the default notice + * queue is backed by, which is what makes the resolver testable without standing up global state. + * + * Detection arrives as a peer like any other, which is the shape the resolver gained when the probe + * left its contract: how a conflict is found is `Conflict\Detector`'s to change, and what to do + * about one is this class's. + */ + /** + * The registry is a peer like the other four, and this is what that buys: the sub-plugin + * resolved here was never registered, so the run reaches no buffer and no registrar. A resolver + * that read the facade instead would find nothing to resolve and deactivate nothing. + */ + public function test_it_resolves_the_registry_it_was_handed(): void { + $reader = new Stub_Registry_Reader( + [ + $this->make_sub_plugin( + [ 'standalone_plugin_basename' => 'give-recurring/give-recurring.php' ] + ), + ] + ); + + // After the provider, because a class id is one the container reports it can build whether or + // not anybody bound it -- so the provider binds its own regardless, and a double put in first + // would be silently replaced. + $this->container()->singleton( + Registry_Reader::class, + static function () use ( $reader ): Registry_Reader { + return $reader; + } + ); + + $this->standalone_is( true ); + + $this->capture_resolution(); + + $this->assertSame( + [ 'give-recurring/give-recurring.php' ], + array_column( $this->deactivations, 'plugins' ), + 'The standalone deactivated has to be the one the reader handed over.' + ); + $this->assertSame( + [], + Absorber::registrar()->all(), + 'Nothing was ever registered: the whole run came off the injected reader.' + ); + } + + /** + * The policy may be a host callable, the notice message is another, and `deactivate_plugins()` + * runs the standalone's own deactivation hook — all of it arbitrary code, all of it inside + * `plugins_loaded`. A throw from one sub-plugin must not cost the site the admin screen it would + * be fixed from, nor leave a second standalone running with nothing said about it. + */ + public function test_a_sub_plugin_that_throws_does_not_stop_the_others(): void { + $this->expect_incorrect_usage(); + $this->standalone_is( true ); + + $this->register( + [ + 'conflict_policy' => static function (): string { + throw new RuntimeException( 'the policy option could not be read' ); + }, + ] + ); + $this->register_fee_recovery(); + + $this->capture_resolution(); + + $this->assertSame( + [ 'give-fee-recovery/give-fee-recovery.php' ], + array_column( $this->deactivations, 'plugins' ), + 'The sub-plugin behind the one that threw still has to be resolved.' + ); + $this->assert_the_library_reported_incorrect_usage(); + } + + public function test_the_collaborators_come_from_the_container(): void { + $detector = new class() extends Detector { + /** + * @var string[] + */ + public $asked = []; + + /** + * No plugin checker, and no parent constructor call. What this class stands in for is the + * answer, not the way it is reached — and how a detector reaches one is DetectorTest's. + */ + public function __construct() { + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin to test. + * + * @return bool + */ + public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { + $this->asked[] = $sub_plugin->get_slug(); + + return true; + } + }; + + $deactivator = new class() implements Plugin_Deactivator_Interface { + /** + * @var string[] + */ + public $deactivated = []; + + /** + * @param string $basename Plugin basename. + * + * @return void + */ + public function deactivate( string $basename ): void { + $this->deactivated[] = $basename; + } + }; + + $notices = new Spy_Queue(); + + // The interface seams go in first, which is where a host binds them and where the guarantee + // lives: nothing can build an interface unprompted, so the container answering to one means a + // binding was made, and the provider leaves it alone. + $container = new Test_Container(); + $container->singleton( + Plugin_Deactivator_Interface::class, + static function () use ( $deactivator ): Plugin_Deactivator_Interface { + return $deactivator; + } + ); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Queue_Interface { + return $notices; + } + ); + + $this->set_up_container( $container ); + + // The detector is a concrete class, so the same question has no answer before the provider + // runs: the container reports it can return a Detector whether or not anyone bound one, and + // the provider cannot tell a deliberate binding from mere autowirability. It therefore binds + // its own, and a double put in above would be silently replaced by the real class — asserted + // against here as a detector that was never asked. + $container->singleton( + Detector::class, + static function () use ( $detector ): Detector { + return $detector; + } + ); + + $this->register(); + + $this->capture_resolution(); + + $this->assertSame( [ 'give-recurring' ], $detector->asked ); + $this->assertSame( [ 'give-recurring/give-recurring.php' ], $deactivator->deactivated ); + $this->assertSame( [ 'give-recurring' ], $notices->merge_notices ); + $this->assertSame( + [], + $this->queued_notices(), + 'The bound queue stands in for the option-backed one, which must be left untouched.' + ); + $this->assertSame( + [], + $this->deactivations, + 'A bound deactivator is what deactivates; WordPress must not have been called as well.' + ); + } + + /** + * The redirector is handed the current request, unslashed — not the referrer. What the user is + * looking at is what has to be re-rendered without the standalone's code in memory, and core slashes + * $_SERVER on the way in, so a query string with an apostrophe in it would otherwise gain a + * backslash every time a conflict was resolved. + */ + public function test_it_asks_the_redirector_where_to_send_the_current_request(): void { + $redirector = new class() extends Redirector { + /** + * @var string[] + */ + public $asked = []; + + /** + * @param mixed $request_uri Request URI under test. + * + * @return string + */ + public function after_deactivation( $request_uri ): string { + $this->asked[] = is_string( $request_uri ) ? $request_uri : ''; + + return admin_url( 'tools.php' ); + } + }; + + // Bound after the provider has run, which for a concrete class is the only order that leaves the + // double in place: the container reports it can return a `Redirector` whether or not one was + // ever bound, so the provider cannot read an existing answer as a host's choice and binds its + // own over the top. Binding first — the order an interface seam wants — would hand the real + // redirector to the resolver and leave `$redirector->asked` empty for a reason no assertion + // here would name. + $container = $this->set_up_container(); + $container->singleton( + Redirector::class, + static function () use ( $redirector ): Redirector { + return $redirector; + } + ); + + $this->standalone_is( true ); + $this->register(); + $_SERVER['REQUEST_URI'] = '/wp-admin/edit.php?s=O\\\'Brien'; + + $location = $this->capture_resolution(); + + $this->assertSame( [ '/wp-admin/edit.php?s=O\'Brien' ], $redirector->asked ); + $this->assertSame( admin_url( 'tools.php' ), $location ); + } + + /** + * A request with no REQUEST_URI is not hypothetical: the too-late fallback runs this sequence + * inline, and a host may have booted from something that never set one. + */ + public function test_it_survives_a_request_with_no_uri(): void { + $redirector = new class() extends Redirector { + /** + * @var string[] + */ + public $asked = []; + + /** + * @param mixed $request_uri Request URI under test. + * + * @return string + */ + public function after_deactivation( $request_uri ): string { + $this->asked[] = is_string( $request_uri ) ? $request_uri : ''; + + return admin_url( 'plugins.php' ); + } + }; + + // After the provider, for the reason the test above states: a concrete class id is one the + // provider rebinds whatever was there before it. + $container = $this->set_up_container(); + $container->singleton( + Redirector::class, + static function () use ( $redirector ): Redirector { + return $redirector; + } + ); + + $this->standalone_is( true ); + $this->register(); + unset( $_SERVER['REQUEST_URI'] ); + + $location = $this->capture_resolution(); + + $this->assertSame( [ '' ], $redirector->asked ); + $this->assertSame( admin_url( 'plugins.php' ), $location ); + } + + public function test_deactivate_deactivates_notifies_and_redirects(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::DEACTIVATE ] ); + + $location = $this->capture_resolution(); + + $this->assertCount( 1, $this->deactivations ); + $this->assertSame( 'give-recurring/give-recurring.php', $this->deactivations[0]['plugins'] ); + $this->assertArrayHasKey( 'give-recurring:merge', $this->queued_notices() ); + + // Through the real redirector, so the screen the user was on is the screen they get back. Which + // exact URL each referrer maps to is RedirectorTest's subject; that it is this screen and not + // some default is this one's. + $this->assertStringContainsString( 'options-general.php', $location ); + } + + /** + * Two bundled plugins, both standalones active. Resolution has to reach the second one: an `exit` + * inside the loop would leave its standalone running with nothing said about it, and would take the + * load pass at the next priority down with it. + * + * One redirect, not two, is asserted by the shape of the stub — it throws, so a redirect taken on + * the first sub-plugin could not have been followed by the second's deactivation. + */ + public function test_it_resolves_every_conflict_and_redirects_once(): void { + $this->standalone_is( true ); + $this->register(); + $this->register_fee_recovery(); + + $this->capture_resolution(); + + $this->assertSame( + [ 'give-recurring/give-recurring.php', 'give-fee-recovery/give-fee-recovery.php' ], + array_column( $this->deactivations, 'plugins' ) + ); + + $queued = $this->queued_notices(); + $this->assertArrayHasKey( 'give-recurring:merge', $queued ); + $this->assertArrayHasKey( 'give-fee-recovery:merge', $queued, 'The second conflict is the one an early exit loses.' ); + } + + /** + * The inline fallback runs immediately after a _doing_it_wrong() that prints on a debugging site, so + * the headers are already gone by the time this code runs. Redirecting there sets no Location and + * the exit behind it would end the request on a blank page — with the merge notice queued and + * nothing left able to draw it. + */ + public function test_it_deactivates_without_redirecting_once_the_headers_are_sent(): void { + // Overrides the false setUp establishes for every other test here. + $this->setFunctionReturn( 'headers_sent', true ); + + $this->standalone_is( true ); + $this->register(); + + $this->resolve_all(); + + $this->assertCount( 1, $this->deactivations ); + $this->assertArrayHasKey( + 'give-recurring:merge', + $this->queued_notices(), + 'The request goes on rendering, so the notice it will render must still be there.' + ); + } + + public function test_deactivate_is_the_default_policy(): void { + $this->standalone_is( true ); + $this->register(); + + $this->capture_resolution(); + + $this->assertCount( 1, $this->deactivations ); + } + + /** + * 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. + * + * Asserted here as well as in PluginDeactivatorTest, because this is the path that actually + * deactivates a site's plugin: a resolver that reached WordPress by some other route would leave + * that unit test green and the site 404ing. + */ + public function test_it_deactivates_silently_and_lets_core_decide_the_scope(): void { + $this->standalone_is( true ); + $this->register(); + + $this->capture_resolution(); + + $this->assertTrue( $this->deactivations[0]['silent'], 'An unattended deactivation must be silent.' ); + $this->assertNull( + $this->deactivations[0]['network_wide'], + 'Core enters the network branch on false !== $network_wide and the blog branch on true !== $network_wide, so null takes both.' + ); + } + + /** + * 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->capture_resolution(); + + $this->assertNotContains( $basename, (array) get_option( 'active_plugins', [] ) ); + + 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->capture_resolution(); + + $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' ); + } + + /** + * 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->capture_resolution(); + + $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_all(); + + $this->assertSame( [], $this->deactivations ); + $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_all(); + + $this->assertSame( [], $this->deactivations ); + $this->assertArrayHasKey( 'give-recurring:conflict', $this->queued_notices() ); + } + + /** + * Mixed policies on one site. The talking branch and the deactivating branch both run, and the one + * redirect at the end belongs to the deactivation — a notice-only conflict on its own never reaches + * it, which is what `test_notice_only_notifies_without_deactivating` pins. + */ + public function test_a_notice_only_conflict_alongside_a_deactivation_still_redirects(): void { + $this->standalone_is( true ); + $this->register( [ 'conflict_policy' => Conflict_Policy::NOTICE_ONLY ] ); + $this->register_fee_recovery( [ 'conflict_policy' => Conflict_Policy::DEACTIVATE ] ); + + $this->capture_resolution(); + + $queued = $this->queued_notices(); + $this->assertArrayHasKey( 'give-recurring:conflict', $queued ); + $this->assertArrayHasKey( 'give-fee-recovery:merge', $queued ); + $this->assertCount( 1, $this->deactivations, 'Only the deactivating sub-plugin is deactivated.' ); + } + + /** + * 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_all(); + + $this->assertSame( [], $this->deactivations, 'An unrecognised policy must never deactivate.' ); + $this->assertArrayHasKey( 'give-recurring:conflict', $this->queued_notices() ); + } + + /** + * @return Generator + */ + public static function unknown_policies(): Generator { + yield 'typo' => [ 'defered' ]; + yield 'empty' => [ '' ]; + yield '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_all(); + + $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_all(); + + $this->assertSame( [], $this->deactivations ); + } + + public function test_it_skips_a_disabled_sub_plugin(): void { + $this->standalone_is( true ); + $this->register( [ 'enabled' => false ] ); + + $this->resolve_all(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->queued_notices(), 'A skipped sub-plugin has nothing to say to the site owner.' ); + } + + public function test_it_skips_when_the_standalone_is_not_active(): void { + $this->standalone_is( false ); + $this->register(); + + $this->resolve_all(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( [], $this->queued_notices(), 'There is no conflict to report when the standalone is gone.' ); + } + + public function test_it_skips_a_sub_plugin_with_no_standalone(): void { + $this->standalone_is( true ); + Absorber::register( + [ + 'slug' => 'give-fee-recovery', + 'bundled_plugin_file' => '/tmp/give-fee-recovery.php', + 'plugin_loaded_constant' => 'GIVE_FEE_RECOVERY_VERSION_FIXTURE', + ] + ); + + $this->resolve_all(); + + $this->assertSame( [], $this->deactivations ); + $this->assertSame( + [], + $this->queued_notices(), + 'A sub-plugin that names no standalone can never be in conflict with one.' + ); + } + + /** + * Without a prefix there is no notice to raise, so there is no honest way to explain a + * deactivation to the site owner — and this runs inside `plugins_loaded`, where throwing is not + * an option however wrong the host's bootstrap is. It stands down whole rather than turning a + * plugin off and going quiet, which is what the load pass does with the same missing prefix. + */ + public function test_it_stands_down_without_a_hook_prefix(): void { + $this->expect_incorrect_usage(); + $this->standalone_is( true ); + $this->register(); + + $resolver = $this->resolver(); + $container = $this->container(); + + // The prefix goes, the container stays: this is about the missing prefix, and a resolver that + // could not reach its registrar would stand down for the other reason. + Config_State::reset(); + Config::set_container( $container ); + + $this->without_ending_the_request( + static function () use ( $resolver ): void { + $resolver->resolve_all(); + } + ); + + $this->assertSame( [], $this->deactivations, 'Nothing may be turned off that cannot be explained.' ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The resolver the container builds, which is the one the conflict step reaches. + * + * @return Resolver_Interface + */ + private function resolver(): Resolver_Interface { + return $this->resolve( Resolver_Interface::class ); + } + + /** + * Run resolution on a path that must not end the request. + * + * @return void + */ + private function resolve_all(): void { + $resolver = $this->resolver(); + + $this->without_ending_the_request( + static function () use ( $resolver ): void { + $resolver->resolve_all(); + } + ); + } + + /** + * Run something that must return rather than redirect. + * + * The redirect is stubbed to throw rather than left alone: unstubbed, a resolver that redirected + * anyway would reach the real `wp_safe_redirect()` and the `exit` behind it, taking the whole test + * process with it. Throwing instead turns that into one failed test naming the path it happened on. + * + * @param callable $action The call under test. + * + * @return void + */ + private function without_ending_the_request( callable $action ): void { + $message = 'The resolver must not end the request on this path.'; + + $this->setFunctionReturn( + 'wp_safe_redirect', + static function () use ( $message ) { + throw new TestException( $message ); + }, + true + ); + + try { + $action(); + } catch ( TestException $exception ) { + $this->fail( $exception->getMessage() ); + } finally { + // In a finally block so a failed assertion cannot strand the stub for the rest of the + // process, where a later test's redirect would throw for no reason it can see. + $this->unsetFunctionReturn( 'wp_safe_redirect' ); + } + } + + /** + * Run resolution on a path that must redirect and terminate, and return where it sent the user. + * + * @return string + */ + private function capture_resolution(): string { + $resolver = $this->resolver(); + + return $this->capture_redirect( + static function () use ( $resolver ): void { + $resolver->resolve_all(); + } + ); + } + + /** + * @param array $overrides Config overrides. + * + * @return void + */ + private function register( array $overrides = [] ): void { + Absorber::register( + array_merge( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/give-recurring.php', + 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION_FIXTURE', + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', + ], + $overrides + ) + ); + } + + /** + * A second bundled sub-plugin, for the tests about a site that absorbed more than one. + * + * @param array $overrides Config overrides. + * + * @return void + */ + private function register_fee_recovery( array $overrides = [] ): void { + $this->register( + array_merge( + [ + 'slug' => 'give-fee-recovery', + 'bundled_plugin_file' => '/tmp/give-fee-recovery.php', + 'plugin_loaded_constant' => 'GIVE_FEE_RECOVERY_VERSION_FIXTURE', + 'standalone_plugin_basename' => 'give-fee-recovery/give-fee-recovery.php', + ], + $overrides + ) + ); + } + + /** + * Only is_plugin_active(), which is the one function Plugin_Checker::is_active() calls — and it + * ORs the network check in itself, so stubbing is_plugin_active_for_network() alongside it + * would be inert and would read as though a network path were being exercised. + * + * @param bool $active Whether the standalone is active. + * + * @return void + */ + private function standalone_is( bool $active ): void { + $this->setFunctionReturn( 'is_plugin_active', $active ); + } +} diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index 342dc7b..d9ba2f5 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -8,9 +8,11 @@ use Codeception\TestCase\WPTestCase; use Generator; use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; use Nexcess\PluginAbsorber\Conflict\Detector; use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Conflict\Redirector; +use Nexcess\PluginAbsorber\Conflict\Resolver; use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; @@ -28,6 +30,7 @@ use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Resolver; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use StellarWP\ContainerContract\ContainerInterface; @@ -82,6 +85,7 @@ public static function default_bindings(): Generator { yield 'the notice renderer' => [ Renderer::class, Renderer::class ]; yield 'the plugin checker' => [ Plugin_Checker_Interface::class, Plugin_Checker::class ]; yield 'the deactivator' => [ Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ]; + yield 'the conflict resolver' => [ Resolver_Interface::class, Resolver::class ]; yield 'the conflict detector' => [ Detector::class, Detector::class ]; yield 'the redirector' => [ Redirector::class, Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class, Gatekeeper::class ]; @@ -114,6 +118,7 @@ public static function single_instance_bindings(): Generator { yield 'the notice queue' => [ Queue_Interface::class ]; yield 'the notice store' => [ Store::class ]; yield 'the notice renderer' => [ Renderer::class ]; + yield 'the conflict resolver' => [ Resolver_Interface::class ]; yield 'the conflict detector' => [ Detector::class ]; yield 'the redirector' => [ Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class ]; @@ -191,8 +196,9 @@ static function () use ( $bound ): object { * @return Generator */ public static function host_bindings(): Generator { - yield 'the registrar' => [ Registrar_Interface::class, new Spy_Registrar() ]; - yield 'the notice queue' => [ Queue_Interface::class, new Spy_Queue() ]; + yield 'the registrar' => [ Registrar_Interface::class, new Spy_Registrar() ]; + yield 'the notice queue' => [ Queue_Interface::class, new Spy_Queue() ]; + yield 'the conflict resolver' => [ Resolver_Interface::class, new Spy_Resolver() ]; } /**