diff --git a/CLAUDE.md b/CLAUDE.md index e4f3764..571beac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,22 +68,33 @@ seams a host may rebind: | Interface | Bound to | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | -| `Notices\Contracts\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | +| `Notices\Contracts\Writer_Interface` | `Notices\Writer` | what each notice says | | `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | one method: which policy branch a conflict takes | | `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | deactivates the standalone, network-aware | | `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | answers whether a plugin is active | | `Contracts\Activator_Interface` | `Activator` | run-once activation-callback tracking | The rest — `Boot\Scheduler`, `Loader`, `Registry_Reader`, `Conflict\Detector`, `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. +`Conflict\Redirector`, `Notices\Store`, `Notices\Renderer`, `Notices\Presenter` — 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_Deactivator`, `Plugin_Checker`, `Activator` — plus `Provider_Interface`. +**`Notices\Writer` and `Notices\Presenter` split because they change for different reasons.** One +answers "what does this notice say", the other "who may see the pending set, and is it gone once they +have" — a host already running `stellarwp/admin-notices` has an opinion about the first and none about +the second. Only `Writer` earns `Writer_Interface`: wording is what a host rebinds, and nothing in the +library dispatches on how a notice reaches the screen, since the trampoline on `all_admin_notices` is +`Presenter`'s only caller — a host that wants no rendering of ours removes that callback rather than +binding a no-op. The capability check stays on `Presenter`, next to the render-then-clear, rather than +moving to `Renderer` alongside the markup: it guards the clearing as much as the drawing, so deciding +those two separately would let a user who may not see the queue destroy it anyway, through a class +that never checked. + **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`, @@ -161,7 +172,7 @@ is not built yet. Currently: | `src/Activator.php` | Runs a sub-plugin's activation callback once ever, recorded in one option. | | `src/Conflict/` | `Detector` (whether a standalone is in the way), `Resolver` (which policy branch to take), `Gatekeeper` (which requests, and which users, may have one resolved), `Redirector` (where the user lands afterwards), `Contracts\Resolver_Interface`. | | `src/Traits/` | `Loads_Plugin_Functions` (pulls in `wp-admin/includes/plugin.php`), `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing). | -| `src/Notices/` | `Queue` (what a notice says, who may consume it), `Store` (keeps it), `Renderer` (draws it), `Contracts\Queue_Interface`. | +| `src/Notices/` | `Writer` (what a notice says, stored under `slug:type`), `Presenter` (who may consume it, render-then-clear), `Store` (keeps it), `Renderer` (draws it), `Contracts\Writer_Interface`. | | `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Activator_Interface`, `Config_Exception`. | ### Boot lifecycle @@ -211,7 +222,7 @@ loaded → dependencies met → file exists → `should_load` filter → `requir callback (only after a *successful* require). The activation callback is the last of those and runs through `Activator`, which `Loader` takes -as a constructor argument like the notice queue and the registry reader. Last, because a bundled plugin is included rather +as a constructor argument like the writer and the registry reader. Last, because a bundled plugin is included rather than activated: `register_activation_hook()` never fires for it, so the callback stands in for whatever that hook would have done, and it has to run with the plugin's own code already in memory. Only after a require that happened, because creating tables and seeding options for a sub-plugin @@ -241,7 +252,7 @@ the screens the mistaken registration would have to be corrected from. The container is no longer the other half of that. A pass is handed a reader that already holds its registrar, so a container that cannot supply one fails while the *pass* is being built — where an -unbuildable `Queue_Interface` or `Plugin_Checker_Interface` has always failed. Read-time and +unbuildable `Writer_Interface` or `Plugin_Checker_Interface` has always failed. Read-time and build-time failures stopped being the same event when the registry became an argument, and the registrar now fails like every other binding rather than being the one collaborator whose broken binding surfaced late and politely. @@ -249,7 +260,7 @@ binding surfaced late and politely. `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and `DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. It is the worked example of required injection — `Conflict\Detector` to say which sub-plugins are in -conflict, `Plugin_Deactivator_Interface` to turn the standalone off, `Queue_Interface` for the notice +conflict, `Plugin_Deactivator_Interface` to turn the standalone off, `Writer_Interface` for the notice and `Conflict\Redirector` for the destination, all four constructor arguments with no default — so the object a test builds is the object the provider builds, and a host's rebinding of either plugin seam reaches it, the deactivator directly and the checker through the detector, without the resolver @@ -297,7 +308,7 @@ resolve. The detector reports and changes nothing, so it is the cheap question t the expensive one. All three live in the step rather than in the resolver, so a host binding its own cannot drop one by omission — and a request that fails any of them never builds a resolver. The capability gate covers every policy, not just the destructive one, and that is free: the other -branches only queue a notice, and `Notices\Queue::render()` refuses to render *or clear* for a user +branches only queue a notice, and `Notices\Presenter::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 is normalised to `NOTICE_ONLY` through `Conflict_Policy::is_valid()` before the diff --git a/docs/configuration.md b/docs/configuration.md index 19ffa65..a01fd77 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -45,7 +45,7 @@ $container->singleton( Registrar_Interface::class, My_Registrar::class ); | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | -| `Notices\Contracts\Queue_Interface` | `Notices\Queue` | Queues and renders the admin notices. | +| `Notices\Contracts\Writer_Interface` | `Notices\Writer` | Words the admin notices. | | `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | Deactivates the standalone. | | `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | Answers whether a plugin is active. | | `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | Detects the active standalone and applies the policy. | diff --git a/docs/notices.md b/docs/notices.md index ddfa461..9ac78bd 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -13,7 +13,7 @@ the site owner is never told their plugin was turned off. ## Who sees them -`Notices\Queue::render()` prints the queue and then clears it, and it is gated on the +`Notices\Presenter::render()` prints the queue and then clears it, and it is gated on the `activate_plugins` capability. Since rendering consumes the queue, a user who cannot act on a notice must not be shown one — a subscriber loading their profile page would otherwise silently swallow the only warning an administrator was ever going to get. @@ -65,7 +65,12 @@ add_action( 'admin_init', function () { 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. `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. +The queue is four classes: `Notices\Writer` decides what a notice says, `Notices\Presenter` decides +who may consume it and does the render-then-clear, `Notices\Store` keeps it, `Notices\Renderer` draws +it. `Writer` takes `Store` as its only constructor argument and is the one bound behind an interface, +`Writer_Interface` — the seam for a host that already runs its own notices library and wants to reword +rather than replace the plumbing. `Presenter` takes `Store` and `Renderer` and is bound by class name: +nothing in the library dispatches on it, since the trampoline on `all_admin_notices` is its only +caller. Rebinding `Notices\Renderer` replaces the markup and leaves the storage alone; rebinding +`Notices\Store` does the reverse; rebinding `Writer_Interface` replaces the wording without touching +either. diff --git a/src/Absorber.php b/src/Absorber.php index 44069e0..b4a79d4 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -10,7 +10,8 @@ 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\Notices\Contracts\Writer_Interface; +use Nexcess\PluginAbsorber\Notices\Presenter; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; use Throwable; @@ -53,10 +54,10 @@ public static function registrar(): Registrar_Interface { * * @throws Config_Exception When no container has been set, or its binding is unusable. * - * @return Queue_Interface + * @return Writer_Interface */ - public static function notices(): Queue_Interface { - return self::collaborator( Queue_Interface::class ); + public static function notices(): Writer_Interface { + return self::collaborator( Writer_Interface::class ); } /** @@ -162,12 +163,12 @@ public static function render_notices(): void { return; } - // The queue is a rebindable seam and the messages inside it are host callables, so rendering - // runs somebody else's code -- on all_admin_notices, which every admin screen fires. A throw - // out of here would white-screen wp-admin, which is exactly where a site owner would go to - // undo whatever caused it. The notice is worth less than the screen it would be read on. + // The messages a presenter draws were worded by host callables, so rendering runs somebody + // else's code -- on all_admin_notices, which every admin screen fires. A throw out of here + // would white-screen wp-admin, which is exactly where a site owner would go to undo whatever + // caused it. The notice is worth less than the screen it would be read on. try { - self::notices()->render(); + self::collaborator( Presenter::class )->render(); } catch ( Throwable $thrown ) { _doing_it_wrong( self::class . '::render_notices', diff --git a/src/Conflict/Gatekeeper.php b/src/Conflict/Gatekeeper.php index 3ac7885..0958a7b 100644 --- a/src/Conflict/Gatekeeper.php +++ b/src/Conflict/Gatekeeper.php @@ -117,7 +117,7 @@ public function request_may_resolve(): bool { * be able to deactivate one, and a replacement that forgot to re-check would reopen exactly that. * * It gates every policy, not only the destructive one, and that costs nothing. The other - * policies queue a notice, and Notices\Queue::render() will not render -- or clear -- for a user + * policies queue a notice, and Notices\Presenter::render() will not render -- or clear -- for a user * with no plugin capability at all. Queuing on a request that cannot act only parks the notice * until an administrator who can act arrives, which is the request this gate lets resolution run * on anyway. Nothing is consumed or suppressed by waiting: the standalone is still there to diff --git a/src/Conflict/Resolver.php b/src/Conflict/Resolver.php index 1e9de2e..54f0c85 100644 --- a/src/Conflict/Resolver.php +++ b/src/Conflict/Resolver.php @@ -11,7 +11,7 @@ use Nexcess\PluginAbsorber\Conflict_Policy; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Registry_Reader; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; @@ -62,7 +62,7 @@ class Resolver implements Resolver_Interface { /** * @since 1.0.0 * - * @var Queue_Interface + * @var Writer_Interface */ private $notices; @@ -79,14 +79,14 @@ class Resolver implements Resolver_Interface { * @param Registry_Reader $registry Which sub-plugins are registered. * @param Detector $detector Whether a sub-plugin is in conflict. * @param Plugin_Deactivator_Interface $plugin_deactivator Turns the standalone off. - * @param Queue_Interface $notices Where the user is told what happened. + * @param Writer_Interface $notices Where the user is told what happened. * @param Redirector $redirector Where the user lands afterwards. */ public function __construct( Registry_Reader $registry, Detector $detector, Plugin_Deactivator_Interface $plugin_deactivator, - Queue_Interface $notices, + Writer_Interface $notices, Redirector $redirector ) { $this->registry = $registry; diff --git a/src/Loader.php b/src/Loader.php index b6b81ff..25c11d3 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -7,7 +7,7 @@ use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; use Throwable; @@ -33,7 +33,7 @@ class Loader { /** * @since 1.0.0 * - * @var Queue_Interface + * @var Writer_Interface */ private $notices; @@ -48,12 +48,12 @@ class Loader { * @since 1.0.0 * * @param Registry_Reader $registry Which sub-plugins are registered. - * @param Queue_Interface $notices Where a sub-plugin that could not load says so. + * @param Writer_Interface $notices Where a sub-plugin that could not load says so. * @param Activator_Interface $activator Runs the activation callback of one that did. */ public function __construct( Registry_Reader $registry, - Queue_Interface $notices, + Writer_Interface $notices, Activator_Interface $activator ) { $this->registry = $registry; diff --git a/src/Notices/Contracts/Queue_Interface.php b/src/Notices/Contracts/Writer_Interface.php similarity index 52% rename from src/Notices/Contracts/Queue_Interface.php rename to src/Notices/Contracts/Writer_Interface.php index e2b423e..b0fd92c 100644 --- a/src/Notices/Contracts/Queue_Interface.php +++ b/src/Notices/Contracts/Writer_Interface.php @@ -3,17 +3,25 @@ * @package Nexcess\PluginAbsorber */ +declare( strict_types=1 ); + namespace Nexcess\PluginAbsorber\Notices\Contracts; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Sub_Plugin; /** - * Admin notices raised by the absorber. Bind a replacement to render them your own way. + * What the absorber has to say about a sub-plugin. Bind a replacement to word or keep it your own way. + * + * The seam, and the only one in this folder: what a notice says is the thing a host has an opinion + * about, and a host already running `stellarwp/admin-notices` binds its own here. How a pending + * notice reaches the screen is `Notices\Presenter`'s, which is a class rather than a contract because + * nothing in the library dispatches on it — the trampoline on `all_admin_notices` is the only caller, + * and a host that wants it gone takes the callback off. * * @since 1.0.0 */ -interface Queue_Interface { +interface Writer_Interface { /** * Queue the "we deactivated the standalone for you" notice. * @@ -21,6 +29,9 @@ interface Queue_Interface { * re-queues it on a later request, so an implementation that drops it drops the only warning * the site owner gets. * + * Whatever an implementation writes has to survive the request that wrote it: the resolver + * redirects, so the notice is almost never read by the request that raised it. + * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin concerned. @@ -58,39 +69,10 @@ public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void; public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void; /** - * Render every queued notice, then clear the queue. - * - * Two obligations an implementation must honour. It has to survive the request that queued - * it, because the resolver redirects and the notice is almost never rendered by the request - * that raised it. And because rendering consumes the queue, it must not render for a user who - * cannot act on the notice — otherwise any logged-in user loading an admin page swallows a - * warning meant for an administrator. - * - * The queue is single-consumer. Rendering consumes it for everybody, so the first eligible - * administrator to load any admin screen is the only person who ever sees a given notice — - * network-wide on multisite, where the queue is one network option. An implementation that - * wants every administrator to see it has to track consumption per user itself. - * - * Messages may carry markup. They come from the host's own configuration or from its filters - * rather than from user input, so the default implementation prints them through - * `wp_kses_post()` — the standard WordPress post-content allowlist — and a link to a - * knowledge-base article, emphasis or a list reaches the screen intact while a script or an - * event handler attribute is stripped. An implementation bound in place of the default owns - * its own escaping. - * - * @since 1.0.0 - * - * @throws Config_Exception When no hook prefix has been set. - * - * @return void - */ - public function render(): void; - - /** - * Where these notices are kept, so a host can render them itself without replacing the queue. + * Where these notices are kept, so a host can render them itself without replacing the writer. * * On the contract rather than on the default implementation, and an instance method rather than - * a static one, because the honest answer depends on which queue a site is running: an + * a static one, because the honest answer depends on which writer a site is running: an * implementation bound in place of the default keeps its notices where it likes, and a host * reading a name off the default class would read an option nothing writes to. * diff --git a/src/Notices/Presenter.php b/src/Notices/Presenter.php new file mode 100644 index 0000000..a4807ed --- /dev/null +++ b/src/Notices/Presenter.php @@ -0,0 +1,101 @@ +store = $store; + $this->renderer = $renderer; + } + + /** + * Draw the queue, then consume it. + * + * The store rather than the bound writer, deliberately: this draws what the *default* writer + * kept, and a host that binds a writer of its own has taken over where notices live along with + * what they say. + * + * @since 1.0.0 + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + public function render(): void { + if ( ! current_user_can( self::CAPABILITY ) ) { + return; + } + + $queue = $this->store->all(); + + if ( $queue === [] ) { + return; + } + + $this->renderer->render( $queue ); + + $this->store->clear(); + } +} diff --git a/src/Notices/Renderer.php b/src/Notices/Renderer.php index bc4bb07..c75cdb3 100644 --- a/src/Notices/Renderer.php +++ b/src/Notices/Renderer.php @@ -30,9 +30,9 @@ class Renderer { * @var array */ private const CLASSES = [ - Queue::TYPE_MERGE => 'notice-warning', - Queue::TYPE_CONFLICT => 'notice-warning', - Queue::TYPE_DEPENDENCY => 'notice-error', + Writer::TYPE_MERGE => 'notice-warning', + Writer::TYPE_CONFLICT => 'notice-warning', + Writer::TYPE_DEPENDENCY => 'notice-error', ]; /** diff --git a/src/Notices/Store.php b/src/Notices/Store.php index e7890c0..795dbf2 100644 --- a/src/Notices/Store.php +++ b/src/Notices/Store.php @@ -17,7 +17,7 @@ * is raised once and never again, so losing it means a site owner is never told their plugin was * deactivated. This queue is not a cache. * - * Separate from Queue so that changing where notices are kept does not mean touching how they are + * Separate from Writer so that changing where notices are kept does not mean touching how they are * worded or drawn. * * @since 1.0.0 diff --git a/src/Notices/Queue.php b/src/Notices/Writer.php similarity index 62% rename from src/Notices/Queue.php rename to src/Notices/Writer.php index b122a0c..6b3edd5 100644 --- a/src/Notices/Queue.php +++ b/src/Notices/Writer.php @@ -3,29 +3,35 @@ * @package Nexcess\PluginAbsorber */ +declare( strict_types=1 ); + namespace Nexcess\PluginAbsorber\Notices; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; /** - * Default queue: option-backed, so it survives the resolver's redirect. + * What a notice says, and under which key it is kept. + * + * One reason to change: the wording. Where the queue is kept is `Store`'s, how a pending notice is + * drawn is `Renderer`'s, and who may consume the queue is `Presenter`'s — so a host can reword a + * sentence without reading any of them, and none of them has to be understood to answer "what does + * the merge notice say". * - * This class decides *what a notice says* and *who is allowed to consume the queue*. Where the - * 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. + * Option-backed through `Store`, so what is written here survives the resolver's redirect: the + * request that raises a notice is almost never the one that shows it. * - * 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 + * The collaborator is a required constructor argument, and `Provider` is what hands it 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 + * A host already using stellarwp/admin-notices can bind its own implementation of Writer_Interface * and read the same option, whose name is `option_name()`. * * @since 1.0.0 */ -class Queue implements Queue_Interface { +class Writer implements Writer_Interface { /** * Notice types. Public because they are the second half of a queue key — an entry is stored * under `slug:type`, and reading the queue yourself means matching against these. @@ -50,19 +56,6 @@ class Queue implements Queue_Interface { */ public const TYPE_DEPENDENCY = 'dependency'; - /** - * Capability required to see, and thereby consume, the queue. - * - * Rendering clears the queue, so a user who cannot act on a notice must not be shown one: - * a subscriber loading their profile page would otherwise silently swallow the only warning - * an administrator was ever going to get. - * - * @since 1.0.0 - * - * @var string - */ - private const CAPABILITY = 'activate_plugins'; - /** * @since 1.0.0 * @@ -73,19 +66,10 @@ class Queue implements Queue_Interface { /** * @since 1.0.0 * - * @var Renderer - */ - private $renderer; - - /** - * @since 1.0.0 - * - * @param Store $store Where the queue is kept. - * @param Renderer $renderer How a queued notice is drawn. + * @param Store $store Where the queue is kept. */ - public function __construct( Store $store, Renderer $renderer ) { - $this->store = $store; - $this->renderer = $renderer; + public function __construct( Store $store ) { + $this->store = $store; } /** @@ -148,35 +132,6 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { $this->queue( $sub_plugin, self::TYPE_DEPENDENCY, $sub_plugin->get_dependency_notice_message() ); } - /** - * Draw the queue, then consume it. - * - * The capability check stays here rather than in the renderer because it guards the clearing - * as much as the drawing: the two have to be decided together or a user who may not see the - * queue could still destroy it. - * - * @since 1.0.0 - * - * @throws Config_Exception When no hook prefix has been set. - * - * @return void - */ - public function render(): void { - if ( ! current_user_can( self::CAPABILITY ) ) { - return; - } - - $queue = $this->store->all(); - - if ( $queue === [] ) { - return; - } - - $this->renderer->render( $queue ); - - $this->store->clear(); - } - /** * @since 1.0.0 * diff --git a/src/Provider.php b/src/Provider.php index a3864f7..915fbd9 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -17,10 +17,11 @@ use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Loader; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; -use Nexcess\PluginAbsorber\Notices\Queue; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; +use Nexcess\PluginAbsorber\Notices\Presenter; use Nexcess\PluginAbsorber\Notices\Renderer; use Nexcess\PluginAbsorber\Notices\Store; +use Nexcess\PluginAbsorber\Notices\Writer; use StellarWP\ContainerContract\ContainerInterface; /** @@ -78,9 +79,16 @@ public function register(): void { // 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 ) ); + Writer_Interface::class, + static function () use ( $container ): Writer { + return new Writer( $container->get( Store::class ) ); + } + ); + + $this->bind_once( + Presenter::class, + static function () use ( $container ): Presenter { + return new Presenter( $container->get( Store::class ), $container->get( Renderer::class ) ); } ); @@ -108,7 +116,7 @@ static function () use ( $container ): Resolver { $container->get( Registry_Reader::class ), $container->get( Detector::class ), $container->get( Plugin_Deactivator_Interface::class ), - $container->get( Queue_Interface::class ), + $container->get( Writer_Interface::class ), $container->get( Redirector::class ) ); } @@ -119,7 +127,7 @@ static function () use ( $container ): Resolver { static function () use ( $container ): Loader { return new Loader( $container->get( Registry_Reader::class ), - $container->get( Queue_Interface::class ), + $container->get( Writer_Interface::class ), $container->get( Activator_Interface::class ) ); } diff --git a/tests/README.md b/tests/README.md index 53e089e..c834a4b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -41,7 +41,7 @@ guarantee those tests exist to pin: ```php $container = new Test_Container(); -$container->singleton( Queue_Interface::class, static fn() => $notices ); +$container->singleton( Writer_Interface::class, static fn() => $notices ); $this->set_up_container( $container ); ``` diff --git a/tests/_support/Spy_Presenter.php b/tests/_support/Spy_Presenter.php new file mode 100644 index 0000000..6c7fdc4 --- /dev/null +++ b/tests/_support/Spy_Presenter.php @@ -0,0 +1,63 @@ +render_calls` off a value typed as `Presenter` is reading a property the parent does not + * declare, and static analysis rightly rejects it. + * + * The parent constructor is deliberately not called. `render()` is overridden here, so the store and + * renderer the real class reads through would only be collaborators to build and hand over for + * nobody to use. + * + * @since 1.0.0 + */ +class Spy_Presenter extends Presenter { + /** + * How many times render() was called. + * + * @var int + */ + public $render_calls = 0; + + /** + * What render() throws instead of drawing anything, when a test asks it to. + * + * The screen this runs on is every admin screen, so a throw from inside render() is the failure + * the trampoline exists to swallow. + * + * @var Throwable|null + */ + public $failure; + + /** + * Built without a store or renderer: nothing here reads either one. + */ + public function __construct() { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedFunction + } + + /** + * @throws Throwable Whatever a test parked in $failure. + * + * @return void + */ + public function render(): void { + ++$this->render_calls; + + if ( $this->failure !== null ) { + throw $this->failure; + } + } +} diff --git a/tests/_support/Spy_Queue.php b/tests/_support/Spy_Writer.php similarity index 70% rename from tests/_support/Spy_Queue.php rename to tests/_support/Spy_Writer.php index da97a1b..57a0d34 100644 --- a/tests/_support/Spy_Queue.php +++ b/tests/_support/Spy_Writer.php @@ -3,28 +3,30 @@ * @package Nexcess\PluginAbsorber */ +declare( strict_types=1 ); + namespace Nexcess\PluginAbsorber\Tests\Support; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; /** - * A notice queue that records what was asked of it, for tests about who the library talks to. + * A notice writer that records what was asked of it, for tests about who the library talks to. * - * A named class rather than an anonymous one: a test reading `$spy->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. + * A named class rather than an anonymous one: a test reading `$spy->merge_notices` off a value typed + * as `Writer_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 + * It stores nothing, which is the point — a test that binds this one proves the default writer was * never resolved by asserting the option is still absent. * * @since 1.0.0 */ -class Spy_Queue implements Queue_Interface { +class Spy_Writer implements Writer_Interface { /** * The option this spy would keep notices in, if it kept any. * - * Deliberately not the default queue's name: a test that reads the real option while a spy is + * Deliberately not the default writer's name: a test that reads the real option while a spy is * bound is reading somewhere nothing was written, and should say so rather than agree. * * @var string @@ -52,13 +54,6 @@ class Spy_Queue implements Queue_Interface { */ public $dependency_notices = []; - /** - * How many times render() was called. - * - * @var int - */ - public $render_calls = 0; - /** * @param Sub_Plugin $sub_plugin Sub-plugin concerned. * @@ -86,13 +81,6 @@ 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; - } - /** * @return string */ diff --git a/tests/unit/AbsorberTest.php b/tests/unit/AbsorberTest.php index 91bab41..1b4db6b 100644 --- a/tests/unit/AbsorberTest.php +++ b/tests/unit/AbsorberTest.php @@ -12,13 +12,13 @@ use Nexcess\PluginAbsorber\Conflict\Resolver; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; -use Nexcess\PluginAbsorber\Notices\Queue; +use Nexcess\PluginAbsorber\Notices\Presenter; +use Nexcess\PluginAbsorber\Notices\Writer; use Nexcess\PluginAbsorber\Registrar; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; use Nexcess\PluginAbsorber\Tests\Support\Config_State; -use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Presenter; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; @@ -154,7 +154,7 @@ public function test_a_missing_container_is_not_reported_as_a_failed_binding(): */ public static function collaborator_accessors(): Generator { yield 'the registrar' => [ 'registrar', Registrar::class ]; - yield 'the notice queue' => [ 'notices', Queue::class ]; + yield 'the notice writer' => [ 'notices', Writer::class ]; yield 'the conflict resolver' => [ 'resolver', Resolver::class ]; } @@ -178,7 +178,7 @@ public function test_an_accessor_without_a_container_is_a_configuration_error( s */ public static function accessor_names(): Generator { yield 'the registrar' => [ 'registrar' ]; - yield 'the notice queue' => [ 'notices' ]; + yield 'the notice writer' => [ 'notices' ]; yield 'the conflict resolver' => [ 'resolver' ]; } @@ -378,39 +378,32 @@ public function test_the_state_helper_clears_buffered_registrations(): void { $this->assertSame( [], Absorber::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 ); + public function test_render_notices_delegates_to_the_bound_presenter(): void { + $presenter = $this->bind_presenter(); Absorber::render_notices(); - $this->assertSame( 1, $notices->render_calls ); + $this->assertSame( 1, $presenter->render_calls ); } /** - * The notice messages are host callables and the queue itself is a rebindable seam, so rendering - * runs host code — on `all_admin_notices`, which every admin screen fires. A throw out of it - * white-screens wp-admin, which is exactly where a site owner would go to undo whatever caused - * it, so it is reported and the render is abandoned instead. + * The notice messages are host callables and the presenter itself resolves whatever store and + * renderer are bound, so rendering runs host code — on `all_admin_notices`, which every admin + * screen fires. A throw out of it white-screens wp-admin, which is exactly where a site owner + * would go to undo whatever caused it, so it is reported and the render is abandoned instead. */ public function test_render_notices_cannot_end_the_admin_request(): void { $this->expect_incorrect_usage(); - $container = new Test_Container(); - $container->singleton( - Queue_Interface::class, - static function (): Queue_Interface { + // After the provider, because Presenter is a class id: a container reports it can build one + // whether or not anything was bound, so the provider rebinds it regardless and a factory + // handed in before set_up_container() would never run. + $this->set_up_container()->singleton( + Presenter::class, + static function (): Presenter { throw new RuntimeException( 'the notice option held something unreadable' ); } ); - $this->set_up_container( $container ); Absorber::render_notices(); @@ -438,6 +431,29 @@ public function test_render_notices_does_nothing_without_a_hook_prefix(): void { $this->assert_the_library_reported_incorrect_usage(); } + /** + * Bind a recording presenter into the container already standing. + * + * After the provider has run, not before it: the id is a class, and a container answers for + * every class that exists whether or not anything was bound to it. The provider cannot tell a + * host's deliberate binding from that, so it rebinds regardless, and a double handed in + * beforehand would be quietly replaced by the real presenter. + * + * @return Spy_Presenter + */ + private function bind_presenter(): Spy_Presenter { + $presenter = new Spy_Presenter(); + + $this->set_up_container()->singleton( + Presenter::class, + static function () use ( $presenter ): Presenter { + return $presenter; + } + ); + + return $presenter; + } + /** * Bind a recording registrar, in the order a host binds one: before the provider fills in what is * missing. diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php index 6e63c17..d74bc71 100644 --- a/tests/unit/Boot/SchedulerTest.php +++ b/tests/unit/Boot/SchedulerTest.php @@ -17,10 +17,10 @@ use Nexcess\PluginAbsorber\Conflict_Policy; use Nexcess\PluginAbsorber\Contracts\Plugin_Checker_Interface; use Nexcess\PluginAbsorber\Loader; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Presenter; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; use Nexcess\PluginAbsorber\Tests\Support\Config_State; -use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Presenter; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; @@ -466,13 +466,13 @@ static function () use ( &$loads_seen ): void { public function test_it_wires_the_notice_step_in_the_admin(): void { set_current_screen( 'dashboard' ); - $notices = $this->bind_spy_queue(); - Absorber::boot(); + $presenter = $this->bind_spy_presenter(); + do_action( 'all_admin_notices' ); - $this->assertSame( 1, $notices->render_calls ); + $this->assertSame( 1, $presenter->render_calls ); } /** @@ -482,10 +482,10 @@ public function test_it_wires_the_notice_step_in_the_admin(): void { public function test_it_does_not_wire_the_notice_step_on_the_front_end(): void { set_current_screen( 'front' ); - $notices = $this->bind_spy_queue(); - Absorber::boot(); + $presenter = $this->bind_spy_presenter(); + // 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; @@ -499,7 +499,7 @@ static function () use ( &$fired ): void { do_action( 'all_admin_notices' ); $this->assertTrue( $fired, 'The hook must really have been dispatched.' ); - $this->assertSame( 0, $notices->render_calls ); + $this->assertSame( 0, $presenter->render_calls ); } /** @@ -745,30 +745,27 @@ private function callbacks_at( string $hook, ?int $priority = null ): int { } /** - * Bind a recording queue in place of the default one. + * Bind a recording presenter in place of the default one, into the container already standing. * - * Bound before boot, the way a host rebinding the seam does it. That survives because the id is an - * interface: the provider skips an id the container can already answer for, and only a binding - * makes it answer for an interface. A class-id double bound here would not survive, since a - * container answers for every class that exists whether or not anything was bound to it — and it - * would be overwritten twice, once by the provider run in `set_up_container()` and again by the - * one inside `boot()`. + * Called *after* boot, unlike a host rebinding the writer's interface seam would be: the id is a + * class, and a container answers for every class that exists whether or not anything was bound to + * it, so the provider cannot tell a double apart from that and rebinds regardless. Late is safe + * here — the notice step is a named static trampoline that resolves its collaborator when it + * fires, not when it is wired. * - * @return Spy_Queue + * @return Spy_Presenter */ - 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; + private function bind_spy_presenter(): Spy_Presenter { + $presenter = new Spy_Presenter(); + + $this->container()->singleton( + Presenter::class, + static function () use ( $presenter ): Presenter { + return $presenter; } ); - $this->set_up_container( $container ); - - return $notices; + return $presenter; } /** diff --git a/tests/unit/Conflict/ResolverTest.php b/tests/unit/Conflict/ResolverTest.php index 881c4bc..51ebc34 100644 --- a/tests/unit/Conflict/ResolverTest.php +++ b/tests/unit/Conflict/ResolverTest.php @@ -19,12 +19,12 @@ use Nexcess\PluginAbsorber\Conflict_Policy; use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Registry_Reader; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; use Nexcess\PluginAbsorber\Tests\Support\Config_State; -use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Writer; use Nexcess\PluginAbsorber\Tests\Support\Stub_Registry_Reader; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\TestException; @@ -269,7 +269,7 @@ public function deactivate( string $basename ): void { } }; - $notices = new Spy_Queue(); + $notices = new Spy_Writer(); // The interface seams go in first, which is where a host binds them and where the guarantee // lives: nothing can build an interface unprompted, so the container answering to one means a @@ -282,8 +282,8 @@ static function () use ( $deactivator ): Plugin_Deactivator_Interface { } ); $container->singleton( - Queue_Interface::class, - static function () use ( $notices ): Queue_Interface { + Writer_Interface::class, + static function () use ( $notices ): Writer_Interface { return $notices; } ); diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index 21445bb..c9b966b 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -12,13 +12,13 @@ use Nexcess\PluginAbsorber\Contracts\Activator_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Loader; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Absorber_State; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Spy_Activator; -use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Writer; use Nexcess\PluginAbsorber\Tests\Support\Stub_Registry_Reader; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; @@ -138,7 +138,7 @@ public function test_it_loads_the_registry_it_was_handed(): void { ), ] ), - new Spy_Queue(), + new Spy_Writer(), new Spy_Activator() ); @@ -701,9 +701,9 @@ public function test_a_container_set_after_register_reaches_the_load_path(): voi $this->register( [ 'dependency_check' => static fn() => false ] ); - $notices = new Spy_Queue(); + $notices = new Spy_Writer(); - // The registrar is bound as well as the queue. Without it the sub-plugin would sit in the + // The registrar is bound as well as the writer. 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(); @@ -715,8 +715,8 @@ static function () use ( $registrar ): Registrar_Interface { } ); $container->singleton( - Queue_Interface::class, - static function () use ( $notices ): Queue_Interface { + Writer_Interface::class, + static function () use ( $notices ): Writer_Interface { return $notices; } ); diff --git a/tests/unit/Notices/PresenterTest.php b/tests/unit/Notices/PresenterTest.php new file mode 100644 index 0000000..0348c57 --- /dev/null +++ b/tests/unit/Notices/PresenterTest.php @@ -0,0 +1,384 @@ +clear_queue(); + + // render() consumes the queue, so it is gated on a capability. Most tests care about the + // queue rather than the gate, so they run as someone who has it — which on multisite is a + // network administrator, see test_a_site_administrator_on_multisite_cannot_consume_the_queue(). + $this->become_plugin_administrator(); + } + + public function tearDown(): void { + $this->clear_queue(); + Config_State::reset(); + parent::tearDown(); + } + + public function test_render_outputs_dismissible_markup(): void { + $this->queue_notice( + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] + ); + + $output = $this->render_to_string( $this->make_presenter() ); + + $this->assertStringContainsString( 'is-dismissible', $output ); + $this->assertStringContainsString( 'Bundled now.', $output ); + } + + /** + * @dataProvider notice_severities + * + * @param string $method Method on Writer that queues the notice. + * @param string $class Expected `notice-*` class. + */ + public function test_render_uses_the_severity_of_the_notice_type( string $method, string $class ): void { + $this->queue_notice( + $method, + [ 'conflict_notice_message' => static fn() => 'Something happened.' ] + ); + + $this->assertStringContainsString( + 'notice ' . $class . ' is-dismissible', + $this->render_to_string( $this->make_presenter() ) + ); + } + + /** + * A dependency notice reports a plugin that did not load, which is an error; the conflict + * pair report something the library handled, which is a warning. + * + * @return Generator + */ + public static function notice_severities(): Generator { + yield 'merge' => [ 'queue_merge_notice', 'notice-warning' ]; + yield 'conflict' => [ 'queue_conflict_notice', 'notice-warning' ]; + yield 'dependency' => [ 'queue_dependency_notice', 'notice-error' ]; + } + + public function test_render_strips_a_script_from_the_message(): void { + $this->queue_notice( + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Careful.' ] + ); + + $output = $this->render_to_string( $this->make_presenter() ); + + // `wp_kses_post()` drops the disallowed tag and keeps the text it wrapped, so the payload + // survives as inert text rather than as markup the browser would run. + $this->assertStringNotContainsString( 'assertStringContainsString( 'alert(1)', $output ); + } + + /** + * Messages come from the host's own configuration or filters rather than from user input, so a + * message is allowed to carry a link — to the knowledge-base article explaining the merge, + * 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 { + $this->queue_notice( + 'queue_merge_notice', + [ + 'conflict_notice_message' => static fn() => + 'See the docs.', + ] + ); + + $output = $this->render_to_string( $this->make_presenter() ); + + $this->assertStringContainsString( 'the docs', $output ); + $this->assertStringNotContainsString( 'onclick', $output ); + } + + public function test_render_clears_the_queue(): void { + $this->queue_notice( 'queue_merge_notice' ); + + $presenter = $this->make_presenter(); + $this->render_to_string( $presenter ); + + $this->assertFalse( $this->queue_exists() ); + $this->assertSame( '', $this->render_to_string( $presenter ), 'A second render must output nothing.' ); + } + + public function test_render_outputs_every_queued_notice(): void { + $this->queue_notice( 'queue_merge_notice', [ 'conflict_notice_message' => static fn() => 'First.' ] ); + $this->queue_notice( + 'queue_dependency_notice', + [ 'dependency_notice_message' => static fn() => 'Second.' ] + ); + + $output = $this->render_to_string( $this->make_presenter() ); + + $this->assertStringContainsString( 'First.', $output ); + $this->assertStringContainsString( 'Second.', $output ); + } + + public function test_render_outputs_nothing_when_the_queue_is_empty(): void { + $this->assertSame( '', $this->render_to_string( $this->make_presenter() ) ); + } + + /** + * The other half of the writer/store seam: the renderer is a constructor argument too, so a + * host wanting different markup — one already using stellarwp/admin-notices, say — has one + * small thing to replace rather than the whole presenter. + */ + public function test_a_replacement_renderer_draws_the_queue(): void { + $renderer = new class() extends Renderer { + /** + * @param array $queue Queue to draw. + * + * @return void + */ + public function render( array $queue ): void { + echo '

