From 7059031b7b1868b019268b0b7a67c2460761c84e Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 15:14:24 +0200 Subject: [PATCH 1/4] Resolve a standalone conflict through injected collaborators Conflict\Resolver takes its four collaborators as required constructor arguments -- the checker, the deactivator, the notice queue and the redirector -- because the container is now mandatory and nothing has to be constructible without one. The nullable peers and the accessors that fell back to a static are gone with the class they fell back to. Conflict\Gatekeeper owns who may resolve: an interactive admin GET, the activate_plugins capability, and a hook prefix. plugins_loaded runs on every request and fires before auth_redirect(), so an unauthenticated GET of an admin URL reaches this code, and the capability gate covers every policy rather than only the destructive one -- the other branches queue a notice the same user could not render anyway. The priority-1 step asks the gatekeeper before it resolves Resolver_Interface at all, so a host binding its own resolver cannot drop either gate by omission. Conflict\Destination becomes Conflict\Redirector. It decides where to send the user 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. A class that returns a URL from a filter still earns the agent noun. The boot barrier now measures from the lowest priority in the sequence rather than from the load priority, or a host booting between conflict resolution and the load would be told nothing while half its wiring silently failed. --- CLAUDE.md | 61 +- README.md | 12 +- docs/configuration.md | 8 + docs/conflict-handling.md | 57 +- docs/filters.md | 4 +- src/Boot/Scheduler.php | 48 +- src/Conflict/Contracts/Resolver_Interface.php | 32 + src/Conflict/Gatekeeper.php | 104 +++ src/Conflict/Redirector.php | 56 ++ src/Conflict/Resolver.php | 164 +++++ src/Loader.php | 12 + src/Provider.php | 18 + tests/README.md | 15 + tests/_support/Spy_Gatekeeper.php | 50 ++ tests/_support/Spy_Resolver.php | 36 + tests/_support/Traits/WithUsers.php | 63 ++ tests/unit/Boot/SchedulerTest.php | 268 +++++++- tests/unit/Conflict/GatekeeperTest.php | 228 +++++++ tests/unit/Conflict/RedirectorTest.php | 57 ++ tests/unit/Conflict/ResolverTest.php | 617 ++++++++++++++++++ tests/unit/LoaderTest.php | 11 +- tests/unit/Notices/QueueTest.php | 41 +- tests/unit/ProviderTest.php | 13 +- 23 files changed, 1897 insertions(+), 78 deletions(-) create mode 100644 src/Conflict/Contracts/Resolver_Interface.php create mode 100644 src/Conflict/Gatekeeper.php create mode 100644 src/Conflict/Redirector.php create mode 100644 src/Conflict/Resolver.php create mode 100644 tests/_support/Spy_Gatekeeper.php create mode 100644 tests/_support/Spy_Resolver.php create mode 100644 tests/_support/Traits/WithUsers.php create mode 100644 tests/unit/Conflict/GatekeeperTest.php create mode 100644 tests/unit/Conflict/RedirectorTest.php create mode 100644 tests/unit/Conflict/ResolverTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 1143f48..7405032 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -118,8 +118,8 @@ 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. -`Loader` keeps the public surface. `registrar()` and `notices()` are one-line delegations to -`$container->get()`, so what a host calls is unchanged; what changed is that a *collaborator* now +`Loader` keeps the public surface. `registrar()`, `notices()` and `resolver()` 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. `Sub_Plugin` is a value object answering the per-sub-plugin questions it can answer **without a @@ -133,8 +133,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 | |---|---| @@ -147,6 +146,7 @@ Currently: | `src/Conflict_Policy.php` | The three policy constants, `default()`, `is_valid()`. | | `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/Conflict/` | `Resolver` (which policy branch to take), `Gatekeeper` (which requests may take one), `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`. | @@ -161,7 +161,7 @@ Loader::boot(); // idempotent → Provider::register() // every binding → Boot\Scheduler // every hook, as a closure over the container -plugins_loaded @1 → Conflict\Resolver::resolve_all() [gated by Conflict\Gatekeeper] +plugins_loaded @1 → Conflict\Gatekeeper, then Conflict\Resolver::resolve_all() plugins_loaded @2 → Load\Runner::load_all() all_admin_notices → Loader::render_notices() [is_admin() only] wp_admin_notice_markup → Loader::filter_activation_error_markup() [is_admin() only] @@ -177,6 +177,14 @@ earlier holds an orphan whose bindings are discarded. This is also why `Loader:: and resolves nothing — registration at plugin-file scope, which the spec sanctions, would otherwise register into the throwaway. +**The too-late barrier measures against the first step in the sequence, not the last.** +`Boot\Scheduler` compares the priority `plugins_loaded` is already dispatching against the lowest +priority it has to wire — conflict resolution at 1, not the load at 2 — and over that line it runs +the whole sequence inline in hook order rather than wiring any of it. Measuring against the load +would let a host booting at priority 1 wire the load and silently lose the conflict pass, which is +the half of the sequence a fatal depends on. The comparison is inclusive, because a callback added +at the priority currently being dispatched is accepted and never reached. + `load_all()` gates each sub-plugin in order, skipping on the first failure: enabled → not already loaded → dependencies met → file exists → `should_load` filter → `require_once` → activation callback (only after a *successful* require). @@ -188,23 +196,40 @@ them after the wrong problem. `docs/filters.md` and the spec agree. `Loader::all()` narrows to `Sub_Plugin` instances itself, so no caller repeats that guard. A host may bind a registrar returning anything, and PHP 7.4 cannot express `array` in -the interface signature — so it is filtered once where the untrusted value enters. +the interface signature — so it is filtered once where the untrusted value enters. Both passes read +through `Loader::all()` rather than through the registrar they could resolve for themselves, because +it flushes the pending registrations before it reads and a registrar asked directly would miss +anything registered since the last flush. `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 — `Plugin_Checker_Interface` to detect the standalone, +`Plugin_Deactivator_Interface` to turn it 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 without the resolver knowing a container exists. + +`Conflict\Redirector::after_deactivation( $referrer )` 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 returns `false` — stay put — when the referrer is +already `plugins.php`, since that list is about to render the deactivation anyway; it sends +`update.php` and `update-core.php` to `plugins.php`, since reloading either re-runs an update; with +no usable referrer, `plugins.php`. It matches on the screen basename, not on a substring of an +absolute URL: `wp_get_referer()` prefers the bare `_wp_http_referer` path that every nonce-bearing +admin form carries, so comparing against `admin_url()` would miss the network admin and every site +behind a TLS-terminating proxy. **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 -`current_user_can( 'activate_plugins' )` (`plugins_loaded` runs before `auth_redirect()`, so an -unauthenticated GET of an admin URL gets that far). The hook resolves the gatekeeper rather than the -resolver, so a host binding its own `Resolver_Interface` cannot drop either gate by omission. The -capability gate covers every policy, not just the destructive one, and that is free: the other -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. +gates on an interactive admin `GET` (`plugins_loaded` fires on every request, including cron, CLI +and a visitor's POST) *and* on `current_user_can( 'activate_plugins' )` (`plugins_loaded` runs +before `auth_redirect()`, so an unauthenticated GET of an admin URL gets that far). The +`plugins_loaded` step asks the gatekeeper *before* it resolves `Resolver_Interface` at all, so a +host binding its own resolver cannot drop either gate by omission — and a request that fails one +never builds a resolver. The capability gate covers every policy, not just the destructive one, and +that is free: the other 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 diff --git a/README.md b/README.md index 7a9ac63..310b576 100644 --- a/README.md +++ b/README.md @@ -39,11 +39,11 @@ add_action( 'plugins_loaded', function () { The container is required — any StellarWP `ContainerInterface` implementation, the one you already hand to Telemetry or Uplink. Every collaborator comes from it. -Keep the `, 0`. `boot()` wires the load at `plugins_loaded` priority 2, and WordPress silently -ignores a callback added at or past the priority it is already dispatching — so configuring the -library from a provider that itself runs at priority 2 or later races the library it is configuring. -Booting later is reported through `_doing_it_wrong()` and loaded inline, but the ordering guarantees -are weaker. +Keep the `, 0`. `boot()` wires conflict resolution at `plugins_loaded` priority 1 and the load at +priority 2, and WordPress silently ignores a callback added at or past the priority it is already +dispatching — so configuring the library from a provider that itself runs at priority 1, which is +where several hosts wire their container today, races the library it is configuring. Booting later is +reported through `_doing_it_wrong()` and loaded inline, but the ordering guarantees are weaker. Put this in the block that owns your container, not in a service provider, and pass the container you intend to keep: a host that builds one lazily and replaces it later leaves us holding an orphan whose @@ -53,7 +53,7 @@ bindings were discarded. - [Installing](docs/installing.md) — Composer, Strauss, and the constants Strauss must leave alone. - [Configuration](docs/configuration.md) — the hook prefix, the container, every sub-plugin key. -- [Conflict handling](docs/conflict-handling.md) — the policies, the load guard, and its limits. +- [Conflict handling](docs/conflict-handling.md) — the policies, when they run, and the guard's limits. - [Filters](docs/filters.md) — the runtime overrides for policies and notice text. - [Notices](docs/notices.md) — where the queue lives, who may see it, and how to render it yourself. diff --git a/docs/configuration.md b/docs/configuration.md index c5ab7f1..3058271 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -47,11 +47,19 @@ $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 `is_plugin_active()` then does not report what is in the database. +Rebinding `Resolver_Interface` does not put you in charge of *when* resolution may run. Both gates — +[an interactive admin `GET`, and the `activate_plugins` +capability](conflict-handling.md#when-resolution-runs) — live in `Conflict\Gatekeeper`, which the +hook consults before it resolves the resolver at all, so an implementation that never thought about +either is still safe. Everything the resolver *does* — which policy branch, what the notice says, +where the user lands — is yours. + `set_container()` is a configuration call like `set_hook_prefix()`, and order does not matter among the configuration calls: it may come before or after your `Loader::register()` calls, so long as it comes before boot. Registering buffers the sub-plugin and resolves nothing, so nothing is decided diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index 43cd75b..e323abc 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -10,8 +10,61 @@ 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. + +## When resolution runs + +At `plugins_loaded` priority 1, one ahead of the load pass at 2: a standalone that survives the +conflict defines the guard constant as it loads, and the load pass has to see that. + +It runs **only on an interactive admin `GET`** — not WP-CLI, not cron, not ajax, not a form POST — +because resolving can deactivate a plugin and end the request with a redirect. Ungated, a visitor's +checkout POST would come back as a 302 that discards what was submitted and drops the order, and a +WP-CLI command would exit having printed nothing, because `header()` is a no-op under the CLI SAPI. +Waiting costs nothing: the standalone is still there to detect on the next page view. + +It also requires `current_user_can( 'activate_plugins' )`: `plugins_loaded` fires well before +`auth_redirect()`, so an unauthenticated GET of an admin URL reaches this code on its way to the +login screen, and whoever cannot activate a plugin must not be able to deactivate one. This applies +to every policy rather than only to `deactivate`, which costs nothing — the other policies just +queue a notice, and a notice is neither shown nor cleared for a user without that same capability, +so nothing is consumed by waiting for one who has it. + +Both gates live in `Conflict\Gatekeeper`, and the hook asks it *before* it resolves +`Conflict\Contracts\Resolver_Interface` at all. So binding your own resolver cannot drop them by +omission: on a request that fails either gate your implementation is never built, let alone called. + +## The redirect + +After deactivating, the user goes back to whatever they were looking at, so it re-renders without the +standalone. Two referrers differ: `plugins.php` stays put, since the list is about to show the change +anyway, and the update screens (`update.php`, `update-core.php`) go to `plugins.php` instead, because +reloading one of those would re-run an update. With no referrer at all, `plugins.php`. + +`Conflict\Redirector` makes that decision and returns it; the redirect itself is the resolver's. The +merge notice is queued before either, so the explanation survives whether or not the request ends in +a redirect. ## 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/Boot/Scheduler.php b/src/Boot/Scheduler.php index 3c69b4c..9589098 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -5,6 +5,8 @@ namespace Nexcess\PluginAbsorber\Boot; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Load\Runner; use Nexcess\PluginAbsorber\Loader; use StellarWP\ContainerContract\ContainerInterface; @@ -34,6 +36,18 @@ class Scheduler { */ private const LOAD_PRIORITY = 2; + /** + * plugins_loaded priority conflict resolution runs at, ahead of the load pass. + * + * A standalone that survives the conflict defines the guard constant as it loads, and the load + * pass has to see that, so resolution cannot share a priority with it. + * + * @since 1.0.0 + * + * @var int + */ + private const RESOLVE_PRIORITY = 1; + /** * @since 1.0.0 * @@ -57,6 +71,10 @@ public function __construct( ContainerInterface $container ) { * collaborator when the hook fires, so a host may still rebind one after boot() and up until * plugins_loaded, and a binding nothing reaches is never built at all. * + * Called too late, the steps run inline instead of being wired — 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 does. + * * @since 1.0.0 * * @return void @@ -77,7 +95,7 @@ public function wire(): void { if ( $this->wiring_window_has_closed() ) { _doing_it_wrong( Loader::class . '::boot', - 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + 'Loader::boot() must run before plugins_loaded priority 1. Resolving and loading inline instead.', '1.0.0' ); @@ -109,12 +127,25 @@ public function wire(): void { * * @since 1.0.0 * - * @return array + * @return non-empty-array */ private function sequence(): array { $container = $this->container; return [ + [ + 'priority' => self::RESOLVE_PRIORITY, + 'run' => static function () use ( $container ): void { + // The gatekeeper first, and the resolver only once it has said yes. Who may have + // a conflict resolved is the library's invariant rather than the host's policy, + // so it is settled before anything a host may have rebound is even built. + if ( ! $container->get( Gatekeeper::class )->may_resolve() ) { + return; + } + + $container->get( Resolver_Interface::class )->resolve_all(); + }, + ], [ 'priority' => self::LOAD_PRIORITY, 'run' => static function () use ( $container ): void { @@ -125,13 +156,17 @@ private function sequence(): array { } /** - * Whether it is already too late to wire the load hook. + * Whether it is already too late to wire the first step of the sequence. + * + * Measured against the earliest priority in sequence(), read rather than restated, because a + * boot that can still wire a later step but has missed an earlier one has missed something — + * and with resolution at 1 and the load at 2, booting between the two is a real window. * * 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. + * Booting from plugins_loaded at that priority 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 * @@ -148,6 +183,7 @@ private function wiring_window_has_closed(): 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() >= min( array_column( $this->sequence(), 'priority' ) ); } } diff --git a/src/Conflict/Contracts/Resolver_Interface.php b/src/Conflict/Contracts/Resolver_Interface.php new file mode 100644 index 0000000..1819b5e --- /dev/null +++ b/src/Conflict/Contracts/Resolver_Interface.php @@ -0,0 +1,32 @@ +is_interactive_admin_request() + && $this->can_resolve_conflicts() + && self::has_hook_prefix(); + } + + /** + * 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 function is_interactive_admin_request(): bool { + if ( defined( 'WP_CLI' ) && WP_CLI ) { + return false; + } + + if ( wp_doing_cron() || wp_doing_ajax() ) { + 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 the current user may have a conflict resolved on their request. + * + * Reaching conflict resolution does not mean anyone is signed in. wp-admin/admin.php loads + * wp-load.php -- which dispatches plugins_loaded -- well before it calls auth_redirect(), so an + * unauthenticated GET of any admin URL gets this far. Without this check a stranger could turn + * the standalone off site-wide by requesting a page they are about to be bounced off. + * + * Here rather than inside the default resolver, because it is the one thing about conflict + * resolution that must survive a host binding its own: whoever cannot activate a plugin must not + * be able to deactivate one, and a replacement that forgot to re-check would reopen exactly that. + * + * It gates every policy, not only the destructive one, and that costs nothing. The other + * policies queue a notice, and Notices\Queue::render() will not render -- or clear -- for a user + * without this same capability. Queuing on a request that cannot act only parks the notice until + * a capable administrator arrives, which is the request this gate lets resolution run on anyway. + * Nothing is consumed or suppressed by waiting: the standalone is still there to detect. + * + * @since 1.0.0 + * + * @return bool + */ + private function can_resolve_conflicts(): bool { + return current_user_can( 'activate_plugins' ); + } +} diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php new file mode 100644 index 0000000..08e8f16 --- /dev/null +++ b/src/Conflict/Redirector.php @@ -0,0 +1,56 @@ +plugin_checker = $plugin_checker; + $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 { + // Loader::all() rather than a registrar of our own: it flushes the pending registrations + // before it reads, and a registrar asked directly would not see anything registered since + // the last read. + foreach ( Loader::all() as $sub_plugin ) { + if ( ! $sub_plugin->is_enabled() || ! $sub_plugin->has_standalone_plugin() ) { + continue; + } + + if ( ! $this->plugin_checker->is_active( $sub_plugin->get_standalone_plugin_basename() ) ) { + continue; + } + + $this->resolve( $sub_plugin ); + } + } + + /** + * @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 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::DEACTIVATE: + $this->deactivate( $sub_plugin ); + + return; + + // 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 ); + } + } + + /** + * @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 after the deactivation but before the redirect, so the explanation is durable + // whether or not the request goes on to end here. + $this->notices->queue_merge_notice( $sub_plugin ); + + $destination = $this->redirector->after_deactivation( wp_get_referer() ); + + if ( $destination !== false ) { + wp_safe_redirect( $destination ); + + exit; + } + } +} diff --git a/src/Loader.php b/src/Loader.php index d2216bf..5f33e77 100644 --- a/src/Loader.php +++ b/src/Loader.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; @@ -64,6 +65,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/Provider.php b/src/Provider.php index 31a200d..16307c0 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -6,6 +6,10 @@ namespace Nexcess\PluginAbsorber; use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +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; @@ -64,6 +68,8 @@ public function register(): void { $this->bind_once( Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ); $this->bind_once( Store::class ); $this->bind_once( Renderer::class ); + $this->bind_once( Redirector::class ); + $this->bind_once( Gatekeeper::class ); // Explicit factories rather than a class name for everything with a constructor argument: // container-contract promises `bind`, `get`, `has` and `singleton` and nothing about @@ -75,6 +81,18 @@ static function () use ( $container ): Queue { } ); + $this->bind_once( + Resolver_Interface::class, + static function () use ( $container ): Resolver { + return new Resolver( + $container->get( Plugin_Checker_Interface::class ), + $container->get( Plugin_Deactivator_Interface::class ), + $container->get( Queue_Interface::class ), + $container->get( Redirector::class ) + ); + } + ); + $this->bind_once( Runner::class, static function () use ( $container ): Runner { diff --git a/tests/README.md b/tests/README.md index adc7d3a..f2246d1 100644 --- a/tests/README.md +++ b/tests/README.md @@ -125,6 +125,21 @@ A fixture helper cannot be called `make()`, `makeEmpty()`, `construct()`, or `WPTestCase` extends, and redeclaring one with narrower visibility is a fatal at class-compile time. The suite does not fail, it fails to start. +## Users and capabilities + +Two of the library's gates turn on `activate_plugins`, so a test that reaches +either needs a user who has it. `WithUsers` owns both halves: + +```php +$this->become_plugin_administrator(); // someone who may resolve a conflict +$this->create_user( 'subscriber' ); // someone who may not +``` + +`become_plugin_administrator()` is not just `create_user( 'administrator' )`. On +multisite `activate_plugins` maps through `manage_network_plugins`, which a site +administrator does not have — so it grants super admin there and sets the current +user either way. A test about *that* difference creates the administrator itself. + ## Stubbing functions Use `UopzFunctions` from wp-browser. Do not add a local `WithUopz` trait — this diff --git a/tests/_support/Spy_Gatekeeper.php b/tests/_support/Spy_Gatekeeper.php new file mode 100644 index 0000000..7248df8 --- /dev/null +++ b/tests/_support/Spy_Gatekeeper.php @@ -0,0 +1,50 @@ +may_resolve_calls` off a + * value the container handed back. + * + * @since 1.0.0 + */ +class Spy_Gatekeeper extends Gatekeeper { + /** + * How many times may_resolve() was called. + * + * @var int + */ + public $may_resolve_calls = 0; + + /** + * @var bool + */ + private $answer; + + /** + * @param bool $answer The answer this gatekeeper always gives. + */ + public function __construct( bool $answer ) { + $this->answer = $answer; + } + + /** + * @return bool + */ + public function may_resolve(): bool { + ++$this->may_resolve_calls; + + return $this->answer; + } +} diff --git a/tests/_support/Spy_Resolver.php b/tests/_support/Spy_Resolver.php new file mode 100644 index 0000000..8d0085f --- /dev/null +++ b/tests/_support/Spy_Resolver.php @@ -0,0 +1,36 @@ +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/_support/Traits/WithUsers.php b/tests/_support/Traits/WithUsers.php new file mode 100644 index 0000000..bf6dd2d --- /dev/null +++ b/tests/_support/Traits/WithUsers.php @@ -0,0 +1,63 @@ + uniqid( 'absorber-' ), + 'user_pass' => wp_generate_password(), + 'role' => $role, + ] + ); + + if ( $user_id instanceof WP_Error ) { + throw new RuntimeException( 'Could not create a ' . $role . ': ' . $user_id->get_error_message() ); + } + + return $user_id; + } + + /** + * Become someone who can activate plugins. + * + * On multisite activate_plugins maps through manage_network_plugins, so an administrator of a + * single site does not have it and the network administrator is the one who does. + * + * @since 1.0.0 + * + * @return int + */ + protected function become_plugin_administrator(): int { + $user_id = $this->create_user( 'administrator' ); + + if ( is_multisite() ) { + grant_super_admin( $user_id ); + } + + wp_set_current_user( $user_id ); + + return $user_id; + } +} diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index 993f54c..6100100 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -10,15 +10,22 @@ use LogicException; use Nexcess\PluginAbsorber\Boot\Scheduler; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +use Nexcess\PluginAbsorber\Conflict\Gatekeeper; +use Nexcess\PluginAbsorber\Conflict_Policy; +use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Loader; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Loader_State; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Gatekeeper; use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Resolver; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithUsers; use ReflectionClass; use WP_Hook; @@ -40,12 +47,18 @@ class SchedulerTest extends WPTestCase { use WithBundledPlugins; use WithContainer; use WithIncorrectUsage; + use WithUsers; /** * @var int */ private $plugins_loaded_count = 0; + /** + * @var string|null + */ + private $request_method; + /** * Hook callbacks these tests added, as [ hook, callback, priority ] triples. * @@ -65,6 +78,12 @@ public function setUp(): void { Config::set_hook_prefix( 'give' ); $this->set_up_container(); $this->reset_bundled_plugin_loads(); + $this->clear_notices(); + + // The conflict step reads the request method, so the one test that lets the real gatekeeper + // answer depends on it rather than on whatever the harness happened to leave behind. + $this->request_method = $_SERVER['REQUEST_METHOD'] ?? null; + $_SERVER['REQUEST_METHOD'] = 'GET'; // The harness has to boot WordPress before it can run anything, so plugins_loaded has already // fired by the time any test starts — and boot() would rightly report that it is too late to @@ -77,6 +96,12 @@ public function setUp(): void { public function tearDown(): void { $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + if ( $this->request_method === null ) { + unset( $_SERVER['REQUEST_METHOD'] ); + } else { + $_SERVER['REQUEST_METHOD'] = $this->request_method; + } + // 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 // current screen before WP_ADMIN. @@ -90,6 +115,7 @@ public function tearDown(): void { $this->stop_expecting_incorrect_usage(); $this->remove_bundled_plugin_files(); + $this->clear_notices(); Loader_State::reset(); Config_State::reset(); $this->tear_down_container(); @@ -105,6 +131,93 @@ public function test_the_load_step_runs_early_in_plugins_loaded(): void { $this->assertSame( 2, $this->load_priority() ); } + /** + * A standalone that survives the conflict defines its guard constant as it loads, and the load + * pass has to see that — so resolution runs first and cannot share a priority with it. + */ + public function test_the_conflict_step_runs_before_the_load_step(): void { + $this->assertSame( 1, $this->resolve_priority() ); + $this->assertLessThan( $this->load_priority(), $this->resolve_priority() ); + } + + public function test_it_wires_the_conflict_step_at_the_resolve_priority(): void { + [ 'resolver' => $resolver ] = $this->bind_conflict_doubles( true ); + + $before = $this->callbacks_at( 'plugins_loaded', $this->resolve_priority() ); + + Loader::boot(); + + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', $this->resolve_priority() ), + 'boot() must wire the conflict step rather than run it.' + ); + $this->assertSame( 0, $resolver->resolve_calls, 'Wiring must not resolve anything yet.' ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $resolver->resolve_calls ); + } + + /** + * The gate is asked first and the resolver is built only once it has said yes. That order is the + * whole guarantee: a host binding its own `Resolver_Interface` decides what a conflict means, not + * who is allowed to have one resolved, and a replacement that forgot a gate cannot reopen it + * because it is never reached. + */ + public function test_the_conflict_step_asks_the_gatekeeper_before_resolving(): void { + [ 'gatekeeper' => $gatekeeper, 'resolver' => $resolver ] = $this->bind_conflict_doubles( false ); + + Loader::boot(); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $gatekeeper->may_resolve_calls, 'The step has to ask the gate.' ); + $this->assertSame( 0, $resolver->resolve_calls, 'A refused request must not reach a resolver at all.' ); + } + + public function test_the_conflict_step_resolves_once_the_gatekeeper_admits_the_request(): void { + [ 'gatekeeper' => $gatekeeper, 'resolver' => $resolver ] = $this->bind_conflict_doubles( true ); + + Loader::boot(); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $gatekeeper->may_resolve_calls ); + $this->assertSame( 1, $resolver->resolve_calls ); + } + + /** + * The real gate, on the request it exists to turn away. The capability check covers the policies + * that only queue a notice as well as the destructive one, and nothing is lost by that — the + * standalone is still there to detect once someone who can act on it arrives, which is what the + * second half asserts. + */ + public function test_a_user_who_cannot_activate_plugins_has_nothing_resolved_or_queued(): void { + set_current_screen( 'dashboard' ); + + $this->bind_active_standalone(); + $this->register_conflicted_sub_plugin(); + + wp_set_current_user( $this->create_user( 'subscriber' ) ); + + Loader::boot(); + + do_action( 'plugins_loaded' ); + + $this->assertSame( [], $this->notice_queue(), 'A user who could never read the notice must not consume it.' ); + + $this->become_plugin_administrator(); + + do_action( 'plugins_loaded' ); + + $this->assertArrayHasKey( + 'give-recurring:conflict', + $this->notice_queue(), + 'The conflict has to still be detectable once someone who can act on it arrives.' + ); + } + public function test_it_wires_the_load_step_at_the_load_priority(): void { $this->register_sub_plugin(); @@ -209,13 +322,15 @@ static function () use ( &$fired ): void { * never fires. Booting from plugins_loaded at the default priority instead of 0 would otherwise * load nothing at all, on a site that looks completely healthy. * - * The load priority itself is the boundary case: a callback added to the priority currently being + * The window is measured from the earliest step in the sequence, so the resolve priority is the + * boundary case rather than the load priority: a callback added to the priority currently being * dispatched is never reached either, because the dispatch loop walks a by-value copy of that - * priority's callback array. + * priority's callback array. Booting between the two steps still reports, and still loads. * * @dataProvider late_boot_priorities * - * @param int $offset How far past the load priority the host boots from. + * @param int $offset How far past the load priority the host boots from; negative for the window + * between conflict resolution and the load pass. */ public function test_booting_too_late_in_plugins_loaded_loads_inline_instead( int $offset ): void { $this->expect_incorrect_usage(); @@ -249,11 +364,42 @@ static function () use ( $path, $constant ): void { * @return Generator */ public static function late_boot_priorities(): Generator { + yield 'at the resolve priority' => [ -1 ]; yield 'at the load priority' => [ 0 ]; yield 'one past it' => [ 1 ]; yield 'the default a host omits' => [ 8 ]; } + /** + * The other side of the same boundary. The window closes at the first step rather than the last, + * so a host booting at priority 0 — which is what the README tells it to do — must still wire both + * steps and be reported on for nothing. + */ + public function test_booting_at_the_start_of_plugins_loaded_still_wires(): void { + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( $path, $constant ): void { + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + Loader::boot(); + }, + 0 + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { $this->expect_incorrect_usage(); @@ -335,6 +481,24 @@ private function load_priority(): int { return $priority; } + /** + * The priority the conflict step is wired at, read from the scheduler rather than restated. + * + * @throws LogicException When the constant is missing or not an int, rather than counting + * callbacks at priority zero and passing for the wrong reason. + * + * @return int + */ + private function resolve_priority(): int { + $priority = ( new ReflectionClass( Scheduler::class ) )->getConstant( 'RESOLVE_PRIORITY' ); + + if ( ! is_int( $priority ) ) { + throw new LogicException( 'Boot\Scheduler::RESOLVE_PRIORITY must be an int.' ); + } + + return $priority; + } + /** * How many callbacks are on a hook, at one priority or in total. * @@ -387,6 +551,104 @@ static function () use ( $notices ): Spy_Queue { return $notices; } + /** + * Bind a gatekeeper with a fixed answer and a resolver that records being reached. + * + * Both bound before the provider runs, which is the only order that leaves them bound. The pair is + * returned rather than resolved back out of the container, so the assertions read a spy's own type + * — `Resolver_Interface` declares no counter, and nothing here narrows a container's return. + * + * @param bool $may_resolve The answer the gate always gives. + * + * @return array{gatekeeper:Spy_Gatekeeper,resolver:Spy_Resolver} + */ + private function bind_conflict_doubles( bool $may_resolve ): array { + $gatekeeper = new Spy_Gatekeeper( $may_resolve ); + $resolver = new Spy_Resolver(); + + $container = new Test_Container(); + $container->singleton( + Gatekeeper::class, + static function () use ( $gatekeeper ): Gatekeeper { + return $gatekeeper; + } + ); + $container->singleton( + Resolver_Interface::class, + static function () use ( $resolver ): Resolver_Interface { + return $resolver; + } + ); + + $this->set_up_container( $container ); + + return [ + 'gatekeeper' => $gatekeeper, + 'resolver' => $resolver, + ]; + } + + /** + * Report every standalone as active, without reaching WordPress for the answer. + * + * @return void + */ + private function bind_active_standalone(): void { + $container = new Test_Container(); + $container->singleton( + Plugin_Checker_Interface::class, + static function (): Plugin_Checker_Interface { + return new class() implements Plugin_Checker_Interface { + /** + * @param string $basename Plugin basename. + * + * @return bool + */ + public function is_active( string $basename ): bool { + return true; + } + }; + } + ); + + $this->set_up_container( $container ); + } + + /** + * Register a sub-plugin whose standalone is in conflict, under the policy that only talks. + * + * @return void + */ + private function register_conflicted_sub_plugin(): void { + $constant = $this->make_guard_constant(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $this->make_bundled_plugin_file( $constant ), + 'plugin_loaded_constant' => $constant, + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', + 'conflict_policy' => Conflict_Policy::NOTICE_ONLY, + ] + ); + } + + /** + * The queue is stored as a site option on every install — on single site that call falls through + * to the plain option table — so there is one place to read it from. + * + * @return array + */ + private function notice_queue(): array { + $queue = get_site_option( 'give_plugin_absorber_notices', [] ); + + return is_array( $queue ) ? $queue : []; + } + + private function clear_notices(): void { + delete_site_option( 'give_plugin_absorber_notices' ); + } + /** * Register a sub-plugin whose bundled file records that it was loaded. * diff --git a/tests/unit/Conflict/GatekeeperTest.php b/tests/unit/Conflict/GatekeeperTest.php new file mode 100644 index 0000000..3af805c --- /dev/null +++ b/tests/unit/Conflict/GatekeeperTest.php @@ -0,0 +1,228 @@ +set_up_container(); + + // plugins_loaded fires on every request, so the request method is part of what the gate reads + // rather than something the harness happens to leave lying around. + $this->request_method = $_SERVER['REQUEST_METHOD'] ?? null; + $_SERVER['REQUEST_METHOD'] = 'GET'; + + set_current_screen( 'dashboard' ); + $this->become_plugin_administrator(); + } + + public function tearDown(): void { + if ( $this->request_method === null ) { + unset( $_SERVER['REQUEST_METHOD'] ); + } else { + $_SERVER['REQUEST_METHOD'] = $this->request_method; + } + + // 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 + // current screen before WP_ADMIN. + set_current_screen( 'front' ); + + $this->stop_expecting_incorrect_usage(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + public function test_it_admits_an_interactive_admin_get(): void { + $this->assertTrue( $this->gatekeeper()->may_resolve() ); + } + + /** + * Unguarded, resolution fires at plugins_loaded on every request: a visitor's checkout POST + * becomes a 302 that drops the order, and a front-end page load deactivates a plugin with nobody + * there to read the notice about it. + */ + public function test_it_refuses_a_front_end_request(): void { + set_current_screen( 'front' ); + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + + set_current_screen( 'dashboard' ); + + $this->assert_the_gate_opens_again(); + } + + /** + * 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_it_refuses_an_admin_form_submission(): void { + $_SERVER['REQUEST_METHOD'] = 'POST'; + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + + $_SERVER['REQUEST_METHOD'] = 'GET'; + + $this->assert_the_gate_opens_again(); + } + + /** + * wp-cron never reaches its event loop if the request it rides on is redirected away. + */ + public function test_it_refuses_a_cron_request(): void { + $this->setConstant( 'DOING_CRON', true ); + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + + // unsetConstant() rather than a second setConstant(): it restores whatever the constant was + // before this test touched it, where setting it twice would record the test's own value as + // the one to put back. + $this->unsetConstant( 'DOING_CRON' ); + + $this->assert_the_gate_opens_again(); + } + + public function test_it_refuses_an_ajax_request(): void { + $this->setConstant( 'DOING_AJAX', true ); + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + + $this->unsetConstant( 'DOING_AJAX' ); + + $this->assert_the_gate_opens_again(); + } + + /** + * header() is a no-op under the CLI SAPI, so a WP-CLI command would exit 0 having printed + * nothing and having deactivated a plugin the operator never asked about. + */ + public function test_it_refuses_a_wp_cli_request(): void { + $this->setConstant( 'WP_CLI', true ); + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + + $this->unsetConstant( 'WP_CLI' ); + + $this->assert_the_gate_opens_again(); + } + + /** + * plugins_loaded is dispatched by wp-load.php, which wp-admin/admin.php requires long before it + * calls auth_redirect(). An unauthenticated GET of an admin URL therefore reaches the conflict + * step, and without the capability check a stranger could deactivate the standalone site-wide. + * + * @dataProvider users_who_cannot_activate_plugins + * + * @param string|null $role Role to ask as, or null for a logged-out visitor. + */ + public function test_it_refuses_a_user_who_cannot_activate_plugins( ?string $role ): void { + wp_set_current_user( $role === null ? 0 : $this->create_user( $role ) ); + + $this->assertFalse( + $this->gatekeeper()->may_resolve(), + 'Only someone who can activate a plugin may deactivate one.' + ); + + $this->become_plugin_administrator(); + + $this->assert_the_gate_opens_again(); + } + + /** + * @return Generator + */ + public static function users_who_cannot_activate_plugins(): Generator { + yield 'a subscriber' => [ 'subscriber' ]; + yield 'a logged-out visitor' => [ null ]; + } + + /** + * The prefix names the conflict_policy filter and the option the notice queue lives in, so + * resolution without one has nowhere to put what it would do. It is the only gate that reports the + * mistake, and it is checked last so that report lands on the admin request that was about to + * resolve something rather than on every front-end request the site serves. + */ + public function test_it_refuses_a_request_with_no_hook_prefix(): void { + $container = $this->container(); + + // The prefix goes, the container stays: a gatekeeper that could not be built at all would + // fail this test for the other reason. + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + $this->assertFalse( $this->gatekeeper()->may_resolve() ); + $this->assert_the_library_reported_incorrect_usage(); + + Config::set_hook_prefix( 'give' ); + + $this->assert_the_gate_opens_again(); + } + + /** + * The gatekeeper as the container builds it, which is how the conflict step reaches it. + * + * @return Gatekeeper + */ + private function gatekeeper(): Gatekeeper { + return $this->resolve( Gatekeeper::class ); + } + + /** + * With the one condition under test put back, the gate has to open. + * + * Without this every test here would pass on a gatekeeper that refused everything, including the + * request it is supposed to admit. + * + * @return void + */ + private function assert_the_gate_opens_again(): void { + $this->assertTrue( + $this->gatekeeper()->may_resolve(), + 'The condition under test has to be the one deciding the answer.' + ); + } +} diff --git a/tests/unit/Conflict/RedirectorTest.php b/tests/unit/Conflict/RedirectorTest.php new file mode 100644 index 0000000..1bc4d8d --- /dev/null +++ b/tests/unit/Conflict/RedirectorTest.php @@ -0,0 +1,57 @@ +assertSame( $expected, ( new Redirector() )->after_deactivation( $referrer ) ); + } + + /** + * Absolute admin URLs and bare paths both appear, because wp_get_referer() returns whichever + * the request carried: the _wp_http_referer field of an admin form holds a path with no scheme + * or host, while the Referer header holds a full URL. + * + * @return Generator + */ + public static function referrers(): Generator { + yield 'no referrer at all' => [ false, admin_url( 'plugins.php' ) ]; + yield 'an empty referrer' => [ '', admin_url( 'plugins.php' ) ]; + + yield 'a plugin update screen' => [ admin_url( 'update.php?action=upgrade-plugin' ), admin_url( 'plugins.php' ) ]; + yield 'the core update screen' => [ admin_url( 'update-core.php' ), admin_url( 'plugins.php' ) ]; + + yield 'the plugins list' => [ admin_url( 'plugins.php' ), false ]; + + // On multisite this is /wp-admin/network/plugins.php, which no comparison against + // admin_url() would recognise. + yield 'the network plugins list' => [ network_admin_url( 'plugins.php' ), false ]; + + yield 'another admin screen' => [ + admin_url( 'options-general.php?settings-updated=true' ), + admin_url( 'options-general.php?settings-updated=true' ), + ]; + yield 'another network admin screen' => [ network_admin_url( 'sites.php' ), network_admin_url( 'sites.php' ) ]; + + // What an admin form POST actually carries. Matching on the screen is what makes these two + // behave the same as their absolute equivalents. + yield 'a bare referrer path to the plugins list' => [ '/wp-admin/plugins.php?plugin_status=all', false ]; + yield 'a bare referrer path to another screen' => [ '/wp-admin/options-general.php', '/wp-admin/options-general.php' ]; + } +} diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php new file mode 100644 index 0000000..fcc4405 --- /dev/null +++ b/tests/unit/Conflict/ResolverTest.php @@ -0,0 +1,617 @@ +> + */ + private $deactivations = []; + + /** + * @var string|null + */ + private $request_method; + + public function setUp(): void { + parent::setUp(); + + Loader_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 = []; + + // Set explicitly rather than inherited from the harness. Nothing here is gated on the request + // method any more, but wp_get_referer() reads $_REQUEST and $_SERVER, so the tests that assert + // a destination depend on the request looking like the one they describe. + $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 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_method === null ) { + unset( $_SERVER['REQUEST_METHOD'] ); + } else { + $_SERVER['REQUEST_METHOD'] = $this->request_method; + } + + $this->clear_notices(); + Loader_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, Loader::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. + */ + public function test_the_collaborators_come_from_the_container(): void { + $checker = new class() implements Plugin_Checker_Interface { + /** + * @var string[] + */ + public $asked = []; + + /** + * @param string $basename Plugin basename. + * + * @return bool + */ + public function is_active( string $basename ): bool { + $this->asked[] = $basename; + + 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(); + + $container = new Test_Container(); + $container->singleton( + Plugin_Checker_Interface::class, + static function () use ( $checker ): Plugin_Checker_Interface { + return $checker; + } + ); + $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 ); + + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', admin_url( 'plugins.php' ) ); + + $this->resolve_all(); + + $this->assertSame( [ 'give-recurring/give-recurring.php' ], $checker->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.' + ); + } + + public function test_it_asks_the_redirector_it_was_given(): void { + $redirector = new class() extends Redirector { + /** + * @var array + */ + public $asked = []; + + /** + * @param string|false $referrer Referrer under test. + * + * @return string|false + */ + public function after_deactivation( $referrer ) { + $this->asked[] = $referrer; + + return admin_url( 'tools.php' ); + } + }; + + $container = new Test_Container(); + $container->singleton( + Redirector::class, + static function () use ( $redirector ): Redirector { + return $redirector; + } + ); + $this->set_up_container( $container ); + + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', admin_url( 'options-general.php' ) ); + + $location = $this->capture_resolution(); + + $this->assertSame( [ admin_url( 'options-general.php' ) ], $redirector->asked ); + $this->assertSame( admin_url( 'tools.php' ), $location ); + } + + 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 ); + + $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() ); + + // The destination, not merely that a redirect happened: a resolver that redirected somewhere + // else entirely would satisfy the count without sending anyone anywhere useful. + $this->assertSame( admin_url( 'plugins.php' ), $location ); + } + + public function test_deactivate_is_the_default_policy(): void { + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', false ); + + $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->setFunctionReturn( 'wp_get_referer', false ); + + $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->setFunctionReturn( 'wp_get_referer', false ); + + $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->setFunctionReturn( 'wp_get_referer', false ); + + $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->setFunctionReturn( 'wp_get_referer', false ); + + $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() ); + } + + /** + * 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 ); + Loader::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.' + ); + } + + /** + * Where a referrer sends the user is the redirector's own decision and is covered case by case in + * RedirectorTest. What belongs here is that the resolver asks it and honours a false. + */ + public function test_it_redirects_to_where_the_redirector_says(): void { + $this->standalone_is( true ); + $this->register(); + $this->setFunctionReturn( 'wp_get_referer', admin_url( 'options-general.php' ) ); + + $this->assertSame( admin_url( 'options-general.php' ), $this->capture_resolution() ); + } + + /** + * A false from the redirector means stay put, and staying put has to include not ending the + * request. `resolve_all()` fails on a redirect, which is what makes the absence of one an + * assertion rather than an assumption — coming from the plugins list there is nothing to send the + * user back to. + */ + 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_all(); + + $this->assertCount( 1, $this->deactivations ); + } + + public function test_resolve_all_needs_a_hook_prefix(): void { + $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 throw the same exception for the other reason. + Config_State::reset(); + Config::set_container( $container ); + + $this->expectException( Config_Exception::class ); + + $resolver->resolve_all(); + } + + /** + * 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. + * + * 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. + * + * @return void + */ + private function resolve_all(): 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 { + $this->resolver()->resolve_all(); + } 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 { + Loader::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 + ) + ); + } + + /** + * 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 ); + } + + private function clear_notices(): void { + delete_site_option( 'give_plugin_absorber_notices' ); + } + + /** + * The queue is stored as a site option on every install — on single site that call falls through + * to the plain option table — so there is one place to read it from. + * + * @return array + */ + private function queued_notices(): array { + $queue = get_site_option( 'give_plugin_absorber_notices', [] ); + + return is_array( $queue ) ? $queue : []; + } +} diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index ebe2fc6..89a6c65 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -8,6 +8,7 @@ use Codeception\TestCase\WPTestCase; use Generator; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Conflict\Resolver; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Loader; @@ -95,8 +96,9 @@ public function test_a_binding_that_does_not_implement_its_interface_is_reported * @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 ]; } /** @@ -118,8 +120,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/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php index 8551bd4..896b31b 100644 --- a/tests/unit/Notices/QueueTest.php +++ b/tests/unit/Notices/QueueTest.php @@ -15,8 +15,7 @@ use Nexcess\PluginAbsorber\Notices\Store; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithSubPlugins; -use RuntimeException; -use WP_Error; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithUsers; use wpdb; /** @@ -24,6 +23,7 @@ */ class QueueTest extends WPTestCase { use WithSubPlugins; + use WithUsers; private const OPTION = 'give_plugin_absorber_notices'; @@ -39,16 +39,9 @@ public function setUp(): void { $this->clear_queue(); // render() consumes the queue, so it is gated on a capability. Most tests care about the - // queue rather than the gate, so they run as someone who has it. - $user_id = $this->create_user( 'administrator' ); - - // On multisite activate_plugins is a network capability, so an administrator of a site is - // not enough — see test_a_site_administrator_on_multisite_cannot_consume_the_queue(). - if ( is_multisite() ) { - grant_super_admin( $user_id ); - } - - wp_set_current_user( $user_id ); + // queue rather than the gate, so they run as someone who has it — which on multisite is a + // network administrator, see test_a_site_administrator_on_multisite_cannot_consume_the_queue(). + $this->become_plugin_administrator(); } public function tearDown(): void { @@ -651,30 +644,6 @@ private function clear_queue(): void { delete_site_option( self::OPTION ); } - /** - * @param string $role Role to give the new user. - * - * @throws RuntimeException When the user cannot be created, rather than letting a later - * capability assertion fail for an unrelated reason. - * - * @return int - */ - private function create_user( string $role ): int { - $user_id = wp_insert_user( - [ - 'user_login' => uniqid( 'absorber-' ), - 'user_pass' => wp_generate_password(), - 'role' => $role, - ] - ); - - if ( $user_id instanceof WP_Error ) { - throw new RuntimeException( 'Could not create a ' . $role . ': ' . $user_id->get_error_message() ); - } - - return $user_id; - } - /** * The queue as the container builds it, or with one half replaced. * diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index c898287..37ad389 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -8,6 +8,10 @@ use Codeception\TestCase\WPTestCase; use Generator; use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +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; @@ -24,6 +28,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; @@ -77,6 +82,9 @@ 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 redirector' => [ Redirector::class, Redirector::class ]; + yield 'the conflict gate' => [ Gatekeeper::class, Gatekeeper::class ]; yield 'the load runner' => [ Runner::class, Runner::class ]; yield 'the boot scheduler' => [ Scheduler::class, Scheduler::class ]; } @@ -133,8 +141,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() ]; } /** From 923df1a9032268464f8388aa76fdfa11922e9c66 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 15:21:36 +0200 Subject: [PATCH 2/4] Run a sub-plugin's activation callback once, ever Activation becomes Activator. An abstract -ion noun names a directory in this ecosystem and never a class -- Activation/ holds an Activator and a Deactivator -- and the class does something rather than being something. Load\Runner takes it injected and calls it immediately after a successful require_once, so a callback never runs for a file that was not loaded. The bookkeeping is an option keyed by slug, because a callback that ran and a callback that failed have to be told apart across requests, and a sub-plugin absorbed into a host has no activation hook of its own to hang this on. There is no Loader::activation() accessor. The activator has exactly one caller and a host that wants different once-ever bookkeeping binds Activator_Interface, which is the same line the accessor would have been -- and public API is forever. --- CLAUDE.md | 16 +- docs/configuration.md | 24 ++ .../2026-08-12-container-required-rework.md | 2 +- src/Activator.php | 83 ++++++ src/Contracts/Activator_Interface.php | 33 +++ src/Load/Runner.php | 27 +- src/Provider.php | 7 +- tests/_support/Spy_Activator.php | 39 +++ tests/unit/ActivatorTest.php | 256 ++++++++++++++++++ tests/unit/Load/RunnerTest.php | 96 +++++++ tests/unit/ProviderTest.php | 8 + 11 files changed, 582 insertions(+), 9 deletions(-) create mode 100644 src/Activator.php create mode 100644 src/Contracts/Activator_Interface.php create mode 100644 tests/_support/Spy_Activator.php create mode 100644 tests/unit/ActivatorTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 7405032..83c5132 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -133,7 +133,8 @@ the plugin to ask about, and the collaborator does the asking. ### What exists today -`Activator` is not built yet. Currently: +The re-activation rewrite — `Loader::filter_activation_error_markup()` on `wp_admin_notice_markup` — +is not built yet. Currently: | Path | What | |---|---| @@ -146,10 +147,11 @@ the plugin to ask about, and the collaborator does the asking. | `src/Conflict_Policy.php` | The three policy constants, `default()`, `is_valid()`. | | `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/Activator.php` | Runs a sub-plugin's activation callback once ever, recorded in one option. | | `src/Conflict/` | `Resolver` (which policy branch to take), `Gatekeeper` (which requests may take one), `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`. | +| `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Activator_Interface`, `Config_Exception`. | ### Boot lifecycle @@ -189,6 +191,16 @@ at the priority currently being dispatched is accepted and never reached. loaded → dependencies met → file exists → `should_load` filter → `require_once` → activation callback (only after a *successful* require). +The activation callback is the last of those and runs through `Activator`, which `Load\Runner` takes +as a constructor argument like the notice queue. Last, because a bundled plugin is included rather +than activated: `register_activation_hook()` never fires for it, so the callback stands in for +whatever that hook would have done, and it has to run with the plugin's own code already in memory. +Only after a require that happened, because creating tables and seeding options for a sub-plugin +whose code is *not* loaded is worse than not creating them — and the once-ever record would then +stand the callback down for good, the first time the sub-plugin really did load. The record is +written after the callback returns, never before, so a callback that throws is retried next request +rather than marked done. + The guard constant is checked **before** the dependency check, not after. It is one `defined()`, it carries the whole re-declaration guarantee, and it is the only gate meaning "this plugin is already running" — warning that requirements are unmet for a plugin the admin can watch working would send diff --git a/docs/configuration.md b/docs/configuration.md index 3058271..ac2fc41 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -48,6 +48,7 @@ $container->singleton( Registrar_Interface::class, My_Registrar::class ); | `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. | +| `Contracts\Activator_Interface` | `Activator` | Runs a sub-plugin's activation callback once, ever. | `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 @@ -103,6 +104,29 @@ at the second `register()` call; a config array the library cannot use is still Register unconditionally and put anything you cannot decide up front — a licence that may not be active, a setting the site owner can change — in `enabled`, which is re-evaluated on every load. +## Activation + +A bundled plugin is `require_once`d, not activated, so `register_activation_hook()` never fires for +it — whatever that hook would have done, creating a table or seeding options, would otherwise never +happen at all. [`activation_callback`](#sub-plugin-keys) fills that gap: + +```php +'activation_callback' => static function ( Sub_Plugin $sub_plugin ) { + \Give\Recurring\Install::create_tables(); +}, +``` + +It runs exactly once ever per slug, is passed the `Sub_Plugin`, and runs only after a require that +actually happened — never for a sub-plugin whose load was skipped, because a schema appearing for a +plugin that is not loaded is worse than no schema at all. + +The record lives in the `{option_prefix}_plugin_absorber_activations` option, a network option on +multisite for the same reason the [notice queue](notices.md) is one: `deactivate_plugins()` is +network-wide, so a merge that happened network-wide must not re-run the callback on every site. The +slug is recorded *after* the callback returns, so a callback that fails is retried on the next +request rather than marked done and silently skipped forever. Bind `Activator_Interface` to record +"once, ever" somewhere else — your own migration table, say. + ## The bundled file is included from a function, not from global scope WordPress includes plugins from `wp-settings.php` at global scope; this library includes them from diff --git a/docs/superpowers/plans/2026-08-12-container-required-rework.md b/docs/superpowers/plans/2026-08-12-container-required-rework.md index ae092fa..6609a96 100644 --- a/docs/superpowers/plans/2026-08-12-container-required-rework.md +++ b/docs/superpowers/plans/2026-08-12-container-required-rework.md @@ -287,7 +287,7 @@ level 9, so most breakage surfaces from `composer test:analysis` without standin ### Task 13 — `13-activation` -- [ ] `Activation` → `Activator`, `Activation_Interface` → `Activator_Interface`; constructor injection; bind in `Provider`. +- [x] `Activation` → `Activator`, `Activation_Interface` → `Activator_Interface`; constructor injection; bind in `Provider`. ### Task 14 — `14-activation-error-rewrite` diff --git a/src/Activator.php b/src/Activator.php new file mode 100644 index 0000000..68b94c7 --- /dev/null +++ b/src/Activator.php @@ -0,0 +1,83 @@ + true map in one option. + * + * @since 1.0.0 + */ +class Activator implements Activator_Interface { + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin that has just been loaded. + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + public function maybe_run( Sub_Plugin $sub_plugin ): void { + $callback = $sub_plugin->get_activation_callback(); + + if ( $callback === null ) { + return; + } + + $slug = $sub_plugin->get_slug(); + $done = $this->completed(); + + if ( ! empty( $done[ $slug ] ) ) { + return; + } + + // Recorded after the callback rather than before it. A callback that fatals halfway leaves + // the site half-migrated either way, but recording first would also mean the next request + // skips it, so the half-finished state becomes permanent and invisible. + $callback( $sub_plugin ); + + $done[ $slug ] = true; + + update_site_option( self::option_name(), $done ); + } + + /** + * The option every slug's activation record lives in. + * + * @since 1.0.0 + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return string + */ + public static function option_name(): string { + return Config::get_option_name( 'activations' ); + } + + /** + * Slugs whose activation callback has already run, keyed by slug. + * + * A site option, not a plain one: `deactivate_plugins()` is network-wide on multisite, so an + * activation that follows a network-wide merge has to be recorded network-wide too, or every + * site in the network runs the callback again. + * + * @since 1.0.0 + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return array + */ + private function completed(): array { + $done = get_site_option( self::option_name(), [] ); + + // Anything that is not an array is replaced rather than trusted. A corrupted option would + // otherwise fatal on the array read, inside plugins_loaded, on every request. + return is_array( $done ) ? $done : []; + } +} diff --git a/src/Contracts/Activator_Interface.php b/src/Contracts/Activator_Interface.php new file mode 100644 index 0000000..9bc16f1 --- /dev/null +++ b/src/Contracts/Activator_Interface.php @@ -0,0 +1,33 @@ +notices = $notices; + private $activator; + + /** + * @since 1.0.0 + * + * @param Queue_Interface $notices Where a sub-plugin that could not load says so. + * @param Activator_Interface $activator Runs the activation callback of one that did. + */ + public function __construct( Queue_Interface $notices, Activator_Interface $activator ) { + $this->notices = $notices; + $this->activator = $activator; } /** @@ -129,5 +139,12 @@ private function load( Sub_Plugin $sub_plugin ): void { // file are function-local as a result -- documented for hosts, because no amount of // wrapping here can hand a required file the global scope it would have had. require_once $file; + + // Only after a require that actually happened. A bundled plugin is included rather than + // activated, so register_activation_hook() never fires for it and whatever that hook would + // have done -- creating a table, seeding options -- would never happen at all. Running it + // for a sub-plugin that was skipped would be worse: the schema would appear for a plugin + // that is not loaded. + $this->activator->maybe_run( $sub_plugin ); } } diff --git a/src/Provider.php b/src/Provider.php index 16307c0..e2a96b0 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -10,6 +10,7 @@ use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Conflict\Redirector; use Nexcess\PluginAbsorber\Conflict\Resolver; +use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; @@ -66,6 +67,7 @@ public function register(): void { $this->bind_once( Registrar_Interface::class, Registrar::class ); $this->bind_once( Plugin_Checker_Interface::class, Plugin_Checker::class ); $this->bind_once( Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ); + $this->bind_once( Activator_Interface::class, Activator::class ); $this->bind_once( Store::class ); $this->bind_once( Renderer::class ); $this->bind_once( Redirector::class ); @@ -96,7 +98,10 @@ static function () use ( $container ): Resolver { $this->bind_once( Runner::class, static function () use ( $container ): Runner { - return new Runner( $container->get( Queue_Interface::class ) ); + return new Runner( + $container->get( Queue_Interface::class ), + $container->get( Activator_Interface::class ) + ); } ); diff --git a/tests/_support/Spy_Activator.php b/tests/_support/Spy_Activator.php new file mode 100644 index 0000000..daf42f3 --- /dev/null +++ b/tests/_support/Spy_Activator.php @@ -0,0 +1,39 @@ +slugs` off a value typed as + * `Activator_Interface` is reading a property the interface does not declare, and static analysis + * rightly rejects it. Named, the spy's own type carries the recorder. + * + * It records rather than runs, which is the point — a test that binds this one proves the load path + * reached the activator it was handed without writing the default's option along the way. + * + * @since 1.0.0 + */ +class Spy_Activator implements Activator_Interface { + /** + * Slugs maybe_run() was called for, in call order. + * + * @var string[] + */ + public $slugs = []; + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin that has just been loaded. + * + * @return void + */ + public function maybe_run( Sub_Plugin $sub_plugin ): void { + $this->slugs[] = $sub_plugin->get_slug(); + } +} diff --git a/tests/unit/ActivatorTest.php b/tests/unit/ActivatorTest.php new file mode 100644 index 0000000..ea5878f --- /dev/null +++ b/tests/unit/ActivatorTest.php @@ -0,0 +1,256 @@ +clear_activations(); + } + + public function tearDown(): void { + $this->clear_activations(); + Config_State::reset(); + parent::tearDown(); + } + + public function test_it_runs_the_activation_callback(): void { + $calls = []; + + ( new Activator() )->maybe_run( $this->recording_sub_plugin( $calls ) ); + + $this->assertCount( 1, $calls ); + } + + /** + * The sub-plugin is the callback's one argument, so a host's migration can read the slug and the + * bundled file it is running for rather than being closed over one of them at config time. + */ + public function test_the_callback_receives_the_sub_plugin(): void { + $calls = []; + $sub_plugin = $this->recording_sub_plugin( $calls ); + + ( new Activator() )->maybe_run( $sub_plugin ); + + $this->assertSame( [ $sub_plugin ], $calls ); + } + + public function test_it_runs_the_callback_only_once(): void { + $calls = []; + $sub_plugin = $this->recording_sub_plugin( $calls ); + $activator = new Activator(); + + $activator->maybe_run( $sub_plugin ); + $activator->maybe_run( $sub_plugin ); + + $this->assertCount( 1, $calls ); + } + + /** + * The record is the option, not the object holding it. Every request builds a fresh collaborator, + * so an in-memory guard would run the callback again on the next page load — forever. + */ + public function test_a_fresh_instance_does_not_run_the_callback_again(): void { + $calls = []; + $sub_plugin = $this->recording_sub_plugin( $calls ); + + ( new Activator() )->maybe_run( $sub_plugin ); + ( new Activator() )->maybe_run( $sub_plugin ); + + $this->assertCount( 1, $calls ); + } + + public function test_it_records_the_slug_in_the_option(): void { + $calls = []; + + ( new Activator() )->maybe_run( $this->recording_sub_plugin( $calls ) ); + + $this->assertSame( [ 'give-recurring' => true ], $this->recorded() ); + } + + /** + * Most sub-plugins configure no callback at all. Writing a row for them would put an option on + * every site that has nothing to record, and one that can never be read for anything. + */ + public function test_it_does_nothing_without_an_activation_callback(): void { + ( new Activator() )->maybe_run( $this->make_sub_plugin() ); + + $this->assertFalse( get_site_option( self::OPTION, false ), 'No callback means no option.' ); + } + + public function test_two_slugs_are_tracked_independently(): void { + $recurring_calls = []; + $recovery_calls = []; + $recurring = $this->recording_sub_plugin( $recurring_calls, [ 'slug' => 'give-recurring' ] ); + $recovery = $this->recording_sub_plugin( $recovery_calls, [ 'slug' => 'give-fee-recovery' ] ); + $activator = new Activator(); + + $activator->maybe_run( $recurring ); + $activator->maybe_run( $recovery ); + $activator->maybe_run( $recurring ); + + $this->assertCount( 1, $recurring_calls ); + $this->assertCount( 1, $recovery_calls, 'One slug being recorded must not stand the other down.' ); + $this->assertSame( + [ 'give-recurring' => true, 'give-fee-recovery' => true ], + $this->recorded() + ); + } + + /** + * @dataProvider corrupted_options + * + * @param mixed $stored Raw option value to seed. + */ + public function test_it_recovers_from_a_corrupted_option( $stored ): void { + update_site_option( self::OPTION, $stored ); + + $calls = []; + + ( new Activator() )->maybe_run( $this->recording_sub_plugin( $calls ) ); + + $this->assertCount( 1, $calls ); + $this->assertSame( [ 'give-recurring' => true ], $this->recorded() ); + } + + /** + * Anything that is not an array reaches an array read inside plugins_loaded, on every request — + * so it is replaced rather than trusted, and the site heals on the next write. + * + * @return Generator + */ + public static function corrupted_options(): Generator { + yield 'a string instead of an array' => [ 'not-a-record' ]; + + yield 'an object instead of an array' => [ (object) [ 'give-recurring' => true ] ]; + + yield 'a number instead of an array' => [ 42 ]; + } + + /** + * Two hosts on one site each get their own record, and a prefix that names filters is folded + * before it becomes a storage key. + */ + public function test_the_option_name_follows_the_hook_prefix(): void { + Config_State::reset(); + Config::set_hook_prefix( 'woo' ); + + $this->assertSame( Config::get_option_name( 'activations' ), Activator::option_name() ); + $this->assertSame( self::OPTION_WOO, Activator::option_name() ); + + $calls = []; + + ( new Activator() )->maybe_run( $this->recording_sub_plugin( $calls ) ); + + $this->assertSame( [ 'give-recurring' => true ], get_site_option( self::OPTION_WOO ) ); + $this->assertFalse( get_site_option( self::OPTION, false ), 'Another host must not be written to.' ); + } + + /** + * The slug is recorded after the callback returns, never before. A callback that fatals halfway + * leaves the site half-migrated either way, but recording first would make the half-finished + * state permanent: the next request would see the record and skip the retry. + */ + public function test_a_throwing_callback_leaves_the_slug_unrecorded(): void { + $failing = $this->make_sub_plugin( + [ + 'activation_callback' => static function (): void { + throw new RuntimeException( 'the migration could not reach the database' ); + }, + ] + ); + + $threw = false; + + try { + ( new Activator() )->maybe_run( $failing ); + } catch ( RuntimeException $exception ) { + $threw = true; + + $this->assertSame( 'the migration could not reach the database', $exception->getMessage() ); + } + + $this->assertTrue( $threw, 'The callback has to have run for this to be about the ordering.' ); + $this->assertFalse( get_site_option( self::OPTION, false ), 'A failed run must not be recorded.' ); + + $calls = []; + + ( new Activator() )->maybe_run( $this->recording_sub_plugin( $calls ) ); + + $this->assertCount( 1, $calls, 'The slug has to be retried, not skipped forever.' ); + } + + /** + * A sub-plugin whose activation callback appends itself to the given array. + * + * The whole `Sub_Plugin` is recorded rather than its slug, so the argument the callback is handed + * can be asserted on as well as counted. + * + * @param array $calls Recorder to append to. + * @param array $overrides Config overrides. + * + * @return Sub_Plugin + */ + private function recording_sub_plugin( array &$calls, array $overrides = [] ): Sub_Plugin { + return $this->make_sub_plugin( + array_merge( + [ + 'activation_callback' => static function ( Sub_Plugin $sub_plugin ) use ( &$calls ): void { + $calls[] = $sub_plugin; + }, + ], + $overrides + ) + ); + } + + /** + * The record as it stands, for whichever prefix the test set. + * + * @return array + */ + private function recorded(): array { + $done = get_site_option( Activator::option_name(), [] ); + + return is_array( $done ) ? $done : []; + } + + /** + * @return void + */ + private function clear_activations(): void { + delete_site_option( self::OPTION ); + delete_site_option( self::OPTION_WOO ); + } +} diff --git a/tests/unit/Load/RunnerTest.php b/tests/unit/Load/RunnerTest.php index 1f89644..dc2e9c9 100644 --- a/tests/unit/Load/RunnerTest.php +++ b/tests/unit/Load/RunnerTest.php @@ -8,6 +8,7 @@ use Codeception\TestCase\WPTestCase; use lucatume\WPBrowser\Traits\UopzFunctions; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Load\Runner; use Nexcess\PluginAbsorber\Loader; @@ -15,12 +16,14 @@ use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Loader_State; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Activator; use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithSubPlugins; /** * The load loop and its gate chain. @@ -36,6 +39,7 @@ class RunnerTest extends WPTestCase { use WithBundledPlugins; use WithContainer; use WithIncorrectUsage; + use WithSubPlugins; /** * Guard constants a test defined through uopz. @@ -62,6 +66,7 @@ public function setUp(): void { Config::set_hook_prefix( 'give' ); $this->set_up_container(); $this->clear_notices(); + $this->clear_activations(); $this->reset_bundled_plugin_loads(); $this->should_load_calls = []; } @@ -79,6 +84,7 @@ public function tearDown(): void { $this->stop_expecting_incorrect_usage(); $this->clear_notices(); + $this->clear_activations(); Loader_State::reset(); Config_State::reset(); $this->tear_down_container(); @@ -316,6 +322,81 @@ public function test_the_should_load_filter_is_not_consulted_when_dependencies_a $this->assert_the_should_load_recorder_works(); } + /** + * The activation callback stands in for the register_activation_hook() a bundled plugin never + * gets, so it has to run with the plugin's own code already in memory: a migration that calls a + * function the bundled file declares would otherwise fatal. + */ + public function test_it_runs_the_activation_callback_after_requiring_the_file(): void { + $loads_at_activation = null; + + $this->register( + [ + 'activation_callback' => function () use ( &$loads_at_activation ): void { + $loads_at_activation = $this->bundled_plugin_loads(); + }, + ] + ); + + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assertSame( 1, $loads_at_activation, 'The bundled file has to be in memory first.' ); + $this->assertSame( [ 'give-recurring' => true ], $this->activation_record() ); + } + + /** + * Activation is tied to a require that actually happened. Running it for a skipped sub-plugin + * would create the tables and seed the options of a plugin whose code is not loaded, and the + * record would then stand the callback down for good once the sub-plugin really did load. + */ + public function test_a_skipped_load_runs_no_activation_callback(): void { + $calls = []; + + $record = static function ( Sub_Plugin $sub_plugin ) use ( &$calls ): void { + $calls[] = $sub_plugin->get_slug(); + }; + + $this->register( [ 'enabled' => false, 'activation_callback' => $record ] ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertSame( [], $calls ); + $this->assertSame( [], $this->activation_record() ); + + // The recorder has to be shown to work. A closure that never reached the config array — a + // mistyped key, a value the constructor refused — leaves this empty for a reason that has + // nothing to do with the load being skipped, and the assertion above proves nothing. + $record( $this->make_sub_plugin() ); + + $this->assertSame( [ 'give-recurring' ], $calls, 'The recorder must catch a call that really happened.' ); + } + + /** + * The activator is injected, not resolved inside the load path, so a host that records "once, + * ever" in its own migration table binds one implementation and the load pass uses it — and the + * default's option is never written behind its back. + */ + public function test_a_bound_activator_reaches_the_load_path(): void { + $activator = new Spy_Activator(); + $container = new Test_Container(); + $container->singleton( + Activator_Interface::class, + static function () use ( $activator ): Activator_Interface { + return $activator; + } + ); + $this->set_up_container( $container ); + + $this->register( [ 'activation_callback' => static fn() => null ] ); + + $this->runner()->load_all(); + + $this->assertSame( [ 'give-recurring' ], $activator->slugs ); + $this->assertSame( [], $this->activation_record(), 'The default activator must not have run too.' ); + } + public function test_it_loads_every_registered_sub_plugin(): void { $this->register( [ 'slug' => 'give-recurring' ] ); $this->register( [ 'slug' => 'give-fee-recovery' ] ); @@ -538,6 +619,21 @@ private function clear_notices(): void { delete_site_option( 'give_plugin_absorber_notices' ); } + private function clear_activations(): void { + delete_site_option( 'give_plugin_absorber_activations' ); + } + + /** + * Slugs whose activation callback has run, as the option holds them. + * + * @return array + */ + private function activation_record(): array { + $done = get_site_option( 'give_plugin_absorber_activations', [] ); + + return is_array( $done ) ? $done : []; + } + /** * The queue is stored as a site option on every install — on single site that call falls through * to the plain option table — so there is one place to read it from. diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index 37ad389..e63e6b5 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -12,6 +12,8 @@ use Nexcess\PluginAbsorber\Conflict\Gatekeeper; use Nexcess\PluginAbsorber\Conflict\Redirector; use Nexcess\PluginAbsorber\Conflict\Resolver; +use Nexcess\PluginAbsorber\Activator; +use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Contracts\Provider_Interface; @@ -26,6 +28,7 @@ use Nexcess\PluginAbsorber\Provider; use Nexcess\PluginAbsorber\Registrar; use Nexcess\PluginAbsorber\Tests\Support\Config_State; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Activator; use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Spy_Resolver; @@ -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 activator' => [ Activator_Interface::class, Activator::class ]; yield 'the conflict resolver' => [ Resolver_Interface::class, Resolver::class ]; yield 'the redirector' => [ Redirector::class, Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class, Gatekeeper::class ]; @@ -144,6 +148,10 @@ 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 conflict resolver' => [ Resolver_Interface::class, new Spy_Resolver() ]; + + // "Once, ever" is recorded in an option here, which is one opinion among several — a host + // that tracks it in its own migration table has to be able to say so. + yield 'the activator' => [ Activator_Interface::class, new Spy_Activator() ]; } /** From 66eba852b457e9286cc1162193a965aa0d129eab Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 15:30:11 +0200 Subject: [PATCH 3/4] Rewrite the activation-error screen for an absorbed standalone An admin who still has the standalone installed and clicks Activate gets WordPress's own fatal-error screen, because the bundled copy already defined everything the standalone is about to. wp_admin_notice_markup lets us replace that wording with an explanation of the merge, on the one screen where the site owner is actively trying to do the thing we have made impossible. The rewrite lives on Notices\Queue rather than in a class of its own: it is notice wording for a sub-plugin, keyed by the same standalone basename the queue already reasons about, and a separate class would have to be handed the registrar to find out which sub-plugin the screen is even about. The filter is a named static trampoline rather than a closure over the container, unlike the plugins_loaded steps. This is the one hook that rewrites a screen WordPress drew instead of adding one of ours, so a host that wants core's wording back needs a callback it can remove_filter() -- and a closure cannot be removed. The plugins_loaded steps are closures because that sequence has to run inline when boot came too late, which is a constraint these two admin hooks do not have. --- CLAUDE.md | 37 +- docs/conflict-handling.md | 27 +- docs/notices.md | 7 + .../2026-08-12-container-required-rework.md | 2 +- src/Boot/Scheduler.php | 8 + src/Loader.php | 27 ++ src/Notices/Contracts/Queue_Interface.php | 22 ++ src/Notices/Queue.php | 106 ++++++ tests/_support/Spy_Queue.php | 28 ++ tests/unit/Boot/SchedulerTest.php | 71 +++- tests/unit/LoaderTest.php | 105 +++++- .../unit/Notices/QueueActivationErrorTest.php | 345 ++++++++++++++++++ 12 files changed, 760 insertions(+), 25 deletions(-) create mode 100644 tests/unit/Notices/QueueActivationErrorTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 83c5132..c76cb86 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -133,13 +133,13 @@ the plugin to ask about, and the collaborator does the asking. ### What exists today -The re-activation rewrite — `Loader::filter_activation_error_markup()` on `wp_admin_notice_markup` — -is not built yet. Currently: +Every behaviour described above is built; nothing in `src/` is still owed. What is left in the plan +is the end-to-end suite and the release pass. | Path | What | |---|---| | `src/Config.php` | Static facade: hook prefix + container. | -| `src/Loader.php` | Static facade: the registration buffer, `boot()`, and the accessors. | +| `src/Loader.php` | Static facade: the registration buffer, `boot()`, the accessors, and the two notice trampolines. | | `src/Provider.php` | Binds every collaborator; the only file that names a default implementation. | | `src/Boot/Scheduler.php` | Hook wiring and boot timing: the sequence, the priorities, and the fallback for a host that boots too late. | | `src/Load/Runner.php` | The load pass: the gate chain, the `require_once`, the activation callback. | @@ -150,7 +150,7 @@ is not built yet. Currently: | `src/Activator.php` | Runs a sub-plugin's activation callback once ever, recorded in one option. | | `src/Conflict/` | `Resolver` (which policy branch to take), `Gatekeeper` (which requests may take one), `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/Notices/` | `Queue` (what a notice says, who may consume it, and the activation-error rewrite), `Store` (keeps it), `Renderer` (draws it), `Contracts\Queue_Interface`. | | `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Activator_Interface`, `Config_Exception`. | ### Boot lifecycle @@ -208,10 +208,11 @@ them after the wrong problem. `docs/filters.md` and the spec agree. `Loader::all()` narrows to `Sub_Plugin` instances itself, so no caller repeats that guard. A host may bind a registrar returning anything, and PHP 7.4 cannot express `array` in -the interface signature — so it is filtered once where the untrusted value enters. Both passes read -through `Loader::all()` rather than through the registrar they could resolve for themselves, because -it flushes the pending registrations before it reads and a registrar asked directly would miss -anything registered since the last flush. +the interface signature — so it is filtered once where the untrusted value enters. All three readers +— the load pass, the conflict pass and the activation-error rewrite — go through `Loader::all()` +rather than through the registrar they could resolve for themselves, because it flushes the pending +registrations before it reads and a registrar asked directly would miss anything registered since +the last flush. `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and `DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. It is @@ -247,6 +248,26 @@ An unknown policy must be handled as its own case via `Conflict_Policy::is_valid a `default:` fallthrough — a typo like `'defered'` would otherwise deactivate a plugin the site owner deliberately turned on. +**The activation-error rewrite lives on `Notices\Queue`, not on a class of its own.** It is the one +conflict the load guard cannot prevent — core includes the plugin being activated *after* the +bundled copy is in memory, so the re-declaration really does fatal — and all this library gets to do +about it is reword the sentence core's sandbox prints. That sentence is +`conflict_notice_message`, the same message the merge notice carries, so wording it belongs where +every other notice is worded; a host that binds its own `Queue_Interface` owns the error screen +along with the rest. `Loader::filter_activation_error_markup()` is the trampoline, and it takes an +**untyped** argument: a filter receives whatever the filter before it returned, and a `string` +declaration would turn another plugin's sloppy return into a TypeError raised from here, on the +screen least able to afford a second one. The rewrite refuses unless the screen is `plugins`, the +`plugin` query arg names a registered standalone and `_error_nonce` verifies — and it sanitises with +`wp_kses_post()` *before* testing for emptiness, since a message that filters down to nothing must +leave core's wording standing rather than blank the notice box. + +**`Boot\Scheduler` wires `wp_admin_notice_markup` as a named static callback, not a closure.** Both +admin-only hooks are `[ Loader::class, … ]` pairs that resolve the queue when they fire, so neither +builds anything at boot; what the name buys on this one is `remove_filter()`, which a host wanting +core's wording back has no other way to reach. The `plugins_loaded` steps are closures for a reason +these two do not share — that sequence has to be runnable inline as well as wirable. + ### Keys - Filters: `{$hook_prefix}/plugin_absorber/should_load`, `{$hook_prefix}/plugin_absorber/conflict_policy` diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index e323abc..6c23467 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -96,7 +96,26 @@ jobs. The cost is a missed detection; the alternative costs the guarantee the gu ## What the guard cannot do -The guard cannot help on the request that *activates* the standalone: WordPress includes it after -the bundled copy has already loaded, so that re-declaration is a real fatal. WordPress catches it -in its activation sandbox, and this library rewrites the resulting error screen into an -explanation. +The guard cannot help on the request that *activates* the standalone: WordPress includes the plugin +being activated **after** the bundled copy has already loaded, so that re-declaration is a real +fatal. Core catches it in its activation sandbox and prints *"Plugin could not be activated because +it triggered a fatal error."* — true, and useless to whoever pressed the button. + +So the library filters `wp_admin_notice_markup` and swaps that sentence for the sub-plugin's +`conflict_notice_message`, falling back to a generic one naming the slug. This is what puts the +WordPress floor at 6.4: the filter does not exist before it. + +It touches nothing else. The markup comes back unchanged unless all three hold — the screen is +`plugins`, the `plugin` query arg names a standalone this library has registered, and `_error_nonce` +verifies against `plugin-activation-error_{basename}`. Another plugin's fatal is another plugin's +business. + +The replacement runs through `wp_kses_post()`, so a knowledge-base link survives, and it is +sanitised *before* it is checked for emptiness: a message that filters down to nothing leaves core's +wording in place rather than blanking the notice. + +The filter is wired by `Boot\Scheduler` under `is_admin()`, as +`[ Loader::class, 'filter_activation_error_markup' ]` — a named callback, so a host that would +rather keep core's wording can `remove_filter()` it. The rewriting itself is +`Notices\Queue::filter_activation_error_markup()`, so a host that binds its own `Queue_Interface` +owns this screen along with the rest of the notices. diff --git a/docs/notices.md b/docs/notices.md index a1e73f0..99e7a19 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -21,6 +21,13 @@ only warning an administrator was ever going to get. On multisite `activate_plugins` maps through `manage_network_plugins`, so it is a network administrator, not the site administrator who installed the plugin, who sees these. +## `conflict_notice_message` is used twice + +The same message backs the queued notice raised when the standalone is deactivated *and* the +rewritten activation-error screen a user meets if they try to re-activate it — see +[conflict handling](conflict-handling.md). Write one sentence that reads sensibly both as a report of +something already done and as the explanation standing in for a fatal-error warning. + ## Rendering them yourself `Notices\Queue::option_name()` is public, so you can render the queue yourself without replacing diff --git a/docs/superpowers/plans/2026-08-12-container-required-rework.md b/docs/superpowers/plans/2026-08-12-container-required-rework.md index 6609a96..7b41c1e 100644 --- a/docs/superpowers/plans/2026-08-12-container-required-rework.md +++ b/docs/superpowers/plans/2026-08-12-container-required-rework.md @@ -291,7 +291,7 @@ level 9, so most breakage surfaces from `composer test:analysis` without standin ### Task 14 — `14-activation-error-rewrite` -- [ ] Rebase; de-null any collaborator constructor the branch adds; keep `wp_admin_notice_markup` wiring in `Boot\Scheduler`. +- [x] Rebase; de-null any collaborator constructor the branch adds; keep `wp_admin_notice_markup` wiring in `Boot\Scheduler`. ### Task 15 — `15-e2e-suite` diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index 9589098..8d2347f 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -86,6 +86,14 @@ public function wire(): void { // superadmin working in the network admin -- exactly where a network-wide // deactivation gets noticed -- would never see the queue rendered. add_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); + + // A named static trampoline like the notice step above, not a closure. Both resolve + // the queue when they fire, so neither builds anything at boot, but a named callback + // can also be taken back with remove_filter() -- which matters most here, on the one + // hook that rewrites a screen WordPress drew rather than adding one of our own. The + // closures below are shaped that way for a reason these two do not share: the + // plugins_loaded sequence has to be runnable inline as well as wirable. + add_filter( 'wp_admin_notice_markup', [ Loader::class, 'filter_activation_error_markup' ] ); } // Adding an action at a priority the current dispatch has already passed is accepted and diff --git a/src/Loader.php b/src/Loader.php index 5f33e77..4abb6d7 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -179,6 +179,33 @@ public static function render_notices(): void { self::notices()->render(); } + /** + * Rewrite the activation-error notice for a standalone this library has absorbed. + * + * The parameter is untyped because a filter receives whatever the filter before it returned, + * and a `string` declaration would turn another plugin's sloppy return into a TypeError raised + * from here. + * + * @since 1.0.0 + * + * @param mixed $markup Notice markup WordPress is about to print. + * + * @throws Config_Exception When no container has been set, or a container binding is unusable. + * + * @return string + */ + public static function filter_activation_error_markup( $markup ): string { + $markup = is_string( $markup ) ? $markup : ''; + + // Returned unchanged rather than thrown out of: this runs while WordPress is drawing an + // error screen, and a second fatal there would replace the one the user came to read. + if ( ! self::has_hook_prefix() ) { + return $markup; + } + + return self::notices()->filter_activation_error_markup( $markup ); + } + /** * The object bound to a collaborator interface, checked before it is handed on. * diff --git a/src/Notices/Contracts/Queue_Interface.php b/src/Notices/Contracts/Queue_Interface.php index e3cf04a..daade27 100644 --- a/src/Notices/Contracts/Queue_Interface.php +++ b/src/Notices/Contracts/Queue_Interface.php @@ -85,4 +85,26 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void; * @return void */ public function render(): void; + + /** + * Replace WordPress's generic fatal-activation text with the sub-plugin's own explanation. + * + * Filters `wp_admin_notice_markup`. This is the one conflict the load guard cannot prevent: + * WordPress includes a plugin being activated after the bundled copy has already loaded, so + * the re-declaration is a real fatal, caught in core's activation sandbox and reported as + * "the plugin triggered a fatal error" — true, and useless to whoever pressed the button. + * + * An implementation must return the markup untouched unless the request really is a + * nonce-verified activation error, on the plugins screen, for a standalone this library + * knows about. + * + * @since 1.0.0 + * + * @param string $markup Notice markup WordPress is about to print. + * + * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * + * @return string + */ + public function filter_activation_error_markup( string $markup ): string; } diff --git a/src/Notices/Queue.php b/src/Notices/Queue.php index dd34180..0e1f1fb 100644 --- a/src/Notices/Queue.php +++ b/src/Notices/Queue.php @@ -6,6 +6,7 @@ namespace Nexcess\PluginAbsorber\Notices; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Loader; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; @@ -177,6 +178,84 @@ public function render(): void { $this->store->clear(); } + /** + * @since 1.0.0 + * + * @param string $markup Notice markup WordPress is about to print. + * + * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * + * @return string + */ + public function filter_activation_error_markup( string $markup ): string { + if ( ! is_admin() || ! function_exists( 'get_current_screen' ) ) { + return $markup; + } + + $screen = get_current_screen(); + + // Both plugin lists, because wp-admin/network/plugins.php is a one-line require of + // wp-admin/plugins.php and draws the identical activation error -- but WP_Screen appends + // `-network` to the id there. On a default multisite the subsite has no plugins UI at all, + // so the network screen is the *only* place a super admin can reactivate the standalone, + // and matching 'plugins' alone would decline on the one screen that matters most. + if ( $screen === null || ! in_array( $screen->id, [ 'plugins', 'plugins-network' ], true ) ) { + return $markup; + } + + // phpcs:disable WordPress.Security.NonceVerification.Recommended -- verified below, once + // the plugin named turns out to be one this library owns. Nothing is acted on until then. + $basename = isset( $_GET['plugin'] ) + ? sanitize_text_field( wp_unslash( $_GET['plugin'] ) ) + : ''; + + if ( $basename === '' ) { + return $markup; + } + + // Looked up before the nonce is checked, deliberately: there is no nonce work to do for a + // plugin this library does not own, and the nonce is still verified before any markup is + // touched. + $sub_plugin = $this->find_by_standalone_basename( $basename ); + + if ( $sub_plugin === null ) { + return $markup; + } + + $nonce = isset( $_GET['_error_nonce'] ) + ? sanitize_text_field( wp_unslash( $_GET['_error_nonce'] ) ) + : ''; + // phpcs:enable WordPress.Security.NonceVerification.Recommended + + if ( ! wp_verify_nonce( $nonce, 'plugin-activation-error_' . $basename ) ) { + return $markup; + } + + $message = $sub_plugin->get_conflict_notice_message( + sprintf( + '%s is bundled with this plugin and loads automatically. The standalone copy cannot' + . ' be activated alongside it.', + $sub_plugin->get_slug() + ) + ); + + // Sanitised before the emptiness check rather than after. wp_kses_post( '' ) + // is the empty string, and swapping WordPress's wording for nothing leaves a blank notice + // box where the explanation should be. wp_kses_post() and not esc_html(), for the reason + // the renderer uses it: these messages come from the host's own config, and a link to a + // knowledge-base article has to survive. + $message = trim( wp_kses_post( $message ) ); + + if ( $message === '' ) { + return $markup; + } + + // phpcs:ignore WordPress.WP.I18n.TextDomainMismatch -- core's own string, matched on purpose. + $core_text = __( 'Plugin could not be activated because it triggered a fatal error.', 'default' ); + + return str_replace( $core_text, $message, $markup ); + } + /** * The option name backing the queue. Read it directly to render these notices yourself. * @@ -210,4 +289,31 @@ public static function option_name(): string { private function queue( Sub_Plugin $sub_plugin, string $type, string $message ): void { $this->store->put( $sub_plugin->get_slug() . ':' . $type, $message ); } + + /** + * The registered sub-plugin a standalone basename belongs to, if any. + * + * Read through `Loader::all()` rather than a registrar of this queue's own, for the reason the + * resolver and the load pass do: it flushes the registrations still buffered on the facade + * before it reads, and a registrar asked directly would miss anything registered since the + * last read. + * + * @since 1.0.0 + * + * @param string $basename Standalone plugin basename named by the request. + * + * @throws Config_Exception When the container cannot produce a usable registrar, or two + * sub-plugins were registered under one slug. + * + * @return Sub_Plugin|null + */ + private function find_by_standalone_basename( string $basename ): ?Sub_Plugin { + foreach ( Loader::all() as $sub_plugin ) { + if ( $sub_plugin->get_standalone_plugin_basename() === $basename ) { + return $sub_plugin; + } + } + + return null; + } } diff --git a/tests/_support/Spy_Queue.php b/tests/_support/Spy_Queue.php index 2b42400..d3563ec 100644 --- a/tests/_support/Spy_Queue.php +++ b/tests/_support/Spy_Queue.php @@ -49,6 +49,23 @@ class Spy_Queue implements Queue_Interface { */ public $render_calls = 0; + /** + * Markup handed to filter_activation_error_markup(), in order. + * + * @var string[] + */ + public $filtered = []; + + /** + * What filter_activation_error_markup() hands back. + * + * Deliberately not the argument: a trampoline that returned its own input instead of the + * queue's answer would be indistinguishable from one that delegated properly. + * + * @var string + */ + public $filtered_markup = '

