diff --git a/README.md b/README.md index 5240a83..36cb667 100644 --- a/README.md +++ b/README.md @@ -41,15 +41,15 @@ you already hand to Telemetry or Uplink. If your plugin can run on multisite, add `Config::set_host_plugin_basename( plugin_basename( __FILE__ ) )`. It is a no-op off a network, so set -it unconditionally: where it matters is the one topology the library must not deactivate a standalone -in — a network-active standalone whose host plugin is not itself network-activated, where a -network-wide deactivation would leave the network's other sites with no copy of it at all. +it unconditionally: it stops the library deactivating a network-active standalone when your host +plugin is not itself network-activated, which would leave the network's other sites with no copy of +it at all. -**Keep the `, 0`.** Anything below `plugins_loaded` priority 5 wires cleanly, but priority 0 is the -recommendation, in the block that owns your container rather than in a service provider. Booting at 5 -or later still works and is reported through `_doing_it_wrong()`, with the whole sequence running -inline instead. [Configuration][configuration] explains both, and closes with a complete bootstrap — -two sub-plugins, every optional key. +**Keep the `, 0`.** Anything below `plugins_loaded` priority 5 wires cleanly; priority 0, in the +block that owns your container rather than a service provider, is the recommendation. Booting at 5 +or later still works, with the whole sequence running inline and reported through +`_doing_it_wrong()`. [Configuration][configuration] explains both, and closes with a complete +bootstrap — two sub-plugins, every optional key. ## Docs @@ -65,15 +65,15 @@ two sub-plugins, every optional key. - [Tests][tests] — running the suite, the fixtures and traits it offers, and every scenario it drives the library through. -`docs/` and `tests/` are both `export-ignore`d, so neither is in a vendored copy of this library — -these point at the repository rather than at a path that would be missing beside the installed -source. +`docs/` and `tests/` are both `export-ignore`d, so neither ships in a vendored copy — these links +point at the repository rather than at paths that would be missing beside the installed source. [installing]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/installing.md [configuration]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/configuration.md [recipes]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/recipes.md [conflicts]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/conflict-handling.md [filters]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/filters.md +[actions]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/actions.md [notices]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/notices.md [extending]: https://github.com/stellarwp/plugin-absorber/blob/main/docs/extending.md [tests]: https://github.com/stellarwp/plugin-absorber/blob/main/tests/README.md diff --git a/docs/configuration.md b/docs/configuration.md index 7195a2d..b1f753a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -9,65 +9,47 @@ Config::set_hook_prefix( 'give' ); // required — keys hooks and optio Config::set_container( give()->container ); // required — everything is resolved from it ``` -The hook prefix accepts letters, numbers, hyphens, and underscores. Anything else throws -`Config_Exception`, as does reading the prefix before it is set. Hook names repeat it verbatim; -option names lowercase it and turn hyphens into underscores, so `Give-Core` hooks -`Give-Core/plugin_absorber/should_load` and stores `give_core_plugin_absorber_notices`. +The hook prefix accepts letters, numbers, hyphens, and underscores; anything else throws +`Config_Exception`, as does reading it before it is set. Hook names repeat it verbatim; option +names lowercase it and fold hyphens to underscores. -Any implementation of StellarWP's `ContainerInterface` will do — the one your plugin already -hands to Telemetry, Uplink or Harbor. It is required: `Config::get_container()` throws -`Config_Exception` when none is set, and `Config::has_container()` is the probe. To replace one -of the library's own pieces, see [Extending](extending.md). +Any implementation of StellarWP's `ContainerInterface` will do. `Config::get_container()` throws +`Config_Exception` when none is set; `Config::has_container()` is the probe. To replace one of the +library's own pieces, see [Extending](extending.md). `Config::set_host_plugin_basename( plugin_basename( __FILE__ ) )` is optional and matters only on -multisite. It lets the library compare your plugin's activation scope against a standalone's, so it -declines to deactivate a network-active standalone when your plugin is not itself network-activated -— a deactivation that would otherwise pull the standalone from the network's other sites, where your -bundled copy never loads. Left unset, that guard stays off and deactivation behaves as it always has. +multisite: it stops the library deactivating a network-active standalone when your plugin is not +itself network-activated, stranding the network's other sites. Left unset, nothing changes. -Both calls belong at `plugins_loaded` priority 0, in the block that owns your container rather -than in a service provider. Priority matters twice, for unrelated reasons: - -- Conflict resolution runs at `plugins_loaded` priority 5 and the load at 6, and WordPress - ignores a callback added at or past the priority it is already dispatching. Boot after that - and the whole sequence runs inline, reported with `_doing_it_wrong()`. -- A host that builds its container lazily may *replace* it at priority 0. Hand this library the - container before that happens and it holds an orphan whose bindings were discarded. - -Only the second reason picks 0 out of 0 through 4; if your container is already built by then, -anywhere below 5 works. Order among the configuration calls does not matter, so long as they all -precede `Absorber::boot()`. - -`Absorber::boot()` is idempotent, so calling it from more than one code path is safe — but the -first call is the one that counts, and the container it saw is the container the library keeps. -Setting a different one afterwards binds nothing into it, and leaves the accessors asking a -container that was never taught about this library. Set the container once, before booting. +Both calls belong at `plugins_loaded` priority 0, in the block that owns your container rather than +in a service provider, since a host that builds its container lazily may replace it at priority 0. +Anything below 5 works for a container already built — conflict resolution runs at +`plugins_loaded` priority 5 and the load at 6, and booting at 5 or later runs the whole sequence +inline, reported with `_doing_it_wrong()`. `Absorber::boot()` is idempotent but keeps the container +its first call saw. ## Registering a sub-plugin | Key | Type | Required | Meaning | |---|---|:--:|---| | `slug` | `string` | ✔ | Unique id — registry key, notice id, activation-tracking key. | -| `bundled_plugin_file` | `string` | ✔ | Absolute path to the **bundled** plugin's main file. This is what gets `require_once`d. | -| `plugin_loaded_constant` | `string` | ✔ | A constant the plugin defines when it loads, **at file scope**. Both copies normally define the *same* name, which is what stands one of them down. **Load guard only** — see [Conflict handling](conflict-handling.md#the-load-guard). | -| `standalone_plugin_basename` | `string` | | The standalone's `dir/file.php` basename, used to detect and deactivate it. Omit when there is no standalone. **Detection only.** | +| `bundled_plugin_file` | `string` | ✔ | Absolute path to the **bundled** plugin's main file — what gets `require_once`d. | +| `plugin_loaded_constant` | `string` | ✔ | A constant the plugin defines **at file scope** when it loads; both copies normally define the *same* name. **Load guard only** — see [Conflict handling](conflict-handling.md#the-load-guard). | +| `standalone_plugin_basename` | `string` | | The standalone's `dir/file.php` basename, used to detect and deactivate it; omit when there is none. **Detection only.** | | `enabled` | `bool\|callable` | | `true` by default. A `callable( Sub_Plugin ): bool` is re-evaluated on every call, not cached. | | `conflict_policy` | `string\|callable` | | `Conflict_Policy::DEACTIVATE` by default. See [Conflict handling](conflict-handling.md#policies). | -| `conflict_notice_message` | `callable` | | Used in all three places a conflict is reported — the merge notice, the still-active notice, and the rewritten activation-error screen. Each falls back to its own generic sentence naming the slug. | +| `conflict_notice_message` | `callable` | | Words all three conflict reports: the merge notice, the still-active notice, and the rewritten activation-error screen. Each falls back to a generic sentence naming the slug. | | `dependency_notice_message` | `callable` | | Shown when `dependency_check` fails. Defaults to a generic, untranslated sentence naming the raw slug. | | `activation_callback` | `callable( Sub_Plugin )` | | Runs **once, ever**, per slug, after a successful load. Make it idempotent. | | `dependency_check` | `callable( Sub_Plugin ): bool` | | Skips the load and queues a notice when it returns false. | -Sub-plugins load in **registration order**, so register a dependency before anything that -extends it at include time, and register each slug exactly once. A config array the library -cannot use throws `Config_Exception` on the spot, in the call you can see in your own stack -trace. A duplicate slug is found later, at the first read of the registry — normally on -`plugins_loaded` — since registrations are buffered until then: it is refused there and reported -through `_doing_it_wrong()`, the first registration under the slug stands, and the second is -discarded. +Sub-plugins load in **registration order**, so register a dependency before anything that extends +it at include time, and register each slug exactly once. An unusable config array throws +`Config_Exception` from `Absorber::register()`; a duplicate slug is caught later, at the first read +of the registry on `plugins_loaded`, where the second registration is discarded and reported through +`_doing_it_wrong()`. -Register unconditionally and put anything you cannot decide up front — a licence, a setting the -site owner can change — in `enabled`, which is re-evaluated on every load. See +Register unconditionally: anything you cannot decide up front belongs in `enabled`. See [Toggle a sub-plugin from a setting](recipes.md#toggle-a-sub-plugin-from-a-setting). ## How a sub-plugin loads @@ -89,20 +71,14 @@ flowchart TD F --> G["activation_callback, once ever"] ``` -Only the dependency gate says anything to the site owner — see [Notices](notices.md). An -unreadable `bundled_plugin_file` is a broken build in your plugin, so it is reported with -`_doing_it_wrong()` instead. - -The guard constant is checked **before** the dependency check, so a plugin the admin can watch -working is never reported as missing its requirements. The -[`should_load` filter](filters.md#the-load-gate) sits last and can only veto: it cannot force a -load past a copy already in memory. +Only the dependency gate says anything to the site owner — see [Notices](notices.md). The guard +constant is deliberately checked before the dependency check, and the +[`should_load` filter](filters.md#the-load-gate) can only veto. ## Activation -A bundled plugin is `require_once`d, not activated, so `register_activation_hook()` never fires -for it. Whatever that hook would have done — create a table, seed options — goes in -`activation_callback` instead: +A bundled plugin is `require_once`d, not activated, so `register_activation_hook()` never fires for +it. Whatever that hook would have done goes in `activation_callback` instead: ```php 'activation_callback' => static function ( Sub_Plugin $sub_plugin ) { @@ -110,34 +86,29 @@ 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 -written *after* the callback returns, so a callback that throws is reported with -`_doing_it_wrong()` and retried next request rather than marked done for good. - -**Write it to be idempotent.** "Once, ever" is bookkeeping, not a lock: the record is read, the -callback runs, and the record is written, so two first requests arriving together can both pass -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. 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 +**The wrapper is load-bearing.** `activation_callback` is `is_callable()`-checked at registration, +before the bundled plugin is loaded, so `[ Install::class, 'create_tables' ]` throws +`Config_Exception` on a class that does not exist yet. A closure names it only when the load pass +calls it. + +It runs only after a require that actually happened. The record lives in the +`{option_prefix}_plugin_absorber_activations` option — a network option on multisite — and is +written *after* the callback returns, so one that throws is reported with `_doing_it_wrong()` and +retried next request. + +**Write it to be idempotent.** "Once, ever" is bookkeeping, not a lock: two first requests arriving +together can both run the callback. A `dbDelta()` migration survives that; a blind `INSERT` of seed +rows does not. + +One record for the network is one *run* for the network, in whichever site's request reached the +load pass first; a site created afterwards never gets it. Per-site work is yours to loop over, and +a later site 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 -WordPress includes plugins at global scope; this library includes them from inside a method, so -variables assigned at the top level of the bundled file are function-local, not globals: +This library includes bundled plugins from inside a method, not at global scope, so variables +assigned at the top level of the bundled file are function-local, not globals: ```php // In the bundled plugin's main file. @@ -150,9 +121,9 @@ is unaffected. ## Messages are callables, never strings -Your config array is built at plugin load — before `init`, and before your textdomain, so -calling `__()` there raises WordPress's `_load_textdomain_just_in_time` notice. The two message -keys therefore take something to call, and refuse a string outright: +Your config array is built before `init` and before your textdomain, so `__()` there raises +WordPress's `_load_textdomain_just_in_time` notice. The two message keys therefore take something +to call, and refuse a string outright: ```php 'conflict_notice_message' => static fn() => __( 'Recurring ships with Give now.', 'give' ), @@ -165,29 +136,24 @@ keys therefore take something to call, and refuse a string outright: 'conflict_notice_message' => __( 'Recurring ships with Give now.', 'give' ), ``` -Each callable is passed the `Sub_Plugin` and called on every read; a return that will not cast -to a string is treated as though nothing were configured. - -**A plain function name is text, not a call.** `date`, `flush` and `key` are all real functions -and all plausible values, so wherever a string *is* accepted it is the value itself — which bars -`'Give_Recurring::get_conflict_message'` as much as `'give_recurring_conflict_message'`. +Each callable is passed the `Sub_Plugin` and called on every read; a return that will not cast to a +string is treated as though nothing were configured. Wherever a string *is* accepted it is the +value itself and never a function name to call. -`conflict_policy` is the one key that takes either, since a policy is never text a user reads: +`conflict_policy` takes either, since a policy is never text a user reads: ```php 'conflict_policy' => Conflict_Policy::DEFER, 'conflict_policy' => static fn( Sub_Plugin $sub_plugin ) => give_conflict_policy_for( $sub_plugin ), ``` -`standalone_plugin_basename` takes a string only: it names a file already on disk. -`dependency_check` and `activation_callback` have nothing a string could collide with, so they +`standalone_plugin_basename` takes a string only. `dependency_check` and `activation_callback` accept every callable form, a plain function name included. -Every typed key rejects a shape it cannot use at registration rather than at read time — -including a `[ class, method ]` pair naming a method that does not exist. `enabled` is the -exception: it is read as a boolean if it is not callable, so an array or an object there passes -registration and evaluates as enabled. Give it a `bool` or a `callable`, and nothing else. The -[filters](filters.md) are the other way in, and run last, after the configured value. +Every typed key rejects a shape it cannot use at registration rather than at read time, including a +`[ class, method ]` pair whose method does not exist. `enabled` is the exception: not callable means +it is read as a boolean, so an array or an object there evaluates as enabled — give it a `bool` or +a `callable`. The [filters](filters.md) are the other way in, and run last. ## Complete example diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index d53f908..ff9fac8 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -2,10 +2,9 @@ ## Policies -A policy is only reached for a sub-plugin that is enabled, that names a `standalone_plugin_basename` -whose plugin is active right now, and that the [`should_load` filter](filters.md#the-load-gate) has -not vetoed — a vetoed sub-plugin has no bundled copy to put in the standalone's place. Everything -else is skipped before any policy is read. +A policy is only reached for a sub-plugin that is enabled, names a `standalone_plugin_basename`, +whose standalone is active right now, and that the [`should_load` filter](filters.md#the-load-gate) +has not vetoed — a vetoed sub-plugin has no bundled copy to put in the standalone's place. | Policy | Behavior | |---|---| @@ -13,11 +12,11 @@ else is skipped before any policy is read. | `Conflict_Policy::DEFER` | Leave the standalone active; the load guard then stands the bundled copy down. | | `Conflict_Policy::NOTICE_ONLY` | Leave it active and ask the user to deactivate it. | -A policy decides what happens to the *standalone*, and nothing else. Under `DEFER` the bundled copy -stands down because of [the load guard](#the-load-guard), not because of the policy. +A policy decides what happens to the *standalone* only: under `DEFER` the bundled copy stands down +because of [the load guard](#the-load-guard), not because of the policy. -Set one per sub-plugin with the `conflict_policy` key — a constant, or a `callable( Sub_Plugin ): -string`. The `conflict_policy` [filter](filters.md) runs after that and has the final say: +Set one with the `conflict_policy` key — a constant or a `callable( Sub_Plugin ): string` — and the +`conflict_policy` [filter](filters.md) runs last: ```php // In the config: leave the standalone alone when it is not the code that was absorbed. @@ -31,92 +30,72 @@ add_filter( 'give/plugin_absorber/conflict_policy', static function ( $policy, $ }, 10, 2 ); ``` -**An unrecognised policy is treated as `NOTICE_ONLY`**, never as consent to deactivate — a typo -like `'defered'`, from a persisted option or from the filter, only produces a notice. +**An unrecognised policy is treated as `NOTICE_ONLY`**, never as consent to deactivate: a typo like +`'defered'` only produces a notice. For what the site owner sees under each policy, see [the recipe](recipes.md#choose-a-policy-and-know-what-the-site-owner-sees). ## When resolution runs -At `plugins_loaded` priority 5, one ahead of the load at priority 6: a standalone that survives the -conflict defines the guard constant as it loads, and the load has to see that. Priority 5 is also -the deadline for `Absorber::boot()`, since this is the first step it wires. - -Resolution runs **only on an interactive admin `GET`** — not WP-CLI, cron, ajax, or a form -POST — because resolving can deactivate a plugin and end the request with a redirect, and a 302 -would discard whatever a POST submitted. Waiting costs nothing: the standalone is still there to -detect on the next page view. - -**A `GET` carrying an action is skipped too.** `update.php?action=upgrade-plugin` and -`plugins.php?action=activate` are admin `GET`s that *do* something, and a redirect discards their -work exactly as it would a POST's. Anything naming `action` or `action2` waits for the next plain -page view — deliberately blunt, so a read-only `post.php?action=edit` waits as well. - -Resolution also requires the capability matching what deactivation does: `manage_network_plugins` on -multisite and `activate_plugins` otherwise, since deactivating a standalone is network-wide wherever -a network exists. The check matters because `plugins_loaded` fires well before `auth_redirect()`, so -an unauthenticated GET of an admin URL reaches this code on its way to the login screen. - -These gates apply whatever the policy is — the non-destructive policies only queue a notice, and the -[notice queue](notices.md#who-sees-them) asks for that same capability by name before it shows a -notice or clears one, so nothing a policy queued is consumed by someone the gate would have refused. - -The deactivation itself is silent, and covers both scopes on multisite. Silent because the -standalone's own deactivation hook would otherwise run this early: a routine `flush_rewrite_rules()` -in it would regenerate the rules before `init` declared a single post type, and every custom -permalink on the site would start 404ing. On multisite it can also decline outright — see -[the stranding guard](#the-multisite-stranding-guard). - -**A site that never loads wp-admin never resolves.** Every gate above needs an interactive admin -page view. Any website administered entirely over SFTP, Composer, or WP-CLI — one whose owner never -opens an admin screen in a browser — keeps the standalone active for as long as that holds. There -is still no fatal, because [the load guard](#the-load-guard) runs on every request and stands the -bundled copy down regardless; what waits is the *switchover*. The standalone, frozen at the version -installed, goes on serving in place of the bundled copy the host ships updates for until the first -request that clears every gate above arrives. That is the price of never ending a non-admin request -with a redirect — one that would drop a visitor's POST or cut a WP-CLI run short — and on such a -site the switchover is simply deferred until then. +At `plugins_loaded` priority 5, one ahead of the load at priority 6, so the load sees the guard +constant a surviving standalone defines. Priority 5 is also the deadline for `Absorber::boot()`. + +Resolution runs **only on an interactive admin `GET`** — not WP-CLI, cron, ajax, or a form POST, +since it can end the request with a redirect that would discard what a POST submitted. Nothing is +lost: the standalone is still there on the next page view. + +**A `GET` carrying an action is skipped too.** Anything naming `action` or `action2` waits for the +next plain page view, because a redirect would discard the work behind +`plugins.php?action=activate`. The rule is deliberately blunt, so `post.php?action=edit` waits too. + +Resolution also requires the capability deactivation needs: `manage_network_plugins` on multisite, +`activate_plugins` otherwise. An unauthenticated admin GET reaches this code before +`auth_redirect()` has sent it to the login screen, and resolves nothing. + +These gates apply whatever the policy is, and the [notice queue](notices.md#who-sees-them) asks for +the same capability. + +Deactivation is silent, and covers both scopes on multisite: the standalone's own deactivation hook +must not run this early, where a `flush_rewrite_rules()` in it would 404 every custom permalink. On +multisite it can also decline outright — see [the stranding guard](#the-multisite-stranding-guard). + +**A site that never loads wp-admin never resolves.** One administered only over SFTP, Composer, or +WP-CLI keeps the standalone active indefinitely. There is no fatal — [the load +guard](#the-load-guard) stands the bundled copy down on every request — but the switchover waits for +the first request that clears every gate above. ## The redirect -The standalone's code is already in memory by the time the conflict is resolved — WordPress -included it before `plugins_loaded` — so the redirect is how the request sheds it. The destination -is **the screen being requested**, not the one the user came from: it re-renders without the -standalone, and the admin stays where they asked to be, network or user admin included. `/wp-admin/` -and the admin roots mean the dashboard; `update.php` and `update-core.php` go to `plugins.php` -instead, because reloading either would re-run an update, and so does anything naming no usable -admin screen. There is no redirect loop: the next request has no active standalone, so nothing -resolves. +The standalone's code is already in memory when the conflict is resolved, so the redirect is how +the request sheds it. The destination is **the screen being requested**, not the one the user +came from, network and user admin included. `/wp-admin/` and the admin roots mean the dashboard; +`update.php` and `update-core.php` go to `plugins.php` instead, since reloading either would re-run +an update, and so does a URI naming no usable admin screen. There is no redirect loop: the next +request has no active standalone to resolve. -With several sub-plugins in conflict, all of them are resolved before the one redirect at the end, -and the redirect is skipped entirely once headers have been sent — the request then finishes -rendering instead of dying blank. The [merge notice](notices.md) is queued first either way, so the -explanation survives whether or not the request ends in a redirect. +With several sub-plugins in conflict, all are resolved before the one redirect at the end, and the +redirect is skipped once headers have been sent. The [merge notice](notices.md) is queued first +either way. ## The multisite stranding guard `deactivate_plugins()` runs with no `$network_wide` argument, so a network-active standalone is taken -out of *every* site's plugins. But the bundled copy only loads where the host plugin runs, so on a -network where the host is active on only some sites, deactivating a network-active standalone would -remove it from the sites the host never reached — leaving them with no copy of it at all, bundled or -standalone. +out of *every* site's plugins — including sites the host plugin never runs on, which would be left +with no copy at all. When the host names itself with `Config::set_host_plugin_basename( plugin_basename( __FILE__ ) )`, -the `DEACTIVATE` policy checks for exactly that case — a network-active standalone whose host is not -itself network-activated — and declines: the standalone is left active, [the load -guard](#the-load-guard) stands the bundled copy down network-wide as under `DEFER`, and a [stranding -notice](notices.md) tells a network administrator why and how to finish, by network-activating the -host or removing the standalone from the Network Admin. It recurs until one of those is done. +the `DEACTIVATE` policy detects exactly that case — a network-active standalone whose host is not +network-activated — and declines: the standalone stays active, [the load guard](#the-load-guard) +stands the bundled copy down network-wide as under `DEFER`, and a [stranding notice](notices.md) +explains it, recurring until a network administrator resolves the topology. -Pass `plugin_basename( __FILE__ )`, not `__FILE__`. A basename no installed plugin answers to reads -as a host that is never network-active, so the guard would decline for ever and the notice would -recur with nothing to act on — the library reports that one to the developer instead, through -`_doing_it_wrong()`, and leaves the standalone where it is. +Pass `plugin_basename( __FILE__ )`, not `__FILE__`: a basename no installed plugin answers to +reads as a host that is never network-active, so the guard would decline for ever. That one is +reported to the developer through `_doing_it_wrong()`, and the standalone is left where it is. -The guard is **opt-in and single-site-safe**: with no host basename set it never fires, and off a -network it never fires, so in every other topology — both network-active, both per-site, or a -per-site standalone — deactivation behaves exactly as it always has. +The guard is **opt-in and single-site-safe**: it never fires without a host basename set, and never +off a network, so every other topology deactivates as it always has. ## The load guard @@ -132,38 +111,37 @@ if ( ! defined( 'GIVE_RECURRING_VERSION' ) ) { } ``` -A standalone that defines it from a bootstrap hooked at `plugins_loaded` or later has not defined it -yet at the moment the guard is read, and the bundled copy would load on top of it. A standalone that -never defines the name at all is never stood down either, and the bundled copy loads alongside it; -[an included recipe](recipes.md#defer-to-a-standalone-that-is-a-new-codebase) is a functional -example of this behavior. +The guard is read on `plugins_loaded` at priority 6, so a standalone defining it from a bootstrap +hooked below that is still seen in time and one hooked past it is not, leaving the bundled copy to +load on top. At 6 itself it comes down to which of the two hooked first. One that never defines the +name is never stood down at all — +[an included recipe](recipes.md#defer-to-a-standalone-that-is-a-new-codebase) is a worked example. ## Reactivating the standalone The guard cannot help on the request that *activates* the standalone: WordPress includes the plugin -being activated **after** the bundled copy has already loaded, so that re-declaration is a real -fatal. Core catches it in its activation sandbox and prints *"Plugin could not be activated because -it triggered a fatal error."* — true, and useless to whoever pressed the button. +being activated **after** the bundled copy has loaded, so that re-declaration is a real fatal. Core +catches it in its activation sandbox and prints *"Plugin could not be activated because it triggered +a fatal error."* So the library filters `wp_admin_notice_markup` and swaps that sentence for the sub-plugin's -`conflict_notice_message`, falling back to a generic one naming the slug. This is what puts the -WordPress floor at 6.4: the filter does not exist before it. +`conflict_notice_message`, falling back to a generic one naming the slug. That filter is what puts +the WordPress floor at 6.4. -It touches nothing else. The markup comes back untouched unless every one of these holds — the -screen is `plugins`, or `plugins-network` in the network admin; the `plugin` query arg names a -standalone this library has registered; and `_error_nonce` verifies. +It touches nothing else: the markup comes back untouched unless the screen is `plugins`, or +`plugins-network` in the network admin; the `plugin` query arg names a registered standalone; and +`_error_nonce` verifies. The replacement runs through `wp_kses_post()`, so a knowledge-base link survives, and a message that -filters down to nothing leaves core's wording in place rather than blanking the notice. A host that -would rather keep core's wording throughout can remove the filter — see [Extending](extending.md). +filters down to nothing leaves core's wording standing. To keep core's wording throughout, remove +the filter — see [Extending](extending.md). ## Out of scope -**Version negotiation.** The library never compares versions. Express it yourself: read the version -and return `Conflict_Policy::DEFER` from the config or the `conflict_policy` [filter](filters.md), -which has the final say — [this recipe](recipes.md#defer-to-a-standalone-that-is-a-new-codebase) is -an example. +**Version negotiation.** The library never compares versions. Read the version yourself and return +`Conflict_Policy::DEFER` from the config or the `conflict_policy` [filter](filters.md), which has the +final say — [this recipe](recipes.md#defer-to-a-standalone-that-is-a-new-codebase) is an example. **Renamed standalone directories.** `standalone_plugin_basename` is the path as installed. A site -that renamed the standalone's directory is not detected, and there is no fallback deriving the path -from the load guard: one key is the guard, the other is the path, and no constant does both jobs. +that renamed the standalone's directory is not detected, and no fallback derives the path from the +load guard: one key is the guard, the other is the path. diff --git a/docs/extending.md b/docs/extending.md index 471aa6c..0f7a982 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -1,13 +1,13 @@ # Extending -Everything the library does past registration is a small object resolved from your container, so any -one piece can be swapped without replacing the rest. This is the one doc that names those classes; -nothing else in the docs asks you to know them. +Everything the library does past registration is a small object resolved from your container, so +any one piece can be swapped on its own. This is the one doc that names those classes. ## The seams -Bind any of these ids before or after `Absorber::boot()` — boot binds the defaults and skips an -*interface* your container already answers for, so your binding wins either way: +Bind any of these interface ids before or after `Absorber::boot()` — boot binds the defaults and +skips an interface your container already answers for, so your binding wins either way; +[class-name ids are different](#class-name-bindings-must-come-after-boot): ```php use Nexcess\PluginAbsorber\Registry\Contracts\Registrar_Interface; @@ -25,29 +25,25 @@ $container->singleton( Registrar_Interface::class, My_Registrar::class ); | `Contracts\Activator_Interface` | `Activator` | Runs a sub-plugin's activation callback once, ever. | **Rebind `Plugin\Contracts\Checker_Interface` when your plugin filters `option_active_plugins` or -`site_option_active_sitewide_plugins`** — LearnDash injects and then strips a synthetic path — because -`is_plugin_active()` then does not report what is in the database. +`site_option_active_sitewide_plugins`**: `is_plugin_active()` then does not report what is in the +database. **Rebind `Activator_Interface` to record "once, ever" somewhere else**: your own migration table, or -a per-site option on a large multisite network where one run for the whole network is not what you -want. See [the recipe](recipes.md#do-per-site-work-on-multisite). +a per-site option on a large network. See [the recipe](recipes.md#do-per-site-work-on-multisite). ## What each seam must implement -The table names the job; these are the methods, and every one of them is required — PHP refuses to -load a class that leaves one out, at class-declaration time rather than when the library first calls -it. Two of this library's types appear below: +Every method below is required. Two of this library's types appear in them: ```php use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Sub_Plugin; ``` -`Sub_Plugin` is one registered configuration, and its accessors — `get_slug()`, -`get_standalone_plugin_basename()`, the message accessors named below — are how an implementation -reads the config keys and runs [their filters](filters.md). A throw from any of these is caught at -the hook boundary and reported through `_doing_it_wrong()`, so it costs that one step rather than the -site; nothing retries it on the same request. +`Sub_Plugin` is one registered configuration; its accessors — `get_slug()` and the message +accessors below — read the config keys and run [their filters](filters.md). A throw from one is +caught at the hook boundary and reported through `_doing_it_wrong()`: it costs that step, not the +site, and the next request runs the step again. ### `Registry\Contracts\Registrar_Interface` @@ -56,12 +52,9 @@ public function register( Sub_Plugin $sub_plugin ): void; public function all(): array; ``` -`register()` stores one sub-plugin, and a slug may only be registered once — throw a -`Config_Exception` on the second rather than overwriting, or a copy-pasted slug silently replaces the -sub-plugin it collided with. `all()` returns every registered sub-plugin keyed by slug, in -registration order: `array`. The library narrows that array to `Sub_Plugin` -instances before reading it, so anything else in it is dropped rather than fataling — which loses a -sub-plugin quietly, and is worth not doing. +`register()` stores one sub-plugin; throw a `Config_Exception` on a slug already registered rather +than overwriting it. `all()` returns `array`, keyed by slug, in registration +order — anything else in it is dropped quietly. ### `Notices\Contracts\Writer_Interface` @@ -73,24 +66,21 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void; public function option_name(): string; ``` -- `queue_merge_notice()` — a deactivation has just been performed. Raised exactly once and never - re-queued, and the resolver redirects immediately afterwards, so it has to be kept somewhere that - outlives the request that wrote it. Word it from +- `queue_merge_notice()` — a deactivation has just been performed. Raised once, never re-queued, + and followed by a redirect, so keep it somewhere that outlives the request. Word it from `$sub_plugin->get_conflict_notice_message( $your_default )`, which applies the config key and its - filter over the default you pass. + filter over it. - `queue_conflict_notice()` — `NOTICE_ONLY`: the standalone is still running and the user is being asked to turn it off. Same accessor, a default that asks rather than reports. - `queue_stranding_notice()` — multisite only, and recurring rather than once-ever: a network-active - standalone was left active because deactivating it network-wide would strand the sites the host - plugin never reaches. Its wording is `$sub_plugin->get_stranding_notice_message()`, and it must not - tell the user to deactivate the standalone — see + standalone was left running rather than strand sites. Word it from + `$sub_plugin->get_stranding_notice_message()`, and do not tell the user to deactivate it — see [the stranding guard](conflict-handling.md#the-multisite-stranding-guard). - `queue_dependency_notice()` — `dependency_check` returned false, so the sub-plugin did not load at all. `$sub_plugin->get_dependency_notice_message()`. -- `option_name()` — where *your* implementation keeps the queue. It is on the contract rather than on - the default class because `Absorber::notices()->option_name()` is what a host - [rendering the queue itself](notices.md#rendering-them-yourself) reads: name an option nothing - writes to and that host reads an empty one. +- `option_name()` — where *your* implementation keeps the queue, and what a host + [rendering the queue itself](notices.md#rendering-them-yourself) reads through + `Absorber::notices()->option_name()`. ### `Plugin\Contracts\Deactivator_Interface` @@ -98,12 +88,10 @@ public function option_name(): string; public function deactivate( string $basename ): void; ``` -One method, one argument: a plugin basename like `give-recurring/give-recurring.php`, the only -identifier WordPress itself accepts. Called unattended during `plugins_loaded`, under the -`DEACTIVATE` policy, on behalf of a user who did not ask for it. It reports nothing — whether the -standalone actually went away is asked of the checker afterwards, not of you — so a no-op -implementation is the honest way to say "plugin state is managed outside WordPress here", and an -implementation that means to deactivate but does not leaves two copies of the plugin to load. +One argument: a plugin basename like `give-recurring/give-recurring.php`. Called unattended during +`plugins_loaded`, under the `DEACTIVATE` policy. It reports nothing — the checker is asked +afterwards whether the standalone went away — so a deliberate no-op is fine, while a silent +failure leaves two copies to load. ### `Plugin\Contracts\Checker_Interface` @@ -112,14 +100,11 @@ public function is_active( string $basename ): bool; public function is_network_active( string $basename ): bool; ``` -Two methods, asked for different reasons. `is_active()` answers "this plugin's code is going to run -this request", so both scopes count: a network activation runs it as surely as a site one. -`is_network_active()` is network scope only, and it is the one question the -[stranding guard](conflict-handling.md#the-multisite-stranding-guard) asks — "would deactivating this -reach every site". Return `false` whenever the site is not multisite, exactly as core's -`is_plugin_active_for_network()` does: callers lean on that instead of guarding with `is_multisite()` -themselves, so answering `true` off a network can have the guard decline a deactivation on a site -with nothing to strand. +`is_active()` answers "this plugin's code is going to run this request", so both scopes count. +`is_network_active()` is network scope only, and is what the +[stranding guard](conflict-handling.md#the-multisite-stranding-guard) asks: return `false` off +multisite, as core's `is_plugin_active_for_network()` does, since `true` there has the guard decline +a deactivation with nothing to strand. ```php use Nexcess\PluginAbsorber\Plugin\Contracts\Checker_Interface; @@ -149,13 +134,11 @@ class My_Checker implements Checker_Interface { public function resolve_all(): void; ``` -Called once per request, at `plugins_loaded` priority 5, and only after the -[request and capability gates](conflict-handling.md#when-resolution-runs) pass and a conflict has -been found. It is handed nothing: read the registry from the container your -implementation was built with, and resolve every sub-plugin whose standalone is active — including -the ones behind the first, which is why the default catches per sub-plugin. Ending the request is -allowed, and the default does (`wp_safe_redirect()` then `exit`) — but after the loop, never inside -it, or a site with two active standalones never reaches the second. +Called once per request at `plugins_loaded` priority 5, after the +[request and capability gates](conflict-handling.md#when-resolution-runs) pass and a conflict is +found. It is handed nothing: read the registry from the container it was built with and resolve +every sub-plugin whose standalone is active, catching per sub-plugin. Ending the request is allowed +— the default does `wp_safe_redirect()` then `exit` — but after the loop, never inside it. ### `Contracts\Activator_Interface` @@ -163,43 +146,37 @@ it, or a site with two active standalones never reaches the second. public function maybe_run( Sub_Plugin $sub_plugin ): void; ``` -Called by the load pass after a `require_once` that actually happened, on every request, for every -sub-plugin that loaded. Deciding it has already run for this slug is the whole job: -`$sub_plugin->get_activation_callback()` hands back the configured `callable` or `null`, it is -invoked with the `Sub_Plugin`, and where "already run" is recorded is yours. Record it after the -callback returns rather than before, so a callback that throws is retried on the next request instead -of being marked done half-finished. +Called on every request, for every sub-plugin whose `require_once` actually happened; deciding it +has already run for this slug is the whole job. `$sub_plugin->get_activation_callback()` hands back +the configured `callable` or `null` — invoke it with the `Sub_Plugin` and record "already run" +after the callback returns, so one that throws is retried rather than marked done. ## Class-name bindings must come after boot Everything without an interface is bound by class name — `Notices\Store`, `Notices\Renderer`, `Notices\Presenter`, `Conflict\Detector`, `Conflict\Gatekeeper`, `Conflict\Redirector`, -`Conflict\Rewriter`, `Loader`, `Registry\Reader`, `Boot\Scheduler`. Bind one of those **after** -`Absorber::boot()`: di52 reports `has()` true for any class that exists, bound or not, so boot cannot -tell your binding from the container's own willingness to build the class, and replaces it. +`Conflict\Rewriter`, `Loader`, `Registry\Reader`, `Boot\Scheduler`. Bind those **after** +`Absorber::boot()`: di52 reports `has()` true for any class that exists, bound or not, so boot +cannot tell your binding from an autowirable class, and overwrites it. -Boot also binds `StellarWP\ContainerContract\ContainerInterface` to the container itself, first and -before anything else, so a container that builds unbound classes reflectively can still satisfy the -library classes that take one. That id is an interface rather than a class, so the skip above applies -to it: a container that already answers for it keeps whatever it has, whenever you bound it. +Boot also binds `StellarWP\ContainerContract\ContainerInterface` to the container itself, so library +classes that take one can be built reflectively — an interface id, so your own binding stands. ## What rebinding does not buy you Binding your own `Conflict\Contracts\Resolver_Interface` does not put you in charge of *when* -resolution may run. The request and capability gates — [an interactive admin `GET` carrying no -action, and the capability to deactivate across the -network](conflict-handling.md#when-resolution-runs) — are asked before your resolver is built at all, -so an implementation that never thought about either is still safe. Everything the resolver *does* — -which policy branch, what the notice says, where the user lands — is yours. +resolution runs: the gates — [an interactive admin `GET` carrying no action, +and the capability to deactivate across the +network](conflict-handling.md#when-resolution-runs) — are asked before your resolver is built. What +it *does* is yours. ## The notice queue Four objects, so you can replace the part you have an opinion about: -- `Notices\Writer` decides what a notice says. The one behind an interface, and the seam for a host - already running its own notices library. -- `Notices\Store` keeps the queue. Rebind to store it elsewhere. -- `Notices\Renderer` draws it. Rebind to change the markup and leave the storage alone. +- `Notices\Writer` decides what a notice says — the one behind an interface. +- `Notices\Store` keeps the queue; rebind to store it elsewhere. +- `Notices\Renderer` draws it; rebind for the markup alone. - `Notices\Presenter` decides who may consume it, and does the render-then-clear. Rendering the queue yourself needs none of this — read @@ -220,22 +197,13 @@ remove_action( 'all_admin_notices', [ Absorber::class, 'render_notices' ] ); ## When a binding is wrong `Absorber::registrar()`, `notices()`, `resolver()` and `all()` check what your container hands back -and throw a `Config_Exception` naming the id they asked for and the class that failed it, rather than -letting a `TypeError` blame this library for your typo inside `plugins_loaded`. A binding your -container cannot build at all is reported the same way and not raised at you raw: whatever it threw -is caught and wrapped in a `Config_Exception` that names the id, keeping the original as -`getPrevious()`. `Absorber::boot()` is the one that does not wrap — a container that cannot build the -provider or the scheduler throws its own exception out of your `boot()` call. Past the check, -`Absorber::all()` also drops anything a rebound registrar returns that is not a `Sub_Plugin`, and -`Absorber::registrar()` hands back a registrar with every registration made so far already in it, so -the two answer alike whenever you ask. - -`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. +and throw a `Config_Exception` naming the id and the class that failed it. A binding the container +cannot build at all is wrapped the same way, keeping the original as `getPrevious()` — except in +`Absorber::boot()`, where a container that cannot build the provider or the scheduler throws its own +exception out of your `boot()` call. Past the check, `Absorber::all()` drops anything a rebound +registrar returns that is not a `Sub_Plugin`, and `Absorber::registrar()` hands back a registrar with +every registration made so far already in it, so the two answer alike whenever you ask. + +`Absorber::resolver()` hands you the resolver, not the gates in front of it: `resolve_all()` +re-checks neither, so calling it yourself can deactivate a plugin and `exit` on a POST, a cron run, +or a request from someone who may not deactivate anything. Make both checks first. diff --git a/docs/filters.md b/docs/filters.md index 8368d2b..57b4182 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -9,10 +9,10 @@ | `{prefix}/plugin_absorber/dependency_notice_message` | `string $message`, `Sub_Plugin $sub_plugin` | Final say over the dependency notice text. Receives the configured message, or the generic default sentence when nothing is configured. | | `{prefix}/plugin_absorber/stranding_notice_message` | `string $message`, `Sub_Plugin $sub_plugin` | Final say over the multisite stranding notice text. Receives the generic default; this notice has no config key, so the filter is its only override. | -Each runs last, after the configured value and any fallback, and fires when the value is asked for -rather than when the sub-plugin is registered — so it is also the place to call `__()`. A -non-scalar return yields an empty string rather than a fatal cast, and a `conflict_policy` return -that is not one of the three constants is treated as [`NOTICE_ONLY`, never as consent to +Each runs last, after the configured value and any fallback, and fires when the value is read rather +than at registration — so it is also the place to call `__()`. A non-scalar return yields an empty +string rather than a fatal cast, and a `conflict_policy` return that is not one of the three +constants is treated as [`NOTICE_ONLY`, never as consent to deactivate](conflict-handling.md#policies). ## The load gate @@ -29,20 +29,17 @@ add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_pl It is consulted only for a sub-plugin that would otherwise have loaded — after the enabled check, the guard constant, the dependency check and the file check. Returning `true` cannot force a load -past the guard constant; anything other than a truthy return skips the load, which is the safe -direction. +past the guard constant, and anything but a truthy return skips the load. -**Conflict handling reads it too**, at `plugins_loaded` priority 5, one ahead of the load. A -sub-plugin you veto is invisible to it: no standalone is deactivated to make room for a bundled copy -that is not going to load. So the filter may be asked more than once in a request, and it has to -*decide* rather than do — no logging, no counters, no writes. +**Conflict handling reads it too**, at `plugins_loaded` priority 5, one ahead of the load: a +sub-plugin you veto is invisible to it, so no standalone is deactivated to make room for a bundled +copy that is not going to load. The filter is therefore asked more than once in a request, and has +to *decide* rather than do — no logging, no counters, no writes. Whether that pass sees your callback +turns on when you call `add_filter()`, not on the priority you give it: registered after +`plugins_loaded` priority 5, it has missed the pass. Put a toggle both passes always read in the +`enabled` [config key](configuration.md) instead. -Whether the conflict pass sees your callback turns on when you call `add_filter()`, not on the -priority you give the callback: one registered after `plugins_loaded` priority 5 has missed that -pass whatever its own priority. For a sub-plugin with a `standalone_plugin_basename`, put the toggle -in the `enabled` [config key](configuration.md) instead: both passes read it, whenever it is set. - -**Watch the polarity when you wire an existing gate to this one.** `should_load` is true means *do +**Watch the polarity when you wire an existing gate to this one.** `should_load` true means *do load*. A host filter named for the opposite — LearnDash's `learndash_module_{x}_disabled`, where -true means *do not load* — inverts the gate if passed through unnegated, and the failure is silent -in the direction that loads a plugin the site turned off. +true means *do not load* — inverts the gate if passed through unnegated, and fails silently in the +direction that loads a plugin the site turned off. diff --git a/docs/installing.md b/docs/installing.md index b533d0a..39bede0 100644 --- a/docs/installing.md +++ b/docs/installing.md @@ -5,16 +5,15 @@ composer require stellarwp/plugin-absorber ``` Requires PHP 7.4+ and WordPress 6.4+. The WordPress floor comes from the `wp_admin_notice_markup` -filter, which does not exist before 6.4; WordPress is not a Composer dependency, so it is stated -here rather than enforced in `require`. +filter; WordPress is not a Composer dependency, so nothing enforces it at install time. ## Strauss Prefix this library with [Strauss](https://github.com/stellarwp/global-docs/blob/main/docs/strauss-setup.md). Two plugins shipping different versions of it will collide otherwise. -> **Nothing may rewrite a sub-plugin's `plugin_loaded_constant`.** Those are real, shared runtime -> constants: the whole safety mechanism depends on the bundled copy and the standalone defining the -> *same* name. This library only ever reads such a name out of your config, so its own source is -> safe to prefix in full — but if your build also runs the bundled plugin's own files through -> Strauss, keep `extra.strauss.constant_prefix` away from them. +> **Nothing may rewrite a sub-plugin's `plugin_loaded_constant`.** The bundled copy and the +> standalone must define the *same* name, or the guard matches nothing. This library only ever reads +> such a name out of your config, so its own source is safe to prefix in full — but if your build +> also runs the bundled plugin's files through Strauss, keep `extra.strauss.constant_prefix` away +> from them. diff --git a/docs/notices.md b/docs/notices.md index 73ec670..15777f0 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -2,31 +2,24 @@ The notices this library raises — the standalone was deactivated, the standalone is still active, a network-active standalone was left active to avoid stranding sites, a dependency check failed — are -queued in a single option named -`{option_prefix}_plugin_absorber_notices`, where `{option_prefix}` is the hook prefix lowercased -with hyphens folded to underscores: a hook prefix of `Give-Core` stores -`give_core_plugin_absorber_notices`. On multisite it is a **network** option, so the queue is shared -across every site on the network. It is an option and not a transient because a persistent object -cache keeps transients out of the database entirely, and a `wp_cache_flush()` would destroy a notice -that is raised exactly once and never re-queued. +queued in a single option named `{option_prefix}_plugin_absorber_notices`, where `{option_prefix}` +is the hook prefix lowercased with hyphens folded to underscores: a hook prefix of `Give-Core` +stores `give_core_plugin_absorber_notices`. On multisite it is a **network** option, so the queue is +shared across every site on the network. It is an option and not a transient because a persistent +object cache keeps transients out of the database, where a `wp_cache_flush()` would destroy a notice +raised exactly once and never re-queued. ## Who sees them Rendering prints the queue and then clears it, gated on the capability [conflict resolution](conflict-handling.md#when-resolution-runs) asks for: `manage_network_plugins` on multisite and `activate_plugins` otherwise. 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. - -On multisite that means a network administrator rather than the site administrator who installed the -plugin, and the network capability is asked for by name rather than left to core's mapping of -`activate_plugins`, which only widens into it while the network keeps the Plugins menu off for -individual sites. On a network that has turned that menu on, every site administrator holds -`activate_plugins` outright: any one of them opening any admin screen would otherwise print a notice -raised for the network and clear it network-wide, and the queue is one option shared by every site, -so it would be gone for everyone else. That covers the dependency notice too — a site administrator -no longer consumes one, which is no loss, since a queue shared by the whole network was never theirs -alone to consume. +act on a notice must not be shown one. + +On multisite that means a network administrator, not the site administrator who installed the +plugin: the queue is one option shared by every site, so a site administrator with `activate_plugins` +opening any admin screen would otherwise print a notice raised for the network and clear it for +everyone. That covers the dependency notice too. ## One message, two places @@ -37,23 +30,22 @@ done and as the explanation standing in for a fatal-error warning. ## The stranding notice -On multisite only, a network-active standalone whose bundled copy ships in a host plugin that is not -itself network-activated is left active rather than deactivated: turning it off across the network -would remove it from the sites the host is not active on, where nothing loads the bundled copy. This -notice explains that, and — unlike the one-time deactivation notice — it recurs until the topology is -resolved, either by network-activating the host or by removing the standalone from the Network Admin. -Its text is the `stranding_notice_message` [filter](filters.md); there is no config key for it. +On multisite only, a network-active standalone whose host plugin is not itself network-activated is +left active rather than deactivated: turning it off across the network would remove it from the +sites the host is not active on, where nothing loads the bundled copy. This notice explains that, +and — unlike the one-time deactivation notice — recurs until the topology is resolved, either by +network-activating the host or by removing the standalone from the Network Admin. Its text is the +`stranding_notice_message` [filter](filters.md); there is no config key for it. ## Rendering them yourself `Absorber::notices()->option_name()` tells you where the queue is kept, so you can render it yourself without replacing anything. The value is an `array` keyed `slug:type`, where the type is `merge`, `conflict`, `stranding` or `dependency` — `give-recurring:merge`, for example. -The first three render as `notice-warning` and the last as `notice-error`, since a dependency notice -reports a plugin that did not load at all. The messages may contain markup; the built-in rendering passes -them through `wp_kses_post()`, so a link, emphasis or a list survives while scripts and event -handlers are stripped. Paragraphs come from `wpautop()`, so send the message unwrapped and let a -blank line break it — a `

