diff --git a/CLAUDE.md b/CLAUDE.md index f07b852..1143f48 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,76 +54,157 @@ declarations). ### Public surface -A two-class static facade — `Config` (hook prefix + optional container) and `Loader` -(resolve/register/boot/load) — matching the shape of `stellarwp/assets` and -`stellarwp/admin-notices`. Everything else is an implementation detail behind it. +A two-class static facade — `Config` (hook prefix + container) and `Loader` (register/boot, plus +accessors) — matching the shape of `stellarwp/assets` and `stellarwp/admin-notices`. Everything +else is an implementation detail behind it. `Loader` is +`final`: every member is private static and every internal call is `self::`, so a subclass could +override nothing and would silently change nothing. ### Collaborators -Four interface-backed collaborators, each with a default implementation: +Every collaborator is constructed by the container and injected. The interface-backed ones are the +seams a host may rebind: -| Interface | Default | Responsibility | +| Interface | Bound to | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | | `Notices\Contracts\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | | `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | standalone detection, deactivation, redirect | -| `Contracts\Activation_Interface` | `Activation` | run-once activation-callback tracking | +| `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | deactivates the standalone, network-aware | +| `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | answers whether a plugin is active | +| `Contracts\Activator_Interface` | `Activator` | run-once activation-callback tracking | + +The rest — `Boot\Scheduler`, `Load\Runner`, `Conflict\Gatekeeper`, `Conflict\Redirector`, +`Notices\Store`, `Notices\Renderer` — are bound as concrete classes. A host that wants one of them +different rebinds the class name; there is no interface because nothing in the library dispatches on +one. An interface belonging to a folder-scoped concern lives in that folder's `Contracts\`, not beside its implementation and not in the top-level `src/Contracts/`. `src/Contracts/` is for the interfaces whose -implementations sit at the root — `Registrar`, `Plugin_State`, `Activation`. - -All four come through one generic helper — `Loader::resolve( string $interface, string -$default_class ): object` — which returns the container binding when `$container->has()`, otherwise -`new $default_class()`, memoized either way. Collaborators reach each other through the accessors -(`Loader::registrar()`, `resolver()`, `notices()`, `activation()`), so rebinding one in the host's -container flows everywhere. - -The container is **never** used to wire hooks. Hooks stay plain static trampolines, which is what -keeps the container genuinely optional. - -`Sub_Plugin` is a value object holding the per-sub-plugin predicates that its *configuration alone* -decides (`is_enabled()`, `is_already_loaded()`, `has_standalone_plugin()`, `get_conflict_policy()`, -…). Questions about the site rather than the config — whether the standalone is actually active — -belong to `Plugin_State_Interface`; `Sub_Plugin` only names the plugin to ask about. Keeping the -config predicates there is what lets collaborators stay thin and lets them be tested without hooks. +implementations sit at the root — `Registrar`, `Plugin_Deactivator`, `Plugin_Checker`, `Activator` — +plus `Provider_Interface`. + +**The container is required.** `Config::get_container()` throws `Config_Exception` when unset, which +is what `uplink`, `telemetry`, `schema` and `harbor` all do; `has_container()` stays as the probe. +Optional was the outlier — of nineteen vendored StellarWP packages exactly one falls back to `new`, +and we had modelled ourselves on it. One requirement is what that outlier costs. "Container binding +when bound, `new $default_class` otherwise" forces every default class to be constructible with no +arguments, which forces `?Peer $peer = null` constructor parameters, which forces a `protected` +accessor per peer falling back to a static. `Container\Resolution` was the class holding that chain +together, and the chain is deleted with it. The hosts qualify: `learndash-core` ships a container +implementing this very contract, exposes it as `App::container()`, and already hands it to Telemetry, +Validation and Harbor. The plugins with no container are the add-ons being absorbed, not the hosts +doing the absorbing. + +**One provider, one method.** `Provider::register()` performs every binding, behind a +`Contracts\Provider_Interface` declaring `register(): void` and nothing else — Harbor's shape, not +`stellarwp/foundation`'s. Foundation floors at PHP 8.3 against our 7.4, hard-depends on +`lucatume/di52` against our container-contract-only rule, and its `deferred`/`provides()` API is a +fatal `TypeError` in practice, because di52 type-hints its own `ServiceProvider` base class which +Foundation's abstract does not extend. + +**`Provider` never overwrites a binding.** It binds only what the container does not already have, so +a host that bound its own implementation wins, and the order in which the host calls +`set_container()` and `boot()` stops deciding which implementation it gets. Everything is a +singleton: each binding is either a registry whose contents are the point — a second `Registrar` +would hold a second, emptier list — or a stateless worker with nothing to gain from a second copy. + +The container is **never** used to wire hooks, and the reason is no longer that it is optional. +`Boot\Scheduler` wires callbacks that resolve *inside* the callback — a closure over the container, +or a static trampoline reading `Loader::notices()` — so wiring instantiates nothing, a host may +rebind right up until the hook fires, and a request that reaches none of them builds none of them. +`$container->callback()` reads better and is what the hand-rolled copies in `learndash-core` use, but +it is di52-only: `stellarwp/container-contract` declares `bind`, `get`, `has` and `singleton`, and +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 +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 +container-bound collaborator** (`is_enabled()`, `is_already_loaded()`, `has_standalone_plugin()`, +`get_conflict_policy()`, …). Note that this is not the same as "config alone": `is_already_loaded()` +reads the global constant table and `is_enabled()` may invoke a host callable that queries anything +it likes. The line is about *dependency direction* — anything needing `Plugin_Checker_Interface` or +the notice queue would drag a container resolution into `Loader::register()`, which deliberately +resolves nothing so the container can arrive at any point before boot. So `Sub_Plugin` only *names* +the plugin to ask about, and the collaborator does the asking. ### What exists today -`Loader`, `Conflict\Resolver` and `Activation` are not built yet. Currently: +`src/Conflict/` — `Resolver`, `Gatekeeper`, `Redirector` — and `Activator` are not built yet. +Currently: | Path | What | |---|---| -| `src/Config.php` | Static facade: hook prefix + optional container. | -| `src/Sub_Plugin.php` | Value object; validates config and answers everything config alone decides. | +| `src/Config.php` | Static facade: hook prefix + container. | +| `src/Loader.php` | Static facade: the registration buffer, `boot()`, and the accessors. | +| `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. | +| `src/Sub_Plugin.php` | Value object; validates config and answers what it can without a container-bound collaborator. | | `src/Conflict_Policy.php` | The three policy constants, `default()`, `is_valid()`. | -| `src/Plugin_State.php` | The only file that touches WordPress plugin functions. | +| `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/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/` | `Plugin_State_Interface`, `Registrar_Interface`, `Config_Exception`. | +| `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Config_Exception`. | ### Boot lifecycle ``` Config::set_hook_prefix( 'give' ); -Config::set_container( $container ); // optional +Config::set_container( $container ); // required Loader::register( [ …config… ] ); // once per sub-plugin; a duplicate slug throws Loader::boot(); // idempotent + → Provider::register() // every binding + → Boot\Scheduler // every hook, as a closure over the container -plugins_loaded @1 → Conflict\Resolver::resolve_all() -plugins_loaded @2 → Loader::load_all() -all_admin_notices → Loader::render_notices() [is_admin() only] +plugins_loaded @1 → Conflict\Resolver::resolve_all() [gated by Conflict\Gatekeeper] +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] ``` -`load_all()` gates each sub-plugin in order, skipping on the first failure: enabled → dependencies -met → not already loaded → file exists → `should_load` filter → `require_once` → activation +**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 `Loader::register()` buffers +and resolves nothing — registration at plugin-file scope, which the spec sanctions, would otherwise +register into the throwaway. + +`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). +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 +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. + `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. -`redirect_destination()` returns `false` when the referrer is already `plugins.php`, so an inline -update is never interrupted. +`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. + +**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. 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 @@ -199,7 +280,8 @@ treatment. Any older sketch showing `Config::reset()` or `Loader::reset()` means - **Asserting a call did *not* happen needs a working recorder.** Record calls into an array, assert it is empty, then invoke the stubbed function once and assert the recorder caught it — otherwise a hook that failed to install passes the test for the wrong reason. -- **Container tests must use `Tests\Support\Test_Container`.** `lucatume\DI52\Container` implements +- **Every test that touches a collaborator sets a container**, since there is no longer a fallback to + fall back to — and it must be `Tests\Support\Test_Container`. `lucatume\DI52\Container` implements PSR-11's `ContainerInterface`, not StellarWP's, so passing it to `Config::set_container()` is a `TypeError`. - **Each load-path test writes its own bundled fixture file.** `require_once` caches by resolved @@ -247,10 +329,33 @@ treatment. Any older sketch showing `Config::reset()` or `Loader::reset()` means - **`deactivate_plugins()` is called silent, with no `$network_wide` argument.** Silent because a `flush_rewrite_rules()` in the standalone's deactivation hook at `plugins_loaded` 404s the site. The `null` default takes both the network and blog branches; a computed `true` strands an entry. -- **`Plugin_State::load_plugin_functions()` guards on `deactivate_plugins()`**, not - `is_plugin_active()` — the latter is a common third-party shim. +- **`Traits\Loads_Plugin_Functions` guards on `deactivate_plugins()`**, not `is_plugin_active()` — + the latter is a common third-party shim. - **Strauss must not rewrite `plugin_loaded_constant` values.** They are shared runtime constants; prefixing them defeats the entire mechanism. +- **Never write a literal guard-constant *name* in `src/`.** Hosts run Strauss with a + `constant_prefix` — `learndash-core` uses `LEARNDASH_`, with an empty exclude list — so a literal + `'GIVE_RECURRING_VERSION'` in our source is rewritten at build time and the `defined()` check then + matches nothing, silently, on the one path whose whole job is preventing a fatal. A constant name + arriving as a config *value* is a string in the host's array and is safe, which is why the design + holds: the library only ever receives these names, never spells one. +- **Collaborators come from the container; value objects and exceptions are constructed inline.** No + `?Peer $peer = null` constructor parameter and no `?? new X()` fallback — a nullable dependency + instantiating its own default is a service locator wearing a constructor signature, and it is what + made every default class owe a no-argument constructor. `new Sub_Plugin( $config )` in + `Loader::register()` and `throw new Config_Exception( … )` stay: one value object per host-supplied + array, built in the call the host can see in its own stack trace, which is what lets registration + validate eagerly while resolving nothing. A `Sub_Plugin_Factory` would put a container resolution + in front of the one method that deliberately performs none. +- **Naming: the suffix names the collaborator's role in the wiring, not whether it causes a side + effect.** A class that returns a URL from a filter still earns `-or` — `Redirector` alongside + `Registrar` and `Granter`, none of which need to perform anything for the name to be right. A class + stays bare while nothing in its folder does the same job (`Conflict\Resolver`, not + `Conflict\Standalone_Resolver`, even once `Gatekeeper` and `Redirector` sit beside it) and takes a + qualifier only when a second class of the same kind lands — `Scheduler` beside a later + `Retry_Scheduler`, never a qualifier bought in advance. An abstract + `-ion`/`-ance` noun is a directory name over agent nouns — `Activation/` holding `Activator` — and + never a class name; a census of 3,343 classes across eight Nexcess/StellarWP codebases found zero. ## Branch and PR workflow @@ -261,19 +366,27 @@ branch exists. `main` is releasable after every merge. - **PR size cap:** ≤10 files, tests and test infrastructure excluded. No logic-bearing PR exceeds 4 source files. - **Commits: no co-author trailers, ever.** -- **PR body is exactly four parts, nothing else** — no boilerplate headings, no restating the diff, - no checklists: +- **PR body is exactly three parts, nothing else** — no boilerplate headings, no restating the diff, + no checklists, and no "Verify" section: the commands are in this file and the coverage is in the + diff, so restating them per PR is filler a reviewer learns to scroll past. ``` - What: one line. + What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. - Why this way: the trade-off taken, and against what. + Why this way: - Verify: the command, and what is deliberately not covered. + **The claim, in bold.** One or two sentences: the trade-off taken, and against what. + + **The next claim.** Same again. ``` + `What` is one line but not a narrow one — a PR that wires two hooks names both, or the second goes + unreviewed. `Why this way` is one bold-led block per decision, never a single paragraph running + several arguments together: a reviewer reads the bold leads, stops at the one they doubt, and the + rest costs them nothing. Cut the connective throat-clearing between claims, never the claims. + - The README grows section-by-section with each PR so it is never ahead of what has shipped. Target ~120 lines: if it runs materially longer, cut rather than add. - New dev-only files belong in `.gitattributes` as `export-ignore` so they stay out of consumer @@ -286,7 +399,11 @@ branch exists. `main` is releasable after every merge. still carrying the old `stellarwp/sub-plugin-loader` package name, the `Nexcess\SubPluginLoader\` namespace, a `Config::set_version()` that was removed, and an `ob_start()` approach replaced by the `wp_admin_notice_markup` filter. `docs/superpowers/plans/2026-07-31-plugin-absorber.md` holds the -task-by-task breakdown. +task-by-task breakdown, and +`docs/superpowers/plans/2026-08-12-container-required-rework.md` supersedes it wherever the two +disagree about the container, the collaborator seams or the class names — that plan is what branches +11 through 16 now implement, and the older plan still describes the optional-container design it +replaced. Once a task's PR merges to `main`, delete that task's section from the plan in the next branch that touches the file; git history keeps it. A shipped task's plan describes code that already exists in diff --git a/README.md b/README.md index 18d0bc0..7a9ac63 100644 --- a/README.md +++ b/README.md @@ -19,22 +19,36 @@ plugins shipping different versions of this library will collide otherwise. See ```php use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Loader; -Config::set_hook_prefix( 'give' ); // required — keys the hooks and options -Config::set_container( give()->container ); // optional — lets you rebind collaborators -``` +add_action( 'plugins_loaded', function () { + Config::set_hook_prefix( 'give' ); // required — keys the hooks and options + Config::set_container( give()->container ); // required — every collaborator resolves from it -Each sub-plugin is then described by a config array: + Loader::register( [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => __DIR__ . '/sub-plugins/recurring/give-recurring.php', + 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION', + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', + ] ); -```php -[ - 'slug' => 'give-stripe', - 'bundled_plugin_file' => __DIR__ . '/sub-plugins/give-stripe/give-stripe.php', - 'plugin_loaded_constant' => 'GIVE_STRIPE_VERSION', - 'standalone_plugin_basename' => 'give-stripe/give-stripe.php', -] + Loader::boot(); +}, 0 ); ``` +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. + +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. + ## Docs - [Installing](docs/installing.md) — Composer, Strauss, and the constants Strauss must leave alone. diff --git a/docs/configuration.md b/docs/configuration.md index 2922713..01cc8ac 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -6,7 +6,7 @@ use Nexcess\PluginAbsorber\Config; Config::set_hook_prefix( 'give' ); // required — keys hooks and options -Config::set_container( give()->container ); // optional — lets you rebind collaborators +Config::set_container( give()->container ); // required — every collaborator resolves from it ``` The hook prefix accepts letters, numbers, hyphens, and underscores. Anything else throws @@ -14,35 +14,55 @@ The hook prefix accepts letters, numbers, hyphens, and underscores. Anything els option names lowercase it and turn hyphens into underscores, so `Give-Core` hooks `Give-Core/plugin_absorber/should_load` and stores `give_core_plugin_absorber_notices`. -The container is optional. Without one, the library instantiates its own collaborators; with one, -a host can rebind them. +## The container + +Both calls are required, and both belong at `plugins_loaded` priority 0, in your own container +block rather than in a service provider. + +Any implementation of StellarWP's `ContainerInterface` will do — the one your plugin already hands +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. ## Rebinding a collaborator -Every collaborator is interface-backed. With a container set, bind one to override the library -globally; with no container, the defaults are used and nothing is required. +`Loader::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: ```php use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; $container->singleton( Registrar_Interface::class, My_Registrar::class ); -Config::set_container( $container ); ``` | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | +| `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. | -`set_container()` is a configuration call like `set_hook_prefix()`, and order does not matter: 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 until the first read. +`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. + +`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 +until the first read. A binding that does not implement the interface it is bound to throws `Config_Exception` when it is resolved, rather than being cached and failing later somewhere less obvious. So does a binding whose factory throws — with the original failure kept as the previous exception. -The container is **not** used to wire hooks — those stay plain static callbacks, so the container -stays genuinely optional. +The container is **not** used to wire hooks. Those are closures that resolve when they fire, so +registering them instantiates nothing and a request that triggers none builds none. ## Sub-plugin keys @@ -70,11 +90,30 @@ 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. Because registrations are buffered until boot, that collision is reported at boot rather than -at the second `register()` call; a config array the library cannot use is still rejected on the spot. +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. 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. +## 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 +inside a method. Variables assigned at the top level of the bundled file are therefore function-local +and do not become globals: + +```php +// In the bundled plugin's main file. +$my_plugin = new My_Plugin(); // Not a global. `global $my_plugin;` elsewhere sees null. +$GLOBALS['my_plugin'] = new My_Plugin(); // Works. +``` + +Everything else — function and class declarations, `define()`, hook registration, `__FILE__` — is +unaffected. Bundle a plugin that publishes its instance through `$GLOBALS`, a singleton or a +container, which is what plugins written in the last decade do anyway. No amount of wrapping on this +side can hand a required file the global scope it would have had. + ## Messages are callables, never strings Your config array is built at plugin load — before `init`, and before your textdomain. Calling diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index d56bca9..43cd75b 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -18,9 +18,28 @@ Set one per sub-plugin with the `conflict_policy` key, or decide it at runtime w Before loading a bundled plugin, the library checks whether `plugin_loaded_constant` is already defined. `defined()` ⇒ skip, which is what prevents the re-declaration fatal. -**The constant must be defined at file scope.** A standalone that defines it from a bootstrap -hooked at `plugins_loaded` or later has not defined it yet at the moment the guard is read, and the -bundled copy would load on top of it. +**The constant must be defined at file scope**, inside a `defined()` check so whichever copy loads +first wins: + +```php +if ( ! defined( 'GIVE_RECURRING_VERSION' ) ) { + define( 'GIVE_RECURRING_VERSION', '2.4.0' ); +} +``` + +A standalone that defines it from a bootstrap hooked at `plugins_loaded` or later has not defined it +yet at the moment the guard is read, and the bundled copy would load on top of it. + +## What is deliberately out of scope + +**Version negotiation.** The library never compares versions, so it will not spare a standalone that +is newer than the bundled copy. Express that yourself: check the version and return +`Conflict_Policy::DEFER` from the `conflict_policy` [filter](filters.md), which has the final say. + +**Renamed standalone directories.** `standalone_plugin_basename` is the path as installed. A site +that renamed the standalone's directory is not detected, and there is no fallback that derives the +path from the load guard: one key is the guard and the other is the path, and no constant does both +jobs. The cost is a missed detection; the alternative costs the guarantee the guard exists for. ## What the guard cannot do diff --git a/docs/filters.md b/docs/filters.md index e081f58..4063f16 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -13,3 +13,25 @@ asked for rather than when the sub-plugin is registered, they are also the place by then the textdomain is loaded. A filter returning a non-scalar yields an empty string rather than a fatal cast. + +## The load gate + +| Filter | Arguments | Purpose | +|---|---|---| +| `{prefix}/plugin_absorber/should_load` | `bool $should_load`, `Sub_Plugin $sub_plugin` | Last word before `require_once`. | + +```php +add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_plugin ) { + return $sub_plugin->get_slug() === 'give-recurring' ? false : $should_load; +}, 10, 2 ); +``` + +It is consulted only for a sub-plugin that would otherwise have loaded — after the enabled check, +the guard constant, the dependency check and the file check, in that order. So returning `true` +cannot force a load past the guard constant: nothing overrides that. Anything other than a truthy +return skips the load, which is the safe direction. + +**Watch the polarity when you wire an existing gate to this one.** `should_load` is true means *do +load*. A host filter named for the opposite — LearnDash's `learndash_module_{x}_disabled`, where true +means *do not load* — inverts the gate if it is passed through unnegated, and the failure is silent +in the direction that loads a plugin the site turned off. diff --git a/docs/notices.md b/docs/notices.md index b91d438..304f11b 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -30,7 +30,36 @@ example — and the messages may contain markup; the default rendering passes th stripped. Paragraphs come from `wpautop()`, so send the message unwrapped and let a blank line break it — a `