Rewritten by the queue.

'; + /** * @param Sub_Plugin $sub_plugin Sub-plugin concerned. * @@ -82,4 +99,15 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { public function render(): void { ++$this->render_calls; } + + /** + * @param string $markup Notice markup WordPress is about to print. + * + * @return string + */ + public function filter_activation_error_markup( string $markup ): string { + $this->filtered[] = $markup; + + return $this->filtered_markup; + } } diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index 6100100..8f7c71d 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -317,6 +317,54 @@ static function () use ( &$fired ): void { $this->assertSame( 0, $notices->render_calls ); } + /** + * The activation-error rewrite is wired next to the notice step and under the same guard: both + * only ever have work to do on an admin screen. It is a named static callback rather than a + * closure, so unlike the plugins_loaded steps it can be asserted on by identity — and, for the + * same reason, taken back off by a host that wants core's wording. + */ + public function test_it_wires_the_activation_error_filter_in_the_admin(): void { + set_current_screen( 'plugins' ); + + $notices = $this->bind_spy_queue(); + + Loader::boot(); + + $this->assertNotFalse( + has_filter( 'wp_admin_notice_markup', [ Loader::class, 'filter_activation_error_markup' ] ) + ); + $this->assertSame( + $notices->filtered_markup, + apply_filters( 'wp_admin_notice_markup', '

Core.

', '', [] ), + 'The wired filter has to reach the bound queue.' + ); + } + + /** + * `wp_admin_notice_markup` is a general-purpose hook another plugin is free to apply anywhere, + * and the rewrite only ever has work to do on an activation-error request in wp-admin. + */ + public function test_it_does_not_wire_the_activation_error_filter_on_the_front_end(): void { + set_current_screen( 'front' ); + + Loader::boot(); + + $observed = [ has_filter( 'wp_admin_notice_markup', [ Loader::class, 'filter_activation_error_markup' ] ) ]; + + // Wired by hand and read a second time. Without that reading, a probe that could never + // report this callback at all — a renamed method, a mistyped hook name — would satisfy the + // assertion below however boot() had behaved. + $this->add_tracked_filter( 'wp_admin_notice_markup', [ Loader::class, 'filter_activation_error_markup' ] ); + + $observed[] = has_filter( 'wp_admin_notice_markup', [ Loader::class, 'filter_activation_error_markup' ] ); + + $this->assertSame( + [ false, 10 ], + $observed, + 'The filter must be admin-only, and the probe must be able to see a registration.' + ); + } + /** * Adding an action at a priority the running dispatch has already passed is accepted and then * never fires. Booting from plugins_loaded at the default priority instead of 0 would otherwise @@ -421,14 +469,16 @@ public function test_booting_after_plugins_loaded_has_finished_loads_inline(): v public function test_the_state_helper_unwires_the_hooks_boot_added(): void { set_current_screen( 'dashboard' ); - $load_step = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); - $notice_step = $this->callbacks_at( 'all_admin_notices' ); + $load_step = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $notice_step = $this->callbacks_at( 'all_admin_notices' ); + $activation_error = $this->callbacks_at( 'wp_admin_notice_markup' ); Loader::boot(); Loader_State::reset(); $this->assertSame( $load_step, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); $this->assertSame( $notice_step, $this->callbacks_at( 'all_admin_notices' ) ); + $this->assertSame( $activation_error, $this->callbacks_at( 'wp_admin_notice_markup' ) ); } /** @@ -680,4 +730,21 @@ private function add_tracked_action( string $hook, callable $callback, int $prio add_action( $hook, $callback, $priority ); } + + /** + * The same, for a filter. Spelled separately from add_tracked_action() even though WordPress + * keeps actions and filters in one registry, so a reader is never left wondering whether a + * filter was wired by an add_action() on purpose. + * + * @param string $hook Hook to add to. + * @param callable $callback Callback to add. + * @param int $priority Priority to add it at. + * + * @return void + */ + private function add_tracked_filter( string $hook, callable $callback, int $priority = 10 ): void { + $this->added_actions[] = [ $hook, $callback, $priority ]; + + add_filter( $hook, $callback, $priority ); + } } diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index 89a6c65..7388c3f 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -24,10 +24,11 @@ use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; use RuntimeException; +use stdClass; use Throwable; /** - * The public surface: the accessors, registration, and the notice trampoline. + * The public surface: the accessors, registration, and the two notice trampolines. * * Boot timing lives in `Boot\SchedulerTest` and the load loop in `Load\RunnerTest`, which is where * those behaviours moved. What is left here is what a host actually calls. @@ -326,15 +327,8 @@ public function test_the_state_helper_clears_buffered_registrations(): void { } public function test_render_notices_delegates_to_the_bound_queue(): void { - $notices = new Spy_Queue(); - $container = new Test_Container(); - $container->singleton( - Queue_Interface::class, - static function () use ( $notices ): Queue_Interface { - return $notices; - } - ); - $this->set_up_container( $container ); + $notices = new Spy_Queue(); + $this->bind_queue( $notices ); Loader::render_notices(); @@ -362,6 +356,97 @@ public function test_render_notices_does_nothing_without_a_hook_prefix(): void { $this->assert_the_library_reported_incorrect_usage(); } + /** + * The trampoline is a named static method rather than a closure over the container, so that a + * host can take the filter back off — but it still has to reach whatever the host bound, or a + * replacement queue would be handed every other notice and never the activation error. + */ + public function test_the_activation_error_trampoline_delegates_to_the_bound_queue(): void { + $notices = new Spy_Queue(); + $this->bind_queue( $notices ); + + $this->assertSame( + $notices->filtered_markup, + Loader::filter_activation_error_markup( '

Core.

' ) + ); + $this->assertSame( [ '

Core.

' ], $notices->filtered ); + } + + /** + * Reported and handed back untouched rather than thrown out of: this runs while WordPress is + * drawing an error screen, and a second fatal there would replace the one the user came to read. + */ + public function test_the_activation_error_trampoline_does_nothing_without_a_hook_prefix(): void { + $notices = new Spy_Queue(); + $this->bind_queue( $notices ); + + // The prefix goes, the container stays: this is about the missing prefix, and a trampoline + // that reached the container first would pass this test for the other reason. + $container = $this->container(); + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + $this->assertSame( '

Core.

', Loader::filter_activation_error_markup( '

Core.

' ) ); + $this->assertSame( [], $notices->filtered, 'The queue must not be reached at all.' ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * A filter receives whatever the filter before it returned, so the trampoline takes an untyped + * argument and coerces it. Declaring `string` there would turn another plugin's sloppy return + * into a TypeError raised from this library, on the screen least able to afford one. + * + * The queue still sees the coerced value: the guard is about what crosses the boundary, not + * about standing the rewrite down, and a trampoline that returned early would leave the + * interface's `string` promise resting on an untested path. + * + * @dataProvider non_string_markup + * + * @param mixed $markup Whatever the previous filter returned. + */ + public function test_the_activation_error_trampoline_coerces_a_non_string( $markup ): void { + $notices = new Spy_Queue(); + $this->bind_queue( $notices ); + + $this->assertSame( $notices->filtered_markup, Loader::filter_activation_error_markup( $markup ) ); + $this->assertSame( [ '' ], $notices->filtered ); + } + + /** + * An integer earns its place alongside the two that would fatal: `42` casts cleanly to `"42"`, + * so it is the case a missing guard would pass rather than crash on. + * + * @return Generator + */ + public static function non_string_markup(): Generator { + yield 'null' => [ null ]; + yield 'an array' => [ [ '

Core.

' ] ]; + yield 'an object' => [ new stdClass() ]; + yield 'an integer' => [ 42 ]; + yield 'false' => [ false ]; + } + + /** + * Bind a recording queue, in the order a host binds one: before the provider fills in what is + * missing. + * + * @param Spy_Queue $notices Queue to bind. + * + * @return void + */ + private function bind_queue( Spy_Queue $notices ): void { + $container = new Test_Container(); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Queue_Interface { + return $notices; + } + ); + + $this->set_up_container( $container ); + } + /** * Bind a recording registrar, in the order a host binds one: before the provider fills in what is * missing. diff --git a/tests/unit/Notices/QueueActivationErrorTest.php b/tests/unit/Notices/QueueActivationErrorTest.php new file mode 100644 index 0000000..3f960d3 --- /dev/null +++ b/tests/unit/Notices/QueueActivationErrorTest.php @@ -0,0 +1,345 @@ +fatal error.'; + + /** + * The notice core is about to print, as `wp_admin_notice_markup` hands it over. + * + * @var string + */ + private const MARKUP = '

' . self::CORE_TEXT . '

'; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + + // The rewrite reads the registry through Loader::all(), which resolves the registrar, so + // these tests need a container like every other test that reaches a collaborator. + $this->set_up_container(); + + // is_admin() reads the current screen before WP_ADMIN, so this is what puts the request in + // the admin as well as on the right screen. + set_current_screen( 'plugins' ); + + // Every test starts from the request core actually redirects to after a sandboxed fatal, and + // states only the part it is about. + $_GET['plugin'] = self::STANDALONE; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . self::STANDALONE ); + } + + public function tearDown(): void { + // In tearDown rather than at the end of each test body: a failed assertion would otherwise + // leave an admin screen and a half-built activation-error request standing for every test + // that runs afterwards in this process. + unset( $_GET['plugin'], $_GET['_error_nonce'] ); + set_current_screen( 'front' ); + + Loader_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + public function test_it_replaces_the_fatal_error_text_with_the_configured_message(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Give Recurring is already bundled with Give.' ] ); + + $filtered = $this->make_queue()->filter_activation_error_markup( self::MARKUP ); + + $this->assertStringContainsString( 'Give Recurring is already bundled with Give.', $filtered ); + $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); + + // The notice box stays core's to draw — its classes, its dismiss button, its wrapper. Only + // the sentence inside belongs to this library. + $this->assertStringStartsWith( '

