diff --git a/CLAUDE.md b/CLAUDE.md index faf9e31..75c74ae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -167,27 +167,43 @@ the plugin to ask about, and the collaborator does the asking. ``` Config::set_hook_prefix( 'give' ); Config::set_container( $container ); // required -Absorber::register( [ …config… ] ); // once per sub-plugin; a duplicate slug throws -Absorber::boot(); // idempotent +Absorber::register( [ …config… ] ); // once per sub-plugin; a duplicate slug throws +Absorber::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 @2 → Loader::load_all() -all_admin_notices → Absorber::render_notices() [is_admin() only] +plugins_loaded @5 → Conflict\Gatekeeper, Conflict\Detector, then Conflict\Resolver::resolve_all() +plugins_loaded @6 → Loader::load_all() +all_admin_notices → Absorber::render_notices() [is_admin() only] wp_admin_notice_markup → Absorber::filter_activation_error_markup() [is_admin() only] ``` **A host calls `Config::set_container()` at `plugins_loaded` priority 0, from its own container -block and not from a service provider.** Priority, because conflict resolution runs at priority 1 and -WordPress silently ignores a callback added at or past the priority it is already dispatching — -LearnDash and MemberDash both wire Harbor's `set_container()` at priority 1, so a host copying that -habit races us. Its own block, because LearnDash's `App::container()` builds a container lazily when -none is set and the plugin then *replaces* it at priority 0: anything that grabbed the container -earlier holds an orphan whose bindings are discarded. This is also why `Absorber::register()` buffers -and resolves nothing — registration at plugin-file scope, which the spec sanctions, would otherwise +block and not from a service provider.** This is a recommendation, not the barrier: booting anywhere +below priority 5 wires cleanly. It is priority 0 rather than plugin-file scope because LearnDash's +`App::container()` builds a container lazily when none is set and the plugin then *replaces* it at +priority 0 — anything that grabbed the container earlier holds an orphan whose bindings are +discarded. Its own block rather than a service provider, for the same reason: a provider runs +whenever the host's bootstrap happens to run it. This is also why `Absorber::register()` buffers 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 5, not the load at 6 — 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 5 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. + +**Resolution sits at 5, not at 1, so the barrier leaves a host somewhere to stand.** At 1 the only +slot left was 0, which turned a documented convention into a hard requirement — and LearnDash and +MemberDash both wire Harbor's `set_container()` at `plugins_loaded` priority 1, so a host copying the +habit it already has landed exactly on the barrier and silently got the inline fallback. Five slots +cost the load pass four priorities it was not using. The load stays one behind resolution, because a +standalone that survives the conflict defines the guard constant as it loads and the load pass has to +see that. + `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). @@ -199,10 +215,17 @@ them after the wrong problem. `docs/filters.md` and the spec agree. `Registry_Reader::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 load pass and -`Conflict\Detector` read through the reader they were constructed with rather than through the -registrar they could resolve for themselves, because it drains 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. Both passes read +through the reader they were constructed with rather than through the registrar they could resolve for +themselves, because it drains the pending registrations before it reads and a registrar asked directly +would miss anything registered since the last flush. + +**Both passes also catch `Config_Exception` around that read.** A duplicate slug is only found when +the buffer reaches the registrar, which is a read — long after both `register()` calls returned — and +it arrives inside `plugins_loaded`, the hook that exists to prevent a fatal, so this is the last place +allowed to cause one. The conflict pass needs the guard more than the load pass, not less: its request +gate means the only requests reaching it are admin page views, so an escaping throw lands on exactly +the screens the mistaken registration would have to be corrected from. The container is no longer the other half of that. A pass is handed a reader that already holds its registrar, so a container that cannot supply one fails while the *pass* is being built — where an @@ -238,11 +261,29 @@ out of the destination — only a validated screen name and a re-encoded query l `admin_url()`, or `network_admin_url()`/`user_admin_url()` in the other two admins, supplies everything in front of them. -**Who may have a conflict resolved is `Conflict\Gatekeeper`'s business, not the resolver's.** It -gates on an interactive admin `GET` (`plugins_loaded` fires on every request) *and* on -`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 +**Who may have a conflict resolved is `Conflict\Gatekeeper`'s business, not the resolver's.** Two +methods, because the two gates are asked at different moments. `request_may_resolve()` reads the +request and nothing else: an interactive admin `GET` (`plugins_loaded` fires on every request, +including cron, CLI and a visitor's POST), and not one carrying an action — a redirect-and-`exit` +discards the work behind `update.php?action=upgrade-plugin` exactly as it would a POST's, and +`plugins.php?action=activate` is the request `plugin_sandbox_scrape()` replays, so exiting there +makes core report the plugin being activated as fatal. Any action arg at all, never a list of the +dangerous ones: half of wp-admin takes an `action`, plugins add their own, and a known-safe list +would have to stay right about every one of them forever. `user_may_resolve()` asks for +`manage_network_plugins` on multisite and `activate_plugins` otherwise — `plugins_loaded` runs +before `auth_redirect()`, so an unauthenticated GET of an admin URL gets that far, and the +capability has to match the reach of the act: `deactivate_plugins()` left at the default +`$network_wide` takes the standalone out of the *network's* active plugins, which a single site's +administrator holds no authority to do. + +`Boot\Scheduler::sequence()` asks the two halves either side of `Conflict\Detector::has_conflict()`, +and that order is the point. `current_user_can()` resolves and caches the current user, so asking it +on every admin GET would settle who is signed in at `plugins_loaded` priority 5 — ahead of an SSO or +JWT plugin adding its `determine_current_user` filter from its own `plugins_loaded` callback, whose +users are then treated as logged out for the rest of the request, on requests with nothing to +resolve. The detector reports and changes nothing, so it is the cheap question that goes in front of +the expensive one. All three live in the step rather than in the resolver, so a host binding its own +cannot drop one by omission — and a request that fails any of them 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. @@ -340,7 +381,7 @@ treatment. Any older sketch showing `Config::reset()` or `Absorber::reset()` mea rejects a bad config array on the spot, at a call in the developer's own stack trace, before anything is hooked. Past that point this library is code on somebody's live site, and a white screen is never the better answer — so every entry point it puts on a hook catches `Throwable`, reports - with `_doing_it_wrong()` and abandons that step alone: the `plugins_loaded` step in + with `_doing_it_wrong()` and abandons that step alone: both `plugins_loaded` steps in `Boot\Scheduler`, and `Absorber::render_notices()` on `all_admin_notices`. `Loader::load_all()` and `Conflict\Resolver::resolve_all()` catch *per sub-plugin* as well, because one sub-plugin's throw must not take the ones behind it in the registration order with it. Everything past those catches is diff --git a/README.md b/README.md index 9aeef9c..2b08c29 100644 --- a/README.md +++ b/README.md @@ -39,21 +39,19 @@ 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. +Boot before `plugins_loaded` priority 5, where conflict resolution runs: WordPress silently ignores a +callback added at or past the priority it is already dispatching. Later is reported through +`_doing_it_wrong()` and run 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 -bindings were discarded. +Priority 0 is the recommendation, in the block that owns your container rather than in a service +provider: a host that builds one lazily and replaces it at priority 0 leaves us holding an orphan +whose bindings were discarded. ## Docs - [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 fe66d18..13dad8d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -23,17 +23,18 @@ Any implementation of StellarWP's `ContainerInterface` will do — the one your to Telemetry, Uplink or Harbor. `Config::get_container()` throws `Config_Exception` when none is set; `Config::has_container()` is the probe if you need to ask. -Priority matters twice. Conflict resolution runs 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 us from a provider that itself runs at priority 1 races us. And a host -that builds its container lazily may *replace* it at priority 0; hand us the container before that -happens and we hold an orphan whose bindings were discarded. +Priority matters twice, for two unrelated reasons. Conflict resolution runs at `plugins_loaded` +priority 5 and the load at priority 6, and WordPress silently ignores a callback added at or past the +priority it is already dispatching — so boot has to land before 5, which leaves 0 through 4. And a +host that builds its container lazily may *replace* it at priority 0; hand us the container before +that happens and we hold an orphan whose bindings were discarded. It is that second one that picks 0 +out of the five, so if your container is already built by then, anywhere below 5 works. ## Rebinding a collaborator `Absorber::boot()` binds the defaults, and skips any id your container already has — so your binding wins whether you make it before boot or after, and nothing is resolved until `plugins_loaded` -priority 1 in any case: +priority 5 in any case: ```php use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; @@ -53,6 +54,13 @@ $container->singleton( Registrar_Interface::class, My_Registrar::class ); `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` that carries no action, and the capability to deactivate across the +network](conflict-handling.md#when-resolution-runs) — live in `Conflict\Gatekeeper`, which the hook +consults rather than the resolver, 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 `Absorber::register()` calls, so long as it comes before boot. Registering buffers the sub-plugin and resolves nothing, so nothing is decided @@ -91,10 +99,11 @@ at include time. Register each slug exactly once. A slug also names the sub-plugin's notices and its once-ever activation record, so a second registration under the same slug is refused with a `Config_Exception` naming both bundled files rather than quietly dropping one of the two from the -load. Registrations are buffered and nothing reads them until the load pass at `plugins_loaded` -priority 2, so that is where the collision surfaces — not at the second `register()` call and not at -`boot()`. It is reported with `_doing_it_wrong()` and that request loads no sub-plugin at all, rather -than thrown out of a core hook. A config array the library cannot use is still rejected on the spot. +load. Registrations are buffered and nothing reads them until `plugins_loaded` — the conflict pass at +priority 5 on an admin page view, the load pass at priority 6 on everything else — so that is where +the collision surfaces, not at the second `register()` call and not at `boot()`. Whichever pass +reads first reports it with `_doing_it_wrong()`, and that request resolves no conflict and loads no +sub-plugin at all, rather than throwing out of a core hook. A config array the library cannot use is still rejected on the spot. 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. diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index f22e364..b806186 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -33,6 +33,70 @@ 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 5, one ahead of the load pass at 6: a standalone that survives the +conflict defines the guard constant as it loads, and the load pass has to see that. Priority 5 is +also the deadline for `Absorber::boot()`, since this is the first step it has to wire. + +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. + +**A `GET` that carries an action is skipped too.** `update.php?action=upgrade-plugin`, +`plugins.php?action=activate` and the `admin-post.php` links are all admin `GET`s that *do* something, +and a redirect discards their work exactly as it would a POST's — the user clicks Update and lands on +a list screen with nothing updated. Anything naming an `action` or `action2`, and the endpoints that +exist only to perform work, wait for the next plain page view. This is deliberately blunt: a +read-only `post.php?action=edit` waits as well. + +It also requires the capability that matches what deactivation actually does. Deactivating a +standalone is network-wide wherever a network exists, so the check is `manage_network_plugins` on +multisite and `activate_plugins` otherwise — `activate_plugins` alone does not imply authority over +every site on a network. The gate matters at all because `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. It 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 the +same capability, so nothing is consumed by waiting for one who has it. + +Both gates live in `Conflict\Gatekeeper`, and the hook asks it rather than the resolver, so binding +your own `Conflict\Contracts\Resolver_Interface` cannot drop either by omission. They are asked in +two halves either side of `Conflict\Detector::has_conflict()`, which only reports: the request-shape +gate first, then the detector, then the capability. The capability check is last because +`current_user_can()` resolves and caches the current user, and at priority 5 that lands ahead of any +`determine_current_user` filter a plugin registers from its own `plugins_loaded` callback — an SSO or +JWT plugin hooked at the default priority would never be consulted, and its users would be treated as +signed out. Asking the detector rather than the resolver keeps detection off the contract a host +rebinds, and means the resolver is built only on a request that passes both gates and has something +to resolve. + +## The redirect + +The standalone's code is already in memory by the time the conflict is resolved — WordPress included +it before `plugins_loaded` — so the redirect is how the request sheds it. The destination is **the +screen being requested**, not the one the user came from: it re-renders without the standalone, and +the admin stays where they asked to be. `/wp-admin/` and the network and user admin roots mean the +dashboard. The update screens (`update.php`, `update-core.php`) go to `plugins.php` instead, because +reloading one of those would re-run an update, and anything that names no usable admin screen falls +back to `plugins.php`. + +The destination is assembled from the screen name and query string through `admin_url()` — or +`network_admin_url()` and `user_admin_url()` in the network and user admins, so a request resolved +in one of those comes back to it — never from the request URI itself, so nothing in the URI decides +the host. There is no redirect loop: the next request has no active standalone, so nothing +resolves. + +With several sub-plugins in conflict, all of them are resolved before the one redirect at the end, +and the redirect is skipped entirely once headers have been sent — which is what a host booting too +late produces, since the `_doing_it_wrong()` notice is output. The request then finishes rendering +instead of dying blank. + +`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 Before loading a bundled plugin, the library checks whether `plugin_loaded_constant` is already diff --git a/docs/notices.md b/docs/notices.md index 8dcb5f0..ddfa461 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -18,8 +18,12 @@ the site owner is never told their plugin was turned off. must not be shown one — a subscriber loading their profile page would otherwise silently swallow the 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. +On multisite `activate_plugins` usually maps through `manage_network_plugins`, so it is normally a +network administrator, not the site administrator who installed the plugin, who sees these. Not +always, though: a network that has enabled the Plugins menu for its sites turns that mapping off, and +site administrators hold `activate_plugins` directly. Conflict resolution therefore asks for +`manage_network_plugins` by name rather than relying on the mapping — see +[conflict handling](conflict-handling.md#when-resolution-runs). ## Rendering them yourself diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index 297344a..d233caf 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -6,6 +6,10 @@ namespace Nexcess\PluginAbsorber\Boot; use Nexcess\PluginAbsorber\Absorber; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +use Nexcess\PluginAbsorber\Conflict\Detector; +use Nexcess\PluginAbsorber\Conflict\Gatekeeper; +use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Loader; use StellarWP\ContainerContract\ContainerInterface; use Throwable; @@ -27,13 +31,41 @@ class Scheduler { * plugins_loaded priority the load pass runs at. * * Ahead of the default priority, so a bundled plugin is in memory before the plugins that - * expect it start their own work, and low enough to leave room for earlier wiring. + * expect it start their own work. + * + * Every priority below this one is a band of the bundled plugin's *own* plugins_loaded + * callbacks that silently never fire: a standalone copy is included by wp-settings.php before + * the action is dispatched at all and keeps every callback it registers, while a bundled copy + * required from a callback here only keeps the ones above the priority that required it. + * Hooking plugins_loaded below the default is already a special case, so the band this gives + * up is a narrow one — but it is given up silently, which is why the number moves down when + * there is any doubt and not up. + * + * @since 1.0.0 + * + * @var int + */ + private const LOAD_PRIORITY = 6; + + /** + * 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. + * + * This is the number a host is measured against, not the load: it is the first step in the + * sequence, so it is the priority `set_container()` and `boot()` have to beat. At 1 the only + * slot left was 0, which made a documented convention a hard requirement — and both LearnDash + * and MemberDash wire Harbor's `set_container()` at priority 1, so a host copying the habit it + * already has landed exactly on the barrier and got the inline fallback instead of the hooks. + * Five slots ahead of it covers both habits with room over, and costs the load pass four + * priorities it was not using. * * @since 1.0.0 * * @var int */ - private const LOAD_PRIORITY = 2; + private const RESOLVE_PRIORITY = 5; /** * @since 1.0.0 @@ -58,6 +90,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 before plugins_loaded + * priority 5, as documented, and it always does. + * * @since 1.0.0 * * @return void @@ -72,13 +108,13 @@ public function wire(): void { } // Adding an action at a priority the current dispatch has already passed is accepted and - // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- - // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with - // no warning and a site that looks entirely healthy. + // then never fires. Booting from plugins_loaded at the default priority -- the commonest + // hook mistake there is -- would otherwise mean nothing loads at all, with no warning and + // a site that looks entirely healthy. if ( $this->wiring_window_has_closed() ) { _doing_it_wrong( Absorber::class . '::boot', - 'Absorber::boot() must run before plugins_loaded priority 2. Loading inline instead.', + 'Absorber::boot() must run before plugins_loaded priority 5. Resolving and loading inline instead.', '1.0.0' ); @@ -110,12 +146,18 @@ 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 { + self::resolve_conflicts( $container ); + }, + ], [ 'priority' => self::LOAD_PRIORITY, 'run' => static function () use ( $container ): void { @@ -126,10 +168,72 @@ private function sequence(): array { } /** - * The load step. + * The conflict step: the two gates, the probe between them, and the resolve behind all three. * * Static, and handed the container rather than reading one, so the closure in sequence() stays a - * closure over the container. + * closure over the container like the load step beside it. + * + * @since 1.0.0 + * + * @param ContainerInterface $container Container each collaborator is resolved from. + * + * @return void + */ + private static function resolve_conflicts( ContainerInterface $container ): void { + try { + $gatekeeper = $container->get( Gatekeeper::class ); + + // The shape of the request first. It reads the request and nothing else, so cron, WP-CLI, a + // POST and every front-end view are turned away having resolved no user and built no + // resolver. + if ( ! $gatekeeper->request_may_resolve() ) { + return; + } + + // Then whether there is a conflict at all, before anyone asks who is signed in. + // current_user_can() resolves and caches the current user, and this step runs at + // plugins_loaded priority 5 -- ahead of the plugins that add their determine_current_user + // filter from a plugins_loaded callback of their own. Ask on every admin GET and an SSO or + // JWT visitor is pinned as logged out for the rest of the request, on requests with nothing + // to resolve. The detector reports and changes nothing, so the capability is only asked for + // where its answer decides something. + if ( ! $container->get( Detector::class )->has_conflict() ) { + return; + } + + if ( ! $gatekeeper->user_may_resolve() ) { + return; + } + + // Both gates and the probe live here rather than inside the resolver, so a host binding + // its own cannot drop one by omission -- and asking them first means a resolver is built + // only on the request that goes on to use it. + $container->get( Resolver_Interface::class )->resolve_all(); + } catch ( Config_Exception $exception ) { + // Reading the registry is where a duplicate slug surfaces, and this step reads it a + // priority ahead of the load pass that has always guarded the same read. Named separately + // from the catch below because it is the one failure here a developer can act on directly, + // and the message says which. + _doing_it_wrong( + self::class, + sprintf( + 'The registered sub-plugins could not be read, so no conflict was resolved: %s', + $exception->getMessage() + ), + '1.0.0' + ); + } catch ( Throwable $thrown ) { + // The backstop, and the promise the whole library rests on: plugins_loaded fires on every + // request a site serves, so a throw out of a step is a white screen on all of them. What + // reaches here is a collaborator a host's factory could not build, a gate, the probe, or a + // resolver a host bound itself -- the passes guard their own per-sub-plugin loops, and this + // guards everything the step touches on the way to them. + self::report_a_step_that_threw( 'conflict pass', 'no conflict was resolved', $thrown ); + } + } + + /** + * The load step. * * @since 1.0.0 * @@ -138,10 +242,10 @@ private function sequence(): array { * @return void */ private static function load( ContainerInterface $container ): void { - // plugins_loaded fires on every request a site serves, so a throw out of this step is a white - // screen on all of them. `Loader::load_all()` already reports per sub-plugin and carries on, - // so what is left for this to catch is the pass itself -- a container that cannot build it - // above all, which is the shape a host's own broken binding takes. + // Guarded like the conflict step, and for the same reason. `Loader::load_all()` already reports + // per sub-plugin and carries on, so what is left for this to catch is the pass itself -- a + // container that cannot build it above all, which is the shape a host's own broken binding + // takes. try { $container->get( Loader::class )->load_all(); } catch ( Throwable $thrown ) { @@ -169,13 +273,17 @@ private static function report_a_step_that_threw( string $step, string $conseque } /** - * 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 5 and the load at 6, 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 * @@ -192,6 +300,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/tests/README.md b/tests/README.md index 0cd0eb4..53e089e 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/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index 21087fe..6e63c17 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -11,6 +11,11 @@ use Nexcess\PluginAbsorber\Absorber; use Nexcess\PluginAbsorber\Boot\Scheduler; use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Conflict\Contracts\Resolver_Interface; +use Nexcess\PluginAbsorber\Conflict\Detector; +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\Absorber_State; @@ -20,6 +25,9 @@ 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\WithNoticeQueue; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithRequestMethod; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithUsers; use ReflectionClass; use RuntimeException; use WP_Hook; @@ -42,12 +50,37 @@ class SchedulerTest extends WPTestCase { use WithBundledPlugins; use WithContainer; use WithIncorrectUsage; + use WithNoticeQueue; + use WithRequestMethod; + use WithUsers; /** * @var int */ private $plugins_loaded_count = 0; + /** + * Every gate and probe the conflict step reached, in the order it reached them. + * + * The step short-circuits, so what it did not ask is as much of the behaviour as what it did — + * and an ordered log says both in one assertion, where a counter per double would let a + * capability check that ran first still satisfy a count of one. + * + * @var array + */ + private $conflict_calls = []; + + /** + * How many times the container was asked to build a resolver. + * + * Counted in the binding's factory rather than on the double, because the property under test is + * that a request turned away before the resolve step builds nothing at all — which a counter on + * the double could not tell apart from one that was built and never asked anything. + * + * @var int + */ + private $resolvers_built = 0; + /** * Hook callbacks these tests added, as [ hook, callback, priority ] triples. * @@ -67,6 +100,14 @@ public function setUp(): void { Config::set_hook_prefix( 'give' ); $this->set_up_container(); $this->reset_bundled_plugin_loads(); + $this->clear_notices(); + + $this->conflict_calls = []; + $this->resolvers_built = 0; + + // 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->set_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 @@ -79,6 +120,8 @@ public function setUp(): void { public function tearDown(): void { $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + $this->restore_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. @@ -92,6 +135,7 @@ public function tearDown(): void { $this->stop_expecting_incorrect_usage(); $this->remove_bundled_plugin_files(); + $this->clear_notices(); Absorber_State::reset(); Config_State::reset(); $this->tear_down_container(); @@ -100,18 +144,32 @@ public function tearDown(): void { /** * Ahead of the default priority, so a bundled plugin is in memory before the plugins that expect - * it start their own work, and low enough to leave room for earlier wiring — conflict resolution - * runs at 1. The number is documented, so it is part of the contract rather than an internal. + * it start their own work. The number is documented, so it is part of the contract rather than an + * internal — and it is asserted literally rather than read back through the constant, which would + * only prove the constant equals itself. + * + * Below the default and no lower than it has to be: every priority under this one is a band of + * the bundled plugin's own plugins_loaded callbacks that never fire, since the standalone copy + * wp-settings.php includes keeps all of them. */ public function test_the_load_step_runs_early_in_plugins_loaded(): void { - $this->assertSame( 2, $this->load_priority() ); + $this->assertSame( 6, self::load_priority() ); } /** - * The outermost guarantee, and the reason it lives here rather than inside the pass: whatever the - * step reaches — a collaborator a host's factory could not build, a pass that got past its own - * guard — `plugins_loaded` fires on every request a site serves, and a throw out of it is a white - * screen on all of them. The step is reported and abandoned on its own. + * 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. + * + * Being first makes this the number a host is measured against, so it is the one that decides how + * much room a host has to configure the library in. Priority 5 leaves 0 through 4, which covers + * booting at 0 as documented and the priority-1 habit LearnDash and MemberDash already have. + */ + /** + * The outermost guarantee, and the reason it lives here rather than in each pass: whatever a step + * reaches — a collaborator a host's factory could not build, a gate, a probe, a pass that got past + * its own guard — `plugins_loaded` fires on every request a site serves, and a throw out of it is + * a white screen on all of them. Each step is reported and abandoned on its own, so the step + * behind it still runs. * * @dataProvider throwing_steps * @@ -139,24 +197,222 @@ static function (): object { } /** - * A provider from the start, because every step the sequence gains has to answer this one. + * Both steps, because a guard on one of them leaves the other able to end the request. * * @return Generator */ public static function throwing_steps(): Generator { - yield 'the load step' => [ Loader::class ]; + yield 'the conflict step' => [ Gatekeeper::class ]; + yield 'the load step' => [ Loader::class ]; + } + + public function test_the_conflict_step_runs_before_the_load_step(): void { + $this->assertSame( 5, self::resolve_priority() ); + $this->assertLessThan( self::load_priority(), self::resolve_priority() ); + } + + public function test_it_wires_the_conflict_step_at_the_resolve_priority(): void { + $this->bind_resolver_double(); + + $before = $this->callbacks_at( 'plugins_loaded', self::resolve_priority() ); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles(); + + // What carries "wired rather than ran" is the callback count and the build counter. The gate + // log cannot: those doubles are only in place from the line above, so a boot that ran the step + // would have asked the real gatekeeper and left the log empty either way. + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', self::resolve_priority() ), + 'boot() must wire the conflict step rather than run it.' + ); + $this->assertSame( [], $this->conflict_calls, 'Wiring must not ask anything yet.' ); + $this->assertSame( 0, $this->resolvers_built, 'Wiring must not build a resolver.' ); + + do_action( 'plugins_loaded' ); + + $this->assertContains( 'resolve_all', $this->conflict_calls ); + } + + /** + * The whole sequence, on the one request that runs all of it, asserted as an order rather than as + * four counts: each step is the reason the next one is worth taking, so a pass that reached them + * in another order is not the behaviour. + */ + public function test_the_conflict_step_probes_for_a_conflict_before_it_asks_who_is_signed_in(): void { + $this->bind_resolver_double(); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles(); + + do_action( 'plugins_loaded' ); + + $this->assertSame( + [ 'request_may_resolve', 'has_conflict', 'user_may_resolve', 'resolve_all' ], + $this->conflict_calls + ); + } + + /** + * The shape gate reads the request and nothing else, and it comes first so that cron, WP-CLI, a + * POST and every front-end view are turned away having resolved no user and built no resolver. + */ + public function test_a_request_the_shape_gate_refuses_builds_no_resolver(): void { + $this->bind_resolver_double(); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles( false ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( [ 'request_may_resolve' ], $this->conflict_calls ); + $this->assertSame( 0, $this->resolvers_built, 'A refused request must not build a resolver.' ); + + // The counter has to be shown to work: a binding that was never reached and a binding that + // cannot be resolved at all produce the same zero. + $this->resolve( Resolver_Interface::class ); + + $this->assertSame( 1, $this->resolvers_built, 'The container really does build through the factory.' ); + } + + /** + * The probe is what keeps the capability check off the requests that have nothing to act on — + * `current_user_can()` caches the current user, and this step runs before the plugins that decide + * who that is have hooked `determine_current_user`. + */ + public function test_a_request_with_no_conflict_never_asks_about_the_user(): void { + $this->bind_resolver_double(); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles( true, false ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( [ 'request_may_resolve', 'has_conflict' ], $this->conflict_calls ); + } + + /** + * And on that request the resolver is never built at all. Detection moving off + * `Resolver_Interface` and onto `Conflict\Detector` is what makes this assertable: while the probe + * was a method on the resolver, asking it meant constructing one — and everything it depends on — + * on every admin GET a site served, whatever the answer turned out to be. + */ + public function test_a_request_with_no_conflict_builds_no_resolver(): void { + $this->bind_resolver_double(); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles( true, false ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 0, $this->resolvers_built, 'Nothing to resolve must mean nothing built to resolve it.' ); + + // The counter has to be shown to work: a binding that was never reached and a binding that + // cannot be resolved at all produce the same zero. + $this->resolve( Resolver_Interface::class ); + + $this->assertSame( 1, $this->resolvers_built, 'The container really does build through the factory.' ); + } + + /** + * The capability gate still stands between a real conflict and acting on it. It lives here rather + * than inside the resolver, so a host that binds its own `Resolver_Interface` decides what a + * conflict means and not who may have one resolved. + */ + public function test_a_user_the_capability_gate_refuses_does_not_reach_resolve_all(): void { + $this->bind_resolver_double(); + + Absorber::boot(); + + $this->bind_gate_and_probe_doubles( true, true, false ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( [ 'request_may_resolve', 'has_conflict', 'user_may_resolve' ], $this->conflict_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' ) ); + + Absorber::boot(); + + do_action( 'plugins_loaded' ); + + $this->assertSame( [], $this->queued_notices(), '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->queued_notices(), + 'The conflict has to still be detectable once someone who can act on it arrives.' + ); + } + + /** + * Reading the registry flushes the registration buffer, and the registrar refuses a slug it + * already holds. The conflict step reads a priority ahead of the load pass, so it — not the load + * pass that has always guarded this — is the first pass a duplicate slug reaches, and a throw + * here arrives inside plugins_loaded, where it takes wp-admin down and locks the developer out + * of the screen where the second registration could be undone. + * + * The front end never reaches it, because the request gate turns away first. That is what makes + * this the worse failure rather than a lesser one: the only requests that fatal are the ones the + * mistake could have been corrected from. + */ + public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_conflict_step(): void { + set_current_screen( 'dashboard' ); + + $this->bind_active_standalone(); + $this->register_conflicted_sub_plugin(); + $this->register_conflicted_sub_plugin(); + + $this->become_plugin_administrator(); + + Absorber::boot(); + + $this->expect_incorrect_usage(); + + do_action( 'plugins_loaded' ); + + // Reaching this line at all is half of what is under test: the step has to return. + $this->assertSame( + [], + $this->queued_notices(), + 'A read that failed has no list to resolve from, so nothing may be resolved.' + ); + $this->assert_the_library_reported_incorrect_usage(); } public function test_it_wires_the_load_step_at_the_load_priority(): void { $this->register_sub_plugin(); - $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $before = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); Absorber::boot(); $this->assertSame( $before + 1, - $this->callbacks_at( 'plugins_loaded', $this->load_priority() ), + $this->callbacks_at( 'plugins_loaded', self::load_priority() ), 'boot() must wire the load step rather than run it.' ); $this->assertSame( 0, $this->bundled_plugin_loads(), 'Wiring must not load anything yet.' ); @@ -169,14 +425,14 @@ public function test_it_wires_the_load_step_at_the_load_priority(): void { public function test_booting_twice_wires_the_load_step_only_once(): void { $this->register_sub_plugin(); - $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $before = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); Absorber::boot(); Absorber::boot(); $this->assertSame( $before + 1, - $this->callbacks_at( 'plugins_loaded', $this->load_priority() ), + $this->callbacks_at( 'plugins_loaded', self::load_priority() ), 'boot() must be idempotent.' ); @@ -251,13 +507,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(); @@ -278,7 +536,7 @@ static function () use ( $path, $constant ): void { Absorber::boot(); }, - $this->load_priority() + $offset + self::load_priority() + $offset ); do_action( 'plugins_loaded' ); @@ -291,9 +549,70 @@ 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 ]; + // Priority 10, where an add_action() naming no priority at all lands — the commonest way to + // boot too late. Derived from the load priority rather than written as an offset, which + // would go on describing this case while quietly testing a different one every time the + // load priority moved. + yield 'the default a host omits' => [ 10 - self::load_priority() ]; + } + + /** + * The other side of the same boundary: every priority the window is still open at must *wire* the + * sequence rather than run it. + * + * That the bundled plugin loaded proves nothing on its own — the inline fallback loads it too, + * which is the whole point of having one. The callback count at the load priority is what + * separates the two: wiring leaves a callback behind, and running inline never adds one. + * + * The band below the resolve priority is the whole reason it is not 1. A host has somewhere to + * stand other than priority 0, so the documented convention stays a convention: move resolution + * back down and the cases below fail instead of quietly taking the inline path. + * + * @dataProvider boot_priorities_that_still_wire + * + * @param int $priority plugins_loaded priority the host boots from. + */ + public function test_booting_before_the_resolve_priority_still_wires( int $priority ): void { + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + $before = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); + + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( $path, $constant ): void { + Absorber::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + Absorber::boot(); + }, + $priority + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads(), 'The bundled plugin has to load either way.' ); + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', self::load_priority() ), + 'Booting inside the window has to wire the load step, not run it inline.' + ); + } + + /** + * @return Generator + */ + public static function boot_priorities_that_still_wire(): Generator { + yield 'at the start, as documented' => [ 0 ]; + yield 'the habit a host arrives with' => [ 1 ]; + yield 'the last slot before the barrier' => [ 4 ]; } public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { @@ -317,13 +636,13 @@ 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() ); + $load_step = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); $notice_step = $this->callbacks_at( 'all_admin_notices' ); Absorber::boot(); Absorber_State::reset(); - $this->assertSame( $load_step, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + $this->assertSame( $load_step, $this->callbacks_at( 'plugins_loaded', self::load_priority() ) ); $this->assertSame( $notice_step, $this->callbacks_at( 'all_admin_notices' ) ); } @@ -332,14 +651,14 @@ public function test_the_state_helper_unwires_the_hooks_boot_added(): void { * rather than a leftover from the first. */ public function test_the_state_helper_allows_booting_again(): void { - $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $before = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); Absorber::boot(); Absorber_State::reset(); Absorber::boot(); - $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', self::load_priority() ) ); } /** @@ -352,11 +671,11 @@ public function test_boot_does_not_need_a_hook_prefix(): void { Config_State::reset(); Config::set_container( $container ); - $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $before = $this->callbacks_at( 'plugins_loaded', self::load_priority() ); Absorber::boot(); - $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', self::load_priority() ) ); } /** @@ -367,7 +686,7 @@ public function test_boot_does_not_need_a_hook_prefix(): void { * * @return int */ - private function load_priority(): int { + private static function load_priority(): int { $priority = ( new ReflectionClass( Scheduler::class ) )->getConstant( 'LOAD_PRIORITY' ); if ( ! is_int( $priority ) ) { @@ -377,6 +696,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 static 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. * @@ -410,7 +747,12 @@ private function callbacks_at( string $hook, ?int $priority = null ): int { /** * Bind a recording queue in place of the default one. * - * Bound before the provider runs, which is the only order that leaves it bound. + * Bound before boot, the way a host rebinding the seam does it. That survives because the id is an + * interface: the provider skips an id the container can already answer for, and only a binding + * makes it answer for an interface. A class-id double bound here would not survive, since a + * container answers for every class that exists whether or not anything was bound to it — and it + * would be overwritten twice, once by the provider run in `set_up_container()` and again by the + * one inside `boot()`. * * @return Spy_Queue */ @@ -429,6 +771,227 @@ static function () use ( $notices ): Spy_Queue { return $notices; } + /** + * Bind a resolver that logs `resolve_all` into `$conflict_calls` and counts every build. + * + * Call before `Absorber::boot()`. `Resolver_Interface` is an interface id, and the provider stands + * down for an id the container can already answer for — which, for an interface, it can only do + * because something bound one. That is the seam a host is invited to replace, and replacing it + * deliberately does not depend on knowing when the library boots. + * + * Nothing is returned: what the assertions read is the log and the build counter, so a step that + * skipped a double is as visible as one that reached it, and neither reading depends on a + * container's untyped return being narrowed back to a double's own class. + * + * @return void + */ + private function bind_resolver_double(): void { + $calls = &$this->conflict_calls; + $builds = &$this->resolvers_built; + + $record = static function ( string $call ) use ( &$calls ): void { + $calls[] = $call; + }; + + $container = new Test_Container(); + $container->singleton( + Resolver_Interface::class, + static function () use ( $record, &$builds ): Resolver_Interface { + ++$builds; + + return new class( $record ) implements Resolver_Interface { + /** + * @var callable + */ + private $record; + + /** + * @param callable $record Logs the call. + */ + public function __construct( callable $record ) { + $this->record = $record; + } + + /** + * @return void + */ + public function resolve_all(): void { + ( $this->record )( 'resolve_all' ); + } + }; + } + ); + + $this->set_up_container( $container ); + } + + /** + * Bind a gatekeeper and a detector with fixed answers, both logging into `$conflict_calls`. + * + * Call *after* `Absorber::boot()`, and before the hook fires. These are class ids, and a container + * reports that it can answer for any class that exists whether or not anything was bound to it — + * so the provider cannot tell a double apart from the container's own willingness to build the + * real class, and rebinds regardless. Binding them alongside the resolver would leave them + * overwritten twice over: once by the provider run inside `set_up_container()`, and again by the + * one `boot()` performs itself. The step would then reach the real `Gatekeeper` and `Detector`, + * the log would come back empty, and the failure would read as a step that never ran. + * + * After boot is early enough because nothing resolves until `plugins_loaded` fires: the scheduler + * wires closures that ask the container when the hook runs, which is the same window a host has + * for rebinding one of the concrete workers. + * + * @param bool $request_may_resolve Whether the shape of the request admits resolution. + * @param bool $has_conflict Whether the detector reports anything to resolve. + * @param bool $user_may_resolve Whether the current user may have it resolved. + * + * @return void + */ + private function bind_gate_and_probe_doubles( + bool $request_may_resolve = true, + bool $has_conflict = true, + bool $user_may_resolve = true + ): void { + $calls = &$this->conflict_calls; + + $record = static function ( string $call ) use ( &$calls ): void { + $calls[] = $call; + }; + + $container = $this->container(); + $container->singleton( + Gatekeeper::class, + static function () use ( $record, $request_may_resolve, $user_may_resolve ): Gatekeeper { + return new class( $record, $request_may_resolve, $user_may_resolve ) extends Gatekeeper { + /** + * @var callable + */ + private $record; + + /** + * @var bool + */ + private $request_answer; + + /** + * @var bool + */ + private $user_answer; + + /** + * @param callable $record Logs the call. + * @param bool $request_answer Answer for the request gate. + * @param bool $user_answer Answer for the capability gate. + */ + public function __construct( callable $record, bool $request_answer, bool $user_answer ) { + $this->record = $record; + $this->request_answer = $request_answer; + $this->user_answer = $user_answer; + } + + /** + * @return bool + */ + public function request_may_resolve(): bool { + ( $this->record )( 'request_may_resolve' ); + + return $this->request_answer; + } + + /** + * @return bool + */ + public function user_may_resolve(): bool { + ( $this->record )( 'user_may_resolve' ); + + return $this->user_answer; + } + }; + } + ); + $container->singleton( + Detector::class, + static function () use ( $record, $has_conflict ): Detector { + return new class( $record, $has_conflict ) extends Detector { + /** + * @var callable + */ + private $record; + + /** + * @var bool + */ + private $answer; + + /** + * No plugin checker, and no parent constructor call: the answer is stated here, and + * how a real detector arrives at one is DetectorTest's subject. + * + * @param callable $record Logs the call. + * @param bool $answer Whether there is a conflict to resolve. + */ + public function __construct( callable $record, bool $answer ) { + $this->record = $record; + $this->answer = $answer; + } + + /** + * @return bool + */ + public function has_conflict(): bool { + ( $this->record )( 'has_conflict' ); + + return $this->answer; + } + }; + } + ); + } + + /** + * 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(); + + Absorber::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, + ] + ); + } + /** * Register a sub-plugin whose bundled file records that it was loaded. * diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index b2a19f1..b3b28bd 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -22,6 +22,7 @@ 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\WithNoticeQueue; use RuntimeException; /** @@ -38,6 +39,7 @@ class LoaderTest extends WPTestCase { use WithBundledPlugins; use WithContainer; use WithIncorrectUsage; + use WithNoticeQueue; /** * Guard constants a test defined through uopz. @@ -219,7 +221,7 @@ public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): $this->loader()->load_all(); $this->assertSame( 0, $this->bundled_plugin_loads() ); - $this->assertArrayHasKey( 'give-recurring:dependency', $this->notice_queue() ); + $this->assertArrayHasKey( 'give-recurring:dependency', $this->queued_notices() ); } public function test_it_skips_when_the_guard_constant_is_already_defined(): void { @@ -275,7 +277,7 @@ public function test_a_missing_bundled_file_reports_to_the_developer_not_the_sit $this->loader()->load_all(); - $this->assertSame( [], $this->notice_queue() ); + $this->assertSame( [], $this->queued_notices() ); $this->assert_the_library_reported_incorrect_usage(); } @@ -350,7 +352,7 @@ public function test_an_already_loaded_sub_plugin_is_not_dependency_checked(): v $this->loader()->load_all(); $this->assertSame( 0, $checked ); - $this->assertSame( [], $this->notice_queue(), 'No notice for a plugin that is already running.' ); + $this->assertSame( [], $this->queued_notices(), 'No notice for a plugin that is already running.' ); } public function test_the_should_load_filter_can_veto_the_load(): void { @@ -615,7 +617,7 @@ static function () use ( $notices ): Queue_Interface { ); $this->assertSame( [], - $this->notice_queue(), + $this->queued_notices(), 'The default queue must not have been resolved alongside it.' ); } @@ -721,22 +723,6 @@ private function stop_recording_incorrect_usage_messages(): void { $this->incorrect_usage_messages = []; } - 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 notice_queue(): array { - $queue = get_site_option( 'give_plugin_absorber_notices', [] ); - - return is_array( $queue ) ? $queue : []; - } - /** * Define a guard constant for the duration of one test, undone in tearDown. * diff --git a/tests/unit/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php index 8cbb5fd..e1933b8 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. *