` of your own is left as it is rather than nested inside another. +```php +use Nexcess\PluginAbsorber\Notices\Queue; + +add_action( 'admin_init', function () { + // Gates the read, not just the delete: `admin_init` fires for every logged-in user, and + // draining the queue for one who cannot act on it destroys the only warning an + // administrator was going to get. + if ( ! current_user_can( 'activate_plugins' ) ) { + return; + } + + $notices = get_site_option( Queue::option_name(), [] ); + + if ( ! is_array( $notices ) || ! $notices ) { + return; + } + + foreach ( $notices as $key => $message ) { + my_plugin_enqueue_notice( $key, $message ); + } + + delete_site_option( Queue::option_name() ); +} ); +``` + +`admin_init` runs before `all_admin_notices`, where the built-in rendering happens, so deleting the +option there leaves ours nothing to draw and the notice is shown once, by you. Do the deleting: a +notice read and not cleared is shown on every request forever. + The queue is three classes: `Notices\Queue` decides what a notice says and who may consume it, -`Notices\Store` keeps it, `Notices\Renderer` draws it. Both collaborators are constructor arguments, -so `new Queue( null, $renderer )` keeps the queue and replaces only the markup, and -`new Queue( $store )` does the reverse. Replacing either one leaves the other alone. +`Notices\Store` keeps it, `Notices\Renderer` draws it. `Queue` takes both as constructor arguments +and all three are bound in the container, so rebinding `Notices\Renderer` replaces the markup and +leaves the storage alone, and rebinding `Notices\Store` does the reverse. diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index e1523d3..56e287f 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -20,16 +20,19 @@ Every task's requirements implicitly include this section. - **Storage keys:** option `"{$option_prefix}_plugin_absorber_activations"`, option `"{$option_prefix}_plugin_absorber_notices"`. Both are assembled by `Config::get_option_name( string $name )` and nowhere else, alongside `Config::get_hook_name()` for filters. **Amended 2026-08-11:** `{$option_prefix}` is the hook prefix lowercased with hyphens folded to underscores, so `Give-Core` yields the option `give_core_plugin_absorber_notices` while still yielding the filter `Give-Core/plugin_absorber/should_load` — the prefix validator admits `A-Z` and `-`, and a hook-naming value should not reach a storage key verbatim. The two normalisations stay separate: folding case into the hook side would silently rename the host's own filters. **Amended 2026-08-03 (PR 10 review):** the notice queue was specified as a *transient* and is now an option. `set_transient()` returns before touching the database whenever an external object cache is present, so on any Redis or Memcached site the queue would live only in the cache — where a routine `wp_cache_flush()` from a deploy script or a "purge cache" button destroys it. The merge notice is raised exactly once and never re-queued, so losing it means a site owner is never told their plugin was deactivated. On multisite both this and the activation option are network options, because the resolver deactivates network-wide. - **Production dependencies:** `stellarwp/container-contract` only. `lucatume/di52` is dev-only. No other StellarWP library. - **PR size cap:** ≤10 files per PR, tests and test infrastructure excluded. No logic-bearing PR exceeds 4 source files. -- **PR body format** — exactly four parts, nothing else. No boilerplate headings, no restating the diff, no checklists: +- **PR body format** — exactly three parts, nothing else. No boilerplate headings, no restating the diff, no checklists, and no `Verify` section (dropped 2026-08-12: the commands live in `CLAUDE.md` and the coverage is in the diff, so restating them per PR is filler a reviewer learns to scroll past): ``` - What: one line. + What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. - Why this way: the trade-off taken, and against what. + Why this way: - Verify: the command, and what is deliberately not covered. + **The claim, in bold.** One or two sentences: the trade-off taken, and against what. + + **The next claim.** Same again. ``` + `Why this way` is one bold-led block per decision, never a single paragraph running several arguments together — a reviewer reads the bold leads and stops at the one they doubt. Cut the connective throat-clearing between claims, never the claims. - **Branching:** stacked. Each branch cuts from the previous branch, and merges to `main` in order. Never open PR N+1 before PR N's branch exists. - **Commits:** no co-author trailers, ever. - **Every source file** carries a file-level docblock with `@package Nexcess\PluginAbsorber` and every method a docblock with `@since 1.0.0`. This binds `src/` only. Test classes and test support classes keep the file-level docblock, but their methods do not need `@since` — the test code in this plan's own tasks is written that way deliberately (ruled 2026-07-31). @@ -659,24 +662,39 @@ that throws, a binding of the wrong type, and the registration order cases. Not ## Task 11: `Loader` boot and load path -**PR 11** · branch `11-loader-load-path` from `10-notices-queue` · 2 source files +**PR 11** · branch `11-loader-load-path` from `10-notices-queue` · 1 source file **Files:** -- Modify: `src/Loader.php`, `tests/_support/Loader_State.php` (the new `$booted` property needs a default), `README.md` -- Create: `tests/unit/LoaderLoadTest.php`, `tests/unit/LoaderBootTest.php` +- Modify: `src/Loader.php`, `tests/_support/Loader_State.php` (the new `$booted` property needs a + default, and the hooks `boot()` added need unwiring), `README.md`, `docs/filters.md`, + `docs/configuration.md`, `docs/conflict-handling.md` +- Create: `tests/unit/LoaderBootTest.php`, `tests/unit/LoaderLoadTest.php` **Interfaces:** -- Consumes: `Sub_Plugin` predicates (Task 7), `Loader::notices()` (Task 10), `Config::get_hook_name()` (Task 4). +- Consumes: `Sub_Plugin` predicates (Task 7), `Loader::all()` and its buffer drain (Task 9), + `Loader::notices()` → `Notices\Contracts\Queue_Interface`, default `Notices\Queue` (Task 10), + `Config::get_hook_name()` and `Config::get_hook_prefix()` (Task 4). - Produces: - `Loader::boot(): void` — idempotent - `Loader::load_all(): void` - `Loader::render_notices(): void` - `Loader::load( Sub_Plugin ): void` — private + - `Loader::wiring_window_has_closed(): bool` — private + - `Loader::has_hook_prefix(): bool` — private + - `Loader::LOAD_PRIORITY` — private const, the `plugins_loaded` priority the load loop runs at - the `"{$prefix}/plugin_absorber/should_load"` filter, args `(bool $should_load, Sub_Plugin $sub_plugin)` Task 12 adds the `plugins_loaded` @1 hook to `boot()`; Task 13 adds the activation call to `load()`. -**Design note:** `boot()` wires only the @2 load hook and `all_admin_notices` in this PR. The @1 conflict-resolution hook arrives in Task 12 with the resolver it delegates to — wiring a trampoline to a collaborator that does not exist yet would not run. +**Design note:** `boot()` wires only the @2 load hook and `all_admin_notices` in this PR. The @1 +conflict-resolution hook arrives in Task 12 with the resolver it delegates to — wiring a trampoline +to a collaborator that does not exist yet would not run. + +**The boot barrier needs no flush of its own.** `register()` buffers and `all()` flushes, which +landed in Task 9, so `load_all()` just iterates `self::all()` and the drain happens transparently on +the first read. That read is at `plugins_loaded` priority 2, which is after the host's own bootstrap +at priority 0 — so the container is set before anything resolves, and a registration made before +`Config::set_container()` still reaches the bound registrar. - [ ] **Step 1: Cut the branch** @@ -684,264 +702,46 @@ that throws, a binding of the wrong type, and the registration order cases. Not git checkout 10-notices-queue && git checkout -b 11-loader-load-path ``` -- [ ] **Step 2: Write the failing load-path test** - -Each test writes its own fixture file. `require_once` caches by resolved path for the whole PHP process, so a shared fixture would make the second test in the run silently pass. - -```php - - */ - private $fixtures = []; - - public function setUp(): void { - parent::setUp(); - - Config::set_hook_prefix( 'give' ); - $GLOBALS['absorber_loads'] = 0; - } - - public function tearDown(): void { - foreach ( $this->fixtures as $fixture ) { - if ( file_exists( $fixture ) ) { - unlink( $fixture ); - } - } - $this->fixtures = []; - - unset( $GLOBALS['absorber_loads'] ); - delete_transient( 'give_plugin_absorber_notices' ); - Loader_State::reset(); - Config::reset(); - parent::tearDown(); - } - - /** - * Write a throwaway bundled plugin that counts its own loads and defines its guard constant. - * - * A unique path per test is required: require_once caches by resolved path for the lifetime of - * the PHP process, so a shared fixture would make later tests pass without loading anything. - * - * @param string $constant Guard constant to define. - */ - private function make_fixture( string $constant ): string { - $path = sys_get_temp_dir() . '/absorber-' . uniqid( '', true ) . '.php'; - - file_put_contents( - $path, - 'fixtures[] = $path; - - return $path; - } - - /** - * @param array $overrides Config overrides. - */ - private function register( array $overrides = [], ?string $constant = null ): string { - $constant = $constant ?? 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); - $path = $this->make_fixture( $constant ); - - Loader::register( - array_merge( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => $constant, - ], - $overrides - ) - ); - - return $constant; - } - - public function test_it_requires_the_bundled_file(): void { - $constant = $this->register(); - - Loader::load_all(); - - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); - $this->assertTrue( defined( $constant ) ); - } - - public function test_it_requires_the_bundled_file_exactly_once(): void { - $this->register(); - - Loader::load_all(); - Loader::load_all(); - - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); - } - - public function test_it_skips_a_disabled_sub_plugin(): void { - $this->register( [ 'enabled' => false ] ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): void { - $this->register( [ 'dependency_check' => static fn() => false ] ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - $this->assertArrayHasKey( - 'give-recurring:dependency', - get_transient( 'give_plugin_absorber_notices' ) - ); - } - - public function test_it_skips_when_the_guard_constant_is_already_defined(): void { - define( 'ABSORBER_ALREADY_LOADED_GUARD', '1.0.0' ); - - $this->register( [], 'ABSORBER_ALREADY_LOADED_GUARD' ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A defined constant means the code is already present.' ); - } - - public function test_it_skips_when_the_bundled_file_is_missing(): void { - Loader::register( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', - 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_GUARD', - ] - ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_the_should_load_filter_can_veto_the_load(): void { - $this->register(); - - add_filter( 'give/plugin_absorber/should_load', '__return_false' ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_the_should_load_filter_receives_the_sub_plugin(): void { - $this->register(); - - $received = null; - add_filter( - 'give/plugin_absorber/should_load', - static function ( $should_load, $sub_plugin ) use ( &$received ) { - $received = $sub_plugin; - - return $should_load; - }, - 10, - 2 - ); - - Loader::load_all(); - - $this->assertInstanceOf( \Nexcess\PluginAbsorber\Sub_Plugin::class, $received ); - $this->assertSame( 'give-recurring', $received->get_slug() ); - } - - public function test_it_loads_every_registered_sub_plugin(): void { - $this->register( [ 'slug' => 'give-recurring' ] ); - $this->register( [ 'slug' => 'give-fee-recovery' ] ); - - Loader::load_all(); - - $this->assertSame( 2, $GLOBALS['absorber_loads'] ); - } -} -``` - -- [ ] **Step 3: Write the failing boot test** - -```php -assertSame( - 2, - has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) - ); - } - - public function test_booting_twice_wires_the_hook_only_once(): void { - Loader::boot(); - Loader::boot(); - - $callbacks = $GLOBALS['wp_filter']['plugins_loaded']->callbacks[2] ?? []; - - $this->assertCount( 1, $callbacks, 'boot() must be idempotent.' ); - } - - public function test_it_wires_the_admin_notices_hook_in_the_admin(): void { - set_current_screen( 'dashboard' ); - - Loader::boot(); - - $this->assertNotFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); - - set_current_screen( 'front' ); - } -} -``` +- [ ] **Step 2: Write the failing boot test** + +`tests/unit/LoaderBootTest.php`, a `WPTestCase` that calls `Loader_State::reset()` and +`Config_State::reset()` in `tearDown()`. Behaviours to cover: + +- `boot()` wires `load_all` to `plugins_loaded` at `LOAD_PRIORITY`, and `render_notices` to + `all_admin_notices` — the latter only under `is_admin()`, so both branches need a case. Asserting + the front-end branch does *not* wire needs a working recorder, per the testing rules: assert the + hook is absent, then wire it by hand and assert the same read finds it. +- Calling `boot()` twice wires each hook exactly once — read + `$GLOBALS['wp_filter']['plugins_loaded']->callbacks` at the priority and count, since `has_action()` + cannot tell one callback from two. +- Booting too late is reported and recovered from. With `plugins_loaded` already dispatched, or + dispatching at a priority at or past `LOAD_PRIORITY`, `boot()` triggers `_doing_it_wrong()` and + loads inline instead of wiring a hook that would never fire. The inclusive comparison — booting + *from* `plugins_loaded` @2 — is its own case, because that is the near miss a host actually hits. +- Booting from `plugins_loaded` at a priority *before* `LOAD_PRIORITY` still wires normally. + +- [ ] **Step 3: Write the failing load-path test** + +`tests/unit/LoaderLoadTest.php`. Each test writes its own fixture file: `require_once` caches by +resolved path for the whole PHP process, so a shared fixture would make the second test in the run +silently pass. A fixture counts its own loads into a global and defines its guard constant inside a +`defined()` check; `tearDown()` unlinks the fixtures, drops the global, deletes the notices option +and resets both facades. Behaviours to cover: + +- A registered, enabled sub-plugin with a readable file is required exactly once, and its guard + constant is defined afterwards. Calling `load_all()` twice still loads once. +- Every registered sub-plugin loads, in registration order. +- Each gate skips: disabled; guard constant already defined; dependencies unmet — which also queues + the dependency notice through `Loader::notices()`; bundled file missing, unreadable, or a + directory, which reports through `_doing_it_wrong()` and queues *nothing*. +- The `should_load` filter is applied under the name `Config::get_hook_name( 'should_load' )`, + receives the `Sub_Plugin` as its second argument, and vetoes the load when it returns falsy. +- The gate order is itself asserted: with the guard constant defined *and* an unmet dependency, no + dependency notice is queued — the already-loaded check runs first. +- `load_all()` and `render_notices()` with no hook prefix set report through `_doing_it_wrong()` and + return, rather than throwing out of a core action. +- A registrar bound in the container that returns a non-`Sub_Plugin` entry is skipped rather than + fataling. - [ ] **Step 4: Run both to verify they fail** @@ -950,16 +750,30 @@ Expected: FAIL — `Call to undefined method Nexcess\PluginAbsorber\Loader::boot - [ ] **Step 5: Add boot and the load path to `src/Loader.php`** -Add the `$booted` property beside `$resolved`: +Add the priority constant and the `$booted` property beside `$resolved` and `$pending`: ```php /** + * plugins_loaded priority the load loop 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. + * + * @since 1.0.0 + * + * @var int + */ + private const LOAD_PRIORITY = 2; + + /** + * Whether the hooks have been wired. + * * @var bool */ private static $booted = false; ``` -Append these methods: +Then the public methods: ```php /** @@ -980,8 +794,6 @@ Append these methods: self::$booted = true; - add_action( 'plugins_loaded', [ self::class, 'load_all' ], 2 ); - if ( is_admin() ) { // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, // network_admin_notices and user_admin_notices as mutually exclusive branches, so a @@ -989,6 +801,24 @@ Append these methods: // deactivation gets noticed -- would never see the queue rendered. add_action( 'all_admin_notices', [ self::class, 'render_notices' ] ); } + + // 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. + if ( self::wiring_window_has_closed() ) { + _doing_it_wrong( + __METHOD__, + 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + '1.0.0' + ); + + self::load_all(); + + return; + } + + add_action( 'plugins_loaded', [ self::class, 'load_all' ], self::LOAD_PRIORITY ); } /** @@ -997,6 +827,13 @@ Append these methods: * @return void */ public static function load_all(): void { + // The load path needs the prefix for the should_load filter and for the notice store. + // Throwing out of a core action would take the whole site down over a bootstrap mistake, + // so it is reported where a developer will see it and the load is abandoned instead. + if ( ! self::has_hook_prefix() ) { + return; + } + foreach ( self::all() as $sub_plugin ) { // Registrar_Interface::all() only declares `array`. A host binding its own registrar // that returns anything else would otherwise fatal inside plugins_loaded on the first @@ -1015,16 +852,27 @@ Append these methods: * @return void */ public static function render_notices(): void { + if ( ! self::has_hook_prefix() ) { + return; + } + self::notices()->render(); } +``` + +And the private ones, below `flush()` and `resolve()` — public, then private, and no helper above +the API it serves: +```php /** - * Load one sub-plugin, in the order the checks are cheapest and most decisive. + * Load one sub-plugin, cheapest and most decisive check first. * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin to load. * + * @throws Config_Exception When a collaborator binding is unusable. + * * @return void */ private static function load( Sub_Plugin $sub_plugin ): void { @@ -1032,131 +880,253 @@ Append these methods: return; } + // Ahead of the dependency check, which calls an arbitrary host callable. This is one + // defined(), it carries the whole re-declaration guarantee, and it is the only gate that + // means "the plugin is already running" -- warning that requirements are unmet for a + // plugin the admin can see working would be worse than useless. + if ( $sub_plugin->is_already_loaded() ) { + return; + } + if ( ! $sub_plugin->are_dependencies_met() ) { self::notices()->queue_dependency_notice( $sub_plugin ); return; } - // The constant is defined => the code is already present, from either copy. Loading the - // bundled file now would be a re-declaration fatal. - if ( $sub_plugin->is_already_loaded() ) { - return; - } + // Not file_exists(): that is true for a directory and for a file with no read permission, + // and require_once fatals on both. A missing file is a broken build in the host plugin + // rather than anything a site owner can act on, so it goes to the developer instead of + // into the notice queue, where it would have displayed the host's own + // dependency_notice_message and sent the owner after the wrong problem entirely. + $file = $sub_plugin->get_bundled_plugin_file(); + + if ( ! is_file( $file ) || ! is_readable( $file ) ) { + _doing_it_wrong( + 'Nexcess\PluginAbsorber\Loader', + sprintf( + 'The bundled plugin file for "%s" is missing or unreadable: %s', + $sub_plugin->get_slug(), + $file + ), + '1.0.0' + ); - if ( ! file_exists( $sub_plugin->get_bundled_plugin_file() ) ) { return; } - $should_load = apply_filters( - Config::get_hook_name( 'should_load' ), - true, - $sub_plugin - ); + // No type guard on the return, unlike the conflict_policy filter: there is no cast here, + // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the + // load, which is the safe direction. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); if ( ! $should_load ) { return; } - require_once $sub_plugin->get_bundled_plugin_file(); + // An include takes the scope of the line it sits on, and this one is inside a method, where + // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled + // 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; + } + + /** + * Whether it is already too late to wire the load hook. + * + * 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. + * + * @since 1.0.0 + * + * @return bool + */ + private static function wiring_window_has_closed(): bool { + if ( ! did_action( 'plugins_loaded' ) ) { + return false; + } + + if ( ! doing_action( 'plugins_loaded' ) ) { + return true; + } + + $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; + + return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + } + + /** + * Whether a hook prefix has been set, reporting to the developer when it has not. + * + * @since 1.0.0 + * + * @return bool + */ + private static function has_hook_prefix(): bool { + try { + Config::get_hook_prefix(); + } catch ( Config_Exception $exception ) { + _doing_it_wrong( 'Nexcess\PluginAbsorber\Loader', $exception->getMessage(), '1.0.0' ); + + return false; + } + + return true; } ``` -- [ ] **Step 6: Teach `tests/_support/Loader_State.php` about the boot flag** +The filter name is built with `Config::get_hook_name( 'should_load' )` and never by concatenating +`Config::get_hook_prefix()` with the rest: `Config` owns the segment between the host's prefix and +the key's own name, and nothing else assembles it. -`Loader_State::reset()` walks `Loader`'s static properties and refuses one it has no default for, -so until `$booted` is listed every test that resets throws a `LogicException` naming it. That is -the helper doing its job: a boot flag left standing would wire the hooks once and then let every -later test's `boot()` no-op. +- [ ] **Step 6: Teach `tests/_support/Loader_State.php` about the boot flag and the hooks** + +`Loader_State::reset()` walks `Loader`'s static properties and refuses one it has no default for, so +until `$booted` is listed every test that resets throws a `LogicException` naming it. That is the +helper doing its job: a boot flag left standing would wire the hooks once and then let every later +test's `boot()` no-op. + +Clearing the flag is not enough on its own. A `Loader` that reports itself unbooted while its +callbacks are still attached is the worse of the two states: the next `boot()` wires nothing and +still looks like it worked, and the stranded callback goes on loading sub-plugins into tests that +never registered any. So the helper unwires both hooks as well — and reads `LOAD_PRIORITY` off the +class by reflection rather than restating `2`, so it cannot go on removing a hook from a priority the +`Loader` no longer wires. ```php protected const DEFAULTS = [ 'resolved' => [], + 'pending' => [], 'booted' => false, ]; -``` -- [ ] **Step 7: Run the tests to verify they pass** + public static function reset(): void { + $reflection = new ReflectionClass( Loader::class ); -Run: `slic run unit` -Expected: PASS — 9 load tests + 3 boot tests. + // Read rather than restated, so the helper cannot go on removing a hook from a priority + // the Loader no longer wires -- which would leave the real callback attached and every + // later test loading sub-plugins it never registered. + $load_priority = $reflection->getConstant( 'LOAD_PRIORITY' ); -- [ ] **Step 8: Confirm static analysis is still clean** + remove_action( 'plugins_loaded', [ Loader::class, 'load_all' ], (int) $load_priority ); + remove_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); -Run: `composer test:analysis` + // ... the property walk from Task 9, unchanged. + } +``` -- [ ] **Step 9: Append to the README** +There is still no `Loader::reset()`, and nothing in this PR adds one. The seam stays in +`tests/_support/`. -```markdown -### Bootstrap +- [ ] **Step 7: Run the tests to verify they pass** -```php -use Nexcess\PluginAbsorber\Config; -use Nexcess\PluginAbsorber\Loader; +Run: `slic run unit`, then `slic run unit --env multisite`. -add_action( 'plugins_loaded', function () { - Config::set_hook_prefix( 'give' ); +- [ ] **Step 8: Confirm static analysis is still clean** - Loader::register( [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => GIVE_PLUGIN_DIR . 'subs/give-recurring/give-recurring.php', - 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION', - ] ); +Run: `composer test:analysis` +Expected: `[OK] No errors`. - Loader::boot(); -}, 0 ); // priority 0 — before the absorber's own @1 and @2 hooks fire -``` +- [ ] **Step 9: Document it, in four places and not in one** -The bundled file must define its guard constant inside a `defined()` check: +The human docs are split out of the README and are not to grow back, so each piece goes where its +subject already lives. The README gets only the bootstrap, short: -```php -if ( ! defined( 'GIVE_RECURRING_VERSION' ) ) { - define( 'GIVE_RECURRING_VERSION', '2.4.0' ); -} +```markdown +Loader::register( [ ... ] ); +Loader::boot(); ``` -### Load gate +wrapped in `add_action( 'plugins_loaded', ..., 0 )`, with a line on why the `, 0` matters: `boot()` +wires the load at priority 2, WordPress silently ignores a callback added at or past the priority it +is already dispatching, and booting later is reported and loaded inline but with weaker ordering +guarantees. -Applied immediately before `require_once`: +`docs/filters.md` gets the load gate — the filter's arguments, the `add_filter()` snippet, and the +order it sits in: consulted only for a sub-plugin that would otherwise have loaded, so returning +`true` cannot force a load past the guard constant. -```php -add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_plugin ) { - return $sub_plugin->get_slug() === 'give-recurring' ? false : $should_load; -}, 10, 2 ); -``` +`docs/configuration.md` gets two things: the `Notices\Contracts\Queue_Interface` → `Notices\Queue` +row in the collaborator table, and the global-scope caveat — top-level assignments in a bundled file +are function-local, `$GLOBALS['my_plugin'] = ...` works, and declarations, `define()`, hooks and +`__FILE__` are unaffected. -A sub-plugin is skipped when it is disabled, its dependencies are unmet, its guard constant is -already defined, its bundled file is missing, or this filter returns false. -``` +`docs/conflict-handling.md` gets the `defined()` snippet the bundled file must wrap its guard +constant in, next to the paragraph that already says the constant must be defined at file scope. + +> **Design notes.** +> +> *The already-loaded check moved ahead of the dependency check.* The first sketch had the two the +> other way round, cheapest-first. `is_already_loaded()` is one `defined()` and the dependency check +> calls an arbitrary host callable, so cheapest-first argues the same way — but the deciding reason +> is what each gate means. A defined guard constant means the plugin is running right now; telling a +> site owner its requirements are unmet, for a plugin they can see working, sends them after a +> problem that does not exist. +> +> *The file check is `is_file()` and `is_readable()`, not `file_exists()`.* `file_exists()` is true +> for a directory and for a file the process cannot read, and `require_once` fatals on both — which +> is the exact failure this library exists to prevent. A missing bundled file is a broken build in +> the host plugin, so it is reported through `_doing_it_wrong()` and queues nothing: the notice queue +> would have shown the host's own `dependency_notice_message` and sent a site owner after the wrong +> problem. +> +> *Booting too late is reported and recovered from, not ignored.* `add_action()` accepts a callback +> at a priority the current dispatch has already passed and then never fires it, so a host that boots +> from `plugins_loaded` at the default priority would load nothing at all, silently, on a site that +> looks healthy. `wiring_window_has_closed()` compares inclusively, because booting from +> `plugins_loaded` @2 is the near miss a host actually hits and an exclusive comparison lets exactly +> that one through unreported. The recovery is an inline `load_all()`: weaker ordering, but the +> sub-plugins load. +> +> *A missing hook prefix returns instead of throwing.* Both `load_all()` and `render_notices()` run +> from core actions, and an exception out of `plugins_loaded` takes the whole site down over a +> bootstrap mistake. `_doing_it_wrong()` puts it in front of the developer who made it and the load +> is abandoned. +> +> *`Loader` still has no `reset()`.* `boot()` adds a third piece of static state and the temptation +> with it, and the answer has not changed: a public reset is API the library supports forever, and a +> host that called it mid-request would drop the registrations the load loop is about to read. +> `Tests\Support\Loader_State::reset()` clears the memo, the buffer and the boot flag by reflection, +> and unwires the two hooks in the same pass — a reset that cleared the flag alone would leave a +> `Loader` that reports itself unbooted with its callbacks still attached. - [ ] **Step 10: Commit, push, open the PR** ```bash -git add src/Loader.php tests/_support/Loader_State.php tests/unit/LoaderLoadTest.php tests/unit/LoaderBootTest.php README.md +git add src/Loader.php tests/_support/Loader_State.php tests/unit/LoaderLoadTest.php tests/unit/LoaderBootTest.php README.md docs/filters.md docs/configuration.md docs/conflict-handling.md git commit -m "Add Loader boot and the load path" git push -u origin 11-loader-load-path gh pr create --base 10-notices-queue --title "Loader boot and load path" --body 'What: `boot()`, `load_all()`, the five-gate load path, and the `should_load` filter. Usage: - Loader::register( [ ... ] ); - Loader::boot(); // wires plugins_loaded @2 and all_admin_notices + add_action( "plugins_loaded", function () { + Config::set_hook_prefix( "give" ); + Loader::register( [ ... ] ); + Loader::boot(); // wires plugins_loaded @2 and all_admin_notices + }, 0 ); add_filter( "give/plugin_absorber/should_load", function ( $should_load, $sub_plugin ) { return $should_load; }, 10, 2 ); -Why this way: gate order is deliberate — `is_enabled()` and `are_dependencies_met()` are cheap -config checks, `is_already_loaded()` is the one that actually prevents the fatal, and the filter -runs last so a host override cannot accidentally re-introduce a re-declaration. The -already-loaded check sits before `file_exists()` because it is both cheaper and more important. +Why this way: the guard constant is checked before the dependency check rather than after it. A +defined constant means the plugin is running right now, and warning that requirements are unmet for +a plugin the admin can see working sends them after a problem that does not exist. The file check is +`is_file()` plus `is_readable()`, not `file_exists()`, which is true for a directory and for an +unreadable file and lets `require_once` fatal on both. Booting past `plugins_loaded` @2 is reported +through `_doing_it_wrong()` and loaded inline instead of wiring a hook that would never fire — +against silently loading nothing on a site that looks healthy. `boot()` wires only the @2 hook here; +the @1 conflict-resolution hook lands with the resolver it delegates to. -`boot()` wires only the @2 hook here; the @1 conflict-resolution hook lands with the resolver it -delegates to, since a trampoline pointing at a collaborator that does not exist yet would not run. - -Verify: `slic run unit` — 12 tests. Each writes its own fixture file, because `require_once` caches -by resolved path for the whole PHP process and a shared fixture would make later tests pass without -loading anything.' +Verify: `slic run unit` on both envs, and `composer test:analysis`. Covered: every gate and its +order, the boot window including the inclusive near miss at @2, and the missing-prefix path. Each +load test writes its own fixture, because `require_once` caches by resolved path for the whole PHP +process. Not covered here: conflict resolution and the activation callback, which land in Tasks 12 +and 13.' ``` --- @@ -1757,22 +1727,27 @@ Usage: : Conflict_Policy::DEACTIVATE; }, -Why this way: `deactivate_plugins()` receives `$network_wide` — a change from the engineering plan, -which detects network activation and then drops the flag. Without it the call silently no-ops -against a network-activated plugin, so every admin request deactivates nothing and redirects again. -That is an infinite redirect loop on multisite, and it is exactly what the plan is own E2E -criterion ("reloading the plugins page does not loop") was meant to catch. +Why this way: + +**`deactivate_plugins()` receives `$network_wide`** — a change from the engineering plan, which +detects network activation and then drops the flag. Without it the call silently no-ops against a +network-activated plugin, so every admin request deactivates nothing and redirects again: an +infinite redirect loop on multisite, and exactly what the plan is own E2E criterion ("reloading the +plugins page does not loop") was meant to catch. -`redirect_destination()` returns false on a plugins.php referrer so an inline update is never -interrupted, and rewrites update.php / update-core.php referrers so the user is not bounced back +**An unknown policy is its own case, never a `default:` fallthrough.** A typo like `defered` would +otherwise land on the deactivate branch and turn off a plugin the site owner deliberately enabled. + +**`redirect_destination()` returns false on a plugins.php referrer**, so an inline update is never +interrupted. update.php and update-core.php referrers are rewritten so the user is not bounced back into an update screen. -Known limitation, deliberate: `resolve_all()` runs on front-end requests too, matching both -reference implementations. Tracked as issue B in the spec. +**`exit` is never mocked.** The stubbed `wp_safe_redirect()` throws `TestException`, halting the +resolver exactly where production calls `exit` while leaving a failing test free to report as +failing. -Verify: `slic run unit` and `slic run unit --env multisite` — 15 tests. `exit` is never mocked: the -stubbed `wp_safe_redirect()` throws `TestException`, which halts the resolver exactly where -production calls `exit` while leaving a failing test free to report as failing.' +**Known limitation, deliberate:** `resolve_all()` runs on front-end requests too, matching both +reference implementations. Tracked as issue B in the spec.' ``` --- @@ -2125,7 +2100,8 @@ idempotent, version-gated migrations — not here. git add src/Activation.php src/Contracts/Activation_Interface.php src/Loader.php tests/unit/ActivationTest.php tests/unit/LoaderLoadTest.php README.md git commit -m "Add run-once activation tracking" git push -u origin 13-activation -gh pr create --base 12-conflict-resolver --title "Activation" --body 'What: run-once-ever activation tracking, wired into the load path. +gh pr create --base 12-conflict-resolver --title "Activation" --body 'What: `Activation` and `Activation_Interface`, reachable as `Loader::activation()`, wired into the +load path as the last step after a successful require. Usage: @@ -2133,19 +2109,20 @@ Usage: \Give\Recurring\Install::create_tables(); }, -Why this way: `register_activation_hook()` never fires for a `require_once`d file, so a plugin -absorbed into a host would never run its original install routine. One option holds a per-slug -flag, and the callback fires after a successful require — never when the load was skipped, which +Why this way: + +**`register_activation_hook()` never fires for a `require_once`d file**, so a plugin absorbed into +a host would never run its original install routine. One option holds a per-slug flag instead. + +**The callback fires only after a successful require** — never when the load was skipped, which would otherwise create tables for code that is not loaded. -A single option rather than one per slug keeps this to one autoloaded row no matter how many +**A single option rather than one per slug** keeps this to one autoloaded row no matter how many sub-plugins a host bundles. -Known limitation, deliberate: read-then-write is not atomic, so two simultaneous first requests can -both run the callback. Tracked as issue E in the spec; `add_option()` as a claim would close it. - -Verify: `slic run unit` — 11 tests, including a corrupted-option recovery and hook-prefix -namespacing.' +**Known limitation, deliberate:** read-then-write is not atomic, so two simultaneous first requests +can both run the callback. Tracked as issue E in the spec; `add_option()` as a claim would close +it.' ``` --- @@ -2500,29 +2477,28 @@ Requires WordPress 6.4+ for the `wp_admin_notice_markup` filter. git add src/Notices/Queue.php src/Notices/Contracts/Queue_Interface.php src/Loader.php tests/unit/Notices/QueueActivationErrorTest.php README.md git commit -m "Replace WordPress fatal-activation text for absorbed standalones" git push -u origin 14-activation-error-notice -gh pr create --base 13-activation --title "Activation-error rewrite" --body 'What: replaces WordPress generic "triggered a fatal error" notice with the sub-plugin own -explanation when a user re-activates an absorbed standalone. +gh pr create --base 13-activation --title "Activation-error rewrite" --body 'What: the `wp_admin_notice_markup` filter and its Loader trampoline, replacing WordPress generic +"triggered a fatal error" notice with the sub-plugin own explanation when a user re-activates an +absorbed standalone. Usage: "conflict_notice_message" => static fn() => __( "Now bundled with Give.", "give" ), -Why this way: a change from the engineering plan, which specified `ob_start()` on -`admin_head-plugins.php` copied from Kadence. The newer LearnDash reference uses the -`wp_admin_notice_markup` filter — same nonce check, same str_replace, but no output buffering, no -risk of mangling unrelated admin output, and directly unit-testable. The cost is a WordPress 6.4 -floor, which is when that filter landed. +Why this way: + +**The `wp_admin_notice_markup` filter, not `ob_start()`.** The engineering plan specified output +buffering on `admin_head-plugins.php`, copied from Kadence. The filter does the same nonce check +and the same str_replace with no buffering, no risk of mangling unrelated admin output, and is +directly unit-testable. The cost is the WordPress 6.4 floor, which is when the filter landed. -Three gates before touching anything: the plugins screen, a `plugin` parameter matching a +**Three gates before touching the markup:** the plugins screen, a `plugin` parameter matching a registered standalone basename, and a valid `plugin-activation-error_{basename}` nonce. Failing any one returns the markup untouched, as does having no configured message — better WordPress wording than none. -This adds a method to `Notices\Contracts\Queue_Interface`, which shipped in PR 10. Pre-1.0 with no -consumers. - -Verify: `slic run unit` — 8 tests, one per gate plus `wp_kses_post()` sanitising and the Loader -trampoline.' +**This adds a method to `Notices\Contracts\Queue_Interface`,** which shipped in PR 10. Pre-1.0 with +no consumers.' ``` --- @@ -2964,20 +2940,23 @@ gh pr create --base 14-activation-error-notice --title "End-to-end suite" --body Usage: `tests/_data/plugins/absorber-host/absorber-host.php` is the worked consumer example — register, set a policy, supply an activation callback, boot. -Why this way: these drive the real `active_plugins` option and let `deactivate_plugins()` actually -run, rather than stubbing it as the unit tests do. Only `wp_safe_redirect` and `wp_get_referer` are -stubbed; the redirect throws `TestException` so the request halts where production calls `exit`, -without mocking `exit` itself. That makes this a genuine integration check of the load guard, -the three policies, and the run-once activation working together. +Why this way: + +**These drive real WordPress state.** The real `active_plugins` option, a real +`deactivate_plugins()` call, rather than the stubs the unit tests use — a genuine integration check +of the load guard, the three policies, and the run-once activation working together. -Bundled fixtures are generated per test rather than committed: `require_once` caches by resolved +**Only `wp_safe_redirect` and `wp_get_referer` are stubbed.** The redirect throws `TestException`, +so the request halts where production calls `exit` without mocking `exit` itself. + +**Bundled fixtures are generated per test, never committed.** `require_once` caches by resolved path for the whole PHP process, so a committed bundled file would execute once for the entire suite and every later test would pass without loading anything. The generator moved into a shared `WithBundledPlugins` trait — the on-disk counterpart to `WithSubPlugins` from PR 7 — and `LoaderLoadTest` now uses it too. -Verify: `slic run unit` and `slic run unit --env multisite` — 9 end-to-end tests. Not covered: real -HTTP requests and a real browser; the redirect is asserted as a call, not followed.' +**Out of scope:** real HTTP requests and a real browser. The redirect is asserted as a call, not +followed.' ``` --- @@ -3100,12 +3079,15 @@ gh pr create --base 15-e2e-fixtures --title "README pass and 1.0.0" --body 'What Usage: see the complete worked example at the end of the README. -Why this way: the README was built up section by section as each PR landed, so it never drifted -from what actually shipped. This pass removes the duplication that approach leaves behind and -checks the ordering reads for a newcomer rather than in merge order. +Why this way: + +**The README was built up section by section as each PR landed,** so it never drifted from what +actually shipped. This pass removes the duplication that approach leaves behind and checks the +ordering reads for a newcomer rather than in merge order. -Verify: `git archive --format=tar HEAD | tar -t` shows only src/, composer.json, LICENSE, README, -CHANGELOG — no tests, docs, or CI config in a consumer install. Both suites and PHPStan green.' +**`.gitattributes` keeps the dev files out of a consumer install.** `git archive` yields `src/`, +`composer.json`, `LICENSE`, `README.md` and `CHANGELOG.md` — no tests, docs, or CI config reaching +a consumer vendor directory.' ``` - [ ] **Step 9: Merge the stack and tag** diff --git a/docs/superpowers/plans/2026-08-12-container-required-rework.md b/docs/superpowers/plans/2026-08-12-container-required-rework.md new file mode 100644 index 0000000..ae092fa --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-container-required-rework.md @@ -0,0 +1,305 @@ +# Container-Required Rework + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or +> superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the container mandatory, delete the machinery that existed only to make it optional, and +land the naming and responsibility corrections that the optional container was hiding. + +**Status:** planned 2026-08-12. Nothing implemented. Branches `11-loader-load-path` … `16-readme-release` +are pushed with PRs open (11, 12, 14, 15, 16, 17) and will all be rewritten. + +## Why + +`Container\Resolution` exists to answer "container binding when bound, `new $default_class` otherwise". +That single requirement is what forces every collaborator to be constructible with no arguments, which is +what forces the `?Peer $peer = null` constructors and their `?? Resolution::peer()` accessors, which is +what makes a class named for an abstract noun feel necessary in the first place. Remove the optional +container and the whole chain goes with it. + +Three surveys of the ecosystem, run 2026-08-12, say this is the normal shape and that it costs our hosts +nothing: + +- **Optional is the outlier.** Across 19 vendored StellarWP packages: 10 have no container at all, 7 + require one and throw (`uplink`, `telemetry`, `schema`, `shepherd`, `harbor`, `validation`, + `foundation`), and exactly one — `admin-notices` — is optional with a `new` fallback. We modelled + `Resolution` on the single outlier. +- **The hosts qualify.** `learndash-core` ships `LearnDash\Core\Container extends DI52Container implements + ContainerInterface`, already requires `stellarwp/container-contract` in production, exposes it as + `App::container()`, and already hands it to Telemetry, Validation and Harbor. Give, Event Tickets and + MemberDash are the same story. The plugins that lack a container are the *add-ons being absorbed*, not + the hosts doing the absorbing. +- **The naming was unsupported.** A census of 3,343 classes across eight Nexcess/StellarWP codebases found + zero classes named `Resolution`, zero named `Destination`, and zero named `Locator`. Abstract `-ion` + nouns are folder names over agent-nouned classes — `Activation/` containing `Activator` and + `Deactivator` — never class names themselves. + +`learndash-core` also settles the question of whether this library is worth shipping: it already contains +**four hand-rolled copies** of it — ProPanel (`@since 4.17.0`), Hub (`4.18.0`), Course Grid (`4.21.4`), +Course Reviews (`4.25.1`) — 987 lines with no shared abstraction, wired on exactly our hooks: + +```php +add_action( 'plugins_loaded', $this->container->callback( Loader::class, 'deactivate' ), 1 ); +add_action( 'plugins_loaded', $this->container->callback( Loader::class, 'load' ), 2 ); +add_filter( 'wp_admin_notice_markup', $this->container->callback( Loader::class, 'update_legacy_plugin_activation_notice' ) ); +``` + +A fifth absorption is a fifth copy. Those four modules are the migration target, and what they get wrong +is the case for the library: all four call `deactivate_plugins()` **non-silent**, none has a capability or +interactive-request gate (deactivation runs on anonymous front-end requests), one stores its merge notice +in a **transient**, and all four derive the standalone basename by stripping `WP_PLUGIN_DIR` off the +**load-guard constant** — the double duty our invariants forbid. + +## Decisions + +### D1 — The container is required + +`Config::get_container()` throws `Config_Exception` when unset, matching `Uplink\Config.php:64`, +`Telemetry\Config.php:71`, `Schema\Config.php:27`, `harbor/src/Harbor/Config.php:86`. `has_container()` +stays as the probe. `Config::set_container()` keeps its name — snake_case is the majority spelling in the +corpus — and keeps type-hinting `StellarWP\ContainerContract\ContainerInterface`. + +Type-hinting the contract is safe under Strauss: the host prefixes both its own container and our copy of +the contract into one namespace, which is why Harbor's identical hint works in LearnDash, Give and +MemberDash today (`sfwd-lms/vendor-prefixed/stellarwp/harbor/src/Harbor/Config.php:7` and +`sfwd-lms/src/Core/Container.php:13` name the same class). + +### D2 — `Container\Resolution` is deleted + +Nothing replaces it. Collaborators are bound by a provider and read with `$container->get()`. +`Loader::registrar()` and `Loader::notices()` remain as public accessors, now reading from the container. +`tests/_support/Resolution_State.php` is deleted with it. + +### D3 — One provider, our own one-method contract + +`Contracts\Provider_Interface` declares `register(): void` and nothing else — Harbor's shape +(`harbor/src/Harbor/Contracts/Provider_Interface.php:5`), not Foundation's. + +Foundation is rejected on three counts: a PHP **8.3** floor against our 7.4, a hard `lucatume/di52` +runtime dependency against our container-contract-only rule, and dead API — its `deferred`/`provides()` +are a fatal `TypeError` in practice, because di52 type-hints its own `ServiceProvider` base class which +Foundation's abstract does not extend. + +### D4 — Hooks wire as closures over the container + +`$container->callback()` is di52-specific and absent from `stellarwp/container-contract`, which declares +only `bind`, `get`, `has`, `singleton`. There are zero uses of `callback()` anywhere in the library corpus. +Harbor's idiom is what we copy: + +```php +add_action( 'plugins_loaded', function () use ( $container ) { + $container->get( Runner::class )->load_all(); +}, 2 ); +``` + +This keeps resolution lazy — registering hooks instantiates nothing — where `[ $resolved_object, 'method' ]` +would force every collaborator to be built at boot, as Telemetry does. + +### D5 — No fallback `new` in constructors + +Every collaborator takes required constructor arguments and is bound by the provider. The +`?Peer $peer = null` parameters and the `protected` accessors falling back to `Resolution::` are deleted. +This is platform-cloud's `AGENTS.md:97` rule verbatim: *"Do not make service dependencies nullable to +instantiate defaults inside constructors. The container should provide collaborators."* + +**The line:** collaborators come from the container; value objects are constructed inline. +`Loader::register()` keeps `new Sub_Plugin( $config )` — one value object per host-supplied config array, +built at the call the host can see in its own stack trace, which is what lets registration validate +eagerly while resolving nothing. `throw new Config_Exception( … )` likewise. No `Sub_Plugin_Factory`: it +would put a container resolution in front of the one method that deliberately performs none. + +### D6 — Names + +| Was | Becomes | Reason | +|---|---|---| +| `Container\Resolution` | *deleted* | zero `-ion` class names in 3,343 | +| `Conflict\Destination` | `Conflict\Redirector` | `Login_Redirector::redirect_after_login(): string` is direct precedent for an agent noun that returns a URL and never navigates | +| `Plugin_State` | `Plugin_Deactivator` + `Plugin_Checker` | it takes an argument and mutates; fact nouns in this corpus report ambient state with no arguments | +| `Contracts\Plugin_State_Interface` | `Contracts\Plugin_Deactivator_Interface` + `Contracts\Plugin_Checker_Interface` | follows the split | +| `Activation` (unbuilt) | `Activator` | `Activation/` is a folder name over `Activator`/`Deactivator` | + +Unchanged: `Config`, `Loader`, `Sub_Plugin`, `Conflict_Policy`, `Registrar`, `Conflict\Resolver`, +`Notices\Queue`, `Notices\Store`, `Notices\Renderer`. + +**Naming rules this establishes**, for `CLAUDE.md`: + +- The suffix names the collaborator's **role in the wiring**, not whether it causes a side effect. Six of + ten agent-nouned classes in platform-cloud return a value rather than calling an API — that is the shape + of a WordPress filter — and are still named `Redirector`, `Registrar`, `Granter`, `Highlighter`. +- **Bare until a sibling doing the same job lands.** `Conflict\Resolver` stays bare beside `Gatekeeper` and + `Redirector`, because none of the three does the other's job; the qualifier arrives only with a second + implementation of the *same* role, as LearnDash's `Scheduler` sits bare beside `Retry_Scheduler`. + LearnDash repeats a namespace segment in only 10% of its classes. +- An abstract `-ion`/`-ance` noun is a **directory**, never a class. + +### D7 — The load path splits out of `Loader` + +`Loader` keeps the public API, the registration buffer, and one-line trampolines. Out go: + +- **`Boot\Scheduler`** — `SEQUENCE`, `LOAD_PRIORITY`, `wiring_window_has_closed()`, and the inline + fallback for a host that boots too late. +- **the load loop** — the gate chain (`is_enabled` → `is_already_loaded` → dependencies → `is_file` → + `should_load` → `require_once` → activation callback). +- **`Conflict\Gatekeeper`** — `is_interactive_admin_request()` and `can_resolve_conflicts()`, which today + sit on `Loader` so a host binding its own `Resolver_Interface` cannot drop either by omission. As a + separately bound collaborator the same guarantee holds, because the trampoline resolves the gatekeeper + rather than the resolver. + +### D8 — The pending-registration buffer stays + +`Loader::$pending` and `flush()` survive the container becoming mandatory, so `register()` still resolves +nothing and registration stays order-free. + +Deleting them would be actively unsafe on our primary host: `App::container()` builds a container lazily +when none is set, and `sfwd_lms.php:116` then **replaces** it at `plugins_loaded` 0. Anything that touched +the container before that point holds an orphan whose bindings are discarded. A host registering at +plugin-file scope — which the spec sanctions — would register into the throwaway container and silently +load nothing. + +### D9 — No admin-notices adapter + +`AdminNotices::show()` does not persist anything: the host calls it every request and admin-notices draws +on `admin_notices`, with dismissal in per-user meta. Our merge notice is raised once, at deactivation, and +must survive to a later request — so the storage half cannot be delegated. + +Worse, the obvious adapter is silently broken. Core fires `admin_notices` **before** `all_admin_notices`, +which is where we render; a renderer calling `AdminNotices::show()` registers one hook too late to be +drawn, and `Queue::render()` has already cleared the store. A once-only notice, lost. + +So: no `Renderer_Interface`, no adapter class, no `class_exists()` guard, no `suggest` entry. `Queue::option_name()` +is already public for exactly this purpose, and `docs/notices.md` grows the fifteen lines a host needs to +read the option at `admin_init` and render it however it likes. + +`Notices\Renderer` and `Notices\Store` still become container-bound, because **everything** does (D5) — +not as a special case for this. + +### D10 — The exit rule stays; the boilerplate moves into a helper + +We do **not** adopt platform-cloud's `platform_cloud_exit()` function seam. It is a test-only seam in +`src/`, which `CLAUDE.md` bans outright, and on PHP 7.4 we cannot write `never`, so it would need a +docblock-only `@phpstan-return never` plus a `files` autoload entry loaded on every request of every host. + +We keep stubbing `wp_safe_redirect` and throwing `TestException`, but the discipline moves into one shared +trait so no test hand-rolls it and no test can silently drop the assertion that the request halted: + +```php +protected function capture_redirect( callable $action ): string { + // stub wp_safe_redirect -> record $location, throw TestException + try { + $action(); + $this->fail( 'Expected the action to redirect and terminate.' ); + } catch ( TestException $exception ) { + // Terminated as expected. + } + + return $location; +} +``` + +## What the host must now do + +Documented in `README.md` and `docs/configuration.md`: + +```php +add_action( 'plugins_loaded', function () { + Config::set_hook_prefix( 'learndash' ); + Config::set_container( App::container() ); + + Loader::register( [ /* … */ ] ); + Loader::boot(); +}, 0 ); +``` + +**Priority 0, in the host's own container block — not from a service provider.** LearnDash and MemberDash +both wire Harbor's `set_container()` at `plugins_loaded` **1**, which is where our conflict resolution +runs; a host copying that habit races us. Priority 0 is also after LearnDash replaces its container, so +the binding is the real one. + +## Invariants to add or amend in `CLAUDE.md` + +- **New:** never write a literal guard-constant name in `src/`. `learndash-core` runs Strauss with + `constant_prefix: "LEARNDASH_"` and an empty exclude list, so a literal would be rewritten and the guard + would stop matching. Constant names arriving as config *values* are safe, which is why the design holds. +- **New:** the naming rules from D6. +- **New:** collaborators from the container, value objects inline (D5). +- **Amend:** the architecture section's account of `Container\Resolution`, the four-collaborator table, and + "the container is genuinely optional" throughout. +- **Amend:** `Notices\Store`, `Notices\Renderer` and `Conflict\Redirector` are no longer "defaulted eagerly + with `?? new X()` because there is nothing to wait for" — that reasoning dies with D1. + +## Documented losses and clashes + +Called out in `docs/conflict-handling.md` and `docs/filters.md` so a migrating host is not surprised: + +- **Version negotiation.** ProPanel deliberately does not deactivate a standalone at or above + `3.0.0-dev`, using `get_plugin_data()` and `version_compare`. We keep this out of scope; the host + expresses it by returning `Conflict_Policy::DEFER` from the `conflict_policy` filter after its own + version check. +- **Renamed standalone directories.** Course Grid finds a standalone in a renamed directory by stripping + `WP_PLUGIN_DIR` off its guard constant. Our string `standalone_plugin_basename` cannot, and the invariant + forbidding one key from doing both jobs is why. +- **Filter polarity.** LearnDash gates modules on `learndash_module_{x}_disabled` where true means *do not + load*; our `should_load` is true means *do load*. Wiring one to the other inverts the gate. +- **Detection is host-filterable.** `learndash-core` installs `option_active_plugins` and + `site_option_active_sitewide_plugins` filters that inject and then strip a synthetic path + (`includes/class-ld-lms.php:175`, `:5173`), so `is_plugin_active()` does not report what is in the + database. `Plugin_Checker` is the seam a host rebinds to correct that. + +## Branch surgery + +Every branch from `11-loader-load-path` up is rewritten, so all six open PRs change. + +1. Snapshot every remote tip first: `git branch -f backup/ origin/` for 11 … 16. +2. Rework `11-loader-load-path`, then `gh stack rebase --committer-date-is-author-date` upward. +3. On a conflict, continue with plain `git rebase --continue` — `gh stack rebase --continue` forwards the + flag and exits 129 — then re-run the stack rebase for the branches above. +4. Force-push, then update each PR body to match what its branch now does. + +## Tasks, in rebase order + +Inventory figures below are measured, not estimated: 176 test methods exist across 12 classes; 31 break +under the mandatory container, 26 under the non-nullable constructors. PHPStan runs over `tests/` at +level 9, so most breakage surfaces from `composer test:analysis` without standing up WordPress. + +### Task 11 — `11-loader-load-path` + +- [ ] `Config::get_container()` throws; `has_container()` kept (it has no `src/` callers, but it is the probe a host uses). +- [ ] Delete `src/Container/Resolution.php` and `src/Container/`. Its only `Config::get_container()` call site goes with it. +- [ ] Add `Contracts\Provider_Interface` and `Provider`; bind each default only when `! $container->has( $id )`. +- [ ] Add `Boot\Scheduler` (`LOAD_PRIORITY`, `SEQUENCE`, `wiring_window_has_closed()`, inline fallback, hook wiring as closures). +- [ ] Add `Load\Runner` (`load_all()`, the gate chain, `has_hook_prefix()`). +- [ ] Split `Plugin_State` → `Plugin_Deactivator` + `Plugin_Checker` + `Traits\Loads_Plugin_Functions`; split the interface. **Note:** these files landed on branch 10, so the split appears in PR 11's diff. +- [ ] `Notices\Queue::__construct( Store $store, Renderer $renderer )` — required. +- [ ] Slim `Loader` to accessors, `register()`, `all()`, `flush()`, `boot()`, `render_notices()`. +- [ ] Tests: delete `Resolution_State`, drop its call from `Loader_State::reset():57`, add a `WithContainer` trait, move load/boot coverage into `Load\RunnerTest` and `Boot\SchedulerTest`, add `ProviderTest`, add `WithHaltedRedirects`. +- [ ] Delete, do not fix: `LoaderResolveTest::test_it_falls_back_to_the_default_registrar_without_a_container`, `::test_it_ignores_a_container_with_no_binding`, `PluginStateTest::test_it_constructs_without_arguments`. +- [ ] `Config_State::reset()` still resets the container to `null`; every test class sets one in `setUp()` instead. +- [ ] Docs on this branch: `CLAUDE.md`, `README.md:26`, `docs/configuration.md:17-18` and its "Rebinding a collaborator" section. + +### Task 12 — `12-conflict-resolver` + +- [ ] `Conflict\Destination` → `Conflict\Redirector`; bind it in `Provider` (it stops being a plain `new`). +- [ ] `Conflict\Resolver::__construct()` takes required `Plugin_Checker_Interface`, `Plugin_Deactivator_Interface`, `Queue_Interface`, `Redirector`; delete the `?? Resolution::` accessors. +- [ ] Move `is_interactive_admin_request()` / `can_resolve_conflicts()` into `Conflict\Gatekeeper`; the trampoline resolves the gatekeeper, so a host binding its own `Resolver_Interface` still cannot drop them. +- [ ] `Loader::plugin_state()` splits or drops; `Loader::resolver()` reads from the container. +- [ ] Tests: `ResolverTest` (29 methods, sets no container today), `Exposed_Resolver` (constructed zero-arg), `DestinationTest` → `RedirectorTest`. +- [ ] Fix the documented contradiction at `docs/configuration.md:39-42`, which says the capability gate lives in the resolver while the code and `docs/conflict-handling.md` say `Loader`. + +### Task 13 — `13-activation` + +- [ ] `Activation` → `Activator`, `Activation_Interface` → `Activator_Interface`; constructor injection; bind in `Provider`. + +### Task 14 — `14-activation-error-rewrite` + +- [ ] Rebase; de-null any collaborator constructor the branch adds; keep `wp_admin_notice_markup` wiring in `Boot\Scheduler`. + +### Task 15 — `15-e2e-suite` + +- [ ] The end-to-end bootstrap now builds a container and calls `Provider::register()` before `Loader::boot()`. + +### Task 16 — `16-readme-release` + +- [ ] Final docs pass. Container-optional survivors to rewrite: `CLAUDE.md` L69, L121, L124-125, L142, L158; `docs/configuration.md` L7, L21-22, L40-41, L125. +- [ ] README gains the priority-0 `Config::set_container()` call the branch previously deleted from its snippet. +- [ ] `docs/notices.md` grows the fifteen-line `admin_init` example for a host rendering through its own notice system. +- [ ] Keep the branch's deletion of the spec and the old plan; delete this plan file too once the stack lands. diff --git a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md index c6dcc74..c3bd7d2 100644 --- a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md +++ b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md @@ -146,9 +146,9 @@ Hooks wired by boot(): load_all() per sub-plugin, in order: is_enabled()? skip if false - are_dependencies_met()? skip + queue dependency notice is_already_loaded()? skip — constant defined, avoids re-declaration fatal - file_exists( bundled_plugin_file )? skip + are_dependencies_met()? skip + queue dependency notice + is_file() && is_readable()? skip + _doing_it_wrong() …/plugin_absorber/should_load? skip if false require_once bundled_plugin_file activation()->maybe_run( $sub_plugin ) @@ -156,8 +156,8 @@ load_all() per sub-plugin, in order: ### Collaborators -Each is interface-backed and container-resolvable through one `Loader::resolve( $interface, -$default_class )` helper: container binding if present, otherwise `new $default_class()`, memoized +Each is interface-backed and container-resolvable through one `Container\Resolution::get( +$interface, $default_class )` helper: container binding if present, otherwise `new $default_class()`, memoized either way. The container is optional and is **not** used to wire hooks — those stay plain static trampolines, per the `admin-notices` precedent. @@ -222,7 +222,7 @@ Dependency-forced, not arbitrary: - `Loader::load()` calls `notices()->queue_dependency_notice()`, and `Conflict\Resolver` calls `Loader::notices()`. Notices (10) therefore precedes the load path (11) and the Resolver (12). - Each `Contracts\*` interface ships **with** its default implementation and its tests, rather - than all four landing up front. `Loader::resolve()` is generic — `(interface, default_class)` — + than all four landing up front. `Container\Resolution::get()` is generic — `(interface, default_class)` — so it never needs the other interfaces to exist; only the accessors do, and each accessor lands with its pair. - `Conflict_Policy` (6) is split from `Sub_Plugin` (7) so PR 7 is purely predicate logic, which is @@ -266,7 +266,7 @@ message. This is documented in `tests/README.md`. rejecting a string of any kind. - **8 — `Registrar`.** register; `all()`; a duplicate slug throws `Config_Exception` naming both bundled files and leaves the registry untouched; `reset()` clears the guard with the registry. -- **9 — `Loader::resolve()`.** No container → default instance; di52 container binding a custom +- **9 — `Container\Resolution::get()`.** No container → default instance; di52 container binding a custom `Registrar_Interface` → bound instance returned; memoized (identical instance twice); `Loader::reset()` clears both the memo and the registry. - **10 — `Notices\Queue`.** Each queue method writes the option; the same slug and type queued twice @@ -327,20 +327,29 @@ a method signature already visible in the table. ## 7. PR body template -Four parts, nothing else: +Three parts, nothing else: ``` -What: one line. +What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. -Why this way: the trade-off taken, and against what. +Why this way: + +**The claim, in bold.** One or two sentences: the trade-off taken, and against what. -Verify: the command, and what is deliberately not covered. +**The next claim.** Same again. ``` No boilerplate headings, no restating the diff, no checklists. +There is no `Verify` section (dropped 2026-08-12). The test commands live in `CLAUDE.md` and the +coverage is in the diff; repeating both in every PR is filler a reviewer learns to scroll past. + +`Why this way` is one bold-led block per decision, never a single paragraph running several +arguments together — a reviewer reads the bold leads and stops at the one they doubt. Cut the +connective throat-clearing between claims, never the claims. + --- ## 8. Verification diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php new file mode 100644 index 0000000..3c69b4c --- /dev/null +++ b/src/Boot/Scheduler.php @@ -0,0 +1,153 @@ +container = $container; + } + + /** + * Wire the WordPress hooks. + * + * Nothing is resolved here. Each step is a closure over the container that asks for its + * 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. + * + * @since 1.0.0 + * + * @return void + */ + public function wire(): void { + if ( is_admin() ) { + // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, + // network_admin_notices and user_admin_notices as mutually exclusive branches, so a + // 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' ] ); + } + + // 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. + if ( $this->wiring_window_has_closed() ) { + _doing_it_wrong( + Loader::class . '::boot', + 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + '1.0.0' + ); + + // In the order the hooks would have run them. + foreach ( $this->sequence() as $step ) { + $run = $step['run']; + + $run(); + } + + return; + } + + foreach ( $this->sequence() as $step ) { + add_action( 'plugins_loaded', $step['run'], $step['priority'] ); + } + } + + /** + * The plugins_loaded steps, in run order, as priority and callback. + * + * Stated once because wire() expresses this order twice — as hook priorities when it can still + * wire, and as straight calls when it is too late to and has to run them inline. Those two are + * the same sequence, and a comment is the only thing that could hold them in agreement. + * Iterating one list cannot drift. + * + * A method rather than a constant because a step is a closure now: naming the step and giving + * its priority in two separate lists would put the drift straight back, one list deep. + * + * @since 1.0.0 + * + * @return array + */ + private function sequence(): array { + $container = $this->container; + + return [ + [ + 'priority' => self::LOAD_PRIORITY, + 'run' => static function () use ( $container ): void { + $container->get( Runner::class )->load_all(); + }, + ], + ]; + } + + /** + * Whether it is already too late to wire the load hook. + * + * 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. + * + * @since 1.0.0 + * + * @return bool + */ + private function wiring_window_has_closed(): bool { + if ( ! did_action( 'plugins_loaded' ) ) { + return false; + } + + if ( ! doing_action( 'plugins_loaded' ) ) { + return true; + } + + $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; + + return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + } +} diff --git a/src/Config.php b/src/Config.php index 30ee51a..abc73b5 100644 --- a/src/Config.php +++ b/src/Config.php @@ -115,9 +115,12 @@ public static function get_option_name( string $name ): string { } /** - * Share the host's container so collaborators become bindable. + * Share the host's container. Required, and required before boot(). * - * Entirely optional — with no container the library instantiates its own defaults. + * Every collaborator this library uses is resolved from it, which is what makes each of them + * replaceable by binding an interface. There is no second, container-less path to keep working + * beside that one: two ways to reach a collaborator means two sets of behaviour to reason + * about, and the one nobody runs is the one that rots. * * @since 1.0.0 * @@ -132,9 +135,17 @@ public static function set_container( ContainerInterface $container ): void { /** * @since 1.0.0 * - * @return ContainerInterface|null + * @throws Config_Exception When no container has been set. + * + * @return ContainerInterface */ - public static function get_container(): ?ContainerInterface { + public static function get_container(): ContainerInterface { + if ( self::$container === null ) { + throw new Config_Exception( + 'You must call Config::set_container() before booting the Plugin Absorber.' + ); + } + return self::$container; } diff --git a/src/Contracts/Plugin_Checker_Interface.php b/src/Contracts/Plugin_Checker_Interface.php new file mode 100644 index 0000000..0b45244 --- /dev/null +++ b/src/Contracts/Plugin_Checker_Interface.php @@ -0,0 +1,37 @@ +notices = $notices; + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception From loading a sub-plugin, which reads the hook prefix the guard + * above has already established is set. + * + * @return void + */ + public function load_all(): void { + // The load path needs the prefix for the should_load filter and for the notice store. + // Throwing out of a core action would take the whole site down over a bootstrap mistake, + // so it is reported where a developer will see it and the load is abandoned instead. + if ( ! self::has_hook_prefix() ) { + return; + } + + // Loader::all() rather than the registrar directly: it flushes the registrations still + // buffered on the facade before it reads, and a registrar asked on its own would miss + // anything registered since the last read. + try { + $sub_plugins = Loader::all(); + } catch ( Config_Exception $exception ) { + // The flush is where a duplicate slug is caught, and reading the registrar is where a + // missing container or an unusable binding is. All three are bootstrap mistakes, and + // all three arrive inside plugins_loaded: letting one out would fatal every request, + // front end and admin alike, and lock the developer out of the screen where the + // registration could be corrected. The hook this runs on exists to prevent a fatal, so + // it is the last place that may cause one -- the mistake is reported to the developer + // and the load is abandoned instead. + _doing_it_wrong( + self::class, + sprintf( + 'The registered sub-plugins could not be read, so none were loaded: %s', + $exception->getMessage() + ), + '1.0.0' + ); + + return; + } + + foreach ( $sub_plugins as $sub_plugin ) { + $this->load( $sub_plugin ); + } + } + + /** + * Load one sub-plugin, cheapest and most decisive check first. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin to load. + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + private function load( Sub_Plugin $sub_plugin ): void { + if ( ! $sub_plugin->is_enabled() ) { + return; + } + + // Ahead of the dependency check, which calls an arbitrary host callable. This is one + // defined(), it carries the whole re-declaration guarantee, and it is the only gate that + // means "the plugin is already running" -- warning that requirements are unmet for a + // plugin the admin can see working would be worse than useless. + if ( $sub_plugin->is_already_loaded() ) { + return; + } + + if ( ! $sub_plugin->are_dependencies_met() ) { + $this->notices->queue_dependency_notice( $sub_plugin ); + + return; + } + + // Not file_exists(): that is true for a directory and for a file with no read permission, + // and require_once fatals on both. A missing file is a broken build in the host plugin + // rather than anything a site owner can act on, so it goes to the developer instead of + // into the notice queue, where it would have displayed the host's own + // dependency_notice_message and sent the owner after the wrong problem entirely. + $file = $sub_plugin->get_bundled_plugin_file(); + + if ( ! is_file( $file ) || ! is_readable( $file ) ) { + _doing_it_wrong( + self::class, + sprintf( + 'The bundled plugin file for "%s" is missing or unreadable: %s', + $sub_plugin->get_slug(), + $file + ), + '1.0.0' + ); + + return; + } + + // No type guard on the return, unlike the conflict_policy filter: there is no cast here, + // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the + // load, which is the safe direction. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); + + if ( ! $should_load ) { + return; + } + + // An include takes the scope of the line it sits on, and this one is inside a method, where + // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled + // 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; + } +} diff --git a/src/Loader.php b/src/Loader.php index 80aa63c..98628f4 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -5,22 +5,29 @@ namespace Nexcess\PluginAbsorber; +use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; use Throwable; /** - * Static facade: collaborator resolution, registration, hook wiring, and the load loop. + * Static facade: registration, and the one call that starts everything. + * + * What a host touches, and deliberately little else. How collaborators are built belongs to + * `Provider`, when they run to `Boot\Scheduler`, and the load pass itself to `Load\Runner` — so + * the only reason to open this file is to change what a host may say to the library. + * + * `final` because it cannot usefully be extended: every member is private static and every internal + * call is `self::`, so a subclass would inherit the API, be unable to override any of it, and change + * nothing — which is the silent no-op this class reports on everywhere else. * * @since 1.0.0 */ -class Loader { - /** - * Resolved collaborators, memoized by interface name. - * - * @var array - */ - private static $resolved = []; +final class Loader { + use Guards_Hook_Prefix; /** * Sub-plugins registered but not yet handed to the registrar. @@ -29,28 +36,48 @@ class Loader { */ private static $pending = []; + /** + * Whether the hooks have been wired. + * + * @var bool + */ + private static $booted = false; + /** * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable instance. + * @throws Config_Exception When no container has been set, or its binding is unusable. * * @return Registrar_Interface */ public static function registrar(): Registrar_Interface { - return self::resolve( Registrar_Interface::class, Registrar::class ); + return self::collaborator( Registrar_Interface::class ); + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When no container has been set, or its binding is unusable. + * + * @return Queue_Interface + */ + public static function notices(): Queue_Interface { + return self::collaborator( Queue_Interface::class ); } /** * Register one bundled sub-plugin. Call once per sub-plugin, before boot(). * * The sub-plugin is buffered rather than handed straight to the registrar, so that registering - * resolves nothing. Resolution needs the container, and a host that registers before it calls - * Config::set_container() would otherwise pin the default registrar and silently ignore the - * binding. Buffering is what lets the container arrive at any point before boot, like every - * other configuration call. + * resolves nothing. Reaching the registrar needs the container, and a host that registers before + * it calls Config::set_container() would otherwise fail on a call that has nothing to do with + * the container. Buffering is what lets the container arrive at any point before boot, like + * every other configuration call. * * The configuration is still validated here: building the Sub_Plugin is what rejects it, and - * that happens at the call the host can see in its own stack trace. + * that happens at the call the host can see in its own stack trace. It is built rather than + * resolved because it is a value object — a container asked for one would need the config + * passed through it, and there is nothing about it to rebind. * * @since 1.0.0 * @@ -69,125 +96,170 @@ public static function register( array $config ): void { * * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable registrar, or two - * sub-plugins were registered under one slug. + * @throws Config_Exception When no container has been set, or two sub-plugins were registered + * under one slug. * * @return array */ public static function all(): array { self::flush(); - return self::registrar()->all(); + // Registrar_Interface::all() can only declare `array` — PHP 7.4 has no way to say + // array in a signature — so a host binding its own registrar may return + // anything at all. Narrowed once here, where the untrusted value crosses into the library, + // rather than at each call site: a consumer that forgot the check would fatal inside + // plugins_loaded on its first predicate call, which is the exact failure this library + // exists to prevent, and every future consumer would have to remember it too. + return array_filter( + self::registrar()->all(), + static function ( $sub_plugin ): bool { + return $sub_plugin instanceof Sub_Plugin; + } + ); } /** - * Hand every buffered registration to the registrar. - * - * The registrar stays the single source of truth: the buffer is a pre-store that needs no - * container, and duplicate-slug detection and ordering remain the registrar's alone rather - * than being restated here in a second dialect. + * Bind the collaborators, then let the scheduler decide when they run. Idempotent — safe to + * call from more than one code path. * - * The buffer is emptied before the loop, so a second read cannot re-register what the - * registrar already holds and trip its duplicate-slug guard. It is emptied *after* the - * registrar resolves, so a container binding that throws leaves the registrations buffered - * for the next read rather than dropping them on the floor. + * The provider is constructed rather than resolved: it is what teaches the container about this + * library, so the container cannot be asked to build it first. It is bound afterwards, and only + * when nothing answers to `Provider_Interface` already, so a host may replace the whole set of + * bindings with one of its own. * * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable registrar, or two - * sub-plugins were registered under one slug. + * @throws Config_Exception When no container has been set. * * @return void */ - private static function flush(): void { - if ( self::$pending === [] ) { + public static function boot(): void { + if ( self::$booted ) { return; } - $registrar = self::registrar(); - $pending = self::$pending; + $container = Config::get_container(); - self::$pending = []; + if ( ! $container->has( Provider_Interface::class ) ) { + $container->singleton( Provider_Interface::class, new Provider( $container ) ); + } - foreach ( $pending as $sub_plugin ) { - $registrar->register( $sub_plugin ); + $container->get( Provider_Interface::class )->register(); + $container->get( Scheduler::class )->wire(); + + // Last, not first. A boot that threw on its way through -- no container, a binding that + // cannot be built -- has wired nothing, and a host that fixes the mistake and calls again + // should get a working library rather than a silent no-op. + self::$booted = true; + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When no container has been set. + * + * @return void + */ + public static function render_notices(): void { + if ( ! self::has_hook_prefix() ) { + return; } + + self::notices()->render(); } /** - * Resolve an interface from the container when bound, else construct the default. + * The object bound to a collaborator interface, checked before it is handed on. * - * The container is never required — with none set, every collaborator is a plain `new`, so - * every default class must be constructible with no arguments. Resolution is memoized, and - * nothing resolves until the first read, which is boot: that is what lets a host set its - * container at any point beforehand. Swapping a collaborator after that would be the worse - * behaviour, since anything already holding the old instance would keep it. + * The container's own return type promises nothing, so a host that bound the wrong class -- a + * typo'd class name, an interface it forgot to implement -- would otherwise surface as a + * TypeError raised inside this library, naming this library's method. That reads as a bug here + * rather than a mistake in the host's own bindings, and it happens inside `plugins_loaded`, + * where nobody is looking. Naming the interface and the class that failed it turns the same + * failure into an instruction. * * @since 1.0.0 * * @template T of object * - * @param class-string $interface Interface to resolve. - * @param class-string $default_class Concrete class to build when nothing is bound. + * @param class-string $interface Collaborator interface to resolve. * - * @throws Config_Exception When the container throws while building the binding, or returns - * something that does not implement the interface it was asked for. + * @throws Config_Exception When no container has been set, when it throws while building the + * binding, or when the binding does not implement the interface it was + * bound to. * * @return T */ - private static function resolve( string $interface, string $default_class ): object { - if ( isset( self::$resolved[ $interface ] ) ) { - // The map holds a different type per key, so it cannot be typed as T as a whole. The - // entry found here still implements $interface: a container binding is only memoized - // once it passes the instanceof below, and the fallback is built from the class-string. - /** @var T $memoized */ - $memoized = self::$resolved[ $interface ]; - - return $memoized; - } - + private static function collaborator( string $interface ): object { + // Resolved outside the try: a missing container is this library's own configuration error + // already, reported in its own words, and re-wrapping it would bury that sentence one + // exception deeper for no gain. $container = Config::get_container(); - if ( $container !== null && $container->has( $interface ) ) { - // has() true only promises the binding exists, not that it can be built: a host factory - // closure is free to throw, and a container asked for a class with an unsatisfiable - // dependency throws its own exception type. Uncaught, either one leaves the host's - // plugins_loaded with a fatal from a vendor namespace that names neither this library - // nor the binding at fault, so both are reported the same way as a binding of the wrong - // type. The original is kept as the previous exception; nothing is memoized. - try { - $instance = $container->get( $interface ); - } catch ( Throwable $thrown ) { - throw new Config_Exception( - sprintf( - 'The container failed to build the binding for %s: %s', - $interface, - $thrown->getMessage() - ), - 0, - $thrown - ); - } + // A host factory closure is free to throw, and a container asked for a binding with an + // unsatisfiable dependency -- or for an interface nothing has bound yet, which is every + // interface before boot() runs the provider -- throws its own exception type. Uncaught, + // either one leaves the host's plugins_loaded with a fatal from a vendor namespace that + // names neither this library nor the binding at fault, so both are reported the same way as + // a binding of the wrong type. The original failure is kept as the previous exception. + try { + $collaborator = $container->get( $interface ); + } catch ( Throwable $thrown ) { + throw new Config_Exception( + sprintf( + 'The container failed to build the binding for %s: %s', + $interface, + $thrown->getMessage() + ), + 0, + $thrown + ); + } - // Checked before it is memoized. Without this the bad instance is cached, and every - // accessor throws a TypeError blaming this library rather than the binding. - if ( ! $instance instanceof $interface ) { - throw new Config_Exception( - sprintf( - 'The container binding for %s must implement it. Got %s.', - $interface, - is_object( $instance ) ? get_class( $instance ) : gettype( $instance ) - ) - ); - } + if ( ! $collaborator instanceof $interface ) { + throw new Config_Exception( + sprintf( + 'The container binding for %s returned %s, which does not implement it.', + $interface, + is_object( $collaborator ) ? get_class( $collaborator ) : gettype( $collaborator ) + ) + ); + } - self::$resolved[ $interface ] = $instance; + return $collaborator; + } - return $instance; + /** + * Hand every buffered registration to the registrar. + * + * The registrar stays the single source of truth: the buffer is a pre-store that needs no + * container, and duplicate-slug detection and ordering remain the registrar's alone rather + * than being restated here in a second dialect. + * + * The buffer is emptied before the loop, so a second read cannot re-register what the + * registrar already holds and trip its duplicate-slug guard. It is emptied *after* the + * registrar resolves, so a container binding that throws leaves the registrations buffered + * for the next read rather than dropping them on the floor. + * + * @since 1.0.0 + * + * @throws Config_Exception When no container has been set, or two sub-plugins were registered + * under one slug. + * + * @return void + */ + private static function flush(): void { + if ( self::$pending === [] ) { + return; } - self::$resolved[ $interface ] = new $default_class(); + $registrar = self::registrar(); + $pending = self::$pending; + + self::$pending = []; - return self::$resolved[ $interface ]; + foreach ( $pending as $sub_plugin ) { + $registrar->register( $sub_plugin ); + } } } diff --git a/src/Notices/Queue.php b/src/Notices/Queue.php index 83d202d..dd34180 100644 --- a/src/Notices/Queue.php +++ b/src/Notices/Queue.php @@ -16,8 +16,9 @@ * queue is kept is Store's job and how it is drawn is Renderer's, so a host can replace either one * without inheriting the other, and neither has to be understood to reword a message. * - * Both collaborators are constructor arguments with defaults, so `new Queue()` still gives the - * standard behaviour — which is what `Loader::resolve()` builds when the container has no binding. + * Both collaborators are required constructor arguments, and `Provider` is what hands them over. + * No defaults: a class that can build its own dependencies has a second way to be constructed that + * bypasses every binding a host made, and it is the one a test or a stray `new` reaches for. * * A host already using stellarwp/admin-notices can bind its own implementation of Queue_Interface * and read the same option, whose name is `self::option_name()`. @@ -79,12 +80,12 @@ class Queue implements Queue_Interface { /** * @since 1.0.0 * - * @param Store|null $store Where the queue is kept. - * @param Renderer|null $renderer How a queued notice is drawn. + * @param Store $store Where the queue is kept. + * @param Renderer $renderer How a queued notice is drawn. */ - public function __construct( ?Store $store = null, ?Renderer $renderer = null ) { - $this->store = $store ?? new Store(); - $this->renderer = $renderer ?? new Renderer(); + public function __construct( Store $store, Renderer $renderer ) { + $this->store = $store; + $this->renderer = $renderer; } /** diff --git a/src/Plugin_Checker.php b/src/Plugin_Checker.php new file mode 100644 index 0000000..e48a1b5 --- /dev/null +++ b/src/Plugin_Checker.php @@ -0,0 +1,34 @@ +load_plugin_functions(); + + // WordPress's own is_plugin_active() already ORs in the network check, so asking + // is_plugin_active_for_network() as well would only buy a second get_site_option() per + // sub-plugin per request. + return is_plugin_active( $basename ); + } +} diff --git a/src/Plugin_State.php b/src/Plugin_Deactivator.php similarity index 50% rename from src/Plugin_State.php rename to src/Plugin_Deactivator.php index e320aa3..ba46b72 100644 --- a/src/Plugin_State.php +++ b/src/Plugin_Deactivator.php @@ -5,32 +5,16 @@ namespace Nexcess\PluginAbsorber; -use Nexcess\PluginAbsorber\Contracts\Plugin_State_Interface; +use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; +use Nexcess\PluginAbsorber\Traits\Loads_Plugin_Functions; /** - * Plugin state, straight from WordPress. - * - * The single place in this library that touches WordPress's plugin functions, so it is also the - * single place that has to load them. + * Turns a plugin off, the way WordPress's own unattended paths do. * * @since 1.0.0 */ -class Plugin_State implements Plugin_State_Interface { - /** - * @since 1.0.0 - * - * @param string $basename Plugin basename. - * - * @return bool - */ - public function is_active( string $basename ): bool { - $this->load_plugin_functions(); - - // WordPress's own is_plugin_active() already ORs in the network check, so asking - // is_plugin_active_for_network() as well would only buy a second get_site_option() per - // sub-plugin per request. - return is_plugin_active( $basename ); - } +class Plugin_Deactivator implements Plugin_Deactivator_Interface { + use Loads_Plugin_Functions; /** * @since 1.0.0 @@ -58,21 +42,4 @@ public function deactivate( string $basename ): void { // a second request and a second deactivation hook to clear. deactivate_plugins( $basename, true ); } - - /** - * WordPress only loads these in the admin, and we run at plugins_loaded on every request. - * - * Guarded on deactivate_plugins() rather than is_plugin_active(), because the latter is a - * common third-party shim: something else defining it would short-circuit this and leave the - * rest of the file unloaded, so the first call that needs a function nobody shimmed fatals. - * - * @since 1.0.0 - * - * @return void - */ - private function load_plugin_functions(): void { - if ( ! function_exists( 'deactivate_plugins' ) ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - } - } } diff --git a/src/Provider.php b/src/Provider.php new file mode 100644 index 0000000..0dc1353 --- /dev/null +++ b/src/Provider.php @@ -0,0 +1,133 @@ +container = $container; + } + + /** + * @since 1.0.0 + * + * @return void + */ + public function register(): void { + $container = $this->container; + + // The container under its own contract, so that a container which builds unbound classes + // reflectively can still satisfy the two collaborators that take one. Bound first because + // everything below it may be resolved that way. + $this->bind_once( ContainerInterface::class, $container ); + + $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( Store::class ); + $this->bind_once( Renderer::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 + // autowiring, so a container that resolves nothing by reflection has to be told. + $this->bind_once( + Queue_Interface::class, + static function () use ( $container ): Queue { + return new Queue( $container->get( Store::class ), $container->get( Renderer::class ) ); + } + ); + + $this->bind_once( + Runner::class, + static function () use ( $container ): Runner { + return new Runner( $container->get( Queue_Interface::class ) ); + } + ); + + $this->bind_once( + Scheduler::class, + static function () use ( $container ): Scheduler { + return new Scheduler( $container ); + } + ); + } + + /** + * Bind as a singleton, unless the host already bound something. + * + * The host binds first and wins: this library's defaults are what a container has when nobody + * said otherwise, and a provider that overwrote a binding would make the order in which a host + * calls `set_container()` and `boot()` decide which implementation it gets. It is also what + * keeps a second `register()` harmless, instead of swapping in a fresh registrar and losing + * every sub-plugin registered so far. + * + * The `class_exists()` half is what makes that question answerable at all. `has()` means "can + * return an entry", not "the host bound this" -- di52 answers it with `isBound() || + * class_exists()`, so for a class id it is true before anything has been bound. Asked alone it + * stands down every binding above whose id is a class, the explicit factories included, leaving + * those collaborators autowired where the container autowires, broken where it does not, and + * singletons nowhere. An interface no container can build unprompted, so there the same call + * answers exactly what is being asked. + * + * What that costs is a host rebinding one of the concrete workers, which has to happen after + * boot: nothing here can tell that binding apart from the container's own willingness to build + * the class. The interface seams -- the ones a host is invited to replace -- are unaffected. + * + * Singletons throughout. Every one of these is either a registry whose contents are the point + * or a stateless worker, and a second registrar would hold a second, emptier list of + * sub-plugins. + * + * @since 1.0.0 + * + * @param string $id Interface or class to bind. + * @param mixed $implementation Class name, instance or factory closure; `null` to have the + * container build `$id` itself. + * + * @return void + */ + private function bind_once( string $id, $implementation = null ): void { + if ( ! class_exists( $id ) && $this->container->has( $id ) ) { + return; + } + + $this->container->singleton( $id, $implementation ); + } +} diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index 875e199..27f7e75 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -12,7 +12,7 @@ * * Deliberately not a window onto WordPress. Asking whether the standalone counterpart is active is * a question about the site rather than about this configuration, and it belongs to - * Plugin_State_Interface; this object only names the plugin to ask about. + * Plugin_Checker_Interface; this object only names the plugin to ask about. * * @since 1.0.0 * diff --git a/src/Traits/Guards_Hook_Prefix.php b/src/Traits/Guards_Hook_Prefix.php new file mode 100644 index 0000000..c269aa7 --- /dev/null +++ b/src/Traits/Guards_Hook_Prefix.php @@ -0,0 +1,41 @@ +getMessage(), '1.0.0' ); + + return false; + } + + return true; + } +} diff --git a/src/Traits/Loads_Plugin_Functions.php b/src/Traits/Loads_Plugin_Functions.php new file mode 100644 index 0000000..fc602ac --- /dev/null +++ b/src/Traits/Loads_Plugin_Functions.php @@ -0,0 +1,35 @@ +set_up_container(); +} +``` + +A test about a *rebinding* host binds its own implementation first and passes +the container in — the provider only binds what nothing else has, which is the +guarantee those tests exist to pin: + +```php +$container = new Test_Container(); +$container->singleton( Queue_Interface::class, static fn() => $notices ); + +$this->set_up_container( $container ); +``` + +Bind interfaces, not concrete classes: DI52 reports `has()` true for any class +name that exists, bound or not, so binding `Notices\Store` first cannot +demonstrate anything the provider does. + +`$this->resolve( Some::class )` is the typed read back out, and +`$this->container()` the container itself. Call `tear_down_container()` from +tearDown, alongside `Config_State::reset()`. + +Container tests must use `Tests\Support\Test_Container`. +`lucatume\DI52\Container` implements PSR-11's `ContainerInterface`, not +StellarWP's, so passing it to `Config::set_container()` is a `TypeError`. + ## Sub-plugin fixtures `WithSubPlugins` builds a well-formed `Sub_Plugin`, so a test states only the @@ -58,6 +100,26 @@ $this->make_sub_plugin( [ 'plugin_loaded_constant' => 'ABSORBER_TEST_LOADED_CONS Overrides are merged last, so a deliberately unusable value still reaches the constructor — that is how the tests for rejected config work. +## Bundled plugin fixtures + +`WithBundledPlugins` writes the file the load path requires: + +```php +$constant = $this->make_guard_constant(); +$path = $this->make_bundled_plugin_file( $constant ); + +// … register, load … + +$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. + A fixture helper cannot be called `make()`, `makeEmpty()`, `construct()`, or `constructEmpty()`: those are public methods on `Codeception\Test\Unit`, which `WPTestCase` extends, and redeclaring one with narrower visibility is a fatal at @@ -156,52 +218,59 @@ class Deactivator { } ``` -Assert it like this: +Assert it through `WithHaltedRedirects`, which owns the whole shape: ```php -use Nexcess\PluginAbsorber\Tests\Support\TestException; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithHaltedRedirects; public function test_redirects_back(): void { - $redirects = []; - - $this->setFunctionReturn( - 'wp_safe_redirect', - static function ( $location ) use ( &$redirects ) { - $redirects[] = $location; - - throw new TestException( 'Halted where production calls exit().' ); - }, - true - ); - $subject = new Deactivator(); - $halted = false; - try { - $subject->redirect_back( 'https://example.test/wp-admin/plugins.php' ); - } catch ( TestException $e ) { - $halted = true; - - $this->assertSame( 'Halted where production calls exit().', $e->getMessage() ); - } + $location = $this->capture_redirect( + static function () use ( $subject ): void { + $subject->redirect_back( 'https://example.test/wp-admin/plugins.php' ); + } + ); - $this->assertTrue( $halted, 'The redirect must halt where production calls exit().' ); - $this->assertSame( [ 'https://example.test/wp-admin/plugins.php' ], $redirects ); + $this->assertSame( 'https://example.test/wp-admin/plugins.php', $location ); } ``` -The `$halted` flag is the part that cannot be dropped. Catching the exception -without asserting that it actually arrived turns "the code under test never -redirected at all" into a silent pass — the same class of failure this section +Two parts of that are easy to drop by hand and silent when dropped, which is +why they live in the trait rather than in each test body. The `fail()` on the +line after the action is what turns "the code under test never redirected at +all" into a failure instead of a pass — the same class of failure this section opens by warning about, moved out of `preventExit()` and into the test body. -Matching on the message as well as the class keeps an unrelated `TestException` -thrown earlier from satisfying the catch for the wrong reason. +Matching on the exception's message as well as its class keeps an unrelated +`TestException` thrown earlier from satisfying the catch for the wrong reason. + +The trait needs `UopzFunctions` on the same class, for the stub. + +`tests/unit/SmokeTest.php` covers both the mechanism and the trait, with +`test_a_stub_can_throw_to_halt_a_code_path` and +`test_the_shared_helper_captures_a_halted_redirect` — the executable proof that +a stub really can stop a code path before it reaches `exit`, and that the shared +helper reports it when one does not. + +## Expecting `_doing_it_wrong()` + +`setExpectedIncorrectUsage()` matches the first argument exactly, which for a +report made with `__METHOD__` means restating a private method name in the test. +That name is an implementation detail of where a gate happens to live — moving +the inline-boot fallback from `Loader` to `Boot\Scheduler` changed it without +changing anything a host can observe. -A bare `expectException( TestException::class )` is fine when the test only -cares that the halt happened and asserts nothing about state afterwards. The -try/catch shape exists so assertions can run after the halt; there is no reason -to use both mechanisms in one test. +`WithIncorrectUsage` registers the expectation from the report itself and +asserts over what was reported instead: + +```php +$this->expect_incorrect_usage(); + +$runner->load_all(); + +$this->assert_the_library_reported_incorrect_usage(); +``` -`tests/unit/SmokeTest.php` covers this with -`test_a_stub_can_throw_to_halt_a_code_path`, which is the executable proof that -a stub really can throw to stop a code path before it reaches `exit`. +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. diff --git a/tests/_support/Loader_State.php b/tests/_support/Loader_State.php index 67cc000..7e5d639 100644 --- a/tests/_support/Loader_State.php +++ b/tests/_support/Loader_State.php @@ -5,22 +5,24 @@ namespace Nexcess\PluginAbsorber\Tests\Support; +use Closure; use LogicException; use Nexcess\PluginAbsorber\Loader; use ReflectionClass; +use ReflectionFunction; use ReflectionProperty; +use WP_Hook; /** - * Restores `Loader`'s static state between tests. + * Restores `Loader`'s static state between tests, and unwires the hooks boot() added. * * `Loader` has no public way to clear itself, and deliberately so: a reset method would be API the - * library then has to support forever for the sake of its own test suite, and a host that reached - * for it mid-request would discard the registrations the load loop is about to read. Reflection - * keeps that seam on this side of the fence. + * library then has to support forever for the sake of its own test suite, and a host that reached for + * it mid-request would discard the registrations the load loop is about to read. Reflection keeps that + * seam on this side of the fence. * - * Dropping the memo is enough for the default collaborators, which are built per resolve. A - * collaborator bound into a container as a singleton comes back populated on the next resolve, so - * a test that binds one must build a fresh instance rather than expect this to empty it. + * Collaborators are the container's now, so there is no memo here to drop: a test gets a fresh set by + * standing up a fresh container, which `Traits\WithContainer` does in one line. */ class Loader_State { /** @@ -33,12 +35,34 @@ class Loader_State { * @var array */ protected const DEFAULTS = [ - 'resolved' => [], - 'pending' => [], + 'pending' => [], + 'booted' => false, ]; /** - * Return every static property of `Loader` to its default. + * The hooks boot() reaches, directly or through `Boot\Scheduler`. + * + * @var string[] + */ + protected const HOOKS = [ + 'plugins_loaded', + 'all_admin_notices', + 'wp_admin_notice_markup', + ]; + + /** + * Namespace every callback this library wires belongs to. + * + * @var string + */ + protected const NAMESPACE_PREFIX = 'Nexcess\\PluginAbsorber\\'; + + /** + * Return every static property of `Loader` to its default, and unwire the hooks it added. + * + * Clearing the boot flag without unwiring would leave a `Loader` that reports itself unbooted + * while its callbacks are still attached: the next `boot()` would wire nothing, and still look + * like it had worked. * * @throws LogicException When `Loader` has grown a static property this helper does not know * about, rather than leaving it to leak between tests. @@ -46,6 +70,8 @@ class Loader_State { * @return void */ public static function reset(): void { + self::unwire(); + $reflection = new ReflectionClass( Loader::class ); foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { @@ -61,4 +87,89 @@ public static function reset(): void { $property->setValue( null, self::DEFAULTS[ $name ] ); } } + + /** + * Take back every callback this library put on the boot hooks. + * + * Identified by where the callback comes from rather than by restating what boot() wires. The + * steps are closures over the container now, so there is no name to match on and no + * `has_action( $hook, [ Loader::class, 'load_all' ] )` to remove them by; and + * `remove_all_actions()` would strip the hook bare, discarding every callback WordPress and the + * rest of the suite have on it for the remainder of the process. + * + * Reading `Boot\Scheduler::SEQUENCE` for its priorities would be the narrower sweep, but it would + * also miss a step wired at a priority the constant no longer names — which is exactly the drift + * an unwire helper exists to survive. + * + * @return void + */ + protected static function unwire(): void { + foreach ( self::HOOKS as $hook ) { + $wp_hook = $GLOBALS['wp_filter'][ $hook ] ?? null; + + if ( ! $wp_hook instanceof WP_Hook ) { + continue; + } + + // Iterating a by-value copy, so removing as we go cannot disturb the walk. + foreach ( $wp_hook->callbacks as $priority => $callbacks ) { + if ( ! is_int( $priority ) || ! is_array( $callbacks ) ) { + continue; + } + + foreach ( $callbacks as $registered ) { + $callback = is_array( $registered ) ? ( $registered['function'] ?? null ) : null; + + if ( $callback !== null && is_callable( $callback ) && self::belongs_to_the_library( $callback ) ) { + remove_action( $hook, $callback, $priority ); + } + } + } + } + } + + /** + * Whether a registered callback came out of this library. + * + * Covers both shapes a hook callback can take here: a closure written in `src/`, and a static or + * instance method on one of the library's own classes. + * + * @param callable $callback Callback registered on one of the boot hooks. + * + * @return bool + */ + protected static function belongs_to_the_library( callable $callback ): bool { + if ( $callback instanceof Closure ) { + $file = ( new ReflectionFunction( $callback ) )->getFileName(); + + return is_string( $file ) && strpos( $file, self::source_directory() ) === 0; + } + + if ( is_array( $callback ) ) { + $target = $callback[0] ?? null; + $class = is_object( $target ) ? get_class( $target ) : $target; + + return is_string( $class ) && strpos( $class, self::NAMESPACE_PREFIX ) === 0; + } + + return is_string( $callback ) && strpos( $callback, self::NAMESPACE_PREFIX ) === 0; + } + + /** + * Where the library's own source lives, read off a class rather than assumed from this file. + * + * @throws LogicException When the path cannot be read, rather than matching every closure on + * an empty prefix and unwiring the whole site. + * + * @return string + */ + protected static function source_directory(): string { + $file = ( new ReflectionClass( Loader::class ) )->getFileName(); + + if ( ! is_string( $file ) || $file === '' ) { + throw new LogicException( sprintf( 'Could not locate the library source from %s.', self::class ) ); + } + + return dirname( $file ) . DIRECTORY_SEPARATOR; + } } diff --git a/tests/_support/Spy_Queue.php b/tests/_support/Spy_Queue.php new file mode 100644 index 0000000..2b42400 --- /dev/null +++ b/tests/_support/Spy_Queue.php @@ -0,0 +1,85 @@ +render_calls` off a value typed as + * `Queue_Interface` is reading a property the interface does not declare, and static analysis rightly + * rejects it. Named, the spy's own type carries the counters. + * + * It stores nothing, which is the point — a test that binds this one proves the default queue was + * never resolved by asserting the option is still absent. + * + * @since 1.0.0 + */ +class Spy_Queue implements Queue_Interface { + /** + * Slugs handed to queue_merge_notice(), in order. + * + * @var string[] + */ + public $merge_notices = []; + + /** + * Slugs handed to queue_conflict_notice(), in order. + * + * @var string[] + */ + public $conflict_notices = []; + + /** + * Slugs handed to queue_dependency_notice(), in order. + * + * @var string[] + */ + public $dependency_notices = []; + + /** + * How many times render() was called. + * + * @var int + */ + public $render_calls = 0; + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { + $this->merge_notices[] = $sub_plugin->get_slug(); + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + $this->conflict_notices[] = $sub_plugin->get_slug(); + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + $this->dependency_notices[] = $sub_plugin->get_slug(); + } + + /** + * @return void + */ + public function render(): void { + ++$this->render_calls; + } +} diff --git a/tests/_support/Test_Container.php b/tests/_support/Test_Container.php index 46e7c1f..8a7c872 100644 --- a/tests/_support/Test_Container.php +++ b/tests/_support/Test_Container.php @@ -42,10 +42,13 @@ public function get( string $id ) { } /** - * Reports whether the id is bound. + * Reports whether the container can return an entry for the id. * * Inherits DI52's permissive semantics: any existing *class* name reports true even with * nothing bound, because DI52 falls back to `class_exists()`. Interface names are unaffected. + * `isBound()` below is the narrower question — whether a binding was actually made — and this + * adapter exposes it so that a caller distinguishing the two reaches the same method on a real + * host container. * * @inheritDoc */ @@ -53,6 +56,21 @@ public function has( string $id ) { return $this->container->has( $id ); } + /** + * Whether something was bound to the id, autowirable class names excluded. + * + * Not part of `ContainerInterface`. It is on DI52, and on the adapters hosts wrap DI52 in — the + * example adapter in `stellarwp/container-contract` forwards unknown calls through `__call()` — + * so a collaborator probing for it finds it here as it would in production. + * + * @param string $id Identifier of the entry to look for. + * + * @return bool + */ + public function isBound( string $id ): bool { + return $this->container->isBound( $id ); + } + /** * @inheritDoc */ diff --git a/tests/_support/Traits/WithBundledPlugins.php b/tests/_support/Traits/WithBundledPlugins.php new file mode 100644 index 0000000..6d96ef2 --- /dev/null +++ b/tests/_support/Traits/WithBundledPlugins.php @@ -0,0 +1,123 @@ +bundled_plugin_files[] = $path; + + return $path; + } + + /** + * A guard constant name no other test can collide with. + * + * The fixture defines its constant for real, and a `define()` lasts for the whole PHP process: a + * name reused by a later test would make its sub-plugin read as already loaded and skip the load + * it was written to exercise. + * + * @since 1.0.0 + * + * @return string + */ + protected function make_guard_constant(): string { + return 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); + } + + /** + * A path no bundled plugin was ever written to. + * + * @since 1.0.0 + * + * @return string + */ + protected function missing_bundled_plugin_file(): string { + return sys_get_temp_dir() . '/absorber-does-not-exist-' . uniqid( '', true ) . '.php'; + } + + /** + * How many times a bundled fixture has been executed this test. + * + * @since 1.0.0 + * + * @return int + */ + protected function bundled_plugin_loads(): int { + $loads = $GLOBALS['absorber_loads'] ?? 0; + + return is_int( $loads ) ? $loads : 0; + } + + /** + * @since 1.0.0 + * + * @return void + */ + protected function reset_bundled_plugin_loads(): void { + $GLOBALS['absorber_loads'] = 0; + } + + /** + * Remove every fixture this test wrote. Call from tearDown. + * + * @since 1.0.0 + * + * @return void + */ + protected function remove_bundled_plugin_files(): void { + foreach ( $this->bundled_plugin_files as $path ) { + if ( file_exists( $path ) ) { + // A test that made a file unreadable to exercise that gate cannot unlink it until the + // permissions come back. + chmod( $path, 0644 ); + unlink( $path ); + } + } + + $this->bundled_plugin_files = []; + + unset( $GLOBALS['absorber_loads'] ); + } +} diff --git a/tests/_support/Traits/WithContainer.php b/tests/_support/Traits/WithContainer.php new file mode 100644 index 0000000..5b15964 --- /dev/null +++ b/tests/_support/Traits/WithContainer.php @@ -0,0 +1,122 @@ +has( ContainerInterface::class ) ) { + $container->singleton( + ContainerInterface::class, + static function () use ( $container ): ContainerInterface { + return $container; + } + ); + } + + ( new Provider( $container ) )->register(); + + Config::set_container( $container ); + + $this->absorber_container = $container; + + return $container; + } + + /** + * The container this test is running against. + * + * @since 1.0.0 + * + * @throws LogicException When the test never stood one up, rather than failing later on a null. + * + * @return Test_Container + */ + protected function container(): Test_Container { + if ( $this->absorber_container === null ) { + throw new LogicException( 'Call set_up_container() before reading the container.' ); + } + + return $this->absorber_container; + } + + /** + * Resolve one id, refusing anything that is not what was asked for. + * + * The contract's `get()` can only declare an untyped return, so every call site would otherwise + * repeat the same narrowing to say what it got back. + * + * @since 1.0.0 + * + * @template T of object + * + * @param class-string $id Id to resolve. + * + * @throws LogicException When the container builds something else. + * + * @return T + */ + protected function resolve( string $id ): object { + $instance = $this->container()->get( $id ); + + if ( ! $instance instanceof $id ) { + throw new LogicException( sprintf( 'The container did not build a %s.', $id ) ); + } + + /** @var T $instance */ + return $instance; + } + + /** + * Forget the container between tests. + * + * Call from tearDown alongside `Config_State::reset()`, which clears the library's own reference. + * + * @since 1.0.0 + * + * @return void + */ + protected function tear_down_container(): void { + $this->absorber_container = null; + } +} diff --git a/tests/_support/Traits/WithHaltedRedirects.php b/tests/_support/Traits/WithHaltedRedirects.php new file mode 100644 index 0000000..8f7ad55 --- /dev/null +++ b/tests/_support/Traits/WithHaltedRedirects.php @@ -0,0 +1,89 @@ +setFunctionReturn( + 'wp_safe_redirect', + static function ( $to ) use ( &$location, $message ) { + $location = is_string( $to ) ? $to : ''; + + throw new TestException( $message ); + }, + true + ); + + try { + $action(); + + // Reached only when the action returned instead of halting, which is the failure this + // helper exists to make impossible to miss. + $this->fail( 'Expected the action to redirect and terminate.' ); + } catch ( TestException $exception ) { + $this->assertSame( + $message, + $exception->getMessage(), + 'The halt has to come from the stubbed redirect, not from something thrown earlier.' + ); + } 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' ); + } + + $this->assertNotSame( '', $location, 'The action must redirect somewhere.' ); + + return $location; + } + + /** + * The message the stubbed redirect throws. + * + * A method rather than a constant: PHP 7.4 has no constants in traits. + * + * @since 1.0.0 + * + * @return string + */ + protected static function halted_at_exit_message(): string { + return 'Halted where production calls exit().'; + } +} diff --git a/tests/_support/Traits/WithIncorrectUsage.php b/tests/_support/Traits/WithIncorrectUsage.php new file mode 100644 index 0000000..6233cb2 --- /dev/null +++ b/tests/_support/Traits/WithIncorrectUsage.php @@ -0,0 +1,105 @@ +incorrect_usage_reports; + + $listener = function ( $function_name ) use ( &$reports ): void { + if ( ! is_string( $function_name ) ) { + return; + } + + $reports[] = $function_name; + + $this->setExpectedIncorrectUsage( $function_name ); + }; + + $this->incorrect_usage_listener = $listener; + + add_action( 'doing_it_wrong_run', $listener ); + } + + /** + * @since 1.0.0 + * + * @return void + */ + protected function assert_the_library_reported_incorrect_usage(): void { + $this->assertNotSame( + [], + $this->incorrect_usage_reports, + 'The library has to report a developer mistake where a developer will see it.' + ); + + foreach ( $this->incorrect_usage_reports as $report ) { + $this->assertStringStartsWith( + 'Nexcess\\PluginAbsorber\\', + $report, + 'The report has to name this library, or the host goes looking in WordPress.' + ); + } + } + + /** + * Take the listener back off. Call from tearDown. + * + * Removed by identity rather than by clearing the hook, which WordPress and the rest of the suite + * are also on. + * + * @since 1.0.0 + * + * @return void + */ + protected function stop_expecting_incorrect_usage(): void { + if ( $this->incorrect_usage_listener !== null ) { + remove_action( 'doing_it_wrong_run', $this->incorrect_usage_listener ); + + $this->incorrect_usage_listener = null; + } + + $this->incorrect_usage_reports = []; + } +} diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php new file mode 100644 index 0000000..993f54c --- /dev/null +++ b/tests/unit/Boot/SchedulerTest.php @@ -0,0 +1,421 @@ + + */ + private $added_actions = []; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + $this->set_up_container(); + $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 a hook at the load priority. Rewind the counter so the wiring tests see the timing a + // real host bootstrap sees. The tests that exercise a late boot set it back. + $this->plugins_loaded_count = did_action( 'plugins_loaded' ); + unset( $GLOBALS['wp_actions']['plugins_loaded'] ); + } + + public function tearDown(): void { + $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + + // 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' ); + + // Only what these tests added by hand. What boot() wired comes off in Loader_State::reset(). + foreach ( $this->added_actions as [ $hook, $callback, $priority ] ) { + remove_action( $hook, $callback, $priority ); + } + $this->added_actions = []; + + $this->stop_expecting_incorrect_usage(); + $this->remove_bundled_plugin_files(); + Loader_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + /** + * 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. + */ + public function test_the_load_step_runs_early_in_plugins_loaded(): void { + $this->assertSame( 2, $this->load_priority() ); + } + + 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() ); + + Loader::boot(); + + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', $this->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.' ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + 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() ); + + Loader::boot(); + Loader::boot(); + + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', $this->load_priority() ), + 'boot() must be idempotent.' + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + /** + * The reason the load step runs at all early: a plugin hooking plugins_loaded at the default + * priority — which is nearly all of them — has to find the bundled code already in memory. + */ + public function test_a_plugin_hooking_at_the_default_priority_finds_the_bundled_code(): void { + $this->register_sub_plugin(); + + Loader::boot(); + + $loads_seen = null; + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( &$loads_seen ): void { + $loads_seen = $GLOBALS['absorber_loads'] ?? 0; + } + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $loads_seen ); + } + + public function test_it_wires_the_notice_step_in_the_admin(): void { + set_current_screen( 'dashboard' ); + + $notices = $this->bind_spy_queue(); + + Loader::boot(); + + do_action( 'all_admin_notices' ); + + $this->assertSame( 1, $notices->render_calls ); + } + + /** + * Nothing renders a notice on the front end, and the queue must survive until an admin load + * consumes it — rendering is what clears it. + */ + public function test_it_does_not_wire_the_notice_step_on_the_front_end(): void { + set_current_screen( 'front' ); + + $notices = $this->bind_spy_queue(); + + Loader::boot(); + + // The recorder has to be shown to work: without it, a do_action() that fired nothing at all + // would satisfy the assertion below for a reason that has nothing to do with the front end. + $fired = false; + $this->add_tracked_action( + 'all_admin_notices', + static function () use ( &$fired ): void { + $fired = true; + } + ); + + do_action( 'all_admin_notices' ); + + $this->assertTrue( $fired, 'The hook must really have been dispatched.' ); + $this->assertSame( 0, $notices->render_calls ); + } + + /** + * 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 + * 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 + * dispatched is never reached either, because the dispatch loop walks a by-value copy of that + * priority's callback array. + * + * @dataProvider late_boot_priorities + * + * @param int $offset How far past the load priority the host boots from. + */ + public function test_booting_too_late_in_plugins_loaded_loads_inline_instead( int $offset ): void { + $this->expect_incorrect_usage(); + + $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(); + }, + $this->load_priority() + $offset + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads(), 'A late boot must still load.' ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * @return Generator + */ + public static function late_boot_priorities(): Generator { + yield 'at the load priority' => [ 0 ]; + yield 'one past it' => [ 1 ]; + yield 'the default a host omits' => [ 8 ]; + } + + public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { + $this->expect_incorrect_usage(); + + do_action( 'plugins_loaded' ); + + $this->register_sub_plugin(); + Loader::boot(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The suite's state helper stands in for the reset() the Loader deliberately does not ship. + * Clearing the boot flag without unwiring would leave a Loader that reports itself unbooted while + * its callbacks are still attached — and every later test would load sub-plugins it never + * registered. + */ + 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' ); + + 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' ) ); + } + + /** + * The hooks are gone by the time this boots again, so the assertion is about the second boot + * 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() ); + + Loader::boot(); + Loader_State::reset(); + + Loader::boot(); + + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + } + + /** + * boot() runs before anything reads the prefix, so it must not be the thing that throws when a + * host forgot to set one — the load path reports that at a point the host can see. + */ + public function test_boot_does_not_need_a_hook_prefix(): void { + $container = $this->container(); + + Config_State::reset(); + Config::set_container( $container ); + + $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + + Loader::boot(); + + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + } + + /** + * The priority the load 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 load_priority(): int { + $priority = ( new ReflectionClass( Scheduler::class ) )->getConstant( 'LOAD_PRIORITY' ); + + if ( ! is_int( $priority ) ) { + throw new LogicException( 'Boot\Scheduler::LOAD_PRIORITY must be an int.' ); + } + + return $priority; + } + + /** + * How many callbacks are on a hook, at one priority or in total. + * + * @param string $hook Hook to count. + * @param int|null $priority Priority to count at, or null for every priority. + * + * @return int + */ + private function callbacks_at( string $hook, ?int $priority = null ): int { + $wp_hook = $GLOBALS['wp_filter'][ $hook ] ?? null; + + if ( ! $wp_hook instanceof WP_Hook ) { + return 0; + } + + $total = 0; + + foreach ( $wp_hook->callbacks as $registered_priority => $callbacks ) { + if ( ! is_array( $callbacks ) ) { + continue; + } + + if ( $priority === null || $registered_priority === $priority ) { + $total += count( $callbacks ); + } + } + + return $total; + } + + /** + * Bind a recording queue in place of the default one. + * + * Bound before the provider runs, which is the only order that leaves it bound. + * + * @return Spy_Queue + */ + private function bind_spy_queue(): Spy_Queue { + $notices = new Spy_Queue(); + $container = new Test_Container(); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Spy_Queue { + return $notices; + } + ); + + $this->set_up_container( $container ); + + return $notices; + } + + /** + * Register a sub-plugin whose bundled file records that it was loaded. + * + * @return void + */ + private function register_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, + ] + ); + } + + /** + * 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_actions[] = [ $hook, $callback, $priority ]; + + add_action( $hook, $callback, $priority ); + } +} diff --git a/tests/unit/ConfigTest.php b/tests/unit/ConfigTest.php index 169526d..e7dd4e1 100644 --- a/tests/unit/ConfigTest.php +++ b/tests/unit/ConfigTest.php @@ -152,7 +152,30 @@ public function test_an_option_name_needs_a_prefix(): void { public function test_it_reports_no_container_by_default(): void { $this->assertFalse( Config::has_container() ); - $this->assertNull( Config::get_container() ); + } + + /** + * The container is required, so reading it without one is a configuration error rather than a + * null every caller then has to test for. Every collaborator comes from the container now: a + * silent null would surface as a TypeError from somewhere inside plugins_loaded, naming this + * library rather than the bootstrap that skipped a step. + */ + public function test_reading_a_container_that_was_never_set_is_a_configuration_error(): void { + $this->expectException( Config_Exception::class ); + + Config::get_container(); + } + + /** + * The probe stays, and answers without throwing — it is how a host asks whether it has already + * configured the library. + */ + public function test_the_probe_answers_without_throwing(): void { + $this->assertFalse( Config::has_container() ); + + Config::set_container( new Test_Container() ); + + $this->assertTrue( Config::has_container() ); } public function test_it_stores_and_returns_a_container(): void { @@ -171,9 +194,14 @@ public function test_the_state_helper_clears_every_value(): void { Config_State::reset(); $this->assertFalse( Config::has_container() ); - $this->assertNull( Config::get_container() ); - $this->expectException( Config_Exception::class ); - Config::get_hook_prefix(); + foreach ( [ 'get_container', 'get_hook_prefix' ] as $accessor ) { + try { + Config::{$accessor}(); + $this->fail( sprintf( 'Config::%s() must throw once the state helper has run.', $accessor ) ); + } catch ( Config_Exception $exception ) { + $this->assertNotSame( '', $exception->getMessage(), 'The host has to be told what is missing.' ); + } + } } } diff --git a/tests/unit/Load/RunnerTest.php b/tests/unit/Load/RunnerTest.php new file mode 100644 index 0000000..b78b4d0 --- /dev/null +++ b/tests/unit/Load/RunnerTest.php @@ -0,0 +1,692 @@ + + */ + private $should_load_calls = []; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + $this->set_up_container(); + $this->clear_notices(); + $this->reset_bundled_plugin_loads(); + $this->should_load_calls = []; + } + + public function tearDown(): void { + $this->remove_bundled_plugin_files(); + + // In tearDown rather than at the end of the test body: a failing assertion would otherwise + // strand the constant for the rest of the process, and every later test would read it as a + // sub-plugin whose code is already present and skip the load it was written to exercise. + foreach ( $this->constants as $constant ) { + $this->unsetConstant( $constant ); + } + $this->constants = []; + + $this->stop_expecting_incorrect_usage(); + $this->stop_recording_incorrect_usage_messages(); + $this->clear_notices(); + Loader_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + public function test_it_requires_the_bundled_file(): void { + $constant = $this->register(); + + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assertTrue( defined( $constant ) ); + } + + public function test_it_requires_the_bundled_file_exactly_once(): void { + $this->register(); + + $this->runner()->load_all(); + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + public function test_it_skips_a_disabled_sub_plugin(): void { + $this->register( [ 'enabled' => false ] ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + } + + public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): void { + $this->register( [ 'dependency_check' => static fn() => false ] ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assertArrayHasKey( 'give-recurring:dependency', $this->notice_queue() ); + } + + public function test_it_skips_when_the_guard_constant_is_already_defined(): void { + $constant = $this->define_guard( 'ABSORBER_ALREADY_LOADED_GUARD' ); + + $this->register( [], $constant ); + + $this->runner()->load_all(); + + $this->assertSame( + 0, + $this->bundled_plugin_loads(), + 'A defined constant means the code is already present.' + ); + } + + public function test_it_skips_when_the_bundled_file_is_missing(): void { + $this->expect_incorrect_usage(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * A broken build is a developer problem, not a site-owner one. It must not reach the notice + * queue, where it would render the host's own dependency_notice_message and send the owner after + * a dependency that is perfectly fine. + * + * The message is configured as a callable because that is the only shape the key takes: a string + * is refused outright, so that a host's __() cannot run while it builds its config array. + */ + public function test_a_missing_bundled_file_reports_to_the_developer_not_the_site_owner(): void { + $this->expect_incorrect_usage(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), + 'dependency_notice_message' => static fn() => 'GiveWP 3.0 or later is required.', + ] + ); + + $this->runner()->load_all(); + + $this->assertSame( [], $this->notice_queue() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * file_exists() is true for a directory, and require_once fatals on one. + */ + public function test_it_skips_when_the_bundled_path_is_a_directory(): void { + $this->expect_incorrect_usage(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => sys_get_temp_dir(), + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * file_exists() is also true for a file the process cannot read, and require_once fatals. + */ + public function test_it_skips_when_the_bundled_file_is_unreadable(): void { + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + chmod( $path, 0000 ); + + if ( is_readable( $path ) ) { + $this->markTestSkipped( 'Running as a user that can read a 0000 file.' ); + } + + $this->expect_incorrect_usage(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The dependency check calls an arbitrary host callable, so it must not run for a sub-plugin + * whose code is already present — and must not warn that requirements are unmet for a plugin the + * admin can see running. + */ + public function test_an_already_loaded_sub_plugin_is_not_dependency_checked(): void { + $constant = $this->define_guard( 'ABSORBER_LOADED_BEFORE_DEPS_GUARD' ); + + $checked = 0; + $this->register( + [ + 'dependency_check' => static function () use ( &$checked ) { + ++$checked; + + return false; + }, + ], + $constant + ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $checked ); + $this->assertSame( [], $this->notice_queue(), 'No notice for a plugin that is already running.' ); + } + + public function test_the_should_load_filter_can_veto_the_load(): void { + $this->register(); + + add_filter( 'give/plugin_absorber/should_load', '__return_false' ); + + $this->runner()->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads() ); + } + + public function test_the_should_load_filter_receives_the_sub_plugin(): void { + $this->register(); + + $received = null; + add_filter( + 'give/plugin_absorber/should_load', + static function ( $should_load, $sub_plugin ) use ( &$received ) { + $received = $sub_plugin; + + return $should_load; + }, + 10, + 2 + ); + + $this->runner()->load_all(); + + $this->assertInstanceOf( Sub_Plugin::class, $received ); + $this->assertSame( 'give-recurring', $received->get_slug() ); + } + + /** + * The filter is the last gate before require_once, so it must not be consulted for a sub-plugin + * that was already going to be skipped — a host counting its invocations would otherwise see calls + * for loads that never happen. + */ + public function test_the_should_load_filter_is_not_consulted_for_a_disabled_sub_plugin(): void { + $this->register( [ 'enabled' => false ] ); + + $this->record_should_load_calls(); + + $this->runner()->load_all(); + + $this->assertSame( [], $this->should_load_calls ); + + $this->assert_the_should_load_recorder_works(); + } + + /** + * The same guarantee at the far end of the chain: a sub-plugin that failed the dependency check + * has already earned its notice, and asking the filter as well would offer a host a veto over a + * load that was never going to happen. + */ + public function test_the_should_load_filter_is_not_consulted_when_dependencies_are_unmet(): void { + $this->register( [ 'dependency_check' => static fn() => false ] ); + + $this->record_should_load_calls(); + + $this->runner()->load_all(); + + $this->assertSame( [], $this->should_load_calls ); + + $this->assert_the_should_load_recorder_works(); + } + + public function test_it_loads_every_registered_sub_plugin(): void { + $this->register( [ 'slug' => 'give-recurring' ] ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + $this->runner()->load_all(); + + $this->assertSame( 2, $this->bundled_plugin_loads() ); + } + + /** + * One sub-plugin failing its checks must not stop the rest, or the failure order would decide + * which plugins a site gets. + */ + public function test_a_skipped_sub_plugin_does_not_stop_the_others(): void { + $this->register( [ 'slug' => 'give-recurring', 'enabled' => false ] ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + /** + * Registrar_Interface::all() can only declare `array`, so a host implementation is free to return + * anything. The default Registrar cannot produce this state — only a bound one can. + */ + public function test_it_ignores_entries_that_are_not_sub_plugins(): void { + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + $sub_plugin = new Sub_Plugin( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + $registrar = new class( $sub_plugin ) implements Registrar_Interface { + /** + * @var array + */ + private $entries; + + public function __construct( Sub_Plugin $sub_plugin ) { + $this->entries = [ + 'junk' => 'not-a-sub-plugin', + 'nope' => 42, + 'real' => $sub_plugin, + ]; + } + + public function register( Sub_Plugin $sub_plugin ): void { + } + + public function all(): array { + // The interface can only declare `array`, and its docblock is a promise a host is free + // to break — which is exactly what this double is here to do. Restating the promised + // shape is what lets the analyser check every other implementation strictly while this + // one hands back the junk the load path has to survive. + /** @var array $entries */ + $entries = $this->entries; + + return $entries; + } + }; + + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $registrar ): Registrar_Interface { + return $registrar; + } + ); + $this->set_up_container( $container ); + + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + /** + * require_once dedupes by resolved path, so one file behind two registrations executes once even + * when the second one's guard constant never gets defined. + */ + public function test_one_bundled_file_behind_two_registrations_loads_once(): void { + $path = $this->make_bundled_plugin_file( $this->make_guard_constant() ); + + foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $slug ) { + Loader::register( + [ + 'slug' => $slug, + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $this->make_guard_constant(), + ] + ); + } + + $this->runner()->load_all(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + /** + * The load path needs the prefix for the should_load filter and for the notice store. Throwing + * out of a core action would take the whole site down over a bootstrap mistake, so it is reported + * where a developer will see it and the load is abandoned instead. + */ + public function test_load_all_does_nothing_without_a_hook_prefix(): void { + $this->register(); + + $runner = $this->runner(); + $container = $this->container(); + + // The prefix goes, the container stays: this is about the missing prefix, and a library that + // reached the container first would fail this test for the other reason. + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + $runner->load_all(); + + $this->assertSame( 0, $this->bundled_plugin_loads(), 'A bootstrap mistake must not fatal the site.' ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The same guarantee for the read itself. Reading flushes the registration buffer, and the + * registrar refuses a slug it already holds — a throw that arrives inside plugins_loaded, where it + * would take down the front end and wp-admin together and lock the developer out of the screen + * where the duplicate registration could be undone. + */ + public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_request(): void { + // Two registrations of the default slug, each with a bundled fixture of its own — one file + // behind both would load once for the second registration and hide the skip under a dedupe. + $this->register(); + $this->register(); + + $this->expect_incorrect_usage(); + $this->record_incorrect_usage_messages(); + + $this->runner()->load_all(); + + // Reaching this line at all is half of what is under test: load_all() has to return. + $this->assertSame( + 0, + $this->bundled_plugin_loads(), + 'A read that failed has no list to load from, so nothing may load.' + ); + $this->assert_the_library_reported_incorrect_usage(); + $this->assert_a_reported_message_contains( + 'give-recurring', + 'The report has to name the slug, or it could have been raised for any other reason.' + ); + } + + /** + * Registration is buffered, which is what lets the container arrive after the sub-plugins do — + * and the registry is only half of that: the load path resolves the notice queue as well. A + * collaborator pinned by an eager resolve inside register() would leave the host's binding bound + * and never used. + */ + public function test_a_container_set_after_register_reaches_the_load_path(): void { + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + + $this->register( [ 'dependency_check' => static fn() => false ] ); + + $notices = new Spy_Queue(); + + // The registrar is bound as well as the queue. Without it the sub-plugin would sit in the + // default registrar either way and the notice would arrive however register() behaved, so the + // test would pass without the buffer existing at all. + $registrar = new Spy_Registrar(); + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $registrar ): Registrar_Interface { + return $registrar; + } + ); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Queue_Interface { + return $notices; + } + ); + $this->set_up_container( $container ); + + $this->runner()->load_all(); + + $this->assertSame( + [ 'give-recurring' ], + $notices->dependency_notices, + 'A binding made after register() has to reach the load path.' + ); + $this->assertSame( + [], + $this->notice_queue(), + 'The default queue must not have been resolved alongside it.' + ); + } + + /** + * The runner as the container builds it, which is how the scheduler reaches it too. + * + * @return Runner + */ + private function runner(): Runner { + return $this->resolve( Runner::class ); + } + + /** + * Record every should_load call, so a test can assert there were none. + * + * @return void + */ + private function record_should_load_calls(): void { + $calls = &$this->should_load_calls; + + add_filter( + 'give/plugin_absorber/should_load', + static function ( $should_load ) use ( &$calls ) { + $calls[] = $should_load; + + return $should_load; + } + ); + } + + /** + * Show the recorder works, having just asserted it caught nothing. + * + * Without this, a filter that never attached — a mistyped hook name, an add_filter() that ran too + * late — leaves the array empty for a reason that has nothing to do with the gate under test, and + * the assertion passes having proved nothing at all. + * + * @return void + */ + private function assert_the_should_load_recorder_works(): void { + apply_filters( 'give/plugin_absorber/should_load', true ); + + $this->assertSame( + [ true ], + $this->should_load_calls, + 'The recorder must catch a call that really happened.' + ); + } + + /** + * Keep the *message* of every incorrect-usage report, which the shared trait deliberately does not. + * + * `WithIncorrectUsage` pins that the library reported something against itself, which is all most + * tests need. A test about one particular failure needs more: a report raised for an unrelated + * reason — no hook prefix, no container — would otherwise satisfy it just as well. + * + * @return void + */ + private function record_incorrect_usage_messages(): void { + $messages = &$this->incorrect_usage_messages; + + $listener = static function ( $function_name, $message ) use ( &$messages ): void { + $messages[] = is_string( $message ) ? $message : ''; + }; + + $this->incorrect_usage_message_listener = $listener; + + add_action( 'doing_it_wrong_run', $listener, 10, 2 ); + } + + /** + * @param string $needle Text one report has to carry. + * @param string $message Why it has to. + * + * @return void + */ + private function assert_a_reported_message_contains( string $needle, string $message ): void { + $this->assertNotSame( [], $this->incorrect_usage_messages, 'Nothing was reported at all.' ); + $this->assertStringContainsString( + $needle, + implode( PHP_EOL, $this->incorrect_usage_messages ), + $message + ); + } + + /** + * Take the recorder back off. Call from tearDown, for the same reason the trait's own removal is + * there: a failing assertion would otherwise leave it listening for the rest of the process. + * + * Removed by identity rather than by clearing the hook, which WordPress and the rest of the suite + * are also on. + * + * @return void + */ + private function stop_recording_incorrect_usage_messages(): void { + if ( $this->incorrect_usage_message_listener !== null ) { + remove_action( 'doing_it_wrong_run', $this->incorrect_usage_message_listener ); + + $this->incorrect_usage_message_listener = null; + } + + $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. + * + * 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; + } + + /** + * @param array $overrides Config overrides. + * @param string|null $constant Guard constant to use, or a fresh one. + * + * @return string + */ + private function register( array $overrides = [], ?string $constant = null ): string { + $constant = $constant ?? $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + + Loader::register( + array_merge( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ], + $overrides + ) + ); + + return $constant; + } +} diff --git a/tests/unit/LoaderResolveTest.php b/tests/unit/LoaderTest.php similarity index 53% rename from tests/unit/LoaderResolveTest.php rename to tests/unit/LoaderTest.php index 2f7b83f..01e8436 100644 --- a/tests/unit/LoaderResolveTest.php +++ b/tests/unit/LoaderTest.php @@ -11,128 +11,89 @@ use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Loader; +use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Queue; use Nexcess\PluginAbsorber\Registrar; 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_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; +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. + * + * 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. + * * @since 1.0.0 */ -class LoaderResolveTest extends WPTestCase { +class LoaderTest extends WPTestCase { + use WithContainer; + use WithIncorrectUsage; + public function setUp(): void { parent::setUp(); Loader_State::reset(); Config_State::reset(); + Config::set_hook_prefix( 'give' ); } public function tearDown(): void { + $this->stop_expecting_incorrect_usage(); Loader_State::reset(); Config_State::reset(); + $this->tear_down_container(); parent::tearDown(); } - public function test_it_falls_back_to_the_default_registrar_without_a_container(): void { - $this->assertInstanceOf( Registrar::class, Loader::registrar() ); - } - - public function test_it_memoizes_the_resolved_collaborator(): void { - $this->assertSame( Loader::registrar(), Loader::registrar() ); - } - /** - * Both binding styles arrive at the same instance: the memo is what makes even a bind(), which - * the container would otherwise rebuild on every call, resolve exactly once. - * - * @dataProvider container_binding_methods + * @dataProvider collaborator_accessors * - * @param string $binding_method Container method the host bound the registrar with. + * @param string $accessor Static method on Loader. + * @param class-string $expected Class the provider's default binding builds. */ - public function test_it_resolves_a_bound_registrar_from_the_container( string $binding_method ): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->{$binding_method}( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); + public function test_the_accessors_read_from_the_container( string $accessor, string $expected ): void { + $this->set_up_container(); - $this->assertSame( $bound, Loader::registrar() ); - $this->assertSame( $bound, Loader::registrar(), 'The binding must be resolved only once.' ); + $this->assertInstanceOf( $expected, Loader::{$accessor}() ); } /** - * @return Generator + * A host binds the wrong class far more easily than it binds none at all, and the container + * hands back whatever it was told to without checking. Left to PHP's own return-type check the + * failure is a TypeError naming this library's method, raised inside plugins_loaded, which + * reads as a bug here rather than as the typo it is. */ - public static function container_binding_methods(): Generator { - yield 'singleton' => [ 'singleton' ]; - yield 'bind' => [ 'bind' ]; - } - - public function test_it_ignores_a_container_with_no_binding(): void { + public function test_a_binding_that_does_not_implement_its_interface_is_reported(): void { $container = new Test_Container(); - $this->assertFalse( - $container->has( Registrar_Interface::class ), - 'DI52 reports has() true for any existing class name; this must stay an interface.' - ); + $container->singleton( Registrar_Interface::class, static function (): object { + return new class() { + // Anything at all, so long as it is not a registrar. + }; + } ); Config::set_container( $container ); - $this->assertInstanceOf( Registrar::class, Loader::registrar() ); - } - - /** - * Checked before the instance is memoized. Caching it would make every accessor throw a - * TypeError blaming the library, with no way back. - * - * @dataProvider unusable_bindings - * - * @param mixed $bound Whatever the host's factory hands back. - * @param string $reported_type How the rejection is expected to name it. - */ - public function test_it_rejects_a_binding_that_does_not_implement_the_interface( - $bound, - string $reported_type - ): void { - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - try { Loader::registrar(); $this->fail( 'Expected a Config_Exception.' ); } catch ( Config_Exception $exception ) { $this->assertStringContainsString( Registrar_Interface::class, $exception->getMessage() ); $this->assertStringContainsString( - $reported_type, + 'does not implement', $exception->getMessage(), - 'The message has to name what came back, or the host cannot tell which binding is wrong.' + 'A binding that built fine and is simply the wrong type must not be reported as one ' + . 'the container could not build: the two send the host to different files.' ); } - - Config_State::reset(); - - $this->assertInstanceOf( - Registrar::class, - Loader::registrar(), - 'The bad instance must not have been cached, or there would be no way back.' - ); - } - - /** - * A factory is free to return anything at all, and the two non-object cases are the ones that - * reach the branch reporting a type name rather than a class name. - * - * @return Generator - */ - public static function unusable_bindings(): Generator { - yield 'an instance of the wrong class' => [ new stdClass(), 'stdClass' ]; - yield 'the class name instead of an instance' => [ Registrar::class, 'string' ]; - yield 'nothing at all' => [ null, 'NULL' ]; } /** @@ -150,7 +111,7 @@ static function () use ( $failure ): Registrar_Interface { throw $failure; } ); - Config::set_container( $container ); + $this->set_up_container( $container ); try { Loader::registrar(); @@ -167,69 +128,92 @@ static function () use ( $failure ): Registrar_Interface { 'The original failure has to stay reachable, or the real cause is lost.' ); } - - Config_State::reset(); - - $this->assertInstanceOf( - Registrar::class, - Loader::registrar(), - 'A binding that could not be built must not have been memoized.' - ); } /** - * A binding that could not be built leaves the registrations buffered, so the read that comes - * after the host fixes its container still has them. Emptying the buffer before the registrar - * resolved would drop them silently and load nothing. + * The missing-container report is this library's own sentence, in its own words. Wrapping it in + * the build-failure message would bury it a level deeper and name an interface the host never + * bound anything to, when what it has to hear is that it set no container at all. */ - public function test_registrations_survive_a_container_that_throws(): void { - $container = new Test_Container(); - $container->singleton( - Registrar_Interface::class, - static function (): Registrar_Interface { - throw new RuntimeException( 'not today' ); - } - ); - Config::set_container( $container ); - - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - + public function test_a_missing_container_is_not_reported_as_a_failed_binding(): void { try { - Loader::all(); + Loader::registrar(); $this->fail( 'Expected a Config_Exception.' ); } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( Registrar_Interface::class, $exception->getMessage() ); + $this->assertStringNotContainsString( 'failed to build', $exception->getMessage() ); + $this->assertNull( $exception->getPrevious(), 'There is no earlier failure to point at.' ); } + } - Config_State::reset(); + /** + * The accessors are the host's own way in, so each one has to survive the container becoming + * required rather than only the paths the library happens to take. + * + * @return Generator + */ + public static function collaborator_accessors(): Generator { + yield 'the registrar' => [ 'registrar', Registrar::class ]; + yield 'the notice queue' => [ 'notices', Queue::class ]; + } - $this->assertArrayHasKey( - 'give-recurring', - Loader::all(), - 'The buffered registration must still be there once the container is usable.' - ); + /** + * Nothing falls back to `new` any more. A host that never set a container gets a + * Config_Exception naming the mistake, not a default collaborator that quietly ignores the + * bindings it was going to make. + * + * @dataProvider accessor_names + * + * @param string $accessor Static method on Loader. + */ + public function test_an_accessor_without_a_container_is_a_configuration_error( string $accessor ): void { + $this->expectException( Config_Exception::class ); + + Loader::{$accessor}(); } /** - * The memo is what makes the container optional without paying for a lookup per call. Reading a - * collaborator is a boot-time act, so a host that has done it has finished configuring. + * @return Generator */ - public function test_a_container_set_after_the_first_resolve_does_not_change_the_memo(): void { - $default = Loader::registrar(); + public static function accessor_names(): Generator { + yield 'the registrar' => [ 'registrar' ]; + yield 'the notice queue' => [ 'notices' ]; + } + /** + * Whichever way the host bound it. `bind()` rebuilds per call where `singleton()` does not, and + * the accessor hands back the host's object either way — a library that cached the first resolve + * itself would make the difference invisible and the rebinding untestable. + * + * @dataProvider container_binding_methods + * + * @param string $binding_method Container method the host bound the registrar with. + */ + public function test_it_resolves_a_bound_registrar_from_the_container( string $binding_method ): void { $bound = new Spy_Registrar(); $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - $this->assertSame( - $default, - Loader::registrar(), - 'Swapping a collaborator mid-request would strand whatever already holds the old one.' + $container->{$binding_method}( + Registrar_Interface::class, + static function () use ( $bound ): Registrar_Interface { + return $bound; + } ); + $this->set_up_container( $container ); + + $this->assertSame( $bound, Loader::registrar() ); + $this->assertSame( $bound, Loader::registrar() ); + } + + /** + * @return Generator + */ + public static function container_binding_methods(): Generator { + yield 'singleton' => [ 'singleton' ]; + yield 'bind' => [ 'bind' ]; } public function test_register_builds_a_sub_plugin_and_stores_it(): void { + $this->set_up_container(); + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); $all = Loader::all(); @@ -240,14 +224,29 @@ public function test_register_builds_a_sub_plugin_and_stores_it(): void { } public function test_register_rejects_an_invalid_config(): void { + $this->set_up_container(); + $this->expectException( Config_Exception::class ); Loader::register( [ 'slug' => 'give-recurring' ] ); } /** - * Registering must not resolve anything, or the first register() call would pin the default - * registrar for the whole request. + * Registering must resolve nothing at all — not even the container. The host container LearnDash + * hands us is *replaced* at plugins_loaded 0, so anything that touched a container before that + * point holds an orphan whose bindings were thrown away. + */ + public function test_register_needs_no_container(): void { + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $this->set_up_container(); + + $this->assertArrayHasKey( 'give-recurring', Loader::all() ); + } + + /** + * The other half of the same guarantee: with a container set, the first register() must still not + * reach into it, or it would pin the registrar before the host finished binding. */ public function test_register_resolves_nothing_until_the_first_read(): void { $builds = 0; @@ -260,7 +259,7 @@ static function () use ( &$builds ): Registrar_Interface { return new Spy_Registrar(); } ); - Config::set_container( $container ); + $this->set_up_container( $container ); Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); @@ -271,32 +270,8 @@ static function () use ( &$builds ): Registrar_Interface { $this->assertSame( 1, $builds, 'The first read is what resolves the registrar.' ); } - /** - * The headline of deferring registration: the container is a configuration call like any other, - * so it may arrive after the sub-plugins do. Resolving inside register() dropped it silently. - */ - public function test_a_container_set_after_register_takes_effect(): void { - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - Loader::all(); - - $this->assertArrayHasKey( - 'give-recurring', - $bound->sub_plugins, - 'The bound registrar must receive registrations made before it was bound.' - ); - } - public function test_register_delegates_to_a_bound_registrar(): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); + $bound = $this->bind_registrar(); Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); Loader::all(); @@ -309,10 +284,7 @@ public function test_register_delegates_to_a_bound_registrar(): void { * already holds and the duplicate guard fires on a registration the host only made once. */ public function test_reading_twice_does_not_register_twice(): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); + $bound = $this->bind_registrar(); Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); @@ -327,6 +299,8 @@ public function test_reading_twice_does_not_register_twice(): void { * first read. It still names both bundled files, which is what the host needs to find them. */ public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { + $this->set_up_container(); + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); Loader::register( [ @@ -346,14 +320,54 @@ public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { } public function test_all_is_empty_before_anything_is_registered(): void { + $this->set_up_container(); + $this->assertSame( [], Loader::all() ); } /** - * The buffer is static state like the memo, so the suite's reset helper has to reach it too. - * A registration left buffered by one test drains into the next test's registrar. + * A binding that cannot be built leaves the registrations buffered, so the read that comes after + * the host fixes its container still has them. Emptying the buffer before the registrar resolved + * would drop them silently and load nothing. + */ + public function test_registrations_survive_a_registrar_the_container_cannot_build(): void { + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function (): Registrar_Interface { + throw new RuntimeException( 'the host factory needed a database connection' ); + } + ); + $this->set_up_container( $container ); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $failed = false; + + try { + Loader::all(); + } catch ( Config_Exception $exception ) { + $failed = true; + } + + $this->assertTrue( $failed, 'A registrar that cannot be built has to surface, not be swallowed.' ); + + $this->set_up_container(); + + $this->assertArrayHasKey( + 'give-recurring', + Loader::all(), + 'The buffered registration must still be there once the container is usable.' + ); + } + + /** + * The buffer is static state, so the suite's reset helper has to reach it: a registration left + * buffered by one test drains into the next test's registrar. */ public function test_the_state_helper_clears_buffered_registrations(): void { + $this->set_up_container(); + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); Loader_State::reset(); @@ -361,6 +375,64 @@ public function test_the_state_helper_clears_buffered_registrations(): void { $this->assertSame( [], Loader::all() ); } + 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 ); + + Loader::render_notices(); + + $this->assertSame( 1, $notices->render_calls ); + } + + public function test_render_notices_does_nothing_without_a_hook_prefix(): void { + $container = $this->set_up_container(); + + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + ob_start(); + + try { + Loader::render_notices(); + } finally { + // In a finally block so a throw cannot leave the suite's own output trapped in an + // abandoned buffer. + $output = (string) ob_get_clean(); + } + + $this->assertSame( '', $output ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * Bind a recording registrar, in the order a host binds one: before the provider fills in what is + * missing. + * + * @return Spy_Registrar + */ + private function bind_registrar(): Spy_Registrar { + $bound = new Spy_Registrar(); + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $bound ): Registrar_Interface { + return $bound; + } + ); + + $this->set_up_container( $container ); + + return $bound; + } + /** * Build the raw configuration array a host writes. * diff --git a/tests/unit/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php index b8dd041..8551bd4 100644 --- a/tests/unit/Notices/QueueTest.php +++ b/tests/unit/Notices/QueueTest.php @@ -59,7 +59,7 @@ public function tearDown(): void { } public function test_the_default_notices_satisfy_the_contract(): void { - $this->assertInstanceOf( Queue_Interface::class, new Queue() ); + $this->assertInstanceOf( Queue_Interface::class, $this->make_queue() ); } /** @@ -78,7 +78,7 @@ public function test_it_queues_a_notice( string $expected, bool $exact ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->{$method}( $this->make_sub_plugin( $overrides ) ); $queue = $this->queue(); @@ -159,7 +159,7 @@ public static function queued_notices(): Generator { * already happened, the other asks the user to do it. Sharing a default would be wrong. */ public function test_the_merge_and_conflict_defaults_differ(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_conflict_notice( $this->make_sub_plugin() ); @@ -169,7 +169,7 @@ public function test_the_merge_and_conflict_defaults_differ(): void { } public function test_a_configured_message_is_used_for_both_conflict_types(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); @@ -184,7 +184,7 @@ public function test_a_configured_message_is_used_for_both_conflict_types(): voi } public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_merge_notice( $this->make_sub_plugin() ); @@ -192,7 +192,7 @@ public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): } public function test_one_slug_can_hold_notices_of_different_types(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_dependency_notice( $this->make_sub_plugin() ); @@ -200,7 +200,7 @@ public function test_one_slug_can_hold_notices_of_different_types(): void { } public function test_different_slugs_do_not_collide(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'slug' => 'give-fee-recovery' ] ) ); @@ -212,7 +212,7 @@ public function test_different_slugs_do_not_collide(): void { } public function test_render_outputs_dismissible_markup(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -230,7 +230,7 @@ public function test_render_outputs_dismissible_markup(): void { * @param string $class Expected `notice-*` class. */ public function test_render_uses_the_severity_of_the_notice_type( string $method, string $class ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->{$method}( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Something happened.' ] ) ); @@ -251,7 +251,7 @@ public static function notice_severities(): Generator { } public function test_render_strips_a_script_from_the_message(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Careful.' ] ) ); @@ -270,7 +270,7 @@ public function test_render_strips_a_script_from_the_message(): void { * typically — while the event handler a message must never be able to ship is stripped. */ public function test_render_keeps_a_link_but_not_an_event_handler(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'See the docs.' ] @@ -284,7 +284,7 @@ public function test_render_keeps_a_link_but_not_an_event_handler(): void { } public function test_render_clears_the_queue(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->render_to_string( $notices ); @@ -294,7 +294,7 @@ public function test_render_clears_the_queue(): void { } public function test_render_outputs_every_queued_notice(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'First.' ] ) ); @@ -309,13 +309,13 @@ public function test_render_outputs_every_queued_notice(): void { } public function test_render_outputs_nothing_when_the_queue_is_empty(): void { - $this->assertSame( '', $this->render_to_string( new Queue() ) ); + $this->assertSame( '', $this->render_to_string( $this->make_queue() ) ); } /** * Where notices are kept is a constructor argument, so a host can move the queue somewhere else - * without also taking on how notices are worded or drawn. Both arguments default, so - * `new Queue()` — which is what Loader::resolve() builds — is unaffected. + * without also taking on how notices are worded or drawn. Both arguments are required and both + * are bound by `Provider`, so a host rebinds the store rather than subclassing the queue. */ public function test_a_replacement_store_is_used_instead_of_the_option(): void { $store = new class() extends Store { @@ -349,7 +349,7 @@ public function clear(): void { } }; - ( new Queue( $store ) )->queue_merge_notice( + $this->make_queue( $store )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -372,7 +372,7 @@ public function render( array $queue ): void { } }; - $notices = new Queue( null, $renderer ); + $notices = $this->make_queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( '

1

', $this->render_to_string( $notices ) ); @@ -400,7 +400,7 @@ public function render( array $queue ): void { } }; - $notices = new Queue( null, $renderer ); + $notices = $this->make_queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); wp_set_current_user( $this->create_user( 'subscriber' ) ); @@ -420,7 +420,7 @@ public function render( array $queue ): void { * @param string|null $role Role to render as, or null for a logged-out visitor. */ public function test_render_does_nothing_for_a_user_who_cannot_activate_plugins( ?string $role ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -450,7 +450,7 @@ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue( $this->markTestSkipped( 'Outside multisite an administrator simply has activate_plugins.' ); } - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -469,7 +469,7 @@ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue( * transient-backed design this class exists to avoid. */ public function test_the_queue_is_a_durable_database_row(): void { - ( new Queue() )->queue_merge_notice( + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -481,7 +481,7 @@ public function test_the_queue_is_a_durable_database_row(): void { wp_cache_flush(); - $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( new Queue() ) ); + $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( $this->make_queue() ) ); } /** @@ -495,7 +495,7 @@ public function test_the_queue_is_a_durable_database_row(): void { public function test_render_ignores_anything_that_is_not_a_message( $stored, ?string $present, array $absent ): void { $this->seed_queue( $stored ); - $output = $this->render_to_string( new Queue() ); + $output = $this->render_to_string( $this->make_queue() ); if ( $present === null ) { $this->assertSame( '', $output ); @@ -544,7 +544,7 @@ public static function malformed_queues(): Generator { public function test_a_corrupted_queue_heals_on_the_next_write(): void { $this->seed_queue( [ 'a:merge' => [ 'nested' ] ] ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); } @@ -555,7 +555,7 @@ public function test_the_option_is_keyed_by_the_hook_prefix(): void { $this->assertSame( self::OPTION_FOR_OTHER_PREFIX, Queue::option_name() ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertIsArray( get_site_option( self::OPTION_FOR_OTHER_PREFIX, false ) ); $this->assertFalse( $this->queue_exists() ); @@ -566,7 +566,7 @@ public function test_queueing_needs_a_hook_prefix(): void { $this->expectException( Config_Exception::class ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); } /** @@ -578,7 +578,7 @@ public function test_the_queue_is_not_autoloaded(): void { $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); } - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); } @@ -675,6 +675,22 @@ private function create_user( string $role ): int { return $user_id; } + /** + * The queue as the container builds it, or with one half replaced. + * + * Both collaborators are required arguments — nothing in `src/` defaults a collaborator to a + * `new` of its own any more — so the standard pair is spelled out once here rather than in every + * test that only cares about what a notice says. + * + * @param Store|null $store Where the queue is kept. + * @param Renderer|null $renderer How a queued notice is drawn. + * + * @return Queue + */ + private function make_queue( ?Store $store = null, ?Renderer $renderer = null ): Queue { + return new Queue( $store ?? new Store(), $renderer ?? new Renderer() ); + } + private function render_to_string( Queue $notices ): string { ob_start(); diff --git a/tests/unit/PluginCheckerTest.php b/tests/unit/PluginCheckerTest.php new file mode 100644 index 0000000..a7c16a8 --- /dev/null +++ b/tests/unit/PluginCheckerTest.php @@ -0,0 +1,106 @@ +checker = new Plugin_Checker(); + } + + public function test_it_implements_the_contract(): void { + $this->assertInstanceOf( Plugin_Checker_Interface::class, $this->checker ); + } + + public function test_it_reports_an_active_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', true ); + + $this->assertTrue( $this->checker->is_active( 'give-recurring/give-recurring.php' ) ); + } + + public function test_it_reports_an_inactive_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', false ); + + $this->assertFalse( $this->checker->is_active( 'give-recurring/give-recurring.php' ) ); + } + + /** + * The basename is what reaches deactivate_plugins() next, so asserting only the return value + * would let the wrong plugin be turned off unnoticed. + */ + public function test_it_passes_the_basename_through_to_wordpress(): void { + $received = null; + + $this->setFunctionReturn( + 'is_plugin_active', + static function ( $basename ) use ( &$received ) { + $received = $basename; + + return true; + }, + true + ); + + $this->checker->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 'give-recurring/give-recurring.php', $received ); + } + + /** + * is_plugin_active() already ORs in the network check, so one call answers both scopes. A second + * get_site_option() per sub-plugin per request would buy nothing. + */ + public function test_the_active_check_costs_one_call(): void { + $calls = 0; + + $this->setFunctionReturn( + 'is_plugin_active', + static function () use ( &$calls ) { + ++$calls; + + return true; + }, + true + ); + $this->setFunctionReturn( + 'is_plugin_active_for_network', + static function () { + throw new LogicException( 'The network check is redundant and must not be called.' ); + }, + true + ); + + $this->checker->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 1, $calls ); + } +} diff --git a/tests/unit/PluginDeactivatorTest.php b/tests/unit/PluginDeactivatorTest.php new file mode 100644 index 0000000..94e25b3 --- /dev/null +++ b/tests/unit/PluginDeactivatorTest.php @@ -0,0 +1,62 @@ +deactivator = new Plugin_Deactivator(); + } + + public function test_it_implements_the_contract(): void { + $this->assertInstanceOf( Plugin_Deactivator_Interface::class, $this->deactivator ); + } + + /** + * Silent, and with no third argument. A noisy deactivation runs the standalone's own deactivation + * hook at plugins_loaded — where a routine flush_rewrite_rules() in it 404s every custom permalink + * on the site — and a computed $network_wide would skip one of the two scopes core's null default + * covers, stranding an entry for a plugin active in both. + */ + public function test_it_deactivates_silently_in_every_scope(): void { + $received = []; + + $this->setFunctionReturn( + 'deactivate_plugins', + static function ( ...$arguments ) use ( &$received ): void { + $received = $arguments; + }, + true + ); + + $this->deactivator->deactivate( 'give-recurring/give-recurring.php' ); + + $this->assertSame( [ 'give-recurring/give-recurring.php', true ], $received ); + } +} diff --git a/tests/unit/PluginStateTest.php b/tests/unit/PluginStateTest.php deleted file mode 100644 index 817423f..0000000 --- a/tests/unit/PluginStateTest.php +++ /dev/null @@ -1,128 +0,0 @@ -plugin_state = new Plugin_State(); - } - - public function test_it_implements_the_contract(): void { - $this->assertInstanceOf( Plugin_State_Interface::class, $this->plugin_state ); - } - - /** - * The loader builds every unbound collaborator with a bare `new`, so a constructor that grew a - * required argument would fatal at plugins_loaded rather than here. - */ - public function test_it_constructs_without_arguments(): void { - $this->assertInstanceOf( Plugin_State::class, new Plugin_State() ); - } - - public function test_it_reports_an_active_plugin(): void { - $this->setFunctionReturn( 'is_plugin_active', true ); - - $this->assertTrue( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); - } - - public function test_it_reports_an_inactive_plugin(): void { - $this->setFunctionReturn( 'is_plugin_active', false ); - - $this->assertFalse( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); - } - - /** - * The basename is what reaches deactivate_plugins(), so asserting only the return value would - * let the wrong plugin be turned off unnoticed. - */ - public function test_it_passes_the_basename_through_to_wordpress(): void { - $received = null; - - $this->setFunctionReturn( - 'is_plugin_active', - static function ( $basename ) use ( &$received ) { - $received = $basename; - - return true; - }, - true - ); - - $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); - - $this->assertSame( 'give-recurring/give-recurring.php', $received ); - } - - /** - * is_plugin_active() already ORs in the network check, so one call answers both scopes. A - * second get_site_option() per sub-plugin per request would buy nothing. - */ - public function test_the_active_check_costs_one_call(): void { - $calls = 0; - - $this->setFunctionReturn( - 'is_plugin_active', - static function () use ( &$calls ) { - ++$calls; - - return true; - }, - true - ); - $this->setFunctionReturn( - 'is_plugin_active_for_network', - static function () { - throw new \LogicException( 'The network check is redundant and must not be called.' ); - }, - true - ); - - $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); - - $this->assertSame( 1, $calls ); - } - - /** - * Silent, and with no third argument. A noisy deactivation runs the standalone's own - * deactivation hook at plugins_loaded, and a computed $network_wide would skip one of the two - * scopes core's null default covers. - */ - public function test_it_deactivates_silently_in_every_scope(): void { - $received = []; - - $this->setFunctionReturn( - 'deactivate_plugins', - static function ( ...$arguments ) use ( &$received ) { - $received = $arguments; - }, - true - ); - - $this->plugin_state->deactivate( 'give-recurring/give-recurring.php' ); - - $this->assertSame( [ 'give-recurring/give-recurring.php', true ], $received ); - } -} diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php new file mode 100644 index 0000000..9d07292 --- /dev/null +++ b/tests/unit/ProviderTest.php @@ -0,0 +1,214 @@ +assertInstanceOf( Provider_Interface::class, new Provider( new Test_Container() ) ); + } + + /** + * @dataProvider default_bindings + * + * @param string $id Id the library resolves. + * @param class-string $expected Class the container must build for it. + */ + public function test_it_binds_every_default( string $id, string $expected ): void { + $container = $this->registered_container(); + + $this->assertInstanceOf( $expected, $container->get( $id ) ); + } + + /** + * Every id the library asks the container for. A collaborator added without a line here is one + * the container cannot build, which is a fatal at plugins_loaded rather than a missing default. + * + * @return Generator + */ + public static function default_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class, Registrar::class ]; + yield 'the notice queue' => [ Queue_Interface::class, Queue::class ]; + yield 'the notice store' => [ Store::class, Store::class ]; + 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 load runner' => [ Runner::class, Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class, Scheduler::class ]; + } + + /** + * The registrar holds the registrations and the queue holds its store, so a binding rebuilt per + * call would hand the load loop a registry the flush never reached. The rest are stateless, and + * are here because a class id that resolves twice over is a class id the provider never bound: + * the container is autowiring it, which means the explicit factory never ran. + * + * @dataProvider single_instance_bindings + * + * @param string $id Id that must resolve to one instance per container. + */ + public function test_a_binding_resolves_to_one_instance( string $id ): void { + $container = $this->registered_container(); + + $this->assertSame( $container->get( $id ), $container->get( $id ) ); + } + + /** + * @return Generator + */ + public static function single_instance_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class ]; + yield 'the notice queue' => [ Queue_Interface::class ]; + yield 'the notice store' => [ Store::class ]; + yield 'the notice renderer' => [ Renderer::class ]; + yield 'the load runner' => [ Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class ]; + } + + /** + * A class id is the case the guard gets wrong for free: DI52 answers `has()` with + * `isBound() || class_exists()`, so every id below reports true before anything is bound. A + * provider trusting that reply binds none of them — the explicit factories written for a + * container that does not autowire included — and nothing says so, because a container that + * *does* autowire still hands back an object. + * + * @dataProvider class_id_bindings + * + * @param string $id Class id the provider must bind regardless. + */ + public function test_it_binds_a_class_id_the_container_reports_it_already_has( string $id ): void { + $container = new Test_Container(); + + $this->assertTrue( $container->has( $id ), 'has() is expected to be true for an existing class.' ); + $this->assertFalse( $container->isBound( $id ), 'Nothing has bound the id yet.' ); + + ( new Provider( $container ) )->register(); + + $this->assertTrue( $container->isBound( $id ) ); + } + + /** + * @return Generator + */ + public static function class_id_bindings(): Generator { + yield 'the notice store' => [ Store::class ]; + yield 'the notice renderer' => [ Renderer::class ]; + yield 'the load runner' => [ Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class ]; + } + + /** + * The guard, which is the whole reason a host can rebind anything: the provider runs over the + * host's own container, so binding first has to be binding last. + * + * @dataProvider host_bindings + * + * @param string $id Id the host bound first. + * @param object $bound Implementation the host bound. + */ + public function test_it_leaves_a_binding_the_host_made_first_alone( string $id, object $bound ): void { + $container = new Test_Container(); + $container->singleton( + $id, + static function () use ( $bound ): object { + return $bound; + } + ); + + ( new Provider( $container ) )->register(); + + $this->assertSame( $bound, $container->get( $id ) ); + } + + /** + * Interface ids only, which is as far as the guard reaches. Nothing can build an interface + * unprompted, so `has()` is true there only where a binding exists and the host's object + * survives. A class id cannot be covered: di52 answers `has()` for one with the same true + * whether or not anything was bound, so the provider cannot see the host's binding and + * replaces it — a host rebinding `Store`, `Renderer`, `Load\Runner` or `Boot\Scheduler` has to + * do it after boot. + * + * @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() ]; + } + + /** + * Registering twice is the shape a host lands in by wiring the provider from two entry points. + * The second pass must not replace instances the first pass already handed out. + */ + public function test_registering_twice_keeps_the_first_instances(): void { + $container = $this->registered_container(); + $registrar = $container->get( Registrar_Interface::class ); + + ( new Provider( $container ) )->register(); + + $this->assertSame( $registrar, $container->get( Registrar_Interface::class ) ); + } + + /** + * A container with the defaults registered into it, plus the self-binding a host container + * ordinarily offers. + * + * @return Test_Container + */ + private function registered_container(): Test_Container { + $container = new Test_Container(); + $container->singleton( + ContainerInterface::class, + static function () use ( $container ): ContainerInterface { + return $container; + } + ); + + ( new Provider( $container ) )->register(); + + return $container; + } +} diff --git a/tests/unit/SmokeTest.php b/tests/unit/SmokeTest.php index 11973f1..3704930 100644 --- a/tests/unit/SmokeTest.php +++ b/tests/unit/SmokeTest.php @@ -9,6 +9,7 @@ use Codeception\TestCase\WPTestCase; use Nexcess\PluginAbsorber\Tests\Support\TestException; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithHaltedRedirects; use lucatume\WPBrowser\Traits\UopzFunctions; /** @@ -16,6 +17,7 @@ */ class SmokeTest extends WPTestCase { use UopzFunctions; + use WithHaltedRedirects; /** * Message carried by the exception that stands in for exit(). @@ -77,6 +79,30 @@ static function () use ( $message ) { $this->assertTrue( $halted, 'A stubbed function must be able to throw in place of exit().' ); } + /** + * The shared form of the same mechanism, proven here rather than on its first real caller. + * + * `capture_redirect()` is what every test of a redirecting branch uses, so a fault in it would + * show up as a failure in whatever code path happened to be under test — and the two parts of it + * that matter most, the fail() when nothing halted and the message match, are exactly the parts + * whose absence is silent. + * + * @since 1.0.0 + */ + public function test_the_shared_helper_captures_a_halted_redirect(): void { + $location = $this->capture_redirect( + static function (): void { + wp_safe_redirect( 'https://example.test/wp-admin/plugins.php' ); + + // Never reached: the stub throws first, which is the whole point. Production really + // does end the request here. + exit; + } + ); + + $this->assertSame( 'https://example.test/wp-admin/plugins.php', $location ); + } + /** * Proves the multisite env is actually a network, on its own tables. *