', $filtered ); + $this->assertStringEndsWith( '

', $filtered ); + } + + public function test_the_default_names_the_sub_plugin(): void { + $this->register(); + + $filtered = $this->make_queue()->filter_activation_error_markup( self::MARKUP ); + + // The fallback is not pinned word for word — it is allowed to be reworded, as long as it + // still names the sub-plugin and still displaces core's sentence. + $this->assertStringContainsString( 'give-recurring', $filtered ); + $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); + } + + /** + * The request names one plugin, and only the sub-plugin that claims that basename may speak for + * it. Reading the first registration instead would put one bundled plugin's explanation on + * another's activation error. + */ + public function test_it_uses_the_sub_plugin_whose_standalone_the_request_names(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'The wrong one.' ] ); + $this->register( + [ + 'slug' => 'give-fee-recovery', + 'standalone_plugin_basename' => 'give-fee-recovery/give-fee-recovery.php', + 'conflict_notice_message' => static fn() => 'The right one.', + ] + ); + + $_GET['plugin'] = 'give-fee-recovery/give-fee-recovery.php'; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_give-fee-recovery/give-fee-recovery.php' ); + + $filtered = $this->make_queue()->filter_activation_error_markup( self::MARKUP ); + + $this->assertStringContainsString( 'The right one.', $filtered ); + $this->assertStringNotContainsString( 'The wrong one.', $filtered ); + } + + /** + * Nothing draws an admin notice on the front end, and `get_current_screen()` does not exist + * there — the guard is what keeps this filter from fataling if another plugin ever applies + * `wp_admin_notice_markup` outside wp-admin. + */ + public function test_it_leaves_the_markup_alone_outside_the_admin(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ); + + set_current_screen( 'front' ); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * The activation error is only ever reported on the plugins screen. Every other admin screen + * that prints a notice carrying core's sentence — a bulk action reported elsewhere, a plugin + * quoting it — is somebody else's. + */ + public function test_it_leaves_the_markup_alone_off_the_plugins_screen(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ); + + set_current_screen( 'dashboard' ); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * wp-admin/network/plugins.php is a one-line require of wp-admin/plugins.php, so it draws the + * same activation error — but WP_Screen appends `-network` to the id. On a default multisite + * the subsite has no plugins UI, so this is the only screen a super admin can reactivate an + * absorbed standalone from, and matching 'plugins' alone would decline exactly there. + */ + public function test_it_rewrites_the_markup_on_the_network_plugins_screen(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ); + + set_current_screen( 'plugins-network' ); + + $this->assertStringContainsString( + 'Ours.', + $this->make_queue()->filter_activation_error_markup( self::MARKUP ) + ); + } + + /** + * The nonce is valid here, so the ownership lookup is the only thing that can stop the rewrite: + * an activation error for a plugin this library knows nothing about keeps core's wording, which + * for that plugin is the accurate one. + */ + public function test_it_leaves_the_markup_alone_for_a_plugin_no_sub_plugin_claims(): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ); + + $_GET['plugin'] = 'akismet/akismet.php'; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_akismet/akismet.php' ); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * @dataProvider requests_that_are_not_an_activation_error + * + * @param callable $arrange Turns the request in setUp() into the one this case is about. + */ + public function test_it_leaves_the_markup_alone( callable $arrange ): void { + $this->register( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ); + + $arrange(); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * The nonce cases are the ones that matter most: `plugin` is attacker-controlled, and without + * verification any link could make an arbitrary admin page quote a sub-plugin's message back at + * whoever followed it. The nonce is built inside the closure rather than in the provider, + * because a provider runs before setUp() and a nonce is bound to the user current at the time. + * + * @return Generator + */ + public static function requests_that_are_not_an_activation_error(): Generator { + yield 'a nonce for another plugin' => [ + static function (): void { + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_akismet/akismet.php' ); + }, + ]; + + yield 'a nonce that is not one' => [ + static function (): void { + $_GET['_error_nonce'] = 'not-a-nonce'; + }, + ]; + + yield 'an empty nonce' => [ + static function (): void { + $_GET['_error_nonce'] = ''; + }, + ]; + + yield 'no nonce at all' => [ + static function (): void { + unset( $_GET['_error_nonce'] ); + }, + ]; + + yield 'no plugin named' => [ + static function (): void { + unset( $_GET['plugin'] ); + }, + ]; + + yield 'an empty plugin name' => [ + static function (): void { + $_GET['plugin'] = ''; + }, + ]; + + // Not a string at all: `plugin[]=x` in the query string arrives as an array, which + // sanitize_text_field() would fatal on if it were not unslashed and sanitised as one. + yield 'a plugin name that is an array' => [ + static function (): void { + $_GET['plugin'] = [ self::STANDALONE ]; + }, + ]; + } + + /** + * The message is sanitised before it is checked for emptiness, and this is the case that pins + * the order: `wp_kses_post( '' )` is the empty string, so swapping core's + * wording for it would leave an empty notice box where the explanation should be. Leaving + * core's sentence in place is the better of the two bad outcomes. + */ + public function test_a_message_that_sanitises_away_leaves_the_markup_alone(): void { + $this->register( [ 'conflict_notice_message' => static fn() => '' ] ); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * A message that is only whitespace is the same failure with a friendlier shape. + */ + public function test_a_whitespace_only_message_leaves_the_markup_alone(): void { + $this->register( [ 'conflict_notice_message' => static fn() => " \n\t" ] ); + + $this->assertSame( self::MARKUP, $this->make_queue()->filter_activation_error_markup( self::MARKUP ) ); + } + + /** + * `wp_kses_post()` and not `esc_html()`, for the reason the renderer uses it: these messages + * come from the host's own configuration and filters rather than from user input, so the link to + * the knowledge-base article explaining the merge has to reach the screen intact — while the + * event handler and the script a message must never be able to ship do not. + */ + public function test_it_strips_unsafe_markup_from_the_replacement_but_keeps_a_link(): void { + $this->register( + [ + 'conflict_notice_message' => static fn() => 'See the docs.' + . '', + ] + ); + + $filtered = $this->make_queue()->filter_activation_error_markup( self::MARKUP ); + + $this->assertStringContainsString( 'the docs', $filtered ); + $this->assertStringNotContainsString( 'onclick', $filtered ); + $this->assertStringNotContainsString( 'assertStringContainsString( 'alert(2)', $filtered ); + } + + /** + * Register a sub-plugin that claims the standalone this suite's request names. + * + * @param array $overrides Config values to override. + * + * @return void + */ + private function register( array $overrides = [] ): void { + $slug = isset( $overrides['slug'] ) && is_string( $overrides['slug'] ) && $overrides['slug'] !== '' + ? $overrides['slug'] + : 'give-recurring'; + + Loader::register( + array_merge( + [ + 'slug' => $slug, + 'bundled_plugin_file' => "/tmp/{$slug}/{$slug}.php", + 'plugin_loaded_constant' => strtoupper( str_replace( '-', '_', $slug ) ) . '_VERSION_FIXTURE', + 'standalone_plugin_basename' => self::STANDALONE, + ], + $overrides + ) + ); + } + + /** + * The queue under test, built directly rather than resolved. + * + * Its two collaborators are required arguments and neither is reached by the rewrite — nothing + * here is stored and nothing here is drawn — so handing over the real pair states plainly that + * this file is about one method. + * + * @return Queue + */ + private function make_queue(): Queue { + return new Queue( new Store(), new Renderer() ); + } +} From 3d5d17c96691a16923fd2552dd5c245e8cb9de3a Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 15:43:45 +0200 Subject: [PATCH 4/4] Drive the whole library end to end against real WordPress Fifteen requests, each one dispatched through the hooks a host would fire rather than by calling the steps directly, so the priorities, the gatekeeper and the boot barrier are exercised instead of bypassed. The container being mandatory is what makes the last two tests possible: a host binds its own registrar, checker, deactivator, notice queue, activator, gatekeeper and resolver before boot, and every one is asserted to be the object the request actually used -- with the defaults asserted not to have run beside it, since active_plugins untouched and both options unwritten is the only way to tell "the binding was used" from "the binding was ignored and nothing happened". Each load-path test writes its own bundled fixture, because require_once caches by resolved path for the whole process and a shared file would make every later test pass without loading anything. The trait that writes them gained the old suite's @after cleanup and kept ours, so a fixture is removed whether or not the test remembers. --- .../2026-08-12-container-required-rework.md | 4 +- tests/README.md | 67 +- tests/_support/Traits/WithBundledPlugins.php | 17 +- tests/unit/EndToEndTest.php | 955 ++++++++++++++++++ tests/unit/Load/RunnerTest.php | 2 + 5 files changed, 1040 insertions(+), 5 deletions(-) create mode 100644 tests/unit/EndToEndTest.php diff --git a/docs/superpowers/plans/2026-08-12-container-required-rework.md b/docs/superpowers/plans/2026-08-12-container-required-rework.md index 7b41c1e..e27f91b 100644 --- a/docs/superpowers/plans/2026-08-12-container-required-rework.md +++ b/docs/superpowers/plans/2026-08-12-container-required-rework.md @@ -295,7 +295,9 @@ level 9, so most breakage surfaces from `composer test:analysis` without standin ### Task 15 — `15-e2e-suite` -- [ ] The end-to-end bootstrap now builds a container and calls `Provider::register()` before `Loader::boot()`. +- [x] The end-to-end bootstrap now hands `Config::set_container()` a bare container and lets `Loader::boot()` + run `Provider::register()` over it, which is the sequence a host runs. A test about a rebinding host binds + its own implementations into that container first. ### Task 16 — `16-readme-release` diff --git a/tests/README.md b/tests/README.md index f2246d1..6c7c165 100644 --- a/tests/README.md +++ b/tests/README.md @@ -116,9 +116,21 @@ $this->assertSame( 1, $this->bundled_plugin_loads() ); Every call writes a *new* file under a unique name, and every guard constant is unique too. Neither is tidiness: `require_once` dedupes by resolved path for the lifetime of the PHP process, so a shared fixture lets a later test pass -without loading anything, and the fixture defines its constant for real, so a -reused name makes a later sub-plugin read as already loaded. Call -`remove_bundled_plugin_files()` from tearDown. +without loading anything — including if the load logic were deleted outright — +and the fixture defines its constant for real, so a reused name makes a later +sub-plugin read as already loaded. + +The generated file does two things and nothing else: it increments a load +counter in `$GLOBALS`, and it defines the constant inside a `defined()` check, +the shape a real bundled sub-plugin has. The counter is what separates "loaded +twice" and "never loaded" from "loaded once"; the constant alone cannot tell +those three apart. + +`remove_bundled_plugin_files()` cleans up on the trait's own `@after` hook, and +the tests that clear other state alongside it call it from tearDown as well. +Never leave it to the end of a test body: a failed assertion aborts the test +where it stands, so that is exactly the line that does not run on the day it +matters. A fixture helper cannot be called `make()`, `makeEmpty()`, `construct()`, or `constructEmpty()`: those are public methods on `Codeception\Test\Unit`, which @@ -289,3 +301,52 @@ $this->assert_the_library_reported_incorrect_usage(); An unexpected report still fails the test, because everything the listener sees is recorded and asserted to belong to this library. Call `stop_expecting_incorrect_usage()` from tearDown. + +## The end-to-end suite + +`tests/unit/EndToEndTest.php` drives the library the way a host plugin does, +against real WordPress state: the real `active_plugins` option, a real +`deactivate_plugins()` that really writes it, and real site options behind the +notice queue and the activation record. Nothing about the library is doubled, +except in the two tests that are *about* a host binding its own collaborators. + +It reaches for no entry point a host does not have. The bootstrap is +`Config::set_hook_prefix()`, `Config::set_container()`, `Loader::register()` and +`Loader::boot()`, and everything after that arrives through the hooks boot() +wired — a request is `do_action( 'plugins_loaded' )`, an admin page load is +`do_action( 'all_admin_notices' )`. The container is handed over bare rather +than through `WithContainer`, because `boot()` running the provider over it is +one of the steps under test. + +Only two functions are stubbed: `wp_safe_redirect`, which throws so the request +halts where production calls `exit`, and `wp_get_referer`, which is a request +header no test can send and which decides where that redirect would have gone. + +Four preconditions have to hold before any of it means anything, and setUp +establishes all four: + +- **An interactive admin GET** — `set_current_screen( 'plugins' )` plus + `$_SERVER['REQUEST_METHOD'] = 'GET'`. `Conflict\Gatekeeper` turns away + anything else, so without both of these every policy test would pass while + resolving nothing at all. +- **A user who can `activate_plugins`** — `WithUsers::become_plugin_administrator()`. + The gatekeeper checks the capability before anything is resolved, and the + queue checks the same one before it renders, so as nobody the suite would be + asserting that a no-op is a no-op. +- **The hook prefix** — `Config::set_hook_prefix()`. Both plugins_loaded steps + report and return without one, and the queue and activation option names are + derived from it. +- **A rewound `plugins_loaded` counter** — the harness dispatched the hook + before any test ran, so `boot()` would rightly report that it is too late to + wire and run everything inline. tearDown puts the count back. + +The screen, the request method and that counter are all process-global, so all +three are restored in teardown; leaving any of them set turns an unrelated later +test into an admin request. + +Run both legs — `slic run unit` and `slic run unit --env multisite`. Multisite +is not a formality here: `deactivate_plugins()` is network-aware, +`activate_plugins` maps through `manage_network_plugins` so the administrator +who passes on singlesite is not the one who passes on multisite, and the queue +and activation record are `get_site_option()` values, which are network options +there. Every precondition above resolves differently on the second leg. diff --git a/tests/_support/Traits/WithBundledPlugins.php b/tests/_support/Traits/WithBundledPlugins.php index 6d96ef2..96f0bd9 100644 --- a/tests/_support/Traits/WithBundledPlugins.php +++ b/tests/_support/Traits/WithBundledPlugins.php @@ -30,6 +30,14 @@ trait WithBundledPlugins { /** * Write a bundled plugin that counts its own loads and defines its guard constant. * + * The counter is what separates "loaded twice" and "never loaded" from "loaded once". The + * constant alone cannot tell those three apart, because it ends up defined exactly once either + * way. + * + * The constant is defined inside a `defined()` check, because the file stands in for a real + * plugin: a bundled copy that redeclared a constant the standalone had already defined would + * raise a notice, and the guard is what a plugin actually ships. + * * @since 1.0.0 * * @param string $constant Guard constant the file defines, as a bundled plugin's own header would. @@ -100,7 +108,14 @@ protected function reset_bundled_plugin_loads(): void { } /** - * Remove every fixture this test wrote. Call from tearDown. + * Remove every fixture this test wrote. + * + * Runs itself, as PHPUnit's own `@after` hook, and is safe to call from tearDown as well — which + * is where the tests that clear other state alongside it do call it. A test body must never be + * the only thing that removes these: a failed assertion aborts the test where it stands, so a + * cleanup line at the end of the body is exactly the one that does not run on the day it matters. + * + * @after * * @since 1.0.0 * diff --git a/tests/unit/EndToEndTest.php b/tests/unit/EndToEndTest.php new file mode 100644 index 0000000..8b8daf1 --- /dev/null +++ b/tests/unit/EndToEndTest.php @@ -0,0 +1,955 @@ +fatal error.'; + + /** + * The notice core is about to print, as `wp_admin_notice_markup` hands it over. + * + * @var string + */ + private const MARKUP = '

' . self::CORE_TEXT . '

'; + + /** + * Guard constants a test defined through uopz, undone in tearDown. + * + * @var string[] + */ + private $constants = []; + + /** + * Hook callbacks these tests added, as [ hook, callback, priority ] triples. + * + * Tracked so tearDown can take back exactly what a test put there. `remove_all_filters()` would + * strip the hook bare instead, discarding every callback WordPress and the rest of the suite have + * on it for the remainder of the process. + * + * @var array + */ + private $added_hooks = []; + + /** + * @var string|null + */ + private $request_method = null; + + /** + * The plugins_loaded count as the harness left it. + * + * @var int + */ + private $plugins_loaded_count = 0; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + + // The first half of the bootstrap. The container is the second, and each test builds its own + // so that a host binding its implementations first has somewhere to bind them. + Config::set_hook_prefix( self::HOOK_PREFIX ); + + // Conflict resolution runs only on an interactive admin GET, since plugins_loaded fires on + // every request. Without both of these every policy test below would pass while resolving + // nothing at all. + set_current_screen( 'plugins' ); + $this->request_method = $_SERVER['REQUEST_METHOD'] ?? null; + $_SERVER['REQUEST_METHOD'] = 'GET'; + + // Deactivating a standalone and consuming the notice queue are both gated on + // activate_plugins, which on multisite maps through manage_network_plugins. + $this->become_plugin_administrator(); + + // A referrer is a header no test can send. False is the ordinary case — a link followed from + // somewhere outside the admin — and it sends the user to the plugins list. + $this->setFunctionReturn( 'wp_get_referer', false ); + + $this->clear_state(); + $this->reset_bundled_plugin_loads(); + + // The harness has to boot WordPress before it can run anything, so plugins_loaded has already + // fired by the time any test starts — and boot() would rightly report that it is too late to + // wire. Rewind the counter so a test sees the timing a host bootstrap sees; the late-boot test + // dispatches the hook itself to close the window again. + $this->plugins_loaded_count = did_action( 'plugins_loaded' ); + unset( $GLOBALS['wp_actions']['plugins_loaded'] ); + } + + public function tearDown(): void { + // In tearDown rather than at the end of each test body: a failed assertion would otherwise + // leave an admin screen, a pinned request method, a half-built activation-error request and a + // rewound hook counter standing for every test that runs afterwards in this process. + $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + + if ( $this->request_method === null ) { + unset( $_SERVER['REQUEST_METHOD'] ); + } else { + $_SERVER['REQUEST_METHOD'] = $this->request_method; + } + + unset( $_GET['plugin'], $_GET['_error_nonce'] ); + set_current_screen( 'front' ); + + foreach ( $this->constants as $constant ) { + $this->unsetConstant( $constant ); + } + $this->constants = []; + + // Only what these tests added by hand. What boot() wired comes off in Loader_State::reset(). + foreach ( $this->added_hooks as [ $hook, $callback, $priority ] ) { + remove_filter( $hook, $callback, $priority ); + } + $this->added_hooks = []; + + $this->stop_expecting_incorrect_usage(); + $this->remove_bundled_plugin_files(); + $this->clear_state(); + Loader_State::reset(); + Config_State::reset(); + parent::tearDown(); + } + + /** + * The happy path, and the one every other scenario is a deviation from: nothing else claims the + * plugin, so the bundled copy loads, defines the guard the standalone would have defined, and + * gets the one-time setup that `register_activation_hook()` never gives it. + */ + public function test_a_fresh_load_defines_the_guard_and_activates_exactly_once(): void { + $activated = []; + + $constant = $this->register( + [ + 'activation_callback' => static function ( Sub_Plugin $sub_plugin ) use ( &$activated ): void { + $activated[] = $sub_plugin->get_slug(); + }, + ] + ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assertTrue( defined( $constant ), 'The bundled copy defines the guard the standalone would have.' ); + $this->assertSame( [ self::SLUG ], $activated ); + $this->assertSame( [ self::SLUG => true ], $this->activation_record() ); + + // The next page view, with nothing re-registered and nothing re-booted. The constant the file + // really defined stands the load down, and the record really written stands the callback down. + $this->run_request(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assertSame( [ self::SLUG ], $activated, 'Activation runs once for the life of the site.' ); + } + + /** + * The default policy, against core's own `deactivate_plugins()` and the real `active_plugins` + * option rather than a stub of either. + */ + public function test_deactivate_deactivates_notifies_and_redirects(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ] + ); + + $this->boot(); + + $location = $this->run_halted_request(); + + $this->assertNotContains( self::STANDALONE, $this->active_plugins() ); + $this->assertArrayHasKey( self::SLUG . ':merge', $this->notice_queue() ); + + // The destination, not merely that one was asked for: a redirect somewhere else entirely + // would satisfy "the request ended in a redirect" without sending anyone anywhere useful. + $this->assertSame( admin_url( 'plugins.php' ), $location ); + + // The request really ended in the resolver. The bundled copy loads on the next one, which is + // what the standalone's own guard constant forces in production. + $this->assertSame( 0, $this->bundled_plugin_loads() ); + } + + /** + * 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, + * or the owner reads the same deactivation report for ever. + */ + public function test_the_merge_notice_renders_on_the_next_admin_screen_and_clears(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $this->register( [ 'standalone_plugin_basename' => self::STANDALONE ] ); + + $this->boot(); + $this->run_halted_request(); + + $rendered = $this->render_admin_notices(); + + $this->assertStringContainsString( self::SLUG, $rendered ); + $this->assertStringContainsString( 'has been deactivated', $rendered ); + $this->assertStringContainsString( + 'notice-warning', + $rendered, + 'A conflict the library has already handled is a warning, not an error.' + ); + $this->assertSame( [], $this->notice_queue(), 'Rendering consumes the queue.' ); + } + + /** + * The failure mode a merge notice queued on every request would produce: a redirect loop, or an + * admin screen that reports the same deactivation for ever. Nothing is re-registered between the + * two requests — a duplicate slug throws — because this is the next page view, not a second + * bootstrap. + */ + public function test_the_request_after_a_deactivation_does_not_loop(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $constant = $this->register( [ 'standalone_plugin_basename' => self::STANDALONE ] ); + + $this->boot(); + $this->run_halted_request(); + + $this->assertArrayHasKey( self::SLUG . ':merge', $this->notice_queue() ); + + // The owner has been told. Emptying the queue is what makes a second notice visible at all: + // re-queuing writes the same `slug:merge` key, so a queue left as it is would look identical + // whether or not the resolver ran again. + delete_site_option( Queue::option_name() ); + + // This one must not halt, and run_request() fails the test if it does — which is the + // redirect loop, asserted rather than described. + $this->run_request(); + + $this->assertSame( [], $this->notice_queue(), 'Nothing is left to resolve, so nothing is left to say.' ); + $this->assertSame( 1, $this->bundled_plugin_loads(), 'With the standalone gone the bundled copy takes over.' ); + $this->assertTrue( defined( $constant ) ); + } + + /** + * DEFER hands the request to the standalone, and WordPress includes an active plugin from + * wp-settings.php long before plugins_loaded — so by the time the resolver runs, the standalone + * has already defined the guard constant. Defining it up front is what makes this the scenario + * the policy actually describes rather than a resolver that merely declined to act. + */ + public function test_defer_leaves_the_standalone_active_and_loads_nothing(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $constant = $this->define_guard( 'ABSORBER_E2E_DEFERRED_GUARD' ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEFER, + ], + $constant + ); + + $this->boot(); + $this->run_request(); + + $this->assertContains( self::STANDALONE, $this->active_plugins() ); + $this->assertSame( 0, $this->bundled_plugin_loads(), 'The standalone won; the guard stands the bundled copy down.' ); + $this->assertSame( [], $this->notice_queue() ); + } + + public function test_notice_only_notifies_without_deactivating(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::NOTICE_ONLY, + 'conflict_notice_message' => static fn() => 'Deactivate the standalone when you get a chance.', + ] + ); + + $this->boot(); + + // A policy that only talks must not end the request, which is what run_request() asserts. + $this->run_request(); + + $this->assertContains( self::STANDALONE, $this->active_plugins() ); + $this->assertSame( + [ self::SLUG . ':conflict' => 'Deactivate the standalone when you get a chance.' ], + $this->notice_queue() + ); + } + + /** + * The gate that survives every policy and every rebinding: whoever cannot activate a plugin must + * not be able to deactivate one by loading an admin page. Nothing is consumed by refusing — the + * standalone is still there to detect on the next request, from someone who can act on it, which + * is what the second half asserts. + */ + public function test_a_user_who_cannot_activate_plugins_resolves_nothing(): void { + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $this->register( [ 'standalone_plugin_basename' => self::STANDALONE ] ); + + wp_set_current_user( $this->create_user( 'subscriber' ) ); + + $this->boot(); + $this->run_request(); + + $this->assertContains( self::STANDALONE, $this->active_plugins(), 'A subscriber must not deactivate anything.' ); + $this->assertSame( [], $this->notice_queue(), 'A user who could never read the notice must not consume it.' ); + + $this->become_plugin_administrator(); + + $this->run_halted_request(); + + $this->assertNotContains( self::STANDALONE, $this->active_plugins() ); + $this->assertArrayHasKey( self::SLUG . ':merge', $this->notice_queue() ); + } + + /** + * The conflict the load guard cannot prevent: the owner reinstalls the standalone and presses + * Activate, WordPress includes it on top of the bundled copy, and the re-declaration is a real + * fatal that core's sandbox reports as "the plugin triggered a fatal error" — true, and useless. + * + * Driven through `Loader::boot()` and core's own filter dispatch rather than by calling the queue + * directly, because the wiring is half of what has to work: an admin-only `add_filter()` that + * never ran leaves the useless sentence on the screen. + */ + public function test_a_reactivation_attempt_yields_the_friendly_message(): void { + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_notice_message' => static fn() => 'Recurring is already bundled with the host plugin.', + ] + ); + + $this->boot(); + + // The request core redirects to once the sandboxed activation has fataled. + $_GET['plugin'] = self::STANDALONE; + $_GET['_error_nonce'] = wp_create_nonce( 'plugin-activation-error_' . self::STANDALONE ); + + $rewritten = apply_filters( 'wp_admin_notice_markup', self::MARKUP, self::CORE_TEXT, [] ); + + $this->assertIsString( $rewritten, 'The filter must hand back markup, whatever it did with it.' ); + + $filtered = is_string( $rewritten ) ? $rewritten : ''; + + $this->assertStringContainsString( 'Recurring is already bundled with the host plugin.', $filtered ); + $this->assertStringNotContainsString( self::CORE_TEXT, $filtered ); + + // The notice box stays core's to draw — its classes, its dismiss button, its wrapper. Only + // the sentence inside belongs to this library. + $this->assertStringStartsWith( '

', $filtered ); + } + + /** + * The guard is not only about the standalone. A must-use copy, a second host plugin bundling the + * same code, or the site owner's own snippet all define the same constant, and any of them means + * the code is already in memory. + */ + public function test_the_bundled_copy_stands_down_when_the_guard_is_already_defined(): void { + $constant = $this->define_guard( 'ABSORBER_E2E_ALREADY_LOADED_GUARD' ); + + $this->register( [], $constant ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertSame( [], $this->notice_queue(), 'A plugin the admin can see running has nothing to explain.' ); + } + + /** + * The toggle is read on every request rather than resolved at registration, so flipping it and + * running the next request is what proves the first load was skipped for the toggle and not for + * something else entirely — a missing file, say, which would leave the same empty counter. + */ + public function test_a_sub_plugin_toggled_off_loads_nothing(): void { + $enabled = false; + + $constant = $this->register( + [ + 'enabled' => static function () use ( &$enabled ) { + return $enabled; + }, + ] + ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertFalse( defined( $constant ) ); + $this->assertSame( [], $this->notice_queue(), 'A sub-plugin nobody asked for has nothing to report.' ); + + $enabled = true; + + $this->run_request(); + + $this->assertSame( 1, $this->bundled_plugin_loads(), 'The toggle is the only thing that was stopping it.' ); + } + + /** + * The host's last word before the require, on the hook name its own prefix builds. + */ + public function test_the_should_load_filter_can_veto_a_load(): void { + $constant = $this->register(); + + $this->add_tracked_filter( + Config::get_hook_name( 'should_load' ), + static function ( $should_load, $sub_plugin ) { + return $sub_plugin instanceof Sub_Plugin && $sub_plugin->get_slug() === self::SLUG + ? false + : $should_load; + }, + 10, + 2 + ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertFalse( defined( $constant ) ); + $this->assertSame( [], $this->notice_queue(), 'A host that vetoed the load does not need telling about it.' ); + } + + /** + * Registration order, not slug order and not filesystem order: a host bundles plugins that + * depend on one another, and the order it registers them in is the only say it gets. + */ + public function test_two_sub_plugins_load_in_one_request_in_registration_order(): void { + $loaded = []; + + // Recorded from the activation callback, which runs immediately after each require — so this + // is the order the files were really required in, not the order they were registered in. + $record = static function ( Sub_Plugin $sub_plugin ) use ( &$loaded ): void { + $loaded[] = $sub_plugin->get_slug(); + }; + + $first = $this->register( [ 'activation_callback' => $record ] ); + $second = $this->register( + [ + 'slug' => 'absorber-fee-recovery', + 'activation_callback' => $record, + ] + ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 2, $this->bundled_plugin_loads() ); + $this->assertTrue( defined( $first ) ); + $this->assertTrue( defined( $second ) ); + $this->assertSame( [ self::SLUG, 'absorber-fee-recovery' ], $loaded ); + } + + /** + * All the way to the screen again, from the other end: the load is skipped, the host's own + * explanation is queued, the render draws it as an error, and the render consumes the queue so + * the owner is told once rather than on every admin page load for ever. + */ + public function test_an_unmet_dependency_blocks_the_load_and_queues_the_explanation(): void { + $this->register( + [ + 'dependency_check' => static fn() => false, + 'dependency_notice_message' => static fn() => 'GiveWP 3.0 or later is required.', + ] + ); + + $this->boot(); + $this->run_request(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertSame( + [ self::SLUG . ':dependency' => 'GiveWP 3.0 or later is required.' ], + $this->notice_queue() + ); + + $rendered = $this->render_admin_notices(); + + $this->assertStringContainsString( 'GiveWP 3.0 or later is required.', $rendered ); + $this->assertStringContainsString( 'notice-error', $rendered, 'A plugin that did not load at all is an error.' ); + $this->assertSame( [], $this->notice_queue(), 'Rendering consumes the queue.' ); + } + + /** + * Booting from plugins_loaded at the default priority is the commonest hook mistake there is, and + * an add_action() at a priority the running dispatch has already passed is accepted and then never + * fires. The library reports the mistake and runs the sequence inline, so the site the host + * shipped still gets its bundled plugins. + */ + public function test_a_host_that_boots_too_late_still_gets_its_sub_plugins(): void { + $this->expect_incorrect_usage(); + + $constant = $this->register(); + + $this->add_tracked_action( + 'plugins_loaded', + function (): void { + $this->boot(); + } + ); + + $this->run_request(); + + $this->assertSame( 1, $this->bundled_plugin_loads(), 'A late boot must still load.' ); + $this->assertTrue( defined( $constant ) ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The whole point of a required container: a host binds its own implementation of an interface + * before boot, and that is the object the library uses for the rest of the request. + * + * One request covers the conflict step and the load pass because the referrer is the plugins + * list, where the redirector says to stay put. The defaults are asserted *not* to have run + * alongside the doubles — a library that resolved a second copy of the queue or the deactivator + * behind the host's back would satisfy every positive assertion here. + */ + public function test_a_host_binding_reaches_every_step_of_the_request(): void { + $registrar = new Spy_Registrar(); + $notices = new Spy_Queue(); + $activator = new Spy_Activator(); + + $checker = new class() implements Plugin_Checker_Interface { + /** + * @var string[] + */ + public $basenames = []; + + /** + * @param string $basename Plugin basename. + * + * @return bool + */ + public function is_active( string $basename ): bool { + $this->basenames[] = $basename; + + return true; + } + }; + + $deactivator = new class() implements Plugin_Deactivator_Interface { + /** + * @var string[] + */ + public $basenames = []; + + /** + * @param string $basename Plugin basename. + * + * @return void + */ + public function deactivate( string $basename ): void { + $this->basenames[] = $basename; + } + }; + + // Really active, so that the default deactivator would have emptied this option had it been + // the one reached. Nothing else in this test would notice the difference. + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $registrar ): Registrar_Interface { + return $registrar; + } + ); + $container->singleton( + Plugin_Checker_Interface::class, + static function () use ( $checker ): Plugin_Checker_Interface { + return $checker; + } + ); + $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; + } + ); + $container->singleton( + Activator_Interface::class, + static function () use ( $activator ): Activator_Interface { + return $activator; + } + ); + + // From the plugins list the redirector says to stay put, so the request runs on into the load + // pass instead of ending in the resolver. + $this->setFunctionReturn( 'wp_get_referer', admin_url( 'plugins.php' ) ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + 'activation_callback' => static fn() => null, + ] + ); + + $this->boot( $container ); + $this->run_request(); + + $this->assertSame( [ self::SLUG ], array_keys( $registrar->sub_plugins ), 'The host registrar holds the registration.' ); + $this->assertSame( [ self::STANDALONE ], $checker->basenames, 'The host checker answers whether the standalone is active.' ); + $this->assertSame( [ self::STANDALONE ], $deactivator->basenames, 'The host deactivator is the one asked to turn it off.' ); + $this->assertSame( [ self::SLUG ], $notices->merge_notices, 'The host queue is told what happened.' ); + $this->assertSame( [ self::SLUG ], $activator->slugs, 'The host activator runs the one-time setup.' ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); + + $this->assertContains( self::STANDALONE, $this->active_plugins(), 'The default deactivator must not have run too.' ); + $this->assertSame( [], $this->notice_queue(), 'The default queue must not have been resolved alongside it.' ); + $this->assertSame( [], $this->activation_record(), 'The default activator must not have recorded anything.' ); + } + + /** + * The same guarantee for the two the conflict step resolves itself. A host owns what a conflict + * means — but not who may have one resolved, which is why the gate is asked first and separately, + * and is asserted here to have been asked at all. + */ + public function test_a_host_binding_replaces_the_gatekeeper_and_the_resolver(): void { + $gatekeeper = new Spy_Gatekeeper( true ); + $resolver = new Spy_Resolver(); + + update_option( 'active_plugins', [ self::STANDALONE ] ); + + $container = new Test_Container(); + $container->singleton( + Gatekeeper::class, + static function () use ( $gatekeeper ): Gatekeeper { + return $gatekeeper; + } + ); + $container->singleton( + Resolver_Interface::class, + static function () use ( $resolver ): Resolver_Interface { + return $resolver; + } + ); + + $this->register( + [ + 'standalone_plugin_basename' => self::STANDALONE, + 'conflict_policy' => Conflict_Policy::DEACTIVATE, + ] + ); + + $this->boot( $container ); + $this->run_request(); + + $this->assertSame( 1, $gatekeeper->may_resolve_calls, 'The conflict step has to ask the gate.' ); + $this->assertSame( 1, $resolver->resolve_calls ); + $this->assertContains( + self::STANDALONE, + $this->active_plugins(), + 'A host resolver that does nothing means nothing is deactivated.' + ); + $this->assertSame( [], $this->notice_queue() ); + $this->assertSame( 1, $this->bundled_plugin_loads(), 'The load pass still runs after it.' ); + } + + /** + * The second half of the bootstrap, and the point every test above starts from. + * + * The container is handed over bare: `Loader::boot()` is what runs the provider over it, so a + * test that pre-registered the bindings would be asserting against a container the library never + * had to teach. + * + * @param Test_Container|null $container Container to bootstrap with, when a test has bound its + * own implementations into one. + * + * @return void + */ + private function boot( ?Test_Container $container = null ): void { + Config::set_container( $container ?? new Test_Container() ); + + Loader::boot(); + } + + /** + * One page view, which must not end in a redirect. + * + * `wp_safe_redirect()` is stubbed even here, where nothing should reach it. The real one is + * followed by `exit`, which would take the whole test process down rather than fail one test — + * so the stub throws, and a request that redirected when it should not have fails right here + * instead of silently passing somewhere else. + * + * @return void + */ + private function run_request(): void { + $message = self::halted_at_exit_message(); + + $this->setFunctionReturn( + 'wp_safe_redirect', + static function () use ( $message ) { + throw new TestException( $message ); + }, + true + ); + + try { + do_action( 'plugins_loaded' ); + } catch ( TestException $exception ) { + $this->fail( 'The request must not redirect and end here. ' . $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' ); + } + } + + /** + * One page view that must end where production calls exit(), and where it sent the user. + * + * @return string + */ + private function run_halted_request(): string { + return $this->capture_redirect( + static function (): void { + do_action( 'plugins_loaded' ); + } + ); + } + + /** + * An admin page load, as far as this library is concerned: the hook it renders the queue on. + * + * Dispatched rather than calling `Loader::render_notices()`, because the admin-only `add_action()` + * is half of what has to work — a queue nothing renders is a queue nothing clears either. + * + * @return string + */ + private function render_admin_notices(): string { + ob_start(); + + do_action( 'all_admin_notices' ); + + return (string) ob_get_clean(); + } + + /** + * Register one sub-plugin, backed by a bundled file that exists. + * + * Called before the container is set, which is legal and deliberate: registration is buffered, so + * a host that builds its config array before it builds its container still works. The guard + * constant is unique per call unless the test names one, because loading the file defines it with + * a real `define()` that lasts for the whole PHP process. + * + * @param array $overrides Config values to override. + * @param string|null $constant Guard constant to use, when the test needs to define it. + * + * @return string + */ + private function register( array $overrides = [], ?string $constant = null ): string { + $constant = $constant ?? $this->make_guard_constant(); + $slug = isset( $overrides['slug'] ) && is_string( $overrides['slug'] ) && $overrides['slug'] !== '' + ? $overrides['slug'] + : self::SLUG; + + Loader::register( + array_merge( + [ + 'slug' => $slug, + 'bundled_plugin_file' => $this->make_bundled_plugin_file( $constant ), + 'plugin_loaded_constant' => $constant, + ], + $overrides + ) + ); + + return $constant; + } + + /** + * Define a guard constant for the duration of one test, undone in tearDown. + * + * uopz is what makes this reversible: a plain `define()` lasts for the whole PHP process, and a + * guard left standing makes every later test read its sub-plugin as already loaded. + * + * @param string $constant Constant to define. + * + * @return string + */ + private function define_guard( string $constant ): string { + $this->constants[] = $constant; + + $this->setConstant( $constant, '1.0.0' ); + + return $constant; + } + + /** + * Add an action tearDown can take back by identity rather than by clearing the whole hook. + * + * @param string $hook Hook to add to. + * @param callable $callback Callback to add. + * @param int $priority Priority to add it at. + * + * @return void + */ + private function add_tracked_action( string $hook, callable $callback, int $priority = 10 ): void { + $this->added_hooks[] = [ $hook, $callback, $priority ]; + + add_action( $hook, $callback, $priority ); + } + + /** + * The same, for a filter. Spelled separately even though WordPress keeps actions and filters in + * one registry, so a reader is never left wondering whether a filter was wired by an add_action() + * on purpose. + * + * @param string $hook Hook to add to. + * @param callable $callback Callback to add. + * @param int $priority Priority to add it at. + * @param int $accepted_args How many arguments the callback takes. + * + * @return void + */ + private function add_tracked_filter( + string $hook, + callable $callback, + int $priority = 10, + int $accepted_args = 1 + ): void { + $this->added_hooks[] = [ $hook, $callback, $priority ]; + + add_filter( $hook, $callback, $priority, $accepted_args ); + } + + /** + * Everything this suite writes outside its own fixtures, cleared before and after each test. + * + * @return void + */ + private function clear_state(): void { + delete_site_option( Queue::option_name() ); + delete_site_option( Activator::option_name() ); + delete_option( 'active_plugins' ); + delete_site_option( 'active_sitewide_plugins' ); + } + + /** + * @return array + */ + private function active_plugins(): array { + return (array) get_option( 'active_plugins', [] ); + } + + /** + * The queue is an option and not a transient: with a persistent object cache a transient never + * reaches the database, and a `wp_cache_flush()` would destroy a merge notice raised exactly once. + * `get_site_option()` is `get_option()` outside multisite, so one read covers both install types. + * + * @return array + */ + private function notice_queue(): array { + $queue = get_site_option( Queue::option_name(), [] ); + + return is_array( $queue ) ? $queue : []; + } + + /** + * @return array + */ + private function activation_record(): array { + $done = get_site_option( Activator::option_name(), [] ); + + return is_array( $done ) ? $done : []; + } +} diff --git a/tests/unit/Load/RunnerTest.php b/tests/unit/Load/RunnerTest.php index dc2e9c9..df5128f 100644 --- a/tests/unit/Load/RunnerTest.php +++ b/tests/unit/Load/RunnerTest.php @@ -482,6 +482,8 @@ static function () use ( $registrar ): Registrar_Interface { * when the second one's guard constant never gets defined. */ public function test_one_bundled_file_behind_two_registrations_loads_once(): void { + // The file's own guard constant is one neither registration names, so both of them still + // reach require_once and only the path dedupe can stop the second load. $path = $this->make_bundled_plugin_file( $this->make_guard_constant() ); foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $slug ) {