From 3899b1cb4ae20a9ea05537e33c87a9c9fdc87020 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 24 Aug 2026 15:37:30 +0200 Subject: [PATCH] Qualify the invariants the code outgrew, and say where the guarantee stops --- AGENTS.md | 65 +++++++++++++------ docs/configuration.md | 14 +++- docs/extending.md | 6 ++ src/Activator.php | 5 ++ src/Conflict/Contracts/Resolver_Interface.php | 2 +- src/Sub_Plugin.php | 12 ++-- src/Traits/Guards_Hook_Prefix.php | 8 ++- 7 files changed, 83 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 59aa15f..598d107 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,8 +71,8 @@ A two-class static facade — `Config` (hook prefix, container, and optionally t basename) and `Absorber` (register/boot, plus accessors) — matching the shape of `stellarwp/assets` and `stellarwp/admin-notices`. Everything else is an implementation detail behind it. `Absorber` is -`final`: every member is private static and every internal call is `self::`, so a subclass could -override nothing and would silently change nothing. +`final`: every member is static, the only property and the only helper are private, and every +internal call is `self::`, so a subclass could override nothing and would silently change nothing. ### Collaborators @@ -90,10 +90,11 @@ seams a host may rebind: The rest — `Boot\Scheduler`, `Loader`, `Registry\Reader`, `Conflict\Detector`, `Conflict\Gatekeeper`, `Conflict\Redirector`, `Conflict\Rewriter`, `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. `Provider` also binds the container -under `ContainerInterface::class`, first and before anything else, so that a container which builds -unbound classes reflectively can still satisfy the collaborators that take one. +are bound as concrete classes. A host that wants one of them different rebinds the class name **after +`boot()`**, for the reason `bind_once()` gives below; there is no interface because nothing in the +library dispatches on one. `Provider` also binds the container under `ContainerInterface::class`, +first and before anything else, so that a container which builds unbound classes reflectively can +still satisfy the collaborators that take 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/`. What is left in `src/Contracts/` is the @@ -144,16 +145,24 @@ floor ever reaches 8.3 *and* something needs `when()`/`needs()` — not before. fatal `TypeError` in practice, because di52 type-hints its own `ServiceProvider` base class which Foundation's abstract does not extend. -**`Provider` never overwrites a binding.** It binds only what the container does not already have, so -a host that bound its own implementation wins, and the order in which the host calls -`set_container()` and `boot()` stops deciding which implementation it gets. Everything is a -singleton: each binding is either a registry whose contents are the point — a second `Registry\Registrar` -would hold a second, emptier list — or a stateless worker with nothing to gain from a second copy. +**`Provider` never overwrites an *interface* binding.** `bind_once()` stands down on +`! class_exists( $id ) && $this->container->has( $id )`, so the guarantee covers the seven interface +ids and not the ten class-name ones, which are re-bound unconditionally. That is deliberate: +`has()` means "can return an entry", not "the host bound this" — di52 answers it with `isBound() || +class_exists()` — so for a class id it is already true before anything is bound, and dropping the +`class_exists()` half would stand down every concrete binding, the explicit factories included, +leaving those collaborators autowired where the container autowires, broken where it does not, and +singletons nowhere. What it costs is that a host replacing one of the concrete workers has to bind +after `boot()`; the interface seams, which are the ones a host is invited to replace, win whenever +they are bound. Everything is a singleton: each binding is either a registry whose contents are the +point — a second `Registry\Registrar` would hold a second, emptier list — or a stateless worker with +nothing to gain from a second copy. The container is **never** used to wire hooks, and the reason is no longer that it is optional. `Boot\Scheduler` wires callbacks that resolve *inside* the callback — a closure over the container, -or a static trampoline reading `Absorber::notices()` — so wiring instantiates nothing, a host may -rebind right up until the hook fires, and a request that reaches none of them builds none of them. +or a static trampoline on `Absorber` that resolves `Notices\Presenter` or `Conflict\Rewriter` when it +fires — so wiring instantiates nothing, a host may rebind right up until the hook fires, and a +request that reaches none of them builds none of them. `$container->callback()` reads better and is what the hand-rolled copies in `learndash-core` use, but it is di52-only: `stellarwp/container-contract` declares `bind`, `get`, `has` and `singleton`, and nothing else. `[ $resolved_object, 'method' ]` is the other wrong answer — it forces every @@ -163,8 +172,11 @@ collaborator to be built at boot. delegations to `$container->get()`, so what a host calls is unchanged; what changed is that a *collaborator* now depends on the peer it was handed rather than on the facade. -**Nothing but `Absorber` names `Absorber`.** The registration buffer belongs to `Registry\Reader`, -which is also what reads it back out: `Absorber::register()` pushes a `Sub_Plugin` into it and +**No collaborator reaches the registry through `Absorber`.** The rule is about the registration +buffer, not about the name: `Boot\Scheduler` spells `Absorber::class` three lines in, because the two +admin hooks are wired as `[ Absorber::class, … ]` pairs and the name is what `remove_filter()` needs +to reach. The registration buffer belongs to `Registry\Reader`, which is also what reads it back +out: `Absorber::register()` pushes a `Sub_Plugin` into it and `Absorber::all()` delegates to it, while `Conflict\Detector`, `Conflict\Resolver` and `Loader` are each handed one. The buffer is static because it must be — `register()` is a static call a host makes at plugin-file scope, before there is a container to resolve a registrar from — and what is decided @@ -369,6 +381,16 @@ capability gate covers every policy, not just the destructive one, and that is f 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. +**That ordering covers the no-conflict case and only that, and the rest is known and not fixed.** A +site that *is* in conflict stays in conflict until a capable admin opens a plain admin screen — days, +on a site nobody administers in a browser — and every admin GET in between reaches +`current_user_can()` at `plugins_loaded` priority 5, ahead of an SSO or JWT plugin that adds +`determine_current_user` from its own `plugins_loaded` callback. There is no earlier answer available: +nothing may deactivate a plugin on a user's behalf without first asking what that user is allowed to +do. A host that hits it adds its `determine_current_user` filter at plugin-file scope instead. What +does *not* happen is a bounce to `wp-login.php` — `auth_redirect()` reads `wp_validate_auth_cookie()` +directly rather than the cached user. + An unknown policy is normalised to `NOTICE_ONLY` through `Conflict_Policy::is_valid()` before the switch, never decided by wherever a `switch` happens to fall through — a typo like `'defered'` would otherwise deactivate a plugin the site owner deliberately turned on. The `default:` branch that @@ -557,10 +579,15 @@ against real WordPress state. `Bootstrap_Test_Case.php` is the abstract parent o - **Notice messages are rendered through `wp_kses_post()`, not escaped.** They come from the host's own config or filter, never from user input, so a knowledge-base link survives. Tightening this to `esc_html()` after 1.0 would break every host that shipped one. -- **A configured string is never called; every other callable form is.** A string function name is - indistinguishable from a string value, so honouring it would make the result depend on what else - the site loaded — `date`, `flush` and `key` are all real functions and plausible values. Closures, - `[ class, method ]` pairs and invokable objects say "call me" and nothing else. +- **On a key that also takes a string, a configured string is never called; every other callable + form is.** That is `conflict_policy` and the two message keys — the three + `Sub_Plugin::resolve_deferred()` reads, and the only three the rule was ever about. There a string + function name is indistinguishable from a string value, so honouring it would make the result + depend on what else the site loaded — `date`, `flush` and `key` are all real functions and + plausible values. Closures, `[ class, method ]` pairs and invokable objects say "call me" and + nothing else. `enabled`, `dependency_check` and `activation_callback` take no string at all, so + nothing under them is ambiguous: a plain function name there is called, deliberately, and + `SubPluginTest` pins that. - **`conflict_notice_message` and `dependency_notice_message` reject a string outright.** Not just string callables — any string. A config array is built before `init`, so a translated string under one of these can only have been produced too early, and nothing in the value distinguishes it from diff --git a/docs/configuration.md b/docs/configuration.md index 13ac09b..7195a2d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -110,6 +110,13 @@ for it. Whatever that hook would have done — create a table, seed options — }, ``` +**The wrapper is load-bearing, not decoration.** Like every callable key, `activation_callback` is +checked with `is_callable()` at registration — which happens before the bundled plugin has been +`require_once`d, so before `Give\Recurring\Install` exists. `[ Install::class, 'create_tables' ]` +fails that check and throws `Config_Exception`, on a class that would have been perfectly loadable by +the time the callback ran. A closure is callable the moment it is written and names the class only +when the load pass calls it, with the plugin's code already in memory. + It runs once ever per slug, is passed the `Sub_Plugin`, and only after a require that actually happened — never for a sub-plugin whose load was skipped. The record lives in the `{option_prefix}_plugin_absorber_activations` option, a network option on multisite, and is @@ -121,8 +128,11 @@ callback runs, and the record is written, so two first requests arriving togethe the check. A `dbDelta()` migration survives that; a blind `INSERT` of seed rows does not. One record for the network is also one *run* for the network, in whichever site's request -reached the load pass first. Per-site work — a `$wpdb->prefix` table, a per-site option — is -yours to loop over: see [Do per-site work on multisite](recipes.md#do-per-site-work-on-multisite). +reached the load pass first. A site created on the network **after** that run never gets the +callback at all: the record is already set, and nothing re-reads it per site. Per-site work — a +`$wpdb->prefix` table, a per-site option — is yours to loop over, and a site created later is yours +to catch on `wp_initialize_site`: see +[Do per-site work on multisite](recipes.md#do-per-site-work-on-multisite). ## What changes for the bundled plugin diff --git a/docs/extending.md b/docs/extending.md index 42a63fb..40bf962 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -82,6 +82,12 @@ a `Config_Exception` naming the interface and the class that failed it, rather t for a binding it cannot build at all comes through unwrapped. `Absorber::all()` drops anything a rebound registrar returns that is not a `Sub_Plugin`. +`Absorber::resolver()` hands you the resolver, not the gates in front of it: `resolve_all()` re-checks +neither the request gate nor the capability gate, because the conflict step has already asked both by +the time it runs. Calling it yourself can therefore deactivate a plugin and `exit` on a POST, a cron +run, or a request from someone who may not deactivate anything. Leave it to the hook unless you have +made both checks first. + Nothing is built at boot beyond the two objects that do the booting: each hook resolves its collaborator when it fires, so a request that reaches none of them builds none of them, and you may rebind right up until the hook runs. diff --git a/src/Activator.php b/src/Activator.php index 73a3035..d78943a 100644 --- a/src/Activator.php +++ b/src/Activator.php @@ -56,6 +56,11 @@ public function maybe_run( Sub_Plugin $sub_plugin ): void { * activation that follows a network-wide merge has to be recorded network-wide too, or every * site in the network runs the callback again. * + * The other end of that is the accepted cost: a site created on the network afterwards finds the + * record already set and never runs the callback at all. One run for the network is what this + * default promises, and per-site work belongs on a host's own new-site hook, or in an + * `Activator_Interface` of its own that records per site. + * * @since 1.0.0 * * @throws Config_Exception When no hook prefix has been set. diff --git a/src/Conflict/Contracts/Resolver_Interface.php b/src/Conflict/Contracts/Resolver_Interface.php index 3b3d95a..a1d130a 100644 --- a/src/Conflict/Contracts/Resolver_Interface.php +++ b/src/Conflict/Contracts/Resolver_Interface.php @@ -29,7 +29,7 @@ interface Resolver_Interface { * * @since 1.0.0 * - * @throws Config_Exception When no hook prefix has been set, or a container binding is unusable. + * @throws Config_Exception When no hook prefix has been set. * * @return void */ diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index 19a7469..1376926 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -10,11 +10,15 @@ use Nexcess\PluginAbsorber\Exceptions\Config_Exception; /** - * One registered sub-plugin: its configuration, and the answers that configuration alone decides. + * One registered sub-plugin: its configuration, and every answer it can give without a + * container-bound collaborator. * - * Deliberately not a window onto WordPress. Asking whether the standalone counterpart is active is - * a question about the site rather than about this configuration, and it belongs to - * Checker_Interface; this object only names the plugin to ask about. + * Not the same as "configuration alone". is_already_loaded() reads the global constant table, and a + * callable under `enabled` may query whatever the host likes. The line drawn here is dependency + * direction: asking whether the standalone counterpart is active needs Checker_Interface, and + * resolving one would put a container read in front of Absorber::register(), which deliberately + * performs none so that the container may arrive at any point before boot. This object only names + * the plugin to ask about. * * @since 1.0.0 * diff --git a/src/Traits/Guards_Hook_Prefix.php b/src/Traits/Guards_Hook_Prefix.php index 62c6382..ac1f332 100644 --- a/src/Traits/Guards_Hook_Prefix.php +++ b/src/Traits/Guards_Hook_Prefix.php @@ -13,9 +13,11 @@ /** * Turns "no hook prefix has been set" into something a developer sees and a request survives. * - * Both of this library's hook callbacks need the prefix — one for the should_load filter, the - * other for the option the notice queue lives in — and both are reached from a core action, where - * throwing would take the whole site down over a bootstrap mistake. A trait rather than a shared + * Every entry point this library puts on a hook needs the prefix — it names the should_load filter, + * the conflict and notice filters, and the option the notice queue lives in — and every one of them + * is reached from a core hook, where throwing would take the whole site down over a bootstrap + * mistake. So the load pass, the conflict pass, the gatekeeper and both of the facade's admin + * trampolines ask this first and stand down when the answer is no. A trait rather than a shared * collaborator because the answer comes from `Config` either way; all that is shared is how the * mistake is reported. *