' . count( $queue ) . '

'; + } + }; + + $this->queue_notice( 'queue_merge_notice' ); + + $output = $this->render_to_string( $this->make_presenter( null, $renderer ) ); + + $this->assertSame( '

1

', $output ); + $this->assertFalse( $this->queue_exists(), 'A replacement renderer still consumes the queue.' ); + } + + /** + * The capability gate guards the clearing as much as the drawing, so it has to sit in front of + * the renderer rather than inside it: a user who may not see the queue must not destroy it. + */ + public function test_a_replacement_renderer_is_never_reached_without_the_capability(): void { + $renderer = new class() extends Renderer { + /** + * @var bool + */ + public $called = false; + + /** + * @param array $queue Queue to draw. + * + * @return void + */ + public function render( array $queue ): void { + $this->called = true; + } + }; + + $this->queue_notice( 'queue_merge_notice' ); + + wp_set_current_user( $this->create_user( 'subscriber' ) ); + + $this->render_to_string( $this->make_presenter( null, $renderer ) ); + + $this->assertFalse( $renderer->called ); + $this->assertTrue( $this->queue_exists() ); + } + + /** + * Rendering consumes the queue, so a user who cannot act on the notice must neither see it + * nor destroy it. The merge notice is raised once and never re-queued. + * + * @dataProvider users_who_cannot_activate_plugins + * + * @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 { + $this->queue_notice( + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] + ); + + wp_set_current_user( $role === null ? 0 : $this->create_user( $role ) ); + + $this->assertSame( '', $this->render_to_string( $this->make_presenter() ) ); + $this->assertTrue( $this->queue_exists(), 'The queue must survive for someone who can act on it.' ); + } + + /** + * @return Generator + */ + public static function users_who_cannot_activate_plugins(): Generator { + yield 'a subscriber' => [ 'subscriber' ]; + yield 'a logged-out visitor' => [ null ]; + } + + /** + * Surprising but intended: on multisite `activate_plugins` maps through + * `manage_network_plugins`, which only a super admin has unless the network has opened the + * plugins menu to site admins. So the person who installed the plugin on their own site is + * not the person who sees the notice — a network administrator is. + */ + public function test_a_site_administrator_on_multisite_cannot_consume_the_queue(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Outside multisite an administrator simply has activate_plugins.' ); + } + + $this->queue_notice( + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] + ); + + wp_set_current_user( $this->create_user( 'administrator' ) ); + + $this->assertSame( '', $this->render_to_string( $this->make_presenter() ) ); + $this->assertTrue( $this->queue_exists(), 'The queue must survive for the network administrator.' ); + } + + /** + * @dataProvider malformed_queues + * + * @param mixed $stored Raw option value to seed. + * @param string|null $present Substring the output must contain, or null when nothing at + * all should be rendered. + * @param array $absent Substrings the output must not contain. + */ + 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( $this->make_presenter() ); + + if ( $present === null ) { + $this->assertSame( '', $output ); + } else { + $this->assertStringContainsString( $present, $output ); + } + + foreach ( $absent as $needle ) { + $this->assertStringNotContainsString( $needle, $output ); + } + } + + /** + * The first two are the likeliest real corruption: another plugin, or a host reading and + * rewriting the option, leaves something behind that is not an array at all. The rest are + * per-entry rubbish, which is dropped without taking the well-formed entries with it. + * + * @return Generator}> + */ + public static function malformed_queues(): Generator { + yield 'a scalar instead of an array' => [ 'not-a-queue', null, [ 'not-a-queue', 'notice' ] ]; + + yield 'an object instead of an array' => [ + (object) [ 'a:merge' => 'Nope.' ], + null, + [ 'Nope.', 'notice' ], + ]; + + yield 'entries that are not strings' => [ + [ + 'a:merge' => 'Fine.', + 'b:merge' => [ 'nested' ], + 'c:merge' => null, + 'd:merge' => 42, + ], + 'Fine.', + [ 'Array', '42' ], + ]; + + yield 'an empty message' => [ [ 'a:merge' => '' ], null, [ 'notice' ] ]; + + // A message that is only whitespace would otherwise print an empty notice box. + yield 'a whitespace-only message' => [ [ 'a:merge' => " \n\t" ], null, [ 'notice' ] ]; + } + + /** + * Queue a notice through a plain Writer over the default, option-backed Store — the same one + * a Presenter built with no store of its own reads. + * + * @param string $method Method on Writer that queues the notice. + * @param array $overrides Sub-plugin config overrides. + * + * @return void + */ + private function queue_notice( string $method, array $overrides = [] ): void { + ( new Writer( new Store() ) )->{$method}( $this->make_sub_plugin( $overrides ) ); + } + + /** + * @param mixed $queue Raw queue contents, well-formed or not. + */ + private function seed_queue( $queue ): void { + update_site_option( self::OPTION, $queue ); + } + + private function clear_queue(): void { + delete_site_option( self::OPTION ); + } + + /** + * Whether the option exists at all, which is what "render cleared the queue" means. + * + * @return bool + */ + private function queue_exists(): bool { + return get_site_option( self::OPTION, false ) !== false; + } + + /** + * The presenter as the container builds it, or with either collaborator replaced. + * + * Both arguments are required — 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 who may consume the queue. + * + * @param Store|null $store Where the queue is kept. + * @param Renderer|null $renderer How a queued notice is drawn. + * + * @return Presenter + */ + private function make_presenter( ?Store $store = null, ?Renderer $renderer = null ): Presenter { + return new Presenter( $store ?? new Store(), $renderer ?? new Renderer() ); + } + + private function render_to_string( Presenter $presenter ): string { + ob_start(); + + try { + $presenter->render(); + } finally { + // In a finally block so a throw from render() cannot leave the suite's own output + // trapped in an abandoned buffer. + $output = (string) ob_get_clean(); + } + + return $output; + } +} diff --git a/tests/unit/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php deleted file mode 100644 index e1933b8..0000000 --- a/tests/unit/Notices/QueueTest.php +++ /dev/null @@ -1,676 +0,0 @@ -clear_queue(); - - // render() consumes the queue, so it is gated on a capability. Most tests care about the - // queue rather than the gate, so they run as someone who has it — which on multisite is a - // network administrator, see test_a_site_administrator_on_multisite_cannot_consume_the_queue(). - $this->become_plugin_administrator(); - } - - public function tearDown(): void { - $this->clear_queue(); - delete_site_option( self::OPTION_FOR_OTHER_PREFIX ); - Config_State::reset(); - parent::tearDown(); - } - - public function test_the_default_notices_satisfy_the_contract(): void { - $this->assertInstanceOf( Queue_Interface::class, $this->make_queue() ); - } - - /** - * @dataProvider queued_notices - * - * @param string $method Method on Queue that queues the notice. - * @param array $overrides Sub-plugin config overrides. - * @param string $key Queue key the notice must land under. - * @param string $expected Expected message, whole or partial. - * @param bool $exact Whether $expected is the whole message. - */ - public function test_it_queues_a_notice( - string $method, - array $overrides, - string $key, - string $expected, - bool $exact - ): void { - $notices = $this->make_queue(); - $notices->{$method}( $this->make_sub_plugin( $overrides ) ); - - $queue = $this->queue(); - - $this->assertArrayHasKey( $key, $queue ); - - if ( $exact ) { - $this->assertSame( $expected, $queue[ $key ] ); - - return; - } - - // The fallbacks are not pinned word for word — they are allowed to be reworded, as long - // as they still name the sub-plugin and are not empty. - $this->assertStringContainsString( $expected, $queue[ $key ] ); - $this->assertNotSame( $expected, $queue[ $key ] ); - } - - /** - * Both the configured message and the fallback for each of the three notice types. The - * fallbacks are covered here rather than in their own methods because the assertion is the - * same one: the right message lands under the right `slug:type` key. - * - * @return Generator,2:string,3:string,4:bool}> - */ - public static function queued_notices(): Generator { - yield 'merge, configured' => [ - 'queue_merge_notice', - [ 'conflict_notice_message' => static fn() => 'Bundled now.' ], - 'give-recurring:merge', - 'Bundled now.', - true, - ]; - - yield 'merge, fallback' => [ - 'queue_merge_notice', - [], - 'give-recurring:merge', - 'give-recurring', - false, - ]; - - yield 'conflict, configured' => [ - 'queue_conflict_notice', - [ 'conflict_notice_message' => static fn() => 'Bundled now.' ], - 'give-recurring:conflict', - 'Bundled now.', - true, - ]; - - yield 'conflict, fallback' => [ - 'queue_conflict_notice', - [], - 'give-recurring:conflict', - 'give-recurring', - false, - ]; - - yield 'dependency, configured' => [ - 'queue_dependency_notice', - [ 'dependency_notice_message' => static fn() => 'Needs Give.' ], - 'give-recurring:dependency', - 'Needs Give.', - true, - ]; - - yield 'dependency, fallback' => [ - 'queue_dependency_notice', - [], - 'give-recurring:dependency', - 'give-recurring could not be loaded because its requirements are not met.', - true, - ]; - } - - /** - * The two conflict-flavoured notices say opposite things — one reports a deactivation that - * 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 = $this->make_queue(); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - $notices->queue_conflict_notice( $this->make_sub_plugin() ); - - $queue = $this->queue(); - - $this->assertNotSame( $queue['give-recurring:merge'], $queue['give-recurring:conflict'] ); - } - - public function test_a_configured_message_is_used_for_both_conflict_types(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) - ); - $notices->queue_conflict_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) - ); - - $queue = $this->queue(); - - $this->assertSame( 'Ours.', $queue['give-recurring:merge'] ); - $this->assertSame( 'Ours.', $queue['give-recurring:conflict'] ); - } - - public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - - $this->assertCount( 1, $this->queue() ); - } - - public function test_one_slug_can_hold_notices_of_different_types(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - $notices->queue_dependency_notice( $this->make_sub_plugin() ); - - $this->assertCount( 2, $this->queue() ); - } - - public function test_different_slugs_do_not_collide(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - $notices->queue_merge_notice( $this->make_sub_plugin( [ 'slug' => 'give-fee-recovery' ] ) ); - - $queue = $this->queue(); - - $this->assertCount( 2, $queue ); - $this->assertArrayHasKey( 'give-recurring:merge', $queue ); - $this->assertArrayHasKey( 'give-fee-recovery:merge', $queue ); - } - - public function test_render_outputs_dismissible_markup(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) - ); - - $output = $this->render_to_string( $notices ); - - $this->assertStringContainsString( 'is-dismissible', $output ); - $this->assertStringContainsString( 'Bundled now.', $output ); - } - - /** - * @dataProvider notice_severities - * - * @param string $method Method on Queue that queues the notice. - * @param string $class Expected `notice-*` class. - */ - public function test_render_uses_the_severity_of_the_notice_type( string $method, string $class ): void { - $notices = $this->make_queue(); - $notices->{$method}( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Something happened.' ] ) - ); - - $this->assertStringContainsString( 'notice ' . $class . ' is-dismissible', $this->render_to_string( $notices ) ); - } - - /** - * A dependency notice reports a plugin that did not load, which is an error; the conflict - * pair report something the library handled, which is a warning. - * - * @return Generator - */ - public static function notice_severities(): Generator { - yield 'merge' => [ 'queue_merge_notice', 'notice-warning' ]; - yield 'conflict' => [ 'queue_conflict_notice', 'notice-warning' ]; - yield 'dependency' => [ 'queue_dependency_notice', 'notice-error' ]; - } - - public function test_render_strips_a_script_from_the_message(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Careful.' ] ) - ); - - $output = $this->render_to_string( $notices ); - - // `wp_kses_post()` drops the disallowed tag and keeps the text it wrapped, so the payload - // survives as inert text rather than as markup the browser would run. - $this->assertStringNotContainsString( 'assertStringContainsString( 'alert(1)', $output ); - } - - /** - * Messages come from the host's own configuration or filters rather than from user input, so a - * message is allowed to carry a link — to the knowledge-base article explaining the merge, - * 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 = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( - [ 'conflict_notice_message' => static fn() => 'See the docs.' ] - ) - ); - - $output = $this->render_to_string( $notices ); - - $this->assertStringContainsString( 'the docs', $output ); - $this->assertStringNotContainsString( 'onclick', $output ); - } - - public function test_render_clears_the_queue(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - - $this->render_to_string( $notices ); - - $this->assertFalse( $this->queue_exists() ); - $this->assertSame( '', $this->render_to_string( $notices ), 'A second render must output nothing.' ); - } - - public function test_render_outputs_every_queued_notice(): void { - $notices = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'First.' ] ) - ); - $notices->queue_dependency_notice( - $this->make_sub_plugin( [ 'dependency_notice_message' => static fn() => 'Second.' ] ) - ); - - $output = $this->render_to_string( $notices ); - - $this->assertStringContainsString( 'First.', $output ); - $this->assertStringContainsString( 'Second.', $output ); - } - - public function test_render_outputs_nothing_when_the_queue_is_empty(): void { - $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 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 { - /** - * @var array - */ - public $written = []; - - /** - * @return array - */ - public function all(): array { - return $this->written; - } - - /** - * @param string $key Queue key. - * @param string $message Resolved message. - * - * @return void - */ - public function put( string $key, string $message ): void { - $this->written[ $key ] = $message; - } - - /** - * @return void - */ - public function clear(): void { - $this->written = []; - } - }; - - $this->make_queue( $store )->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) - ); - - $this->assertSame( [ 'give-recurring:merge' => 'Bundled now.' ], $store->written ); - $this->assertFalse( $this->queue_exists(), 'The default option must not have been written to.' ); - } - - /** - * The other half of the same seam: different markup, same queue and same consumption rules. - */ - public function test_a_replacement_renderer_draws_the_queue(): void { - $renderer = new class() extends Renderer { - /** - * @param array $queue Queue to draw. - * - * @return void - */ - public function render( array $queue ): void { - echo '

' . count( $queue ) . '

'; - } - }; - - $notices = $this->make_queue( null, $renderer ); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - - $this->assertSame( '

1

', $this->render_to_string( $notices ) ); - $this->assertFalse( $this->queue_exists(), 'A replacement renderer still consumes the queue.' ); - } - - /** - * The capability gate guards the clearing as much as the drawing, so it has to sit in front of - * the renderer rather than inside it: a user who may not see the queue must not destroy it. - */ - public function test_a_replacement_renderer_is_never_reached_without_the_capability(): void { - $renderer = new class() extends Renderer { - /** - * @var bool - */ - public $called = false; - - /** - * @param array $queue Queue to draw. - * - * @return void - */ - public function render( array $queue ): void { - $this->called = true; - } - }; - - $notices = $this->make_queue( null, $renderer ); - $notices->queue_merge_notice( $this->make_sub_plugin() ); - - wp_set_current_user( $this->create_user( 'subscriber' ) ); - - $this->render_to_string( $notices ); - - $this->assertFalse( $renderer->called ); - $this->assertTrue( $this->queue_exists() ); - } - - /** - * Rendering consumes the queue, so a user who cannot act on the notice must neither see it - * nor destroy it. The merge notice is raised once and never re-queued. - * - * @dataProvider users_who_cannot_activate_plugins - * - * @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 = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) - ); - - wp_set_current_user( $role === null ? 0 : $this->create_user( $role ) ); - - $this->assertSame( '', $this->render_to_string( $notices ) ); - $this->assertTrue( $this->queue_exists(), 'The queue must survive for someone who can act on it.' ); - } - - /** - * @return Generator - */ - public static function users_who_cannot_activate_plugins(): Generator { - yield 'a subscriber' => [ 'subscriber' ]; - yield 'a logged-out visitor' => [ null ]; - } - - /** - * Surprising but intended: on multisite `activate_plugins` maps through - * `manage_network_plugins`, which only a super admin has unless the network has opened the - * plugins menu to site admins. So the person who installed the plugin on their own site is - * not the person who sees the notice — a network administrator is. - */ - public function test_a_site_administrator_on_multisite_cannot_consume_the_queue(): void { - if ( ! is_multisite() ) { - $this->markTestSkipped( 'Outside multisite an administrator simply has activate_plugins.' ); - } - - $notices = $this->make_queue(); - $notices->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) - ); - - wp_set_current_user( $this->create_user( 'administrator' ) ); - - $this->assertSame( '', $this->render_to_string( $notices ) ); - $this->assertTrue( $this->queue_exists(), 'The queue must survive for the network administrator.' ); - } - - /** - * The resolver redirects, so the queue has to come back off a durable database row rather - * than out of the object cache the redirecting request happened to warm. Asserting the row - * itself, not just that a flush is survivable: on a site with no persistent object cache a - * transient lands in the options table too, so a flush test alone would pass for the - * transient-backed design this class exists to avoid. - */ - public function test_the_queue_is_a_durable_database_row(): void { - $this->make_queue()->queue_merge_notice( - $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) - ); - - $this->assertStringContainsString( - 'Bundled now.', - $this->stored_row(), - 'The queue must be a row in the database, not a cache entry.' - ); - - wp_cache_flush(); - - $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( $this->make_queue() ) ); - } - - /** - * @dataProvider malformed_queues - * - * @param mixed $stored Raw option value to seed. - * @param string|null $present Substring the output must contain, or null when nothing at - * all should be rendered. - * @param array $absent Substrings the output must not contain. - */ - 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( $this->make_queue() ); - - if ( $present === null ) { - $this->assertSame( '', $output ); - } else { - $this->assertStringContainsString( $present, $output ); - } - - foreach ( $absent as $needle ) { - $this->assertStringNotContainsString( $needle, $output ); - } - } - - /** - * The first two are the likeliest real corruption: another plugin, or a host reading and - * rewriting the option, leaves something behind that is not an array at all. The rest are - * per-entry rubbish, which is dropped without taking the well-formed entries with it. - * - * @return Generator}> - */ - public static function malformed_queues(): Generator { - yield 'a scalar instead of an array' => [ 'not-a-queue', null, [ 'not-a-queue', 'notice' ] ]; - - yield 'an object instead of an array' => [ - (object) [ 'a:merge' => 'Nope.' ], - null, - [ 'Nope.', 'notice' ], - ]; - - yield 'entries that are not strings' => [ - [ - 'a:merge' => 'Fine.', - 'b:merge' => [ 'nested' ], - 'c:merge' => null, - 'd:merge' => 42, - ], - 'Fine.', - [ 'Array', '42' ], - ]; - - yield 'an empty message' => [ [ 'a:merge' => '' ], null, [ 'notice' ] ]; - - // A message that is only whitespace would otherwise print an empty notice box. - yield 'a whitespace-only message' => [ [ 'a:merge' => " \n\t" ], null, [ 'notice' ] ]; - } - - public function test_a_corrupted_queue_heals_on_the_next_write(): void { - $this->seed_queue( [ 'a:merge' => [ 'nested' ] ] ); - - $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); - - $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); - } - - public function test_the_option_is_keyed_by_the_hook_prefix(): void { - Config_State::reset(); - Config::set_hook_prefix( 'woo' ); - - $this->assertSame( self::OPTION_FOR_OTHER_PREFIX, $this->make_queue()->option_name() ); - - $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() ); - } - - public function test_queueing_needs_a_hook_prefix(): void { - Config_State::reset(); - - $this->expectException( Config_Exception::class ); - - $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); - } - - /** - * The queue is empty on nearly every request and only ever read in the admin, so it must not - * ride along in the autoloaded bundle on every front-end request. - */ - public function test_the_queue_is_not_autoloaded(): void { - if ( is_multisite() ) { - $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); - } - - $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); - - $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); - } - - /** - * The queue as the class stores it. Always an array, so callers can index and count it: use - * queue_exists() to ask whether there is a row at all. - * - * @return array - */ - private function queue(): array { - $queue = get_site_option( self::OPTION, [] ); - - return is_array( $queue ) ? $queue : []; - } - - /** - * Whether the option exists at all, which is what "render cleared the queue" means. - * - * @return bool - */ - private function queue_exists(): bool { - return get_site_option( self::OPTION, false ) !== false; - } - - /** - * The serialized option value straight out of the database, bypassing the object cache. - * - * The table name goes through the `%i` identifier placeholder rather than into the string, so - * the query stays a literal and nothing interpolated ever reaches the parser. - * - * @return string - */ - private function stored_row(): string { - /** @var wpdb $wpdb */ - global $wpdb; - - if ( is_multisite() ) { - $stored = $wpdb->get_var( - $wpdb->prepare( - 'SELECT meta_value FROM %i WHERE meta_key = %s AND site_id = %d', - $wpdb->sitemeta, - self::OPTION, - get_current_network_id() - ) - ); - } else { - $stored = $wpdb->get_var( - $wpdb->prepare( - 'SELECT option_value FROM %i WHERE option_name = %s', - $wpdb->options, - self::OPTION - ) - ); - } - - $this->assertIsString( $stored, 'The queue option has no row in the database.' ); - - return $stored; - } - - /** - * @param mixed $queue Raw queue contents, well-formed or not. - */ - private function seed_queue( $queue ): void { - update_site_option( self::OPTION, $queue ); - } - - private function clear_queue(): void { - delete_site_option( self::OPTION ); - } - - /** - * 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(); - - try { - $notices->render(); - } finally { - // In a finally block so a throw from render() cannot leave the suite's own output - // trapped in an abandoned buffer. - $output = (string) ob_get_clean(); - } - - return $output; - } -} diff --git a/tests/unit/Notices/RendererTest.php b/tests/unit/Notices/RendererTest.php index 47642b9..a1d516a 100644 --- a/tests/unit/Notices/RendererTest.php +++ b/tests/unit/Notices/RendererTest.php @@ -7,8 +7,8 @@ use Codeception\TestCase\WPTestCase; use Generator; -use Nexcess\PluginAbsorber\Notices\Queue; use Nexcess\PluginAbsorber\Notices\Renderer; +use Nexcess\PluginAbsorber\Notices\Writer; /** * The drawing half of the queue. @@ -48,9 +48,9 @@ public function test_the_type_half_of_the_key_picks_the_severity( string $key, s * @return Generator */ public static function notice_severities(): Generator { - yield 'merge' => [ 'give-recurring:' . Queue::TYPE_MERGE, 'notice-warning' ]; - yield 'conflict' => [ 'give-recurring:' . Queue::TYPE_CONFLICT, 'notice-warning' ]; - yield 'dependency' => [ 'give-recurring:' . Queue::TYPE_DEPENDENCY, 'notice-error' ]; + yield 'merge' => [ 'give-recurring:' . Writer::TYPE_MERGE, 'notice-warning' ]; + yield 'conflict' => [ 'give-recurring:' . Writer::TYPE_CONFLICT, 'notice-warning' ]; + yield 'dependency' => [ 'give-recurring:' . Writer::TYPE_DEPENDENCY, 'notice-error' ]; yield 'unknown type' => [ 'give-recurring:invented', 'notice-warning' ]; yield 'no type at all' => [ 'give-recurring', 'notice-warning' ]; } @@ -62,7 +62,7 @@ public static function notice_severities(): Generator { public function test_the_type_is_the_last_segment_of_the_key(): void { $this->assertStringContainsString( 'notice-error', - $this->render( [ 'give:recurring:' . Queue::TYPE_DEPENDENCY => 'Requirements not met.' ] ) + $this->render( [ 'give:recurring:' . Writer::TYPE_DEPENDENCY => 'Requirements not met.' ] ) ); } diff --git a/tests/unit/Notices/WriterTest.php b/tests/unit/Notices/WriterTest.php new file mode 100644 index 0000000..602a202 --- /dev/null +++ b/tests/unit/Notices/WriterTest.php @@ -0,0 +1,397 @@ +clear_queue(); + } + + public function tearDown(): void { + $this->clear_queue(); + delete_site_option( self::OPTION_FOR_OTHER_PREFIX ); + Config_State::reset(); + parent::tearDown(); + } + + public function test_the_default_writer_satisfies_the_contract(): void { + $this->assertInstanceOf( Writer_Interface::class, $this->make_writer() ); + } + + /** + * @dataProvider queued_notices + * + * @param string $method Method on Writer that queues the notice. + * @param array $overrides Sub-plugin config overrides. + * @param string $key Queue key the notice must land under. + * @param string $expected Expected message, whole or partial. + * @param bool $exact Whether $expected is the whole message. + */ + public function test_it_queues_a_notice( + string $method, + array $overrides, + string $key, + string $expected, + bool $exact + ): void { + $writer = $this->make_writer(); + $writer->{$method}( $this->make_sub_plugin( $overrides ) ); + + $queue = $this->queue(); + + $this->assertArrayHasKey( $key, $queue ); + + if ( $exact ) { + $this->assertSame( $expected, $queue[ $key ] ); + + return; + } + + // The fallbacks are not pinned word for word — they are allowed to be reworded, as long + // as they still name the sub-plugin and are not empty. + $this->assertStringContainsString( $expected, $queue[ $key ] ); + $this->assertNotSame( $expected, $queue[ $key ] ); + } + + /** + * Both the configured message and the fallback for each of the three notice types. The + * fallbacks are covered here rather than in their own methods because the assertion is the + * same one: the right message lands under the right `slug:type` key. + * + * @return Generator,2:string,3:string,4:bool}> + */ + public static function queued_notices(): Generator { + yield 'merge, configured' => [ + 'queue_merge_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ], + 'give-recurring:merge', + 'Bundled now.', + true, + ]; + + yield 'merge, fallback' => [ + 'queue_merge_notice', + [], + 'give-recurring:merge', + 'give-recurring', + false, + ]; + + yield 'conflict, configured' => [ + 'queue_conflict_notice', + [ 'conflict_notice_message' => static fn() => 'Bundled now.' ], + 'give-recurring:conflict', + 'Bundled now.', + true, + ]; + + yield 'conflict, fallback' => [ + 'queue_conflict_notice', + [], + 'give-recurring:conflict', + 'give-recurring', + false, + ]; + + yield 'dependency, configured' => [ + 'queue_dependency_notice', + [ 'dependency_notice_message' => static fn() => 'Needs Give.' ], + 'give-recurring:dependency', + 'Needs Give.', + true, + ]; + + yield 'dependency, fallback' => [ + 'queue_dependency_notice', + [], + 'give-recurring:dependency', + 'give-recurring could not be loaded because its requirements are not met.', + true, + ]; + } + + /** + * The two conflict-flavoured notices say opposite things — one reports a deactivation that + * 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 { + $writer = $this->make_writer(); + $writer->queue_merge_notice( $this->make_sub_plugin() ); + $writer->queue_conflict_notice( $this->make_sub_plugin() ); + + $queue = $this->queue(); + + $this->assertNotSame( $queue['give-recurring:merge'], $queue['give-recurring:conflict'] ); + } + + public function test_a_configured_message_is_used_for_both_conflict_types(): void { + $writer = $this->make_writer(); + $writer->queue_merge_notice( + $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) + ); + $writer->queue_conflict_notice( + $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) + ); + + $queue = $this->queue(); + + $this->assertSame( 'Ours.', $queue['give-recurring:merge'] ); + $this->assertSame( 'Ours.', $queue['give-recurring:conflict'] ); + } + + public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): void { + $writer = $this->make_writer(); + $writer->queue_merge_notice( $this->make_sub_plugin() ); + $writer->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertCount( 1, $this->queue() ); + } + + public function test_one_slug_can_hold_notices_of_different_types(): void { + $writer = $this->make_writer(); + $writer->queue_merge_notice( $this->make_sub_plugin() ); + $writer->queue_dependency_notice( $this->make_sub_plugin() ); + + $this->assertCount( 2, $this->queue() ); + } + + public function test_different_slugs_do_not_collide(): void { + $writer = $this->make_writer(); + $writer->queue_merge_notice( $this->make_sub_plugin() ); + $writer->queue_merge_notice( $this->make_sub_plugin( [ 'slug' => 'give-fee-recovery' ] ) ); + + $queue = $this->queue(); + + $this->assertCount( 2, $queue ); + $this->assertArrayHasKey( 'give-recurring:merge', $queue ); + $this->assertArrayHasKey( 'give-fee-recovery:merge', $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. It is a required argument and it is bound by + * `Provider`, so a host rebinds the store rather than subclassing the writer. + */ + public function test_a_replacement_store_is_used_instead_of_the_option(): void { + $store = new class() extends Store { + /** + * @var array + */ + public $written = []; + + /** + * @return array + */ + public function all(): array { + return $this->written; + } + + /** + * @param string $key Queue key. + * @param string $message Resolved message. + * + * @return void + */ + public function put( string $key, string $message ): void { + $this->written[ $key ] = $message; + } + + /** + * @return void + */ + public function clear(): void { + $this->written = []; + } + }; + + $this->make_writer( $store )->queue_merge_notice( + $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) + ); + + $this->assertSame( [ 'give-recurring:merge' => 'Bundled now.' ], $store->written ); + $this->assertFalse( $this->queue_exists(), 'The default option must not have been written to.' ); + } + + /** + * The resolver redirects, so the queue has to come back off a durable database row rather + * than out of the object cache the redirecting request happened to warm. Asserting the row + * itself, not just that a flush is survivable: on a site with no persistent object cache a + * transient lands in the options table too, so a flush test alone would pass for the + * transient-backed design this class exists to avoid. + */ + public function test_the_queue_is_a_durable_database_row(): void { + $this->make_writer()->queue_merge_notice( + $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) + ); + + $this->assertStringContainsString( + 'Bundled now.', + $this->stored_row(), + 'The queue must be a row in the database, not a cache entry.' + ); + + wp_cache_flush(); + + $this->assertSame( 'Bundled now.', $this->queue()['give-recurring:merge'] ?? '' ); + } + + public function test_a_corrupted_queue_heals_on_the_next_write(): void { + $this->seed_queue( [ 'a:merge' => [ 'nested' ] ] ); + + $this->make_writer()->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); + } + + public function test_the_option_is_keyed_by_the_hook_prefix(): void { + Config_State::reset(); + Config::set_hook_prefix( 'woo' ); + + $this->assertSame( self::OPTION_FOR_OTHER_PREFIX, $this->make_writer()->option_name() ); + + $this->make_writer()->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertIsArray( get_site_option( self::OPTION_FOR_OTHER_PREFIX, false ) ); + $this->assertFalse( $this->queue_exists() ); + } + + public function test_queueing_needs_a_hook_prefix(): void { + Config_State::reset(); + + $this->expectException( Config_Exception::class ); + + $this->make_writer()->queue_merge_notice( $this->make_sub_plugin() ); + } + + /** + * The queue is empty on nearly every request and only ever read in the admin, so it must not + * ride along in the autoloaded bundle on every front-end request. + */ + public function test_the_queue_is_not_autoloaded(): void { + if ( is_multisite() ) { + $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); + } + + $this->make_writer()->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); + } + + /** + * The queue as the class stores it. Always an array, so callers can index and count it: use + * queue_exists() to ask whether there is a row at all. + * + * @return array + */ + private function queue(): array { + $queue = get_site_option( self::OPTION, [] ); + + return is_array( $queue ) ? $queue : []; + } + + /** + * Whether the option exists at all, which is what "the writer wrote somewhere else" means. + * + * @return bool + */ + private function queue_exists(): bool { + return get_site_option( self::OPTION, false ) !== false; + } + + /** + * The serialized option value straight out of the database, bypassing the object cache. + * + * The table name goes through the `%i` identifier placeholder rather than into the string, so + * the query stays a literal and nothing interpolated ever reaches the parser. + * + * @return string + */ + private function stored_row(): string { + /** @var wpdb $wpdb */ + global $wpdb; + + if ( is_multisite() ) { + $stored = $wpdb->get_var( + $wpdb->prepare( + 'SELECT meta_value FROM %i WHERE meta_key = %s AND site_id = %d', + $wpdb->sitemeta, + self::OPTION, + get_current_network_id() + ) + ); + } else { + $stored = $wpdb->get_var( + $wpdb->prepare( + 'SELECT option_value FROM %i WHERE option_name = %s', + $wpdb->options, + self::OPTION + ) + ); + } + + $this->assertIsString( $stored, 'The queue option has no row in the database.' ); + + return $stored; + } + + /** + * @param mixed $queue Raw queue contents, well-formed or not. + */ + private function seed_queue( $queue ): void { + update_site_option( self::OPTION, $queue ); + } + + private function clear_queue(): void { + delete_site_option( self::OPTION ); + } + + /** + * The writer as the container builds it, or with its store replaced. + * + * The store is a required argument — nothing in `src/` defaults a collaborator to a `new` of + * its own any more — so the standard shape is spelled out once here rather than in every test. + * + * @param Store|null $store Where the queue is kept. + * + * @return Writer + */ + private function make_writer( ?Store $store = null ): Writer { + return new Writer( $store ?? new Store() ); + } +} diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index f45134c..0d8e0c3 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -20,10 +20,11 @@ use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Loader; -use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; -use Nexcess\PluginAbsorber\Notices\Queue; +use Nexcess\PluginAbsorber\Notices\Contracts\Writer_Interface; +use Nexcess\PluginAbsorber\Notices\Presenter; use Nexcess\PluginAbsorber\Notices\Renderer; use Nexcess\PluginAbsorber\Notices\Store; +use Nexcess\PluginAbsorber\Notices\Writer; use Nexcess\PluginAbsorber\Plugin_Checker; use Nexcess\PluginAbsorber\Plugin_Deactivator; use Nexcess\PluginAbsorber\Provider; @@ -31,8 +32,8 @@ use Nexcess\PluginAbsorber\Registry_Reader; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Spy_Activator; -use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Writer; use Nexcess\PluginAbsorber\Tests\Support\Spy_Resolver; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; use StellarWP\ContainerContract\ContainerInterface; @@ -83,9 +84,10 @@ public function test_it_binds_every_default( string $id, string $expected ): voi public static function default_bindings(): Generator { yield 'the registrar' => [ Registrar_Interface::class, Registrar::class ]; yield 'the registry reader' => [ Registry_Reader::class, Registry_Reader::class ]; - yield 'the notice queue' => [ Queue_Interface::class, Queue::class ]; + yield 'the notice writer' => [ Writer_Interface::class, Writer::class ]; yield 'the notice store' => [ Store::class, Store::class ]; yield 'the notice renderer' => [ Renderer::class, Renderer::class ]; + yield 'the notice presenter' => [ Presenter::class, Presenter::class ]; yield 'the plugin checker' => [ Plugin_Checker_Interface::class, Plugin_Checker::class ]; yield 'the deactivator' => [ Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ]; yield 'the activator' => [ Activator_Interface::class, Activator::class ]; @@ -119,9 +121,10 @@ public function test_a_binding_resolves_to_one_instance( string $id ): void { public static function single_instance_bindings(): Generator { yield 'the registrar' => [ Registrar_Interface::class ]; yield 'the registry reader' => [ Registry_Reader::class ]; - yield 'the notice queue' => [ Queue_Interface::class ]; + yield 'the notice writer' => [ Writer_Interface::class ]; yield 'the notice store' => [ Store::class ]; yield 'the notice renderer' => [ Renderer::class ]; + yield 'the notice presenter' => [ Presenter::class ]; yield 'the conflict resolver' => [ Resolver_Interface::class ]; yield 'the conflict detector' => [ Detector::class ]; yield 'the redirector' => [ Redirector::class ]; @@ -159,6 +162,7 @@ public static function class_id_bindings(): Generator { yield 'the registry reader' => [ Registry_Reader::class ]; yield 'the notice store' => [ Store::class ]; yield 'the notice renderer' => [ Renderer::class ]; + yield 'the notice presenter' => [ Presenter::class ]; yield 'the conflict detector' => [ Detector::class ]; yield 'the redirector' => [ Redirector::class ]; yield 'the conflict gate' => [ Gatekeeper::class ]; @@ -194,14 +198,14 @@ static function () use ( $bound ): object { * 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`, `Loader` or `Boot\Scheduler` has to - * do it after boot. + * replaces it — a host rebinding `Store`, `Renderer`, `Presenter`, `Loader` 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() ]; + yield 'the notice writer' => [ Writer_Interface::class, new Spy_Writer() ]; yield 'the conflict resolver' => [ Resolver_Interface::class, new Spy_Resolver() ]; // "Once, ever" is recorded in an option here, which is one opinion among several — a host