` of your own is left as it is rather than nested inside another. +The first three render as `notice-warning` and the last as `notice-error`. The messages may contain +markup; the built-in rendering passes them through `wp_kses_post()`, so a link, emphasis or a list +survives while scripts and event handlers are stripped. Paragraphs come from `wpautop()`, so send +the message unwrapped and let a blank line break it. ```php use Nexcess\PluginAbsorber\Absorber; @@ -61,7 +53,7 @@ use Nexcess\PluginAbsorber\Absorber; add_action( 'admin_init', function () { // Gates the read, not just the delete: `admin_init` fires for every logged-in user, and // draining the queue for one who cannot act on it destroys the only warning an - // administrator was going to get. The same capability the built-in rendering asks for. + // administrator was going to get. if ( ! current_user_can( is_multisite() ? 'manage_network_plugins' : 'activate_plugins' ) ) { return; } diff --git a/docs/recipes.md b/docs/recipes.md index 93a5566..b593413 100644 --- a/docs/recipes.md +++ b/docs/recipes.md @@ -7,8 +7,8 @@ behind them. ## Toggle a sub-plugin from a setting -Register unconditionally and put the condition in `enabled`. It is re-read on every request rather -than resolved at registration, so the settings screen saves an option and does nothing else: +Register unconditionally and put the condition in `enabled`, re-read on every request, so the +settings screen saves an option and does nothing else: ```php Absorber::register( [ @@ -20,32 +20,19 @@ Absorber::register( [ ] ); ``` -`enabled` is the first gate, ahead of the guard constant — -[how a sub-plugin loads](configuration.md#how-a-sub-plugin-loads) has the rest of the chain. Three -things follow from where it sits. +`enabled` is the first gate in [how a sub-plugin loads](configuration.md#how-a-sub-plugin-loads), +and gates conflict resolution too: off leaves the standalone alone as well. -**Switching the toggle off unloads nothing.** The `require_once` on this request already happened; -the next request is the one that skips it. Anything that has to stop immediately is the sub-plugin's -own business to gate. +**Switching the toggle off unloads nothing**: the next request is the one that skips the +`require_once`, so anything that has to stop sooner the sub-plugin gates itself. -**A disabled sub-plugin is invisible to conflict resolution too.** The toggle is checked first there -as well, so an off toggle also stops the standalone being deactivated: off means this library leaves -the plugin alone, standalone included. - -**Keep the callable cheap.** It runs on the conflict pass and again on the load pass, so at least -twice on an admin page view. An option read is fine; a remote licence check belongs behind a value -you have already cached. +**Keep the callable cheap** — it runs on both the conflict pass and the load pass: an option read +is fine, a remote licence check is not. ## Register several add-ons from one manifest -One `Absorber::register()` call per sub-plugin still happens here; the loop only builds each config -array on the way in. The manifest carries what differs between entries, the `+` union adds the keys -they share — so `bundled_plugin_file` is derived from each entry's own slug rather than written out -per entry — and the union never overwrites the left-hand side, so an entry that spells a shared key -out itself keeps its own value. - -The loop is also where load order is decided: sub-plugins load in registration order, so anything -extended at include time has to be registered before its extender. +The manifest carries what differs between entries; the loop supplies the keys they share. Its order +is load order. ```php $sub_plugins = [ @@ -64,17 +51,13 @@ foreach ( $sub_plugins as $slug => $constant ) { } ``` -An entry the library cannot use throws `Config_Exception` out of the `Absorber::register()` call it -is in, so a typo names itself in a stack trace pointing at your loop. A duplicate `slug` is found -later, at the first read of the registry — normally on `plugins_loaded` — because registrations are -buffered until then. It is refused there and reported through `_doing_it_wrong()`: the first -registration under the slug stands, the second is discarded, and every other sub-plugin loads as -normal. +An unusable entry throws `Config_Exception` from its own `Absorber::register()` call, so the stack +trace points at your loop; a duplicate `slug` is caught at the first read, on `plugins_loaded`, where +the second registration is discarded and reported through `_doing_it_wrong()`. ## Choose a policy, and know what the site owner sees -The policy is only reached for a sub-plugin that is enabled, names a `standalone_plugin_basename`, -and whose standalone is active right now: +What each branch leaves the site owner with: ```mermaid flowchart TD @@ -90,19 +73,17 @@ flowchart TD | Policy | The standalone | This request | Afterwards | |---|---|---|---| -| `DEACTIVATE` | turned off, silently, network-wide | its code is still in memory, so the bundled copy stands down; the user is redirected back to the screen they asked for | the bundled copy loads, and a merge notice explains the swap | +| `DEACTIVATE` | turned off silently, network-wide | still in memory, so the bundled copy stands down; the user is redirected to the screen they asked for | the bundled copy loads; a merge notice explains the swap | | `NOTICE_ONLY` | left running | the bundled copy stands down | unchanged until someone acts on the notice | | `DEFER` | left running | the bundled copy stands down | unchanged, and nothing is said | -The redirect under `DEACTIVATE` re-renders the screen with the standalone's code gone. It happens -once per request however many standalones were turned off, and only on an interactive admin `GET` -carrying no `action` — [conflict handling](conflict-handling.md#when-resolution-runs) has the gates. +Only an interactive admin `GET` with no `action` reaches any of this — +[conflict handling](conflict-handling.md#when-resolution-runs) has the rest of the gates. ## Ship the absorption over several releases -Bundling the code and taking over from the standalone do not have to be the same release. Moving the -`conflict_policy` one step per release lets a site be warned before anything of theirs is turned -off: +Bundling the code and taking over from the standalone need not be the same release. Moving the +`conflict_policy` one step per release warns a site before anything of theirs is turned off: ```mermaid flowchart LR @@ -111,16 +92,13 @@ flowchart LR R3["Release 3
DEACTIVATE
the remainder are merged,
and told so"] ``` -Release 1 is the safe one to leave in place for a while: the bundled copy ships dormant on every -site that has the standalone, which is the population you are least sure about. Release 3 is the -only one that touches a site's active plugins. Each step is a one-constant change — or none at all, -if the policy comes from a callable reading a value you can move without shipping: +Only release 3 touches a site's active plugins. Each step is a one-constant change — or none, if +the policy reads a value you can move without shipping: ```php /** - * Read fresh on every conflict pass, so whatever writes this option -- a settings screen, a - * support tool, WP-CLI, a migration -- changes how the next admin page view resolves the - * conflict. Nothing is cached, and no release has to ship for the stage to move. + * Read fresh on every conflict pass, so a settings screen, WP-CLI or a migration can move the + * stage with no release. */ 'conflict_policy' => static fn() => get_option( 'give_absorption_stage', Conflict_Policy::DEFER ), ``` @@ -130,14 +108,11 @@ misspelt option cannot turn a plugin off. ## Defer to a standalone that is a new codebase -This is an edge case, but one that has occurred before: a later version of a standalone that shares -no code with the version you bundled, while still shipping under the same folder and file name. - -LearnDash's ProPanel is a case where this happened. `learndash-propanel/learndash_propanel.php` was -ProPanel 2.x, which LearnDash absorbed into its Reports module. ProPanel 3.0 arrived at that same -path as a new codebase — its own namespace, its own `LDRP_*` constants, none of the 2.x code. The -basename is all the two versions have in common, so `standalone_plugin_basename` cannot tell them -apart. The version installed at that path can, so read it and defer: +An edge case that has happened: `learndash-propanel/learndash_propanel.php` was ProPanel 2.x, which +LearnDash absorbed into its Reports module, and ProPanel 3.0 arrived at that same path as an +unrelated codebase with its own namespace and its own `LDRP_*` constants. +`standalone_plugin_basename` cannot tell the two apart; the installed version can, so read it and +defer: ```php add_filter( 'learndash/plugin_absorber/conflict_policy', static function ( $policy, $sub_plugin ) { @@ -155,10 +130,7 @@ add_filter( 'learndash/plugin_absorber/conflict_policy', static function ( $poli false ); - /** - * 3.0 and up at this path is the newer codebase, not an older copy of ours, so there is - * nothing to take over from. Anything below it is the version that was absorbed. - */ + // 3.0 and up at this path is the newer codebase, not an older copy of ours. return version_compare( $installed['Version'], '3.0.0-dev', '>=' ) ? Conflict_Policy::DEFER : $policy; @@ -173,16 +145,14 @@ add_filter( 'learndash/plugin_absorber/conflict_policy', static function ( $poli | an older version of the same code | defined by it | stands down | | a new codebase at the same path | never defined | **loads, alongside it** | -ProPanel 2.x defines `LD_PP_PLUGIN_DIR` and 3.x defines only `LDRP_*`, so with 3.x active the -bundled Reports module loads too — correct here, since the two share a file name and nothing else. -Treat it as the exception it is: two copies loading at once is normally the fatal this library -exists to prevent, and it is safe only because these two are not copies of each other. +ProPanel 3.x defines only `LDRP_*`, never 2.x's `LD_PP_PLUGIN_DIR`, so the bundled Reports module +loads alongside it. Safe only because the two are not copies of each other: two copies loading at +once is the fatal this library exists to prevent. ## Do per-site work on multisite -Deactivation is network-wide, the notice queue is a network option, and so is the activation record, -so `activation_callback` runs **once for the network** — in whichever site's request reached the -load pass first. Per-site work loops: +The activation record is a network option, so `activation_callback` runs **once for the network**. +Per-site work loops: ```php 'activation_callback' => static function ( Sub_Plugin $sub_plugin ) { @@ -200,8 +170,7 @@ load pass first. Per-site work loops: }, ``` -That is the right shape for a handful of sites and the wrong one for a large network, where the loop -runs inside `plugins_loaded` on one unlucky request. Replace the once-ever bookkeeping — see -[Extending](extending.md) — and record it per site, so each site pays only for itself. Either way, -write the callback to be idempotent: "once, ever" is bookkeeping rather than a lock, and -[activation](configuration.md#activation) has the retry and concurrency detail. +That loop runs inside `plugins_loaded` on one unlucky request, so on a large network replace the +once-ever bookkeeping and record it per site instead — see [Extending](extending.md). Either way +write the callback to be idempotent; [activation](configuration.md#activation) has the retry and +concurrency detail. diff --git a/src/Absorber.php b/src/Absorber.php index 4b9fc85..090d146 100644 --- a/src/Absorber.php +++ b/src/Absorber.php @@ -22,14 +22,8 @@ /** * Static facade: registration, and the one call that starts everything. * - * What a host touches, and deliberately little else. How collaborators are built belongs to - * `Provider`, when they run to `Boot\Scheduler`, and the load pass itself to `Loader` — so - * the only reason to open this file is to change what a host may say to the library. - * - * `final` because it cannot usefully be extended: every member is static, the one property and the - * one helper behind the API are private, and every internal call is `self::`, so a subclass would - * inherit the API, be unable to change what any of it does, and change nothing — which is the silent - * no-op this class reports on everywhere else. + * `final` because every member is static and every internal call is `self::` — a subclass could + * override nothing, and would silently change nothing. * * @since 1.0.0 */ @@ -37,8 +31,6 @@ final class Absorber { use Guards_Hook_Prefix; /** - * Whether the hooks have been wired. - * * @var bool */ private static $booted = false; @@ -67,7 +59,7 @@ final class Absorber { * * @since 1.0.0 * - * @throws Config_Exception When no container has been set, or its binding is unusable. + * @throws Config_Exception When the container is unset, or its binding unusable. * * @return Registrar_Interface */ @@ -84,7 +76,7 @@ public static function registrar(): Registrar_Interface { /** * @since 1.0.0 * - * @throws Config_Exception When no container has been set, or its binding is unusable. + * @throws Config_Exception When the container is unset, or its binding unusable. * * @return Writer_Interface */ @@ -95,7 +87,7 @@ public static function notices(): Writer_Interface { /** * @since 1.0.0 * - * @throws Config_Exception When no container has been set, or its binding is unusable. + * @throws Config_Exception When the container is unset, or its binding unusable. * * @return Resolver_Interface */ @@ -106,16 +98,9 @@ public static function resolver(): Resolver_Interface { /** * Register one bundled sub-plugin. Call once per sub-plugin, before boot(). * - * The sub-plugin is buffered rather than handed straight to the registrar, so that registering - * resolves nothing — not even the container. A host that registers before it calls - * Config::set_container() would otherwise fail on a call that has nothing to do with the - * container. The buffer belongs to `Registry\Reader`, which is where it is read back out: this - * class hands its collaborators no work and holds none of their state. - * - * The configuration is still validated here: building the Sub_Plugin is what rejects it, and - * that happens at the call the host can see in its own stack trace. It is built rather than - * resolved because it is a value object — a container asked for one would need the config - * passed through it, and there is nothing about it to rebind. + * Buffered rather than handed to the registrar, so registration resolves nothing — not even the + * container, which a host may set after this call. `Sub_Plugin` still validates the config here, + * in the host's own stack trace. * * @since 1.0.0 * @@ -132,10 +117,8 @@ public static function register( array $config ): void { /** * Every registered sub-plugin, keyed by slug, in registration order. * - * A delegation like the accessors above it, and for the same reason: what a host calls is here, - * what it does is the collaborator's. The passes that read the registry are handed that - * collaborator directly rather than calling back through this method — a facade sits in front of - * its collaborators, never underneath them. + * The passes read the registry through the reader they were handed, never back through here: a + * facade sits in front of its collaborators, never underneath them. * * @since 1.0.0 * @@ -148,20 +131,11 @@ public static function all(): array { } /** - * Bind the collaborators, then let the scheduler decide when they run. Idempotent — safe to - * call from more than one code path. - * - * Idempotent means the first call wins outright, and the container is part of what it wins. - * A Config::set_container() after this has returned binds nothing: the scheduler keeps the - * container it closed over, while the accessors and the notice trampolines resolve from - * whatever Config holds when they are called, so the two halves would answer to different - * containers and the accessors would ask an unbound one. Set the container first — the - * recommended slot is plugins_loaded priority 0 — and do not replace it afterwards. + * Bind the collaborators, then let the scheduler decide when they run. Idempotent. * - * The provider is constructed rather than resolved: it is what teaches the container about this - * library, so the container cannot be asked to build it first. It is bound afterwards, and only - * when nothing answers to `Provider_Interface` already, so a host may replace the whole set of - * bindings with one of its own. + * Set the container before booting: the scheduler closes over the one it finds here. The provider + * is bound only when nothing answers to `Provider_Interface` already, so a host may replace the + * whole set of bindings. * * @since 1.0.0 * @@ -183,9 +157,8 @@ public static function boot(): void { $container->get( Provider_Interface::class )->register(); $container->get( Scheduler::class )->wire(); - // Last, not first. A boot that threw on its way through -- no container, a binding that - // cannot be built -- has wired nothing, and a host that fixes the mistake and calls again - // should get a working library rather than a silent no-op. + // Last, not first: a boot that threw part-way through wired nothing, so calling again after + // the fix must give a working library rather than a no-op. self::$booted = true; } @@ -201,10 +174,8 @@ public static function render_notices(): void { return; } - // 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. + // Notice wording comes from host callables, on a hook every admin screen fires: the notice is + // worth less than the wp-admin a throw would white-screen. try { self::collaborator( Presenter::class )->render(); } catch ( Throwable $thrown ) { @@ -219,9 +190,9 @@ public static function render_notices(): void { /** * Rewrite the activation-error notice for a standalone this library has absorbed. * - * The parameter is untyped because a filter receives whatever the filter before it returned, - * and a `string` declaration would turn another plugin's sloppy return into a TypeError raised - * from here. + * The parameter is untyped deliberately: a filter receives whatever the filter before it + * returned, and a `string` declaration would raise a TypeError from the screen least able to + * afford one. * * @since 1.0.0 * @@ -236,10 +207,8 @@ public static function filter_activation_error_markup( $markup ): string { return $markup; } - // Guarded like render_notices, and for a sharper version of the same reason: this runs while - // WordPress is drawing the screen that reports a fatal, so a throw out of here would replace - // the error the admin came to read with one of ours. The markup goes back as it arrived and - // core's wording stands. + // This draws the screen that reports a fatal: a throw would replace the error the admin came + // to read with one of ours. try { return self::collaborator( Rewriter::class )->rewrite( $markup ); } catch ( Throwable $thrown ) { @@ -256,12 +225,9 @@ public static function filter_activation_error_markup( $markup ): string { /** * The object bound to a collaborator interface, checked before it is handed on. * - * The container's own return type promises nothing, so a host that bound the wrong class -- a - * typo'd class name, an interface it forgot to implement -- would otherwise surface as a - * TypeError raised inside this library, naming this library's method. That reads as a bug here - * rather than a mistake in the host's own bindings, and it happens inside `plugins_loaded`, - * where nobody is looking. Naming the interface and the class that failed it turns the same - * failure into an instruction. + * A host that bound the wrong class would otherwise surface as a TypeError inside + * `plugins_loaded` reading as a bug here; naming the interface and the class that failed it + * makes it an instruction instead. * * @since 1.0.0 * @@ -269,24 +235,19 @@ public static function filter_activation_error_markup( $markup ): string { * * @param class-string $interface Collaborator interface to resolve. * - * @throws Config_Exception When no container has been set, when it throws while building the - * binding, or when the binding does not implement the interface it was - * bound to. + * @throws Config_Exception When no container has been set, when building the binding throws, or + * when the binding does not implement the interface. * * @return T */ private static function collaborator( string $interface ): object { - // Resolved outside the try: a missing container is this library's own configuration error - // already, reported in its own words, and re-wrapping it would bury that sentence one - // exception deeper for no gain. + // Outside the try: a missing container is already this library's own error, in its own words, + // and re-wrapping would bury that sentence an exception deeper. $container = Config::get_container(); - // A host factory closure is free to throw, and a container asked for a binding with an - // unsatisfiable dependency -- or for an interface nothing has bound yet, which is every - // interface before boot() runs the provider -- throws its own exception type. Uncaught, - // either one leaves the host's plugins_loaded with a fatal from a vendor namespace that - // names neither this library nor the binding at fault, so both are reported the same way as - // a binding of the wrong type. The original failure is kept as the previous exception. + // A host factory may throw, and so does the container asked for an interface nothing has + // bound yet. Uncaught, either fatals from a vendor namespace naming neither this library + // nor the binding at fault. try { $collaborator = $container->get( $interface ); } catch ( Throwable $thrown ) { diff --git a/src/Activator.php b/src/Activator.php index d78943a..9d341e6 100644 --- a/src/Activator.php +++ b/src/Activator.php @@ -39,9 +39,8 @@ public function maybe_run( Sub_Plugin $sub_plugin ): void { return; } - // Recorded after the callback rather than before it. A callback that fatals halfway leaves - // the site half-migrated either way, but recording first would also mean the next request - // skips it, so the half-finished state becomes permanent and invisible. + // Recorded after the callback, never before: recording first would freeze a callback that + // throws halfway, since the next request would skip it rather than retry. $callback( $sub_plugin ); $done[ $slug ] = true; @@ -52,14 +51,9 @@ public function maybe_run( Sub_Plugin $sub_plugin ): void { /** * Slugs whose activation callback has already run, keyed by slug. * - * A site option, not a plain one: `deactivate_plugins()` is network-wide on multisite, so an - * activation that follows a network-wide merge has to be recorded network-wide too, or every - * site in the network runs the callback again. - * - * 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. + * A site option, not a plain one: `deactivate_plugins()` is network-wide on multisite, so the + * record is too. The cost is that a site created afterwards finds it set and never runs the + * callback; per-site work belongs in an `Activator_Interface` of the host's own. * * @since 1.0.0 * @@ -70,19 +64,15 @@ public function maybe_run( Sub_Plugin $sub_plugin ): void { private function completed(): array { $done = get_site_option( $this->option_name(), [] ); - // Anything that is not an array is replaced rather than trusted. A corrupted option would - // otherwise fatal on the array read, inside plugins_loaded, on every request. + // A corrupted option would otherwise fatal on the array read, on every request. return is_array( $done ) ? $done : []; } /** * The option every slug's activation record lives in. * - * Private, and not static, unlike `Notices\Store::option_name()`: that one is public because - * `docs/notices.md` tells a host to read the queue option itself, and this record has no such - * reader. Nothing outside this class composes the name, so nothing outside it needs to be told - * the name, and a host that wants the bookkeeping elsewhere binds `Activator_Interface` rather - * than reading around this one. + * Private, unlike `Notices\Store::option_name()`: nothing outside reads it, and a host wanting + * the bookkeeping elsewhere binds `Activator_Interface` instead. * * @since 1.0.0 * diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php index 310fc59..f15f0d8 100644 --- a/src/Boot/Scheduler.php +++ b/src/Boot/Scheduler.php @@ -17,13 +17,9 @@ use WP_Hook; /** - * When this library's work happens: which hook, at which priority, and what to do when it is - * already too late to say so. + * Which hook this library's work runs on, at which priority, and what to do when it is too late. * - * Its own class because timing is the part of booting that changes for its own reasons — a step - * added, a priority moved, a hook that fires too late to wire — and none of those are reasons to - * touch registration or the load pass. The provider says how collaborators are built and this says - * when they run, so neither has to be read to change the other. + * Its own class because the provider says how collaborators are built and this says when they run. * * @since 1.0.0 */ @@ -31,16 +27,8 @@ class Scheduler { /** * plugins_loaded priority the load pass runs at. * - * Ahead of the default priority, so a bundled plugin is in memory before the plugins that - * expect it start their own work. - * - * Every priority below this one is a band of the bundled plugin's *own* plugins_loaded - * callbacks that silently never fire: a standalone copy is included by wp-settings.php before - * the action is dispatched at all and keeps every callback it registers, while a bundled copy - * required from a callback here only keeps the ones above the priority that required it. - * Hooking plugins_loaded below the default is already a special case, so the band this gives - * up is a narrow one — but it is given up silently, which is why the number moves down when - * there is any doubt and not up. + * Every priority below this one is a band of the bundled plugin's *own* plugins_loaded callbacks + * that silently never fire, which is why this number moves down when in doubt and not up. * * @since 1.0.0 * @@ -49,18 +37,12 @@ class Scheduler { private const LOAD_PRIORITY = 6; /** - * plugins_loaded priority conflict resolution runs at, ahead of the load pass. - * - * A standalone that survives the conflict defines the guard constant as it loads, and the load - * pass has to see that, so resolution cannot share a priority with it. + * plugins_loaded priority conflict resolution runs at, one ahead of the load pass. * - * This is the number a host is measured against, not the load: it is the first step in the - * sequence, so it is the priority `set_container()` and `boot()` have to beat. At 1 the only - * slot left was 0, which made a documented convention a hard requirement — and both LearnDash - * and MemberDash wire Harbor's `set_container()` at priority 1, so a host copying the habit it - * already has landed exactly on the barrier and got the inline fallback instead of the hooks. - * Five slots ahead of it covers both habits with room over, and costs the load pass four - * priorities it was not using. + * A surviving standalone defines the guard constant as it loads and the load pass has to see + * that. Also the number a host is measured against, being the first step in the sequence — 5 + * rather than 1 because LearnDash and MemberDash already wire Harbor's `set_container()` at + * priority 1, so a host copying that habit would land on the barrier. * * @since 1.0.0 * @@ -78,7 +60,7 @@ class Scheduler { /** * @since 1.0.0 * - * @param ContainerInterface $container Container every step resolves from, when it runs. + * @param ContainerInterface $container Container every step resolves from when it runs. */ public function __construct( ContainerInterface $container ) { $this->container = $container; @@ -87,13 +69,9 @@ public function __construct( ContainerInterface $container ) { /** * Wire the WordPress hooks. * - * Nothing is resolved here. Each step is a closure over the container that asks for its - * collaborator when the hook fires, so a host may still rebind one after boot() and up until - * plugins_loaded, and a binding nothing reaches is never built at all. - * - * Called too late, the steps run inline instead of being wired — and conflict resolution can - * end the request, so on an admin page load this call may not return. Boot before plugins_loaded - * priority 5, as documented, and it always does. + * Nothing is resolved here: every callback asks the container for its collaborator when the hook + * fires, so a host may still rebind one after boot() and a binding nothing reaches is never + * built. Called too late, the steps run inline instead — and may then not return. * * @since 1.0.0 * @@ -101,25 +79,16 @@ public function __construct( ContainerInterface $container ) { */ public function wire(): void { if ( is_admin() ) { - // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, - // network_admin_notices and user_admin_notices as mutually exclusive branches, so a - // superadmin working in the network admin -- exactly where a network-wide - // deactivation gets noticed -- would never see the queue rendered. + // all_admin_notices, not admin_notices: the latter's three branches are mutually exclusive, + // so a superadmin in the network admin would never see the queue rendered. add_action( 'all_admin_notices', [ Absorber::class, 'render_notices' ] ); - // A named static trampoline like the notice step above, not a closure. Both resolve - // their collaborator when they fire, so neither builds anything at boot, but a named - // callback can also be taken back with remove_filter() -- which matters most here, on - // the one hook that rewrites a screen WordPress drew rather than adding one of our own. - // The closures below are shaped that way for a reason these two do not share: the - // plugins_loaded sequence has to be runnable inline as well as wirable. + // Named rather than a closure: remove_filter() is a host's only way back to core's wording. add_filter( 'wp_admin_notice_markup', [ Absorber::class, 'filter_activation_error_markup' ] ); } - // Adding an action at a priority the current dispatch has already passed is accepted and - // then never fires. Booting from plugins_loaded at the default priority -- the commonest - // hook mistake there is -- would otherwise mean nothing loads at all, with no warning and - // a site that looks entirely healthy. + // A callback added at a priority the dispatch has already passed is accepted and never fires, + // so a host that boots too late would otherwise get a healthy-looking site that loads nothing. if ( $this->wiring_window_has_closed() ) { _doing_it_wrong( Absorber::class . '::boot', @@ -185,13 +154,8 @@ public static function registration_window_has_closed(): bool { /** * The plugins_loaded steps, in run order, as priority and callback. * - * Stated once because wire() expresses this order twice — as hook priorities when it can still - * wire, and as straight calls when it is too late to and has to run them inline. Those two are - * the same sequence, and a comment is the only thing that could hold them in agreement. - * Iterating one list cannot drift. - * - * A method rather than a constant because a step is a closure now: naming the step and giving - * its priority in two separate lists would put the drift straight back, one list deep. + * Stated once because wire() expresses this order twice — as hook priorities, and as straight + * calls when it is too late to wire — and iterating one list cannot drift. * * @since 1.0.0 * @@ -219,9 +183,6 @@ private function sequence(): array { /** * The conflict step: the two gates, the probe between them, and the resolve behind all three. * - * Static, and handed the container rather than reading one, so the closure in sequence() stays a - * closure over the container like the load step beside it. - * * @since 1.0.0 * * @param ContainerInterface $container Container each collaborator is resolved from. @@ -232,20 +193,15 @@ private static function resolve_conflicts( ContainerInterface $container ): void try { $gatekeeper = $container->get( Gatekeeper::class ); - // The shape of the request first. It reads the request and nothing else, so cron, WP-CLI, a - // POST and every front-end view are turned away having resolved no user and built no - // resolver. + // Request shape first: cron, WP-CLI, POSTs and front-end views turn back with no user + // resolved and no resolver built. if ( ! $gatekeeper->request_may_resolve() ) { return; } - // Then whether there is a conflict at all, before anyone asks who is signed in. - // current_user_can() resolves and caches the current user, and this step runs at - // plugins_loaded priority 5 -- ahead of the plugins that add their determine_current_user - // filter from a plugins_loaded callback of their own. Ask on every admin GET and an SSO or - // JWT visitor is pinned as logged out for the rest of the request, on requests with nothing - // to resolve. The detector reports and changes nothing, so the capability is only asked for - // where its answer decides something. + // Then whether there is a conflict at all, before anyone asks who is signed in: + // current_user_can() caches the current user, and at priority 5 that would settle it ahead + // of an SSO plugin's own plugins_loaded filter, on requests with nothing to resolve. if ( ! $container->get( Detector::class )->has_conflict() ) { return; } @@ -254,16 +210,13 @@ private static function resolve_conflicts( ContainerInterface $container ): void return; } - // Both gates and the probe live here rather than inside the resolver, so a host binding - // its own cannot drop one by omission -- and asking them first means a resolver is built - // only on the request that goes on to use it. + // Both gates and the probe live here, not in the resolver, so a host binding its own cannot + // drop one by omission. $container->get( Resolver_Interface::class )->resolve_all(); } catch ( Throwable $thrown ) { - // The backstop, and the promise the whole library rests on: plugins_loaded fires on every - // request a site serves, so a throw out of a step is a white screen on all of them. What - // reaches here is a collaborator a host's factory could not build, a gate, the probe, or a - // resolver a host bound itself -- the passes guard their own per-sub-plugin loops, and this - // guards everything the step touches on the way to them. + // Nothing reached from a hook may throw: plugins_loaded fires on every request a site + // serves, so a throw out of a step is a white screen on all of them. Report it and abandon + // this step alone -- the passes guard their own per-sub-plugin loops. self::report_a_step_that_threw( 'conflict pass', 'no conflict was resolved', $thrown ); } } @@ -278,10 +231,8 @@ private static function resolve_conflicts( ContainerInterface $container ): void * @return void */ private static function load( ContainerInterface $container ): void { - // Guarded like the conflict step, and for the same reason. `Loader::load_all()` already reports - // per sub-plugin and carries on, so what is left for this to catch is the pass itself -- a - // container that cannot build it above all, which is the shape a host's own broken binding - // takes. + // Guarded like the conflict step; `Loader::load_all()` already reports per sub-plugin, so what + // is left to catch is a container that cannot build the pass at all. try { $container->get( Loader::class )->load_all(); } catch ( Throwable $thrown ) { @@ -311,15 +262,9 @@ private static function report_a_step_that_threw( string $step, string $conseque /** * Whether it is already too late to wire the first step of the sequence. * - * Measured against the earliest priority in sequence(), read rather than restated, because a - * boot that can still wire a later step but has missed an earlier one has missed something — - * and with resolution at 5 and the load at 6, booting between the two is a real window. - * - * The comparison is inclusive. A callback added to the priority currently being dispatched is - * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` - * with a by-value foreach, so the append lands on an array the running loop has already copied. - * Booting from plugins_loaded at that priority is the case a host is likeliest to hit by - * accident, and an exclusive comparison would let exactly that one through unreported. + * Measured against the earliest priority in sequence(), read rather than restated: with resolution + * at 5 and the load at 6, booting between the two would otherwise wire the load and silently lose + * the conflict pass. Inclusive, because a callback added at the priority dispatching never runs. * * @since 1.0.0 * diff --git a/src/Config.php b/src/Config.php index b18edbe..46a39e2 100644 --- a/src/Config.php +++ b/src/Config.php @@ -34,8 +34,7 @@ class Config { /** * Set the unique per-host slug that keys this library's hooks and options. * - * It is stored exactly as given: hook names repeat it verbatim, and only `get_option_name()` - * folds it. + * Stored exactly as given: hook names repeat it verbatim, and only `get_option_name()` folds it. * * @since 1.0.0 * @@ -79,9 +78,7 @@ public static function get_hook_prefix(): string { /** * Build the name of one of this library's filters. * - * The prefix lives here, so everything derived from it is built here too. A collaborator that - * assembled its own name would be repeating the segment between the host's prefix and the - * hook's own name, and would have to be found and corrected if it ever changed. + * Nothing else assembles the segment between the host's prefix and the key's own name. * * @since 1.0.0 * @@ -98,17 +95,10 @@ public static function get_hook_name( string $name ): string { /** * Build the name of one of this library's options. * - * The prefix is a hook-naming value: `set_hook_prefix()` takes anything WordPress will accept - * inside a filter name, mixed case and hyphens included, and `get_hook_name()` repeats it byte - * for byte — a host that passed `Give-Core` must be able to hook - * `Give-Core/plugin_absorber/should_load` and have it fire. A storage key answers to a - * narrower convention, so the folding happens here and nowhere else: the same prefix produces + * Deliberately does not share `get_hook_name()`'s normalisation, and must not be collapsed into + * it: a host that passed `Give-Core` hooks that verbatim, while the option folds to * `give_core_plugin_absorber_notices`. * - * The `_plugin_absorber_` segment lives here for the reason `get_hook_name()` gives for its - * own: the notice queue is not the only option keyed this way, and a segment each caller - * assembled would have to be found in every one of them if it ever changed. - * * @since 1.0.0 * * @param string $name Option name, less the prefix and this library's namespace. @@ -124,10 +114,8 @@ public static function get_option_name( string $name ): string { /** * Share the host's container. Required, and required before boot(). * - * Every collaborator this library uses is resolved from it, which is what makes each of them - * replaceable by binding an interface. There is no second, container-less path to keep working - * beside that one: two ways to reach a collaborator means two sets of behaviour to reason - * about, and the one nobody runs is the one that rots. + * Every collaborator resolves from it, which is what makes each replaceable by binding an + * interface. There is deliberately no container-less path. * * @since 1.0.0 * @@ -166,15 +154,11 @@ public static function has_container(): bool { } /** - * Tell the library the host plugin's own basename, so a standalone's activation scope can be - * compared against the host's on multisite. + * Tell the library the host plugin's own basename, for the multisite stranding guard. * - * Optional, and read by one thing: the conflict resolver's guard against deactivating a - * network-active standalone whose bundled replacement ships in a host that is not itself - * network-active -- a deactivation that would strand the sites the host never reached. Left - * unset, that guard stands down and deactivation behaves exactly as it always has. Stored - * exactly as given: a basename is a path, `directory/file.php`, not a hook-naming value, so the - * hook-prefix validator's character rules deliberately do not apply here. + * Optional, and read by one thing: the guard that declines to deactivate a network-active + * standalone whose host is not. Left unset, it stands down. Stored exactly as given -- a + * basename is a path, not a hook-naming value, so the prefix rules do not apply. * * @since 1.0.0 * @@ -189,9 +173,8 @@ public static function set_host_plugin_basename( string $basename ): void { /** * The host plugin basename, or an empty string when none was set. * - * Does not throw the way `get_hook_prefix()` and `get_container()` do. Those name a step the host - * must take before boot; this is optional, and an empty string is the honest answer for a host - * that did not opt into the multisite guard -- the same value that stands the guard down. + * Does not throw the way `get_hook_prefix()` and `get_container()` do: an empty string is the + * honest answer for a host that did not opt in. * * @since 1.0.0 * @@ -204,10 +187,9 @@ public static function get_host_plugin_basename(): string { /** * The hook prefix folded into the shape a storage key takes. * - * Only option names are folded, and only on the way out — the stored prefix keeps whatever the - * host passed, because that is what its hook names are made of. Two prefixes differing only in - * case or in hyphens against underscores would land on the same option, which is the price of - * asking a host for one prefix rather than two; no host runs both `Give-Core` and `give_core`. + * Folded on the way out only — the stored prefix keeps whatever the host's hook names are made + * of. Two prefixes differing only in case or hyphens collide here, the price of asking a host + * for one prefix rather than two. * * @since 1.0.0 * diff --git a/src/Conflict/Contracts/Resolver_Interface.php b/src/Conflict/Contracts/Resolver_Interface.php index a1d130a..4ead43e 100644 --- a/src/Conflict/Contracts/Resolver_Interface.php +++ b/src/Conflict/Contracts/Resolver_Interface.php @@ -12,14 +12,9 @@ /** * Decides what happens when a bundled sub-plugin's standalone counterpart is still active. * - * Bind a replacement to change what happens about a conflict. One method, because the policy branch - * is all this contract promises: finding the conflict belongs to `Conflict\Detector` and deciding - * who may have one resolved to `Conflict\Gatekeeper`. - * - * Both of those are the caller's to ask, and the conflict step asks them before it builds an - * implementation of this — so a replacement cannot drop a guard by omission, is never asked to - * resolve on a request that fails one, and is not built at all on a request with nothing to - * resolve. + * Bind a replacement to change what happens about a conflict. One method, because finding the + * conflict belongs to `Conflict\Detector` and deciding who may have one resolved to + * `Conflict\Gatekeeper` -- both asked before an implementation of this is built. * * @since 1.0.0 */ diff --git a/src/Conflict/Detector.php b/src/Conflict/Detector.php index 7a74840..a740ff8 100644 --- a/src/Conflict/Detector.php +++ b/src/Conflict/Detector.php @@ -17,19 +17,9 @@ /** * Whether a bundled sub-plugin's standalone counterpart is still active. * - * Its own class because detection is asked for on its own: the conflict step needs something cheap - * to ask between the request gate and the capability gate, so that `current_user_can()` — which - * pins the current user for the rest of the request — is never reached on a request with nothing to - * resolve. Answering that from `Resolver_Interface` would put a detection query on the contract of - * every host that binds its own resolver, and give the resolver two reasons to change: how a - * conflict is found, and what to do about one. - * - * Nothing here leaves a mark on the request. Nothing resolves a user, - * deactivates a plugin or queues a notice — an answer is all a caller gets, and the acting is the - * resolver's. The one thing either of them writes is a report to the developer about a bootstrap - * that cannot be right, which changes nothing on the site and is addressed to nobody on it. - * - * Not `final`: it is bound by class name, which is the seam a host rebinds and a test subclasses. + * Its own class so the conflict step has something cheap to ask before `current_user_can()`, which + * pins the current user for the rest of the request. It changes no plugin's activation state. Not + * `final`: it is bound by class name, the seam a host rebinds and a test subclasses. * * @since 1.0.0 */ @@ -73,8 +63,8 @@ public function __construct( Reader $registry, Checker_Interface $plugin_checker /** * Whether any registered sub-plugin's standalone counterpart is currently active. * - * Short-circuits on the first one found: the caller only needs to know whether the rest of the - * conflict step is worth entering, and the resolver walks the whole registry again anyway. + * Short-circuits: the caller only needs to know whether the rest of the conflict step is worth + * entering. * * @since 1.0.0 * @@ -83,9 +73,7 @@ public function __construct( Reader $registry, Checker_Interface $plugin_checker * @return bool */ public function has_conflict(): bool { - // The reader rather than a registrar of our own: it drains the registrations still buffered - // on the facade before it reads, and a registrar asked directly would miss anything - // registered since the last read. + // The reader rather than a registrar of our own: it drains the buffered registrations first. foreach ( $this->registry->all() as $sub_plugin ) { if ( $this->is_in_conflict( $sub_plugin ) ) { return true; @@ -98,8 +86,8 @@ public function has_conflict(): bool { /** * Whether this sub-plugin's standalone counterpart is active and ours to act on. * - * Policy is not consulted: a sub-plugin set to defer is still in conflict, and the resolver is - * where the decision to leave it alone belongs. + * Policy is not consulted: a sub-plugin set to defer is still in conflict, and leaving it alone + * is the resolver's decision. * * The `should_load` filter is, and it is the one piece of host code this class reads. It decides * whether the bundled copy will be in memory at all, one priority behind this — so a sub-plugin @@ -143,16 +131,9 @@ public function is_in_conflict( Sub_Plugin $sub_plugin ): bool { /** * Whether deactivating this standalone would strand sites the bundled copy will never reach. * - * `deactivate_plugins()` takes a network-active standalone out of *every* site's plugins, but the - * bundled copy only loads where the host plugin runs. So on the one topology of a network-active - * standalone whose host is not itself network-active, a network-wide deactivation removes it from - * the sites the host never loads on, where nothing stands in for it. The resolver reads this and - * declines, leaving the load guard to defer the bundled copy network-wide instead. - * - * Opt-in, and cheap in the common case: with no host basename configured the guard stands down on - * a single string compare, before any option is read. It needs no `is_multisite()` test either -- - * `Checker_Interface::is_network_active()` is `false` off a network, so the whole predicate is - * `false` on a single site. + * `deactivate_plugins()` pulls a network-active standalone from *every* site, while the bundled + * copy loads only where the host does -- so a network-active standalone under a host that is not + * network-active leaves those sites with nothing. False off a network: `is_network_active()` is. * * The basename itself is checked against the installed plugins on the way past, because a name no * plugin answers to answers "not network-active" for ever, and that is indistinguishable from the diff --git a/src/Conflict/Gatekeeper.php b/src/Conflict/Gatekeeper.php index a57ca7b..27804c6 100644 --- a/src/Conflict/Gatekeeper.php +++ b/src/Conflict/Gatekeeper.php @@ -13,20 +13,9 @@ /** * Whether this request may have a conflict resolved at all. * - * Separate from the resolver, and resolved ahead of it, because the two answer different questions: - * a resolver decides what a conflict means and what to do about it, while this decides who is - * allowed to have one resolved. That split is what makes the guarantee survive rebinding — the - * conflict step asks the gatekeeper first and only then builds a `Resolver_Interface`, so a host - * that binds its own resolver cannot drop a gate by omission, and equally is never asked to resolve - * on a request that fails one. - * - * The two gates are asked at different moments, which is why they are two methods rather than one. - * The request gate is cheap and reads nothing but the request, so it runs first and keeps a resolver - * from being built at all. The user gate is asked last, once a conflict is known to exist, because - * `current_user_can()` resolves the current user and caches it: called from plugins_loaded priority - * 5 it would settle who is signed in before an SSO or JWT plugin that hooks `determine_current_user` - * from its own plugins_loaded callback has been given the chance to, and that plugin's users would - * then be treated as logged out for the rest of the request. A site with no conflict never pays it. + * Separate from the resolver, and asked ahead of it, so a host binding its own `Resolver_Interface` + * cannot drop a gate by omission. Two methods because the cheap request gate runs first, while the + * user gate waits: `current_user_can()` pins the current user ahead of an SSO or JWT plugin. * * @since 1.0.0 */ @@ -37,8 +26,8 @@ class Gatekeeper { /** * Admin scripts that exist only to perform work. * - * Every one of them does its job and then redirects or prints a result, so there is no page here - * to resolve a conflict on — only work to interrupt. + * Each redirects or prints a result, so there is no page here to resolve on — only work to + * interrupt. * * @since 1.0.0 * @@ -54,8 +43,8 @@ class Gatekeeper { /** * Query args that name an action for the current screen to perform. * - * Both, because core's list tables put a bulk selector above the table and a second one below it, - * and the lower one submits as `action2` with `action` left at its empty value. + * Both, because core's list tables carry a second bulk selector that submits as `action2`, + * leaving `action` at its empty value. * * @since 1.0.0 * @@ -67,7 +56,7 @@ class Gatekeeper { * The value core gives an action arg when no bulk action is selected. * * A list table submits its selector whether or not anything was chosen, so this arrives on - * ordinary paging and search requests and means nothing is being asked for. + * ordinary paging and search requests. * * @since 1.0.0 * @@ -78,14 +67,9 @@ class Gatekeeper { /** * Whether this request is the kind conflict resolution may run on. * - * Reads nothing about who is making it: the capability lives in user_may_resolve(), so that - * asking this question does not decide the current user before the plugins that have an opinion - * about it have loaded. - * - * The hook prefix is checked last. It is the only one of these that reports to the developer, and - * resolution runs from plugins_loaded, so checking it first would put a _doing_it_wrong() in the - * log of every front-end request rather than of the admin request that was about to resolve - * something. + * The hook prefix is checked last because it is the only one of these that reports to the + * developer: checked first, it would log a _doing_it_wrong() from every front-end request rather + * than from the admin request that was about to resolve something. * * @since 1.0.0 * @@ -100,26 +84,9 @@ public function request_may_resolve(): bool { /** * Whether the current user may have a conflict resolved on their request. * - * Reaching conflict resolution does not mean anyone is signed in. wp-admin/admin.php loads - * wp-load.php -- which dispatches plugins_loaded -- well before it calls auth_redirect(), so an - * unauthenticated GET of any admin URL gets this far. Without this check a stranger could turn - * the standalone off site-wide by requesting a page they are about to be bounced off. - * - * Which capability that is belongs to Traits\Guards_Plugin_Capability, because the notice - * presenter has to ask the identical question: rendering the queue clears it for everybody, so a - * user who may not have a conflict resolved may not consume the report of one either. Spelling - * the capability here as well is what let the two answers drift apart. - * - * Here rather than inside the default resolver, because it is the one thing about conflict - * resolution that must survive a host binding its own: whoever cannot activate a plugin must not - * be able to deactivate one, and a replacement that forgot to re-check would reopen exactly that. - * - * It gates every policy, not only the destructive one, and that costs nothing. The other - * policies queue a notice, and Notices\Presenter::render() will not render -- or clear -- for a - * user this same guard turns away. 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 - * detect. + * plugins_loaded dispatches well before auth_redirect(), so an unauthenticated GET of any admin + * URL gets this far. It gates every policy, not only the destructive one, and that costs nothing: + * Notices\Presenter::render() refuses to render or clear for a user this same guard turns away. * * @since 1.0.0 * @@ -132,13 +99,9 @@ public function user_may_resolve(): bool { /** * Whether this request is one a person is watching in wp-admin. * - * Conflict resolution deactivates a plugin and ends the request, so it must only run where - * someone is there to see the result. Unguarded it fires at plugins_loaded on every request: - * a visitor's checkout POST becomes a 302 that drops the order, a login POST bounces back to - * a blank form, wp-cron never reaches its event loop, and a WP-CLI command exits 0 having - * printed nothing, because header() is a no-op under the CLI SAPI. - * - * is_admin() alone is not enough: admin-ajax.php and admin-post.php both define WP_ADMIN. + * Resolution deactivates a plugin and ends the request, so unguarded it turns a visitor's + * checkout POST into a 302 that drops the order. is_admin() alone is not enough: admin-ajax.php + * and admin-post.php both define WP_ADMIN. * * @since 1.0.0 * @@ -153,11 +116,9 @@ private function is_interactive_admin_request(): bool { return false; } - // Only a GET. A redirect discards the request, and the browser follows it with a GET, so - // anything submitted is gone -- which is exactly what would happen to a form posted to - // admin-post.php or options.php, both of which define WP_ADMIN and neither of which - // wp_doing_ajax() catches. Core draws the same line in wp_cron(). Deferring resolution to - // the next page view costs nothing: the standalone is still there to detect. + // Only a GET: a redirect discards whatever was submitted -- as it would for a form posted to + // options.php, which defines WP_ADMIN and which wp_doing_ajax() does not catch. Deferring to + // the next page view costs nothing, since the standalone is still there to detect. if ( ( $_SERVER['REQUEST_METHOD'] ?? 'GET' ) !== 'GET' ) { return false; } @@ -168,20 +129,9 @@ private function is_interactive_admin_request(): bool { /** * Whether this GET is asking wp-admin to do something rather than to draw something. * - * A GET is not automatically safe to discard. `update.php?action=upgrade-plugin`, - * `plugins.php?action=activate` and an `admin-post.php` link are all admin GETs that perform - * work, and resolving on one deactivates, redirects and exits before core reaches the work: the - * user clicks "Update Now" and lands on a list screen with nothing updated. That is the same - * silent discard of a submitted action the POST branch exists to prevent, one verb over. - * - * `plugins.php?action=activate` matters twice, because it is the request - * plugin_sandbox_scrape() replays while activating a plugin -- an exit there aborts the - * activation itself and core reports the plugin as fatal. - * - * The test is deliberately blunt: any action arg at all, not a list of the dangerous ones. Half - * the screens in wp-admin take an `action`, plugins define their own, and a list of known-safe - * values would have to be right about every one of them forever. Refusing them all costs a - * deferral to the next plain page view, and the standalone is still there to be detected then. + * A GET is not automatically safe to discard: resolving on `plugins.php?action=activate` exits + * inside the request plugin_sandbox_scrape() replays, and core reports the plugin as fatal. Blunt + * on purpose -- any action arg at all, since a known-safe list would have to stay right forever. * * @since 1.0.0 * @@ -195,27 +145,17 @@ private function is_action_request(): bool { foreach ( self::ACTION_ARGS as $arg ) { $raw = $_GET[ $arg ] ?? null; - // Anything that is not a string names no action core could dispatch on: an array never - // matches one of core's action names, and a missing arg asks for nothing at all. There is - // no work on either to interrupt. + // Anything but a string names no action core could dispatch on. if ( ! is_string( $raw ) ) { continue; } - // Compared as it arrived rather than through sanitize_key(): admin.php dispatches - // admin_action_{$action} on the raw value, so an action named outside a-z0-9_- -- a - // non-Latin script, a bare '+' -- is work a plugin can be hooked to, and sanitizing - // first would empty it and admit the very request this gate exists to refuse. - // - // Not through wp_unslash() either, for the reason Conflict\Resolver reads the request URI - // raw: core adds its slashes in wp_magic_quotes(), which wp-settings.php calls *after* - // do_action( 'plugins_loaded' ), so there are none on this value yet and unslashing is a - // plain stripslashes() over the query string. '\' would strip to '' and '-\1' to '-1' -- - // both of them values this gate reads as asking for nothing -- and the request would be - // admitted and resolved out from under work core is still going to dispatch. Should this - // ever be reached after the slashing, from the inline fallback a too-late boot reports, - // a slashed value only ever reads as more of an action than it is, which is the - // direction a gate may safely be wrong in. + // Compared as it arrived: admin.php dispatches admin_action_{$action} on the raw value, so + // sanitize_key() would empty an action named outside a-z0-9_- and admit that request. Not + // unslashed either, for the reason Conflict\Resolver reads the request URI raw -- core + // slashes in wp_magic_quotes(), after plugins_loaded, so stripslashes() would only damage + // the value, turning '\' into '' and '-\1' into '-1', both of which read here as no action + // at all. if ( $raw !== '' && $raw !== self::NO_ACTION ) { return true; } @@ -227,15 +167,9 @@ private function is_action_request(): bool { /** * The admin script this request is running, as a bare file name. * - * $pagenow is what core's own admin code branches on, and wp-settings.php requires the file that - * sets it before it loads a single plugin, so it is already populated when plugins_loaded - * dispatches. It is also the value that has been reduced to a bare file name for all three - * admins -- site, network and user -- where matching on a path would have to know about each of - * those prefixes. - * - * The fallback is not superstition: core derives $pagenow from PHP_SELF, which some SAPI and - * proxy configurations leave empty, and a host is free to have unset the global. SCRIPT_NAME is - * consulted first there because it is the one every FastCGI SAPI fills in. + * $pagenow is set before any plugin loads and is already a bare name in all three admins, where + * matching on a path would have to know each prefix. The fallback is not superstition: core + * derives $pagenow from PHP_SELF, which some SAPI and proxy configurations leave empty. * * @since 1.0.0 * diff --git a/src/Conflict/Redirector.php b/src/Conflict/Redirector.php index 1e98165..14e964c 100644 --- a/src/Conflict/Redirector.php +++ b/src/Conflict/Redirector.php @@ -10,18 +10,9 @@ /** * Where the user lands after a standalone has been deactivated. * - * The point of the redirect is to re-render whatever the user asked for, now that the standalone is - * gone -- so the destination is derived from the *current* request, not from the referrer. The - * referrer is the page before this one: an admin who clicks through from a public post, or who - * opens an admin screen from a bookmark, carries a referrer that names something other than what - * they are looking at, and following it sends them somewhere they did not ask to go. - * - * The request URI is never trusted as a URL. Only the screen and the query survive it, and the - * destination is assembled inside the admin URL space from those two parts. - * - * Decides where, and never goes there: `wp_safe_redirect()` and the `exit` after it stay in - * Resolver. That is what lets every destination be asserted directly, without a test having to - * stand in for the end of a request. + * The destination comes from the *current* request, not the referrer, which for an admin arriving + * from a bookmark names somewhere they never asked to go. The URI is never trusted as a URL -- only + * the screen and the query survive it. `wp_safe_redirect()` and the `exit` stay in Resolver. * * @since 1.0.0 */ @@ -29,20 +20,9 @@ class Redirector { /** * Where to send the user after deactivating. * - * Re-requesting the screen the user is already on is the point rather than a waste: the - * standalone's code is in memory for this request and only a fresh one sheds it. That includes - * the plugins list, which is why there is no "stay put" answer. It cannot loop, either -- - * the next request finds no active standalone, so nothing resolves and nothing redirects. - * - * The update screens are the exception, because reloading either of them re-runs an update. - * - * The parameter is documented `string` and declared as nothing, and the two say different things - * on purpose. `string` is the contract: Resolver reads $_SERVER['REQUEST_URI'] and makes a string - * of whatever it finds there, so a caller that hands over anything else has a bug static analysis - * should name. Declaring it would turn that same bug into a TypeError raised under strict_types - * from inside plugins_loaded -- on the one path whose job is to get an admin back to a working - * screen -- for a value the SAPI supplies and any plugin may have filtered on the way. So the - * type is a promise to callers, and the guard below is what happens when the promise is broken. + * Re-requesting the screen the user is already on is the point, not a waste: only a fresh request + * sheds the standalone's code from memory, and it cannot loop. The parameter is documented + * `string` and declared as nothing, so a filtered $_SERVER value cannot TypeError from here. * * @since 1.0.0 * @@ -51,8 +31,7 @@ class Redirector { * @return string Absolute admin URL to send the user to. */ public function after_deactivation( $request_uri ): string { - // Both halves earn their place: the type check is the only refusal a non-string ever meets, - // and an empty string names no screen to go back to. + // A non-string meets no other refusal; an empty string names no screen. if ( ! is_string( $request_uri ) || $request_uri === '' ) { return $this->admin_url_for( 'plugins.php' ); } @@ -61,13 +40,12 @@ public function after_deactivation( $request_uri ): string { $screen = $this->screen_from_path( is_string( $path ) ? $path : '' ); - // Nothing that names an admin screen, so there is nothing to re-render: a front-end - // permalink, a directory that is not an admin root, a php file this admin does not serve, a - // traversal attempt. Those take the same route as no request URI at all. + // Names no admin screen -- a front-end permalink, a traversal attempt -- nothing to re-render. if ( $screen === '' ) { return $this->admin_url_for( 'plugins.php' ); } + // The exception to re-requesting: reloading either update screen re-runs an update. if ( $screen === 'update.php' || $screen === 'update-core.php' ) { return $this->admin_url_for( 'plugins.php' ); } @@ -78,13 +56,9 @@ public function after_deactivation( $request_uri ): string { /** * The admin screen a request path names, or an empty string if it names none. * - * Read from the path's basename rather than from the URI, and returned only once it is a screen: - * the name has to be well formed, and this admin has to have a file of that name to serve. The - * request URI is a path on a site that may live in a subdirectory, may be behind a - * TLS-terminating proxy whose scheme disagrees with admin_url(), and on multisite may sit under - * the network or user admin -- so nothing built from admin_url() would recognise it. Taking the - * basename is also what keeps a crafted URI out of the destination: only a validated screen name - * leaves here, and admin_url_for() supplies everything in front of it. + * Read from the path's basename rather than the URI: the request URI is a bare path on a site + * that may sit in a subdirectory, behind a TLS-terminating proxy, or under the network or user + * admin, so nothing built from admin_url() would recognise it. It also keeps a crafted URI out. * * @since 1.0.0 * @@ -95,21 +69,16 @@ public function after_deactivation( $request_uri ): string { private function screen_from_path( string $path ): string { $screen = basename( $path ); - // Two questions, and both have to be yes. Anchored with \z rather than $, which in PCRE also - // matches immediately before a trailing newline -- so "edit.php\n" would satisfy $ and a line - // break would leave here inside the one value this class promises is validated. The pattern - // runs first because it is also what makes the name safe to put after a directory below. + // Anchored with \z rather than $, which in PCRE also matches before a trailing newline -- so + // "edit.php\n" would satisfy $ and leave here inside the one value this class promises is + // validated. The pattern runs first: it is what makes the name safe to put after a directory. if ( (bool) preg_match( '/^[A-Za-z0-9_-]+\.php\z/', $screen ) && $this->is_admin_screen( $screen ) ) { return $screen; } - // The admin roots name the dashboard by leaving it out, the way core's own /wp-admin/ link - // does -- so a path that resolves to one is not a nameless directory, it is index.php. The - // network and user admins have roots of their own, and admin_url_for() picks the base to put - // in front of the screen. - // - // Matched against the end of the path rather than against its last segment alone, because a - // front-end permalink ending in /network/ would otherwise read as the network admin's root. + // The admin roots name the dashboard by leaving it out, as core's own /wp-admin/ link does. + // Matched against the end of the path rather than its last segment alone, or a front-end + // permalink ending in /network/ would read as the network admin's root. $trimmed = rtrim( $path, '/' ); foreach ( [ '/wp-admin', '/wp-admin/network', '/wp-admin/user' ] as $root ) { @@ -124,26 +93,9 @@ private function screen_from_path( string $path ): string { /** * Whether the admin this request belongs to has a screen of that name to be sent back to. * - * Well formed is not the same as naming a screen. `wp-login.php`, `wp-cron.php` and a plugin's - * own bootstrap file all satisfy the pattern above, and each of them would be rebuilt as an admin - * URL for a file that is not there -- the web server's own 404, in place of the plugins list this - * class documents for a request that names no admin screen. - * - * Asked of the filesystem rather than of a list of core's screens, because a list would be wrong - * the first time a plugin registered a top-level page: a host's screens are `admin.php`, - * `edit.php`, `options-general.php` or `tools.php` with a `page` argument on them, so the file is - * core's however many screens are hung off it and every one of them still comes back intact. What - * `is_file()` cannot tell apart is a screen from one of the admin's own includes -- but a browser - * is only ever on the first kind, and both are inside wp-admin either way. - * - * The three branches are admin_url_for()'s, in the same order and for the same reason: the answer - * has to be about the directory the destination will be built in. The network and user admins - * serve only the files core gives them -- there is no `wp-admin/network/options-general.php` -- - * and a request under one of them cannot have been on a screen it does not hold. - * - * The name reaching here has already matched the pattern above, so it is [A-Za-z0-9_-] and a - * '.php' and nothing else: there is no way for it to climb out of the directory being asked - * about, and no readability question either, since the web server is what serves the file, not us. + * Well formed is not the same as naming a screen: `wp-login.php` satisfies the pattern above and + * would be rebuilt as an admin URL for a file that is not there. Asked of the filesystem rather + * than a list of core's screens, since a plugin's screens are core's files with a `page` arg. * * @since 1.0.0 * @@ -166,15 +118,9 @@ private function is_admin_screen( string $screen ): bool { /** * The current request's query, re-encoded, ready to append to a screen name. * - * The query carries which list, which page and which filter the user was looking at, so - * dropping it would re-render the screen showing something else. It is taken apart and rebuilt - * rather than carried over verbatim, because it arrives from the URL bar and nothing about it - * has been checked. - * - * http_build_query() rather than add_query_arg(), which is the usual answer: add_query_arg() - * writes the array it is given straight into the result -- build_query() passes $urlencode as - * false -- so a value holding an '&' or a '#' would go on to add a parameter or a fragment of - * its own. Here both halves of every pair are encoded, which is the whole reason for rebuilding. + * The query carries which list, page and filter the user was looking at, so dropping it would + * re-render something else. Rebuilt with http_build_query() rather than add_query_arg(), which + * passes $urlencode as false -- a value holding an '&' would add a parameter of its own. * * @since 1.0.0 * @@ -206,17 +152,9 @@ private function query_string( string $request_uri ): string { /** * The parsed query with CR, LF and NUL taken out of every string in it, and nothing else. * - * The property being protected is that the destination cannot end a header: it is handed to - * wp_safe_redirect(), which puts it in a Location. That is all that is being protected, because - * it is all that is left to protect -- http_build_query() re-encodes both halves of every pair - * with PHP_QUERY_RFC3986, so no value can add a parameter, open a fragment or arrive as markup, - * whatever it holds. - * - * Deliberately not sanitize_text_field(). wp_parse_str() has already url-decoded these values, - * and _sanitize_text_fields() deletes every '%xx' sequence it can find and entity-encodes a bare - * '<' -- so a search for '100%ab' would be re-run as '100', and one for 'aregistry->all() as $sub_plugin ) { - // The policy may be a host callable behind a filter any plugin on the site may have - // hooked, the notice message is another callable, and deactivate_plugins() runs the - // standalone's own deactivation hook -- so this loop calls arbitrary code, from inside - // plugins_loaded, on an admin page view. A throw out of here would take away the screen - // the site owner would have used to undo whatever caused it, and would leave a second - // standalone running with nothing said about it. Reported per sub-plugin, and the next - // one is still resolved. + // The policy, the notice message and the standalone's own deactivation hook are all + // arbitrary code. Caught per sub-plugin, so one throw does not take the rest with it. try { if ( ! $this->detector->is_in_conflict( $sub_plugin ) ) { continue; @@ -149,15 +128,9 @@ public function resolve_all(): void { } } - // After the loop, never inside it. A site bundling two sub-plugins can have both standalones - // active, and an `exit` on the first would leave the second's standalone running with no - // notice raised about it — and would take the load pass at the next priority with it, so - // nothing bundled loaded on the request that was supposed to fix the conflict. - // - // And only where a standalone really did go away. A request that deactivated nothing has - // nothing to shed from memory, so re-requesting the screen would arrive at the same conflict - // — while the notices this pass queued are waiting on a request that reaches - // `all_admin_notices`, which a redirect never does. + // After the loop, never inside it: an `exit` on the first of two active standalones leaves the + // second running and takes the load pass at the next priority with it. And only where one + // really went away — a request that deactivated nothing would redirect into the same conflict. if ( $standalone_gone ) { $this->redirect(); } @@ -175,9 +148,8 @@ public function resolve_all(): void { protected function resolve( Sub_Plugin $sub_plugin ): bool { $policy = $sub_plugin->get_conflict_policy(); - // A host may persist a policy in an option and a filter may return anything. Falling - // through to deactivate() would turn off a plugin the site owner deliberately activated - // on the strength of a typo, so an unrecognised policy takes the conservative branch. + // A filter may return anything, and falling through to deactivate() would turn a plugin off + // on the strength of a typo. if ( ! Conflict_Policy::is_valid( $policy ) ) { $policy = Conflict_Policy::NOTICE_ONLY; } @@ -188,12 +160,9 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { return false; case Conflict_Policy::DEACTIVATE: - // A network-active standalone whose bundled replacement ships in a host that is not - // itself network-active: deactivating it network-wide would pull it from the sites the - // host never reached, where nothing loads the bundled copy. Leave it -- the load guard - // defers the bundled copy network-wide instead -- and say why. Opt-in and - // single-site-safe: false whenever no host basename is configured, and false off a - // network, so this is the ordinary deactivation in every other case. + // Deactivating network-wide would pull the standalone from sites the host never + // reaches, where nothing loads the bundled copy. Leave it -- the load guard defers + // the bundled copy network-wide instead -- and say why. if ( $this->detector->deactivation_would_strand_sites( $sub_plugin ) ) { $this->notices->queue_stranding_notice( $sub_plugin ); @@ -202,9 +171,8 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { return $this->deactivate( $sub_plugin ); - // NOTICE_ONLY, and anything is_valid() would accept that this switch has grown no - // branch for. The default sits on the branch that only talks, never on the one that - // deactivates: a policy nobody wrote must not be read as consent to turn a plugin off. + // NOTICE_ONLY, and any valid policy this switch has grown no branch for. The default only + // talks: a policy nobody wrote must never be read as consent to turn a plugin off. default: $this->notices->queue_conflict_notice( $sub_plugin ); @@ -231,25 +199,17 @@ protected function resolve( Sub_Plugin $sub_plugin ): bool { protected function deactivate( Sub_Plugin $sub_plugin ): bool { $this->plugin_deactivator->deactivate( $sub_plugin->get_standalone_plugin_basename() ); - // Asked again rather than assumed, because turning the standalone off is not the same event as - // the standalone being off. A site or mu-plugin filtering `option_active_plugins` puts it - // straight back, a host may have rebound `Plugin\Contracts\Deactivator_Interface` to something - // that does nothing, and a rebound `Plugin\Contracts\Checker_Interface` may mean by "active" - // something `deactivate_plugins()` never touches. + // Asked again rather than assumed: an mu-plugin filtering `option_active_plugins` puts it + // straight back, and either plugin seam may have been rebound to something that never turns + // it off. if ( $this->detector->is_in_conflict( $sub_plugin ) ) { - // Nothing is said and nothing is reported. The merge notice would tell the owner a plugin - // they can watch still running had been deactivated, and would say it again on every admin - // GET for as long as the site kept putting it back; and this is the DEFER outcome reached - // by another route, so there is no failure to announce either -- the standalone is running, - // which means its own guard constant stands the bundled copy down and nothing re-declares. - // Every way of arriving here is a site's own configuration rather than a mistake in the - // host's code, and `Plugin\Contracts\Deactivator_Interface` invites one of them by name. + // Said nothing about, and reported nowhere. This is the DEFER outcome by another route -- + // the standalone is running, so its own guard constant stands the bundled copy down -- and + // every way of arriving here is a site's configuration rather than the host's mistake. return false; } - // Queued as each deactivation is confirmed rather than once at the end, so the explanation is - // durable whether or not the request goes on to redirect — and so a site with two standalones - // gets one notice per plugin it lost. + // Queued per deactivation, so a site with two standalones gets one notice per plugin it lost. $this->notices->queue_merge_notice( $sub_plugin ); return true; @@ -263,11 +223,9 @@ protected function deactivate( Sub_Plugin $sub_plugin ): bool { * @return void */ protected function redirect(): void { - // Boot\Scheduler runs this whole sequence inline when a host boots too late, immediately - // after a _doing_it_wrong() that prints on a debugging site — so the headers are gone before - // we get here, wp_safe_redirect() would warn and set no Location, and the exit behind it - // would end the request on a blank page. Falling through instead lets the page finish - // rendering, which is where the merge notice queued above is waiting to be read. + // Boot\Scheduler runs this sequence inline when a host boots too late, behind a + // _doing_it_wrong() that prints on a debugging site — so wp_safe_redirect() would set no + // Location and the exit would end the request on a blank page. Falling through renders it. if ( headers_sent() ) { return; } @@ -281,8 +239,7 @@ protected function redirect(): void { // cost is a stray backslash in a re-encoded query arg rather than a deleted one. $request_uri = $_SERVER['REQUEST_URI'] ?? ''; - // $_SERVER carries whatever the SAPI put there, so the string the redirector is promised is - // made one here rather than assumed. + // $_SERVER carries whatever the SAPI put there, so the promised string is made one here. if ( ! is_string( $request_uri ) ) { $request_uri = ''; } diff --git a/src/Conflict/Rewriter.php b/src/Conflict/Rewriter.php index a0a6bb0..e0d9af7 100644 --- a/src/Conflict/Rewriter.php +++ b/src/Conflict/Rewriter.php @@ -14,23 +14,9 @@ /** * Rewrites the fatal-error screen an absorbed standalone earns when someone activates it. * - * This is the one conflict the load guard cannot prevent. WordPress includes the plugin being - * activated *after* the bundled copy is in memory, so the re-declaration is a real fatal; core - * catches it in its activation sandbox and reports "the plugin triggered a fatal error" — true, and - * useless to whoever pressed the button. All this library gets to do about it is reword that one - * sentence, and the wording is the sub-plugin's own `conflict_notice_message`. - * - * Its own class rather than a method on the notice writer, though the message is shared with the - * merge notice. Nothing here is stored, drawn or queued: it reads the request, checks the screen, - * verifies a nonce and edits markup core wrote. The writer would have needed a registry to find out - * which sub-plugin the screen is even about — a collaborator only that one method used — and every - * host binding its own `Notices\Contracts\Writer_Interface` would have had to implement an error - * screen to get its notices worded. - * - * In `Conflict\` rather than `Notices\` for the same reason: what it is about is the standalone - * conflict, and it changes when that story does, alongside `Detector`, `Resolver` and `Redirector`. - * - * Not `final`: it is bound by class name, which is the seam a host rebinds and a test subclasses. + * The one conflict the load guard cannot prevent: core includes the plugin being activated *after* + * the bundled copy, so the re-declaration really does fatal and all this can do is reword core's + * sentence, with the sub-plugin's own `conflict_notice_message`. Not `final`: bound by class name. * * @since 1.0.0 */ @@ -54,8 +40,8 @@ public function __construct( Reader $registry ) { /** * The markup to print in place of the one WordPress was about to. * - * Handed back untouched unless the request really is a nonce-verified activation error, on a - * plugins screen, for a standalone this library has registered. + * Handed back untouched unless the request is a nonce-verified activation error, on a plugins + * screen, for a standalone this library has registered. * * @since 1.0.0 * @@ -72,37 +58,25 @@ public function rewrite( string $markup ): string { $screen = get_current_screen(); - // Both plugin lists, because wp-admin/network/plugins.php is a one-line require of - // wp-admin/plugins.php and draws the identical activation error -- but WP_Screen appends - // `-network` to the id there. On a default multisite the subsite has no plugins UI at all, - // so the network screen is the *only* place a super admin can reactivate the standalone, - // and matching 'plugins' alone would decline on the one screen that matters most. + // Both plugin lists: wp-admin/network/plugins.php is a one-line require of the other, but + // WP_Screen appends `-network` to the id there -- and on a default multisite it is the only + // screen an absorbed standalone can be reactivated from. if ( $screen === null || ! in_array( $screen->id, [ 'plugins', 'plugins-network' ], true ) ) { return $markup; } - // phpcs:disable WordPress.Security.NonceVerification.Recommended -- verified below, once - // the plugin named turns out to be one this library owns. Nothing is acted on until then. + // phpcs:disable WordPress.Security.NonceVerification.Recommended -- verified below, before + // anything is acted on. $basename = isset( $_GET['plugin'] ) ? wp_unslash( $_GET['plugin'] ) : ''; - // Unslashed and no further. Core mints the activation-error nonce from - // wp_unslash( $_REQUEST['plugin'] ) verbatim (wp-admin/plugins.php), so sanitizing here - // would verify an action core never signed: a plugin whose folder name holds a '%xx' - // sequence, a '<' or a leading space comes back changed from sanitize_text_field(), and - // both the nonce check and the registry lookup below would then miss -- silently, and on - // the one screen this class exists to improve. Nothing sanitizing would remove is needed - // here either: the value is compared against a basename the host configured and hashed into - // a nonce action, and never reaches the page. What does reach it is the sub-plugin's message. - // - // is_string() because sanitize_text_field() was doing that job: '?plugin[]=x' arrives as an - // array, and an array reaching wp_verify_nonce() is a string conversion, not a refusal. + // Unslashed and no further: core mints the nonce from the unslashed value verbatim, so + // sanitizing a folder name holding a '%xx' sequence would make both the nonce check and the + // registry lookup miss, silently. is_string() because '?plugin[]=x' arrives as an array. if ( ! is_string( $basename ) || $basename === '' ) { return $markup; } - // Looked up before the nonce is checked, deliberately: there is no nonce work to do for a - // plugin this library does not own, and the nonce is still verified before any markup is - // touched. + // Looked up first: there is no nonce work to do for a plugin this library does not own. $sub_plugin = $this->find_by_standalone_basename( $basename ); if ( $sub_plugin === null ) { @@ -126,11 +100,8 @@ public function rewrite( string $markup ): string { ) ); - // Sanitised before the emptiness check rather than after. wp_kses_post( '' ) - // is the empty string, and swapping WordPress's wording for nothing leaves a blank notice - // box where the explanation should be. wp_kses_post() and not esc_html(), for the reason - // the renderer uses it: these messages come from the host's own config, and a link to a - // knowledge-base article has to survive. + // Sanitised before the emptiness check rather than after: wp_kses_post( '' ) + // is the empty string, and swapping core's wording for nothing leaves a blank notice box. $message = trim( wp_kses_post( $message ) ); if ( $message === '' ) { @@ -146,10 +117,8 @@ public function rewrite( string $markup ): string { /** * The registered sub-plugin a standalone basename belongs to, if any. * - * Read through the reader this class was handed rather than a registrar of its own, for the - * reason the conflict and load passes do: the reader drains the registrations still buffered - * before it reads, and a registrar asked directly would miss anything registered since the last - * read. + * Read through the reader rather than a registrar of its own: it drains the buffered + * registrations first. * * @since 1.0.0 * diff --git a/src/Conflict_Policy.php b/src/Conflict_Policy.php index 0b44339..1602484 100644 --- a/src/Conflict_Policy.php +++ b/src/Conflict_Policy.php @@ -14,8 +14,8 @@ */ final class Conflict_Policy { /** - * Deactivate the standalone, notify, and redirect. The bundled copy loads on the next - * request, since the standalone has already defined the guard constant on this one. + * Deactivate the standalone, notify, and redirect. The bundled copy loads next request, since the + * standalone has already defined the guard constant on this one. * * @since 1.0.0 * @@ -44,13 +44,9 @@ final class Conflict_Policy { /** * The policy that applies when a sub-plugin configures none. * - * Deactivating is the default because two copies of the same plugin are the failure this - * library exists to prevent, and a sub-plugin that has not thought about the question wants - * the outcome where its bundled copy ends up running. - * - * Distinct from the branch a caller takes for a policy it does not recognise: not configuring - * one is a choice to accept the default, whereas an unrecognised value is a value nobody - * chose, and reading it as consent to deactivate would act on a typo. + * Deactivating, because two copies of the same plugin are the failure this library exists to + * prevent. Configuring nothing accepts that; an unrecognised policy does not, so callers must + * not reach this default for one — it would act on a typo. * * @since 1.0.0 * @@ -63,10 +59,8 @@ public static function default(): string { /** * Whether a policy string is one this library understands. * - * Hosts may persist a policy in an option and filters may return anything, so callers that - * dispatch on a policy should reject unknown values here rather than letting them fall - * through to a default branch — deactivating a plugin the site owner deliberately turned on - * is the most surprising of the three outcomes to arrive at by accident. + * A filter may return anything, and an unknown value must not fall through a `switch` into a + * deactivation nobody asked for. * * @since 1.0.0 * diff --git a/src/Contracts/Activator_Interface.php b/src/Contracts/Activator_Interface.php index 437d707..0aaba55 100644 --- a/src/Contracts/Activator_Interface.php +++ b/src/Contracts/Activator_Interface.php @@ -14,8 +14,7 @@ * Runs a sub-plugin's activation routine once, ever. * * A bundled plugin is `require_once`d rather than activated, so `register_activation_hook()` never - * fires for it and whatever that hook would have done — creating a table, seeding options — never - * happens. Bind a replacement to change how "once, ever" is recorded. + * fires for it. Bind a replacement to change how "once, ever" is recorded. * * @since 1.0.0 */ diff --git a/src/Contracts/Provider_Interface.php b/src/Contracts/Provider_Interface.php index d5e3ac9..f3f5b2a 100644 --- a/src/Contracts/Provider_Interface.php +++ b/src/Contracts/Provider_Interface.php @@ -10,9 +10,8 @@ /** * Teaches a container how to build this library's collaborators. * - * One method, and it binds — it wires no hooks and resolves nothing. When things run is - * `Boot\Scheduler`'s subject, and a provider that also hooked would have to be replaced wholesale - * by a host that only wanted a different implementation of one binding. + * It binds and nothing else: when things run is `Boot\Scheduler`'s subject, and a provider that + * also hooked would have to be replaced wholesale by a host wanting one binding changed. * * @since 1.0.0 */ @@ -20,8 +19,7 @@ interface Provider_Interface { /** * Bind this library's collaborators. * - * Called once, from `Absorber::boot()`, and expected to be safe to call again: boot() is - * idempotent and a host may bind its own provider that another code path also registers. + * Must be safe to call again: `boot()` is idempotent, and another code path may register too. * * @since 1.0.0 * diff --git a/src/Loader.php b/src/Loader.php index 4238983..da9899d 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -17,9 +17,7 @@ /** * The load pass: every registered sub-plugin, in registration order, gated one at a time. * - * Separate from `Absorber` because it is the one thing here that runs rather than configures. It is - * reached from a hook, it needs the notice queue and the activator, and it is the piece a host is - * likeliest to want to watch or replace — none of which is true of registration. + * Separate from `Absorber` because it runs rather than configures. * * @since 1.0.0 */ @@ -70,9 +68,7 @@ public function __construct( * @return void */ public function load_all(): void { - // The load path needs the prefix for the should_load filter and for the notice store. - // Throwing out of a core action would take the whole site down over a bootstrap mistake, - // so it is reported where a developer will see it and the load is abandoned instead. + // No prefix, no should_load filter and no notice store; the trait reports it. if ( ! self::has_hook_prefix() ) { return; } @@ -135,10 +131,9 @@ private function load( Sub_Plugin $sub_plugin ): void { return; } - // Ahead of the dependency check, which calls an arbitrary host callable. This is one - // defined(), it carries the whole re-declaration guarantee, and it is the only gate that - // means "the plugin is already running" -- warning that requirements are unmet for a - // plugin the admin can see working would be worse than useless. + // Ahead of the dependency check, never after: it carries the whole re-declaration guarantee, + // and warning of unmet requirements for a plugin already running sends the admin after the + // wrong problem. if ( $sub_plugin->is_already_loaded() ) { $this->announce_skip( $sub_plugin, Skip_Reason::ALREADY_LOADED ); @@ -152,11 +147,8 @@ private function load( Sub_Plugin $sub_plugin ): void { return; } - // Not file_exists(): that is true for a directory and for a file with no read permission, - // and require_once fatals on both. A missing file is a broken build in the host plugin - // rather than anything a site owner can act on, so it goes to the developer instead of - // into the notice queue, where it would have displayed the host's own - // dependency_notice_message and sent the owner after the wrong problem entirely. + // Not file_exists(): that is true for a directory and for an unreadable file, and + // require_once fatals on both. A broken build reports to the developer, not a site owner. $file = $sub_plugin->get_bundled_plugin_file(); if ( ! is_file( $file ) || ! is_readable( $file ) ) { @@ -179,9 +171,8 @@ private function load( Sub_Plugin $sub_plugin ): void { return; } - // No type guard on the return, unlike the conflict_policy filter: there is no cast here, - // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the - // load, which is the safe direction. + // Truthiness, not a cast: only a falsy return vetoes, so an object or a non-empty array here + // loads rather than fataling on the way to a string. $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); if ( ! $should_load ) { @@ -190,32 +181,16 @@ private function load( Sub_Plugin $sub_plugin ): void { return; } - // An include takes the scope of the line it sits on, and this one is inside a method, where - // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled - // file are function-local as a result -- documented for hosts, because no amount of - // wrapping here can hand a required file the global scope it would have had. + // An include takes the scope of the line it sits on, so top-level assignments in the bundled + // file are function-local where wp-settings.php would have made them global. Not fixable. require_once $file; - // The same defined() the second gate asked, on the other side of the require -- asked of the - // constant table here rather than through `Sub_Plugin::is_already_loaded()`, because it is no - // longer the same question. In front of the require that predicate means "the code is already - // here, from whichever copy"; behind it, the only thing worth asking is whether this require - // did what the guard promises. `Sub_Plugin` still owns the name, so nothing here spells a guard - // constant for Strauss's constant_prefix to rewrite at build time. - // - // Nothing else checks: a typo in `plugin_loaded_constant`, or a bundled plugin that defines its - // constant from its own plugins_loaded callback rather than at file scope, leaves the code in - // memory with nothing standing a standalone copy down -- and a re-declaration fatal, which PHP - // does not raise as a Throwable and nothing here can catch, is then one activation away while - // every counter and every action says the load went perfectly. - // - // Reported rather than skipped, and the load carries on. The require happened and cannot be - // undone: the file's code is in memory whatever the guard says, so the activation callback - // behind this still has to run and `loaded` still has to fire -- announcing a skip would tell - // a host that code which is running is not there, and withholding the callback would leave - // the sub-plugin loaded with the tables it expects never created. What is broken is the - // host's build, which is a developer's to fix and nothing a site owner's screen can help with -- - // so it goes to _doing_it_wrong() and to no notice. + // The same defined() the second gate asked, on the other side of the require: in front of it + // the question is whether any copy is loaded, behind it whether this require kept the guard's + // promise. Reported and not skipped -- the require cannot be undone, so the callback and + // `loaded` still have to happen, and a broken host build is nothing a site owner's screen can + // fix. `Sub_Plugin` still owns the name, so nothing here spells a constant for Strauss to + // rewrite. $guard_constant = $sub_plugin->get_plugin_loaded_constant(); if ( ! defined( $guard_constant ) ) { @@ -232,11 +207,10 @@ private function load( Sub_Plugin $sub_plugin ): void { ); } - // Only after a require that actually happened. A bundled plugin is included rather than - // activated, so register_activation_hook() never fires for it and whatever that hook would - // have done -- creating a table, seeding options -- would never happen at all. Running it - // for a sub-plugin that was skipped would be worse: the schema would appear for a plugin - // that is not loaded. + // Last, and only after a require that happened: register_activation_hook() never fires for + // an included plugin, so this stands in for it with the sub-plugin's code in memory, and + // creating its tables for code that is not loaded would spend the once-ever record before + // the first real load. $this->activator->maybe_run( $sub_plugin ); // Behind the activation callback, not in front of it. A listener here is host code that will diff --git a/src/Notices/Contracts/Writer_Interface.php b/src/Notices/Contracts/Writer_Interface.php index 4a6009d..3978a07 100644 --- a/src/Notices/Contracts/Writer_Interface.php +++ b/src/Notices/Contracts/Writer_Interface.php @@ -11,13 +11,11 @@ use Nexcess\PluginAbsorber\Sub_Plugin; /** - * What the absorber has to say about a sub-plugin. Bind a replacement to word or keep it 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. + * The only seam in this folder, because nothing dispatches on how a notice reaches the screen: a + * host that wants no rendering of ours takes the `all_admin_notices` callback off instead. * * @since 1.0.0 */ @@ -25,12 +23,8 @@ interface Writer_Interface { /** * Queue the "we deactivated the standalone for you" notice. * - * Queued after the deactivation has already happened, and raised exactly once — nothing - * 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. + * Raised exactly once, so an implementation that drops it drops the only warning the site owner + * gets — and it must survive the request that wrote it, since the resolver then redirects. * * @since 1.0.0 * @@ -58,9 +52,9 @@ public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void; /** * Queue the "we left the standalone active to avoid stranding sites" notice. * - * Raised in one topology only: on multisite, a network-active standalone whose host plugin is not - * network-activated, where deactivating it network-wide would remove it from the sites the host - * never reached. Its wording must not tell the user to deactivate the standalone. + * Raised in one topology only: a network-active standalone whose host is not network-activated, + * where a network-wide deactivation would remove it from the sites the host never reached. So + * its wording must not tell the user to deactivate the standalone. * * @since 1.0.0 * @@ -88,10 +82,8 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void; /** * 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 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. + * An instance method on the contract because the answer depends on which writer a site runs: a + * name read off the default class would name an option nothing writes to. * * @since 1.0.0 * diff --git a/src/Notices/Presenter.php b/src/Notices/Presenter.php index 977ad1f..1cec960 100644 --- a/src/Notices/Presenter.php +++ b/src/Notices/Presenter.php @@ -13,26 +13,9 @@ /** * Who may consume the queue, and what happens when they do. * - * Separate from `Writer` because the two change for different reasons: one is asked "what does this - * notice say", the other "may this user see the pending set, and is it gone once they have". Nothing - * here words a notice, and nothing in the writer decides who reads one. - * - * The capability check lives here rather than in `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. Which capability is `Traits\Guards_Plugin_Capability`'s answer, shared with the gate - * that decides who may have a conflict resolved at all — the conflict notices report what that gate - * let happen, so consuming one is the same authority as causing it. The dependency notice is queued - * off the load path, behind no gate at all, and is held to the same capability for the reason above: - * whoever consumes the queue consumes all of it. - * - * 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. A host that wants every - * administrator to see it has to track consumption per user itself. - * - * Bound by class name rather than behind a contract: the trampoline on `all_admin_notices` is the - * only caller, so nothing in the library dispatches on this, and a host that wants no rendering of - * ours takes that callback off rather than binding an implementation that does nothing. + * The queue is single-consumer: rendering consumes it for everybody, network-wide on multisite. The + * capability check lives here rather than in `Renderer` because it guards the clearing as much as + * the drawing — decided apart, a user who may not see the queue could still destroy it. * * @since 1.0.0 */ @@ -67,9 +50,8 @@ public function __construct( Store $store, 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. + * Reads the store, not the bound writer: a host binding its own writer has taken over where + * notices live along with what they say. * * @since 1.0.0 * @@ -78,10 +60,8 @@ public function __construct( Store $store, Renderer $renderer ) { * @return void */ public function render(): void { - // 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, and on multisite the queue they swallowed it out of is - // shared by every site on the network. + // Rendering clears the queue, so a subscriber loading their profile page would otherwise + // swallow the only warning an administrator was ever going to get. if ( ! self::user_may_manage_plugins() ) { return; } diff --git a/src/Notices/Renderer.php b/src/Notices/Renderer.php index cb14651..1064e17 100644 --- a/src/Notices/Renderer.php +++ b/src/Notices/Renderer.php @@ -10,12 +10,8 @@ /** * How a queued notice is drawn. * - * Deliberately minimal markup so the library stays dependency-free, and deliberately its own class: - * a host that only wants different markup — one already using stellarwp/admin-notices, say — has - * one small thing to replace rather than the whole queue. - * - * Knows nothing about where the queue came from or who is allowed to see it. It is handed messages - * and prints them. + * Minimal markup so the library stays dependency-free, and its own class so a host wanting other + * markup has one small thing to replace. * * @since 1.0.0 */ @@ -23,9 +19,8 @@ class Renderer { /** * The `notice-*` class each notice type renders with. * - * A dependency notice reports a plugin that did not load at all, which is `notice-error` by - * WordPress convention. The other three report a conflict the library has already handled or held - * off — the site works, so they are warnings. + * A dependency notice reports a plugin that did not load at all, `notice-error` by WordPress + * convention. The other three report a conflict already handled: the site works, so they warn. * * @since 1.0.0 * @@ -41,17 +36,9 @@ class Renderer { /** * Print every message in the queue. * - * Messages go through `wp_kses_post()`, the standard WordPress post-content allowlist, so a - * link to a knowledge-base article, emphasis or a list survives, while a script or an event - * handler attribute does not. - * - * The paragraph comes from `wpautop()` rather than a literal `

` around the message. That - * same allowlist keeps a `

`, so a host may well send one already wrapped, and a hard wrap - * around it is markup no browser can honour: the parser closes the outer paragraph at the - * inner one and leaves a stray `

`, which draws an empty line above the notice. A `
    ` - * fares worse — it cannot legally sit in a `

    ` at all, so the list the allowlist just - * preserved would break straight back out of it. `wpautop()` wraps only what needs wrapping, - * and turns the blank line a plain translated string uses as a break into a real one. + * `wp_kses_post()` rather than `esc_html()`, so a host's knowledge-base link or list survives + * while a script does not; and `wpautop()` rather than a literal `

    ` around the message, + * because a `

      ` cannot legally sit inside one and would break straight back out of it. * * @since 1.0.0 * @@ -61,10 +48,8 @@ class Renderer { */ public function render( array $queue ): void { foreach ( $queue as $key => $message ) { - // Filtered before the emptiness check rather than after it, because filtering can - // empty a message on its own: `wp_kses_post( '' )` is the empty - // string. Either that or a whitespace-only message would print an empty notice box, - // which reads as a bug. + // Filtered before the emptiness check: `wp_kses_post( '' )` is the + // empty string, and an empty notice box reads as a bug. $message = trim( wp_kses_post( $message ) ); if ( $message === '' ) { @@ -74,9 +59,7 @@ public function render( array $queue ): void { printf( '
      %s
      ', esc_attr( $this->notice_class( (string) $key ) ), - // Already filtered: escaping it again here would undo the whole point and print a - // link as literal angle brackets. Trimmed because `wpautop()` leaves a trailing - // newline, which would otherwise sit inside the div on every notice. + // Already filtered: escaping again would print a link as literal angle brackets. trim( wpautop( $message ) ) ); } @@ -95,8 +78,8 @@ private function notice_class( string $key ): string { $parts = explode( ':', $key ); $type = (string) end( $parts ); - // An entry written by an older version, or by a host reading and rewriting the option, is - // shown rather than dropped: a warning is the safe severity for something unrecognised. + // An unrecognised type — an older version's entry, or a host's own — is shown at the safe + // severity rather than dropped. return self::CLASSES[ $type ] ?? 'notice-warning'; } } diff --git a/src/Notices/Store.php b/src/Notices/Store.php index a8cf5fd..4c2686d 100644 --- a/src/Notices/Store.php +++ b/src/Notices/Store.php @@ -13,14 +13,9 @@ /** * Where the notice queue lives: one option, keyed `slug:type`. * - * An option rather than a transient. With an external object cache, `set_transient()` never - * touches the database at all — the queue would live only in Redis or Memcached, where a routine - * `wp_cache_flush()` from a deploy script or a "purge cache" button destroys it. The merge notice - * 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 Writer so that changing where notices are kept does not mean touching how they are - * worded or drawn. + * An option rather than a transient: with an external object cache a transient never reaches the + * database, so a routine `wp_cache_flush()` destroys it — and the merge notice is raised exactly + * once, so losing it means the site owner is never told. * * @since 1.0.0 */ @@ -29,11 +24,6 @@ class Store { * The option name backing the queue. Read it from here rather than composing it, since the hook * prefix is normalised on its way into a storage key. * - * An instance method, not a static one: it is the answer for *this* store, and a host that binds - * a queue keeping its notices somewhere else has to be able to give a different one. A static - * would answer for the default implementation whatever the site actually uses, which is the - * wrong answer stated with confidence. - * * @since 1.0.0 * * @throws Config_Exception When no hook prefix has been set. @@ -54,16 +44,15 @@ public function option_name(): string { * @return array */ public function all(): array { - // Outside multisite `get_site_option()` is `get_option()`, so this reads back whatever - // put() wrote on either install type. + // Outside multisite the `*_site_option()` family is the `*_option()` one, so the reads and + // writes here cover both. $queue = get_site_option( $this->option_name(), [] ); if ( ! is_array( $queue ) ) { return []; } - // Anything that is not a string message is dropped rather than printed. put() writes the - // filtered array back, so a corrupted entry heals itself. + // put() writes the filtered array back, so a corrupted entry heals itself. return array_filter( $queue, 'is_string' ); } @@ -84,10 +73,8 @@ public function put( string $key, string $message ): void { $queue[ $key ] = $message; - // One call covers both install types. Outside multisite `update_site_option()` ends in - // `update_option( $option, $value, false )`, or `add_option( $option, $value, '', false )` - // the first time — either way autoload is off, which is exactly what this queue wants: it - // is empty on almost every request and only ever read in the admin. + // Autoload off, which is what this queue wants: empty on almost every request, read only in + // admin. update_site_option( $this->option_name(), $queue ); } @@ -99,7 +86,6 @@ public function put( string $key, string $message ): void { * @return void */ public function clear(): void { - // Outside multisite `delete_site_option()` is `delete_option()`. delete_site_option( $this->option_name() ); } } diff --git a/src/Notices/Writer.php b/src/Notices/Writer.php index 1b5dfd2..a951be0 100644 --- a/src/Notices/Writer.php +++ b/src/Notices/Writer.php @@ -14,27 +14,14 @@ /** * 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". - * - * 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. - * - * 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 Writer_Interface - * and read the same option, whose name is `option_name()`. + * Wording only: `Store` keeps the queue, `Renderer` draws it, `Presenter` decides who consumes it. * * @since 1.0.0 */ 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. + * Notice types. Public because an entry is stored under `slug:type`, so reading the queue + * yourself means matching against these. * * @since 1.0.0 * @@ -102,9 +89,6 @@ public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { } /** - * The default differs from the merge notice's on purpose: this one asks the user to act, - * where that one reports something already done. - * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin concerned. @@ -127,12 +111,8 @@ public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { } /** - * The "we left the standalone active to avoid stranding sites" notice. - * - * Its own type and its own default because it must not carry the conflict notice's "you can - * safely deactivate the standalone": on the topology it fires for -- a network-active standalone - * whose host is not network-activated -- a network-wide deactivation is the very thing that would - * strand the sites the host never reached. The wording lives on `Sub_Plugin` with the others. + * Its own type and default because it must not carry the conflict notice's "you can safely + * deactivate the standalone" -- here that deactivation is what would strand the sites. * * @since 1.0.0 * @@ -171,11 +151,10 @@ public function option_name(): string { } /** - * Store one notice, keyed by slug and type so different types can coexist. + * Store one notice, keyed `slug:type`. * - * A sub-plugin can legitimately earn a merge notice while the conflict is resolved and a - * dependency notice while the load is attempted, in the same request. Keying by slug alone - * would silently drop one of them. + * One sub-plugin can earn a merge notice and a dependency notice in the same request; keying by + * slug alone would silently drop one of them. * * @since 1.0.0 * diff --git a/src/Plugin/Checker.php b/src/Plugin/Checker.php index db136f8..c343da1 100644 --- a/src/Plugin/Checker.php +++ b/src/Plugin/Checker.php @@ -27,9 +27,7 @@ class Checker implements Checker_Interface { public function is_active( string $basename ): bool { $this->load_plugin_functions(); - // WordPress's own is_plugin_active() already ORs in the network check, so asking - // is_plugin_active_for_network() as well would only buy a second get_site_option() per - // sub-plugin per request. + // is_plugin_active() already ORs in the network check. return is_plugin_active( $basename ); } @@ -43,8 +41,7 @@ public function is_active( string $basename ): bool { public function is_network_active( string $basename ): bool { $this->load_plugin_functions(); - // is_plugin_active_for_network() checks is_multisite() itself and returns false off a - // network, so the stranding guard that reads this needs no is_multisite() test of its own. + // Returns false off a network on its own, so callers need no is_multisite() test. return is_plugin_active_for_network( $basename ); } } diff --git a/src/Plugin/Contracts/Checker_Interface.php b/src/Plugin/Contracts/Checker_Interface.php index 14ce389..0d91b6b 100644 --- a/src/Plugin/Contracts/Checker_Interface.php +++ b/src/Plugin/Contracts/Checker_Interface.php @@ -8,17 +8,12 @@ namespace Nexcess\PluginAbsorber\Plugin\Contracts; /** - * The library's one way of asking WordPress about a plugin. + * The library's one way of asking WordPress about a plugin. Bind a replacement to answer from + * somewhere other than the active-plugins option. * - * Stated in terms of a plugin basename — "give-recurring/give-recurring.php" — because that is the - * only identifier WordPress itself accepts. Bind a replacement to answer from somewhere other than - * the active-plugins option. - * - * Separate from `Plugin\Contracts\Deactivator_Interface` because the two are asked for by different code for - * different reasons: reading plugin state is a question anything may ask, while turning a plugin - * off is an action exactly one policy branch takes. A host that wants deactivation to be a no-op — - * plugin state managed outside WordPress, say — should not have to reimplement the reading half - * to say so. + * Separate from `Deactivator_Interface` because reading plugin state is a question anything may + * ask, while turning a plugin off is an action one policy branch takes: a host making deactivation + * a no-op should not have to reimplement the reading half to say so. * * @since 1.0.0 */ @@ -26,8 +21,8 @@ interface Checker_Interface { /** * Whether the plugin is active, in either scope. * - * Site-wide and network-wide both count. The conflict this answers for is "the standalone's - * code is going to run this request", and a network activation runs it just as surely. + * The question is "is the standalone's code going to run this request", which a network + * activation answers yes just as surely as a site one. * * @since 1.0.0 * @@ -40,12 +35,8 @@ public function is_active( string $basename ): bool; /** * Whether the plugin is active across the whole network. * - * Network scope only, unlike `is_active()`, which counts either scope. The two are asked for - * different reasons: `is_active()` answers "the standalone's code is going to run this request", - * while this answers "deactivating it reaches every site" -- the question the conflict resolver's - * stranding guard turns on. Returns `false` whenever the site is not multisite, because - * WordPress's own `is_plugin_active_for_network()` does, so a caller needs no `is_multisite()` - * guard of its own and the stranding guard is a no-op off a network. + * Network scope only, where `is_active()` counts either. Must return `false` off a network, as + * core's `is_plugin_active_for_network()` does, so no caller needs an `is_multisite()` guard. * * @since 1.0.0 * diff --git a/src/Plugin/Contracts/Deactivator_Interface.php b/src/Plugin/Contracts/Deactivator_Interface.php index 0cc9f18..7214029 100644 --- a/src/Plugin/Contracts/Deactivator_Interface.php +++ b/src/Plugin/Contracts/Deactivator_Interface.php @@ -10,9 +10,7 @@ /** * The library's one way of turning a plugin off. * - * Stated in terms of a plugin basename — "give-recurring/give-recurring.php" — because that is - * the only identifier WordPress itself accepts. Bind a replacement to make deactivation a no-op - * in an environment where plugin state is managed outside WordPress. + * Bind a replacement to make deactivation a no-op where plugin state is managed outside WordPress. * * @since 1.0.0 */ @@ -20,9 +18,8 @@ interface Deactivator_Interface { /** * Deactivate the plugin in every scope it is active in. * - * Called unattended, during plugins_loaded, on behalf of a user who did not ask for it. An - * implementation that reports success without deactivating leaves two copies of the same - * plugin to load, which is the failure this library exists to prevent. + * Called unattended, during plugins_loaded. An implementation that returns without deactivating + * leaves two copies of the same plugin to load, which is the failure this library prevents. * * @since 1.0.0 * diff --git a/src/Plugin/Deactivator.php b/src/Plugin/Deactivator.php index 2cbff93..987e20f 100644 --- a/src/Plugin/Deactivator.php +++ b/src/Plugin/Deactivator.php @@ -27,20 +27,11 @@ class Deactivator implements Deactivator_Interface { public function deactivate( string $basename ): void { $this->load_plugin_functions(); - // Silent, and with no $network_wide argument. - // - // Silent because this is an unattended deactivation, and the standalone's own deactivation - // hook has already been registered this request. Running it at plugins_loaded means a - // routine flush_rewrite_rules() in that callback regenerates the rules before init has - // registered a single post type, and every custom permalink on the site starts 404ing. - // WordPress core makes the same call: its interactive paths are noisy, its automatic ones - // -- validate_active_plugins(), the plugin upgrader -- are silent. - // - // The $network_wide default is null, not false, and null is the value that handles both - // scopes. WordPress core enters the network branch on `false !== $network_wide` and the - // blog branch on `true !== $network_wide`, so null takes both. A computed true would skip - // the blog branch, stranding an entry for a plugin that is active in both, which then takes - // a second request and a second deactivation hook to clear. + // Silent, because a flush_rewrite_rules() in the standalone's deactivation hook runs here + // at plugins_loaded and regenerates the rules before init has registered a post type, + // 404ing every custom permalink. And no $network_wide argument: its null default enters + // both of core's branches, where a computed true would skip the blog one and strand an + // entry for a plugin active in both. deactivate_plugins( $basename, true ); } } diff --git a/src/Plugin/Loads_Plugin_Functions.php b/src/Plugin/Loads_Plugin_Functions.php index 5b5fb74..d23e338 100644 --- a/src/Plugin/Loads_Plugin_Functions.php +++ b/src/Plugin/Loads_Plugin_Functions.php @@ -10,10 +10,8 @@ /** * Makes WordPress's plugin functions available to the class using it. * - * A trait rather than a fifth collaborator: there is nothing here to replace. It has no state, no - * decision and no alternative implementation — the file either is loaded or it is not — so an - * interface and a binding would buy a seam nobody would ever bind to, and both classes that touch - * plugin functions would then have to be handed it. + * A trait rather than a collaborator: no state, no decision and no alternative implementation, so + * an interface and a binding would buy a seam nobody would bind to. * * @since 1.0.0 */ @@ -21,9 +19,9 @@ trait Loads_Plugin_Functions { /** * WordPress only loads these in the admin, and we run at plugins_loaded on every request. * - * Guarded on deactivate_plugins() rather than is_plugin_active(), because the latter is a - * common third-party shim: something else defining it would short-circuit this and leave the - * rest of the file unloaded, so the first call that needs a function nobody shimmed fatals. + * Guarded on deactivate_plugins() rather than is_plugin_active(), because the latter is a common + * third-party shim: a shimmed copy would leave the rest of the file unloaded, and the first call + * to a function nobody shimmed then fatals. * * @since 1.0.0 * diff --git a/src/Provider.php b/src/Provider.php index 789f8f1..3281d34 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -34,12 +34,9 @@ /** * Teaches the host's container how to build every collaborator this library uses. * - * Binding only. Nothing here hooks, and nothing here resolves: `register()` runs at boot, when a - * host may still be binding, and building an object at that point would pin whichever - * implementation happened to be bound first. - * - * `final` because it is a list of bindings, and the way to change one is to bind it yourself - * before boot rather than to inherit the list. + * Binding only, and nothing here resolves: `register()` runs at boot, when a host may still be + * binding, so building an object then would pin whichever implementation was bound first. `final` + * because a binding is changed by binding it yourself before boot, not by inheriting the list. * * @since 1.0.0 */ @@ -68,9 +65,8 @@ public function __construct( ContainerInterface $container ) { public function register(): void { $container = $this->container; - // The container under its own contract, so that a container which builds unbound classes - // reflectively can still satisfy the two collaborators that take one. Bound first because - // everything below it may be resolved that way. + // The container under its own contract, first of all, so that a container which builds unbound + // classes reflectively can still satisfy the collaborators below that take one. $this->bind_once( ContainerInterface::class, $container ); $this->bind_once( Registrar_Interface::class, Registrar::class ); @@ -82,9 +78,8 @@ public function register(): void { $this->bind_once( Redirector::class ); $this->bind_once( Gatekeeper::class ); - // Explicit factories rather than a class name for everything with a constructor argument: - // container-contract promises `bind`, `get`, `has` and `singleton` and nothing about - // autowiring, so a container that resolves nothing by reflection has to be told. + // Explicit factories for everything with a constructor argument: container-contract promises + // nothing about autowiring, so a container that resolves nothing by reflection has to be told. $this->bind_once( Writer_Interface::class, static function () use ( $container ): Writer { @@ -156,29 +151,11 @@ static function () use ( $container ): Scheduler { } /** - * Bind as a singleton, unless the host already bound something. - * - * The host binds first and wins: this library's defaults are what a container has when nobody - * said otherwise, and a provider that overwrote a binding would make the order in which a host - * calls `set_container()` and `boot()` decide which implementation it gets. It is also what - * keeps a second `register()` harmless, instead of swapping in a fresh registrar and losing - * every sub-plugin registered so far. - * - * The `class_exists()` half is what makes that question answerable at all. `has()` means "can - * return an entry", not "the host bound this" -- di52 answers it with `isBound() || - * class_exists()`, so for a class id it is true before anything has been bound. Asked alone it - * stands down every binding above whose id is a class, the explicit factories included, leaving - * those collaborators autowired where the container autowires, broken where it does not, and - * singletons nowhere. An interface no container can build unprompted, so there the same call - * answers exactly what is being asked. - * - * What that costs is a host rebinding one of the concrete workers, which has to happen after - * boot: nothing here can tell that binding apart from the container's own willingness to build - * the class. The interface seams -- the ones a host is invited to replace -- are unaffected. + * Bind as a singleton -- every binding here is one -- unless the host bound this id first. * - * Singletons throughout. Every one of these is either a registry whose contents are the point - * or a stateless worker, and a second registrar would hold a second, emptier list of - * sub-plugins. + * The guard stands down on interface ids only: `has()` means "can return an entry", not "the host + * bound this" -- di52 answers it with `isBound() || class_exists()` -- so dropping that half would + * stand down every concrete binding above. A host replacing a concrete worker binds after boot. * * @since 1.0.0 * diff --git a/src/Registry/Reader.php b/src/Registry/Reader.php index 8e0ec6e..6e31ee4 100644 --- a/src/Registry/Reader.php +++ b/src/Registry/Reader.php @@ -15,19 +15,9 @@ /** * Every registered sub-plugin, as something a pass can be handed rather than reach for. * - * Registration is buffered before it is stored, and the buffer is static because it has to be: - * `Absorber::register()` is a static call a host makes at plugin-file scope, before there is a - * container to resolve a registrar from. What has to be decided is which class that costs — and it - * is this one, not the facade. Everything that reads the registry (`Conflict\Detector`, - * `Conflict\Resolver`, `Loader`, and `Conflict\Rewriter`) declares this - * object in its constructor, so no collaborator reaches the registry through `Absorber`, and the - * dependency between the facade and the collaborators runs one way. - * - * The buffer is deliberately shared across instances. It is one process's registrations, and a second - * reader holding a second, emptier list is the bug `Provider` binds every collaborator as a singleton - * to avoid. - * - * Not `final`: it is bound by class name, which is the seam a host rebinds and a test subclasses. + * The buffer registrations land in is static because it has to be: `Absorber::register()` is a + * static call a host makes at plugin-file scope, before there is a container to resolve a registrar + * from. It lives here, not on the facade, so the dependency on `Absorber` runs one way. * * @since 1.0.0 */ @@ -60,10 +50,9 @@ public function __construct( Registrar_Interface $registrar ) { /** * Hold a registration until something reads. * - * Static, and it stores rather than registers, because `Absorber::register()` must resolve - * nothing: the host container LearnDash hands us is *replaced* at `plugins_loaded` priority 0, so - * a registration that reached a registrar before that point would go into the container being - * thrown away. Buffering is what lets the container arrive at any point before boot. + * It stores rather than registers because `Absorber::register()` must resolve nothing: a host + * container may still be *replaced* at `plugins_loaded` priority 0, so a registration that + * reached a registrar earlier would land in the one being thrown away. * * A registration that arrives after the load pass has gone by is buffered like any other and * reported, because a buffer nothing reads again leaves next to nothing behind to go on: no @@ -117,8 +106,7 @@ public static function buffer( Sub_Plugin $sub_plugin ): void { * * The buffer is drained on the way past, which is why a pass is handed this rather than the * registrar it could resolve for itself: a registrar asked directly would miss everything - * registered since the last read, a host registering from its own `plugins_loaded` callback - * included. + * registered since the last read. * * A read always answers with what the registrar legitimately holds. A duplicate slug is refused * and reported as it drains, never raised out of here: every caller is inside `plugins_loaded`, @@ -132,12 +120,8 @@ public static function buffer( Sub_Plugin $sub_plugin ): void { public function all(): array { $this->flush(); - // Registrar_Interface::all() can only declare `array` -- PHP 7.4 has no way to say - // array in a signature -- so a host binding its own registrar may return - // anything at all. Narrowed once here, where the untrusted value crosses into the library, - // rather than at each call site: a consumer that forgot the check would fatal inside - // plugins_loaded on its first predicate call, which is the exact failure this library - // exists to prevent, and every future consumer would have to remember it too. + // A host may bind a registrar returning anything, and PHP 7.4 cannot say array + // in the interface signature -- so narrow once here, where the untrusted value enters. return array_filter( $this->registrar->all(), static function ( $sub_plugin ): bool { @@ -147,17 +131,10 @@ static function ( $sub_plugin ): bool { } /** - * Hand every buffered registration to the registrar. - * - * Public because the drain is wanted without the read. `Absorber::registrar()` hands a host the - * registrar itself, which is empty until something drains into it, and reading `all()` there for - * its effect and discarding the list said nothing about why the line was there. The two are the - * whole of this class's surface and a rebound reader owes both: `all()` is this method plus the - * registrar's contents, narrowed. + * Hand every buffered registration to the registrar, which stays the single source of truth. * - * The registrar stays the single source of truth: the buffer is a pre-store that needs no - * container, and duplicate-slug detection and ordering remain the registrar's alone rather than - * being restated here in a second dialect. + * Public because the drain is wanted without the read: `Absorber::registrar()` hands back a + * registrar that is empty until something drains into it, and a rebound reader owes both halves. * * The buffer is emptied before the loop, so a second read cannot re-register what the registrar * already holds and trip its duplicate-slug guard. Nothing has to empty it *after* a failure diff --git a/src/Registry/Registrar.php b/src/Registry/Registrar.php index a832585..a0fd64f 100644 --- a/src/Registry/Registrar.php +++ b/src/Registry/Registrar.php @@ -26,11 +26,8 @@ class Registrar implements Registrar_Interface { * Register a sub-plugin. * * A slug is an identity, not a key this map happens to use: it also names the sub-plugin's - * notices and its once-ever activation record. Letting a second registration win would drop - * the first sub-plugin from the load silently and hand its activation record to the winner, so - * the collision is refused instead. There is no legitimate second registration to protect — - * a decision the host cannot make up front belongs in the `enabled` callable, which is - * re-evaluated on every load, not in a second call to this method. + * notices and its once-ever activation record, so letting a second registration win would + * silently drop the first from the load and hand it that record. * * @since 1.0.0 * @@ -45,7 +42,7 @@ public function register( Sub_Plugin $sub_plugin ): void { if ( isset( $this->sub_plugins[ $slug ] ) ) { // Both files, because the two registrations routinely come from different host plugins - // and the stack trace only shows the one that lost. + // and the stack trace shows only the one that lost. throw new Config_Exception( sprintf( 'Two sub-plugins are registered under the slug "%1$s": %2$s and %3$s.' diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index 1376926..bea7fe5 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -13,12 +13,9 @@ * One registered sub-plugin: its configuration, and every answer it can give without a * container-bound collaborator. * - * 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. + * The line is dependency direction, not "configuration alone": resolving a Checker_Interface to ask + * whether the standalone is active would put a container read in front of Absorber::register(), + * which performs none so the container may arrive any time before boot. * * @since 1.0.0 * @@ -37,8 +34,6 @@ */ class Sub_Plugin { /** - * Keys without which this object cannot do its job. - * * @since 1.0.0 * * @var string[] @@ -46,8 +41,6 @@ class Sub_Plugin { private const REQUIRED_KEYS = [ 'slug', 'bundled_plugin_file', 'plugin_loaded_constant' ]; /** - * Keys that are only ever a callable, never a value. - * * @since 1.0.0 * * @var string[] @@ -55,8 +48,6 @@ class Sub_Plugin { private const CALLABLE_KEYS = [ 'dependency_check', 'activation_callback' ]; /** - * Optional keys that are only ever a string, never a callable. - * * @since 1.0.0 * * @var string[] @@ -64,7 +55,7 @@ class Sub_Plugin { private const STRING_KEYS = [ 'standalone_plugin_basename' ]; /** - * Keys that take a string, or something to call for one when it is wanted. + * Keys taking a string, or something to call for one. * * @since 1.0.0 * @@ -73,7 +64,7 @@ class Sub_Plugin { private const DEFERRABLE_KEYS = [ 'conflict_policy' ]; /** - * Keys that carry human-readable text, and so only ever hold something to call for it. + * Keys carrying user-facing text, which hold something to call for it, never the text. * * @since 1.0.0 * @@ -95,9 +86,8 @@ class Sub_Plugin { * * @param array $config Sub-plugin configuration. * - * @throws Config_Exception When a required key is missing, empty, or not a string; when a - * callable-only key holds something that cannot be called; or when a - * message key holds a string rather than something to call for one. + * @throws Config_Exception When a required key is missing, empty or not a string, a callable-only + * key cannot be called, or a message key holds a string. */ public function __construct( array $config ) { foreach ( self::REQUIRED_KEYS as $required ) { @@ -111,9 +101,8 @@ public function __construct( array $config ) { ); } - // Not just a truthiness check. An array survives one of those and then casts to the - // string "Array", which every sub-plugin with the same mistake would share as its - // registry key, its activation-tracking key, and its notice id. + // is_string(), not truthiness: an array passes truthiness, then casts to the string + // "Array", which every sub-plugin making the mistake would share as its key. if ( ! is_string( $config[ $required ] ) || $config[ $required ] === '' ) { throw new Config_Exception( sprintf( @@ -126,10 +115,8 @@ public function __construct( array $config ) { } } - // Rejected here rather than ignored at read time, where "not configured" and "configured - // but uncallable" would collapse into the same answer. A dependency_check that is a - // private method or a typo'd function name would otherwise report dependencies met and - // let the load proceed into the fatal it exists to prevent. + // Rejected at registration, not at read time, where "not configured" and "configured but + // uncallable" collapse and a typo'd dependency_check would report dependencies met. foreach ( self::CALLABLE_KEYS as $key ) { if ( isset( $config[ $key ] ) && ! is_callable( $config[ $key ] ) ) { throw new Config_Exception( @@ -143,9 +130,7 @@ public function __construct( array $config ) { } } - // A basename names a file already on disk; nothing about it waits on anything. A closure - // under it would reach deactivate_plugins() as the string "Closure", so it is worth a loud - // failure at registration rather than a puzzling one much later. + // A basename names a file already on disk, so there is nothing here to defer. foreach ( self::STRING_KEYS as $key ) { if ( isset( $config[ $key ] ) && ! is_string( $config[ $key ] ) ) { throw new Config_Exception( @@ -159,9 +144,7 @@ public function __construct( array $config ) { } } - // Anything that is neither a string nor a callable is rejected where the mistake was made. - // At read time an array would cast to the string "Array", and a [ class, method ] pair - // naming a method that does not exist would become the value itself. + // Either form: a policy is usually a constant with nothing to defer, and is never text. foreach ( self::DEFERRABLE_KEYS as $key ) { if ( ! isset( $config[ $key ] ) ) { continue; @@ -180,11 +163,8 @@ public function __construct( array $config ) { } } - // A string is refused outright here, unlike everywhere else. Text a host has already - // translated cannot be told from text it has not, and a config array is built before init - // -- so accepting the one shape that can only have been produced too early would leave the - // just-in-time textdomain notice these keys exist to prevent. One `static fn()` at the call - // site fails here the first time the code runs, rather than in someone else's log. + // Any string is refused, not just a string callable: a config array is built before init, so + // text here can only have been translated too early, and the value cannot say it was not. foreach ( self::MESSAGE_KEYS as $key ) { if ( ! isset( $config[ $key ] ) ) { continue; @@ -205,8 +185,8 @@ public function __construct( array $config ) { } } - // The loops above have proved every key this class reads. Only `enabled` is read without a - // type behind it, and its two forms -- a bool and a callable -- cannot be confused. + // `enabled` is the one key with no type behind it: it is read as a bool when it is not + // callable, so an array or an object there counts as enabled. /** @var Sub_Plugin_Config $config */ $this->config = $config; } @@ -245,9 +225,8 @@ public function get_plugin_loaded_constant(): string { /** * The configured policy, after the filter has had the final say. * - * The result is not checked against the known policies here: a filter may legitimately - * return anything, and rejecting it at this boundary would hide the override rather than - * report it. Callers that dispatch on the value check it with Conflict_Policy::is_valid(). + * The result is deliberately not validated here — rejecting a filter's return would hide the + * override rather than report it. Callers that dispatch on it use Conflict_Policy::is_valid(). * * @since 1.0.0 * @@ -262,12 +241,10 @@ public function get_conflict_policy(): string { /** * Filters the policy applied when a standalone copy of a sub-plugin is found active. * - * The dynamic portion of the hook name, `$hook_prefix`, is the prefix given to - * Config::set_hook_prefix(). + * The dynamic portion, `$hook_prefix`, is the prefix given to Config::set_hook_prefix(). * - * Fires after the configured value and its default, so a host can decide per request - * rather than at registration. Returning a value that is not one of the Conflict_Policy - * constants is not consent to deactivate — callers leave the standalone alone instead. + * Fires last, after the configured value and its default, so a host can decide per request. A + * return that is not a Conflict_Policy constant is not consent to deactivate. * * @since 1.0.0 * @@ -276,8 +253,7 @@ public function get_conflict_policy(): string { */ $policy = apply_filters( Config::get_hook_name( 'conflict_policy' ), $policy, $this ); - // An empty string matches no known policy, so a filter that returned something uncastable - // lands at the conservative branch rather than at a deactivation. + // An uncastable filter return becomes '', which matches no policy, so callers stay put. return $this->as_string( $policy ); } @@ -289,17 +265,15 @@ public function get_conflict_policy(): string { public function is_enabled(): bool { $enabled = $this->config['enabled'] ?? true; - // A bool is never callable, so nothing here is guessing at intent the way it would if this - // key also took a string. Every callable form works, a plain function name included, and a - // callable is re-evaluated on each call rather than resolved once at registration. + // No string form here, so a plain function name is called too, deliberately, on every read. return (bool) ( is_callable( $enabled ) ? $enabled( $this ) : $enabled ); } /** * True when the plugin's code is already present, from either copy. The fatal guard. * - * Only sound when the constant is defined at file scope. A standalone that defines it from a - * bootstrap hooked at plugins_loaded or later has not defined it yet when this is asked. + * Only sound when the constant is defined at file scope: a standalone defining it from a hook + * past the load pass has not defined it yet when this is asked. * * @since 1.0.0 * @@ -345,11 +319,7 @@ public function are_dependencies_met(): bool { /** * Shown when the standalone is auto-deactivated, and when the user tries to re-activate it. * - * The fallback is a parameter because the two contexts want different wording, and because - * a caller with no fallback of its own would otherwise render nothing at all. - * - * Configured as a callable and never as a string, so that __() cannot run in the config array, - * where it would run before the textdomain is loaded. + * The fallback is a parameter because those two contexts want different wording. * * @since 1.0.0 * @@ -371,11 +341,10 @@ public function get_conflict_notice_message( string $default = '' ): string { * Filters the notice shown when a standalone copy is deactivated, and when the user tries * to activate it again. * - * The dynamic portion of the hook name, `$hook_prefix`, is the prefix given to - * Config::set_hook_prefix(). + * The dynamic portion, `$hook_prefix`, is the prefix given to Config::set_hook_prefix(). * - * Fires when the message is asked for rather than when the sub-plugin is registered, which - * is what makes this the place to translate it: the textdomain is loaded by then. + * Fires last, when the message is asked for rather than at registration, so the textdomain + * is loaded and the text can be translated here. * * @since 1.0.0 * @@ -384,15 +353,11 @@ public function get_conflict_notice_message( string $default = '' ): string { */ $message = apply_filters( Config::get_hook_name( 'conflict_notice_message' ), $message, $this ); - // An empty string renders no notice, which is where a filter returning an array or an - // object lands rather than in a fatal cast. return $this->as_string( $message ); } /** - * Shown when a dependency_check fails. Falls back to a generic, untranslated sentence — the key - * takes a callable, and never a string, so that a host's own __() cannot run in its config - * array, before the textdomain is loaded. + * Shown when a dependency_check fails, falling back to a generic, untranslated sentence. * * @since 1.0.0 * @@ -414,12 +379,9 @@ public function get_dependency_notice_message(): string { /** * Filters the notice shown when a sub-plugin's dependency_check fails. * - * The dynamic portion of the hook name, `$hook_prefix`, is the prefix given to - * Config::set_hook_prefix(). + * The dynamic portion, `$hook_prefix`, is the prefix given to Config::set_hook_prefix(). * - * Fires when the message is asked for rather than when the sub-plugin is registered, which - * is what makes this the place to translate the generic default: the textdomain is loaded - * by then. + * Fires when the message is asked for, so the default can be translated here. * * @since 1.0.0 * @@ -428,16 +390,14 @@ public function get_dependency_notice_message(): string { */ $message = apply_filters( Config::get_hook_name( 'dependency_notice_message' ), $message, $this ); - // An empty string renders no notice, which is where a filter returning an array or an - // object lands rather than in a fatal cast. return $this->as_string( $message ); } /** - * Shown on multisite when a network-active standalone is left active because the host plugin is - * not itself network-activated -- deactivating it network-wide would strand the sites the host - * never reached. Self-contained, with no config key: the text has no per-host variant worth a - * registration-time value, and the filter below is the seam for rewording or translating it. + * Shown when a network-active standalone is left active to avoid stranding sites. + * + * Deactivating it network-wide would take it from the sites a host that is not network-activated + * never reaches. There is no config key -- the filter below is the seam for rewording it. * * @since 1.0.0 * @@ -456,14 +416,11 @@ public function get_stranding_notice_message(): string { ); /** - * Filters the notice shown when a network-active standalone is left active to avoid stranding - * the sites a host that is not network-activated does not reach. + * Filters the notice shown when a network-active standalone is left active. * - * The dynamic portion of the hook name, `$hook_prefix`, is the prefix given to - * Config::set_hook_prefix(). + * The dynamic portion, `$hook_prefix`, is the prefix given to Config::set_hook_prefix(). * - * Fires when the message is asked for rather than when the sub-plugin is registered, which is - * what makes this the place to translate the default: the textdomain is loaded by then. + * Fires when the message is asked for, so the default can be translated here. * * @since 1.0.0 * @@ -472,8 +429,6 @@ public function get_stranding_notice_message(): string { */ $message = apply_filters( Config::get_hook_name( 'stranding_notice_message' ), $message, $this ); - // An empty string renders no notice, which is where a filter returning an array or an - // object lands rather than in a fatal cast. return $this->as_string( $message ); } @@ -491,19 +446,9 @@ public function get_activation_callback(): ?callable { /** * Take a configured value as it stands, or call it for one. * - * A string is always the value itself, however real a function of that name happens to be. - * `date`, `flush` and `key` are all existing functions and all plausible configured values, so - * honouring a string as a call would make the answer depend on what else the site had loaded -- - * and calling one of those with a Sub_Plugin argument is a fatal, not a value. Only the policy - * arrives here as one; the message keys take no string at all. - * - * Every other callable form says "call me" and nothing else: a closure, a [ class, method ] - * pair, an invokable object, a container callback. That is what lets a host defer __() to the - * moment the text is wanted, rather than translating while it builds its config array -- which - * happens before init, and is what raises WordPress's just-in-time textdomain notice. - * - * Called on every read. Resolving once in the constructor would move the too-early call from - * the config array to the line after it. + * A string is always the value itself, however real a function of that name happens to be: + * `date`, `flush` and `key` are all functions and all plausible configured values. Every other + * callable form says "call me", which is what lets a host defer __() until the text is wanted. * * @since 1.0.0 * @@ -512,8 +457,7 @@ public function get_activation_callback(): ?callable { * @return mixed */ private function resolve_deferred( $value ) { - // The constructor has already made this good. It stays because reaching a call on something - // uncallable would be a fatal where the wrong shape is merely a notice that says so. + // Proved by the constructor already, but kept: calling something uncallable is a fatal. if ( is_string( $value ) || ! is_callable( $value ) ) { return $value; } @@ -524,13 +468,9 @@ private function resolve_deferred( $value ) { /** * Reduce whatever a filter or a configured callable returned to a string. * - * Only the cast is shared. Each filter is applied at the method that owns it rather than - * through a common helper, so that every hook keeps a documented call site a reader and a hook - * scanner can both find, and a version of its own. - * - * Both run at plugins_loaded, where casting an array or an object would be a fatal. Anything - * uncastable is treated as though nothing had come back at all; what nothing means is for each - * caller to say. + * Only the cast is shared -- each filter stays applied at the method that owns it, so every hook + * keeps a documented call site. An array or an object would fatal on the cast, so it reads as + * nothing come back, and what nothing means is each caller's to say. * * @since 1.0.0 * @@ -545,16 +485,8 @@ private function as_string( $value ): string { /** * Name the entry a rejection is about, for a host registering several from one loop. * - * A host calling Absorber::register() inside a loop -- over a manifest array of five entries, - * for example -- gets the same file and line in the stack trace whichever entry was rejected, - * because there is only the one call. The rest of the message names the key at fault; only this - * names the entry that key belongs to. - * - * The slug first, because it is what every other message about a sub-plugin names, and the - * bundled file when the slug is itself the key at fault -- it identifies the entry just as well, - * and it names the file the host has to go and look at. Both are read defensively, since either - * may be the missing or malformed key being reported; when neither can be read, saying so is - * still better than a message that quietly names nothing. + * A host calling Absorber::register() in a loop gets the same file and line in the stack trace + * whichever entry was rejected. Both are read defensively: either may be the key at fault. * * @since 1.0.0 * @@ -577,9 +509,9 @@ private function describe_config_entry( array $config ): string { /** * Name the type a key was given, so that a stray value can be found rather than hunted for. * - * An object reports its class: "object given" leaves a Closure and a WP_Error looking the same, - * and they are very different mistakes. The empty string is named as one, because "string given" - * under a key that must be a non-empty string says nothing the host does not already know. + * An object reports its class, since "object given" leaves a Closure and a WP_Error looking the + * same, and the empty string is named as one, since "string given" under a key that must be a + * non-empty string says nothing. * * @since 1.0.0 * diff --git a/src/Traits/Guards_Hook_Prefix.php b/src/Traits/Guards_Hook_Prefix.php index 735bfb6..e9f8c66 100644 --- a/src/Traits/Guards_Hook_Prefix.php +++ b/src/Traits/Guards_Hook_Prefix.php @@ -13,13 +13,9 @@ /** * Turns "no hook prefix has been set" into something a developer sees and a request survives. * - * 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. + * Every entry point needing the prefix is reached from a core hook, where throwing would take the + * whole site down over a bootstrap mistake — so each asks this first and stands down on a no. A + * trait, not a collaborator: the answer comes from `Config` either way, only the report is shared. * * @since 1.0.0 */ diff --git a/src/Traits/Guards_Plugin_Capability.php b/src/Traits/Guards_Plugin_Capability.php index 695918d..526833f 100644 --- a/src/Traits/Guards_Plugin_Capability.php +++ b/src/Traits/Guards_Plugin_Capability.php @@ -10,27 +10,13 @@ /** * The one answer to "may this user act on a plugin, at the reach this library's acts have". * - * Two places ask it about the same act. `Conflict\Gatekeeper` asks before a standalone is - * deactivated, and `Notices\Presenter` asks before the queue reporting that deactivation is printed - * — which also clears it, for everybody. So the two have to agree by construction: a consume gate - * looser than the resolve gate hands the only copy of a notice to somebody who could not have caused - * what it reports and cannot undo it. + * `Conflict\Gatekeeper` asks before a standalone is deactivated, `Notices\Presenter` before the + * queue reporting it is printed and cleared for everybody. They have to agree by construction: a + * looser consume gate hands the only copy of a notice to somebody who cannot act on it. * - * The capability is network-scoped wherever a network exists, because that is how far the act - * reaches. `Plugin\Deactivator` leaves `deactivate_plugins()`'s `$network_wide` at its default, which - * core reads as both scopes, so the standalone comes out of the network's active plugins whichever - * site the request arrived on; and the notice queue is one network option, so consuming it consumes - * it for every site. `activate_plugins` does not establish that authority and cannot be relied on to - * imply it: core widens it into the network capability only while the `menu_items` site option keeps - * the Plugins menu off, so on a network that has turned that menu on, every site administrator holds - * the site capability outright and none of them holds the network one. - * - * A trait rather than a shared collaborator, for the same reason `Guards_Hook_Prefix` is one: the - * answer comes from `current_user_can()` either way, and all that is shared is which capability to - * name. A collaborator would want a container binding, a constructor argument on both classes and an - * interface nothing dispatches on, to carry one boolean. What it may not be is a literal in each - * class with a docblock in each pointing at the other — that is what the two had, and they drifted - * apart without a single test noticing. + * Network-scoped wherever a network exists, because that is how far the act reaches. + * `activate_plugins` cannot be relied on to imply the network capability: core widens it only while + * the `menu_items` site option keeps the Plugins menu off, and a network may turn that menu on. * * @since 1.0.0 */