From 4d5678780ee4c5091ae7f2464599d708460bc5c5 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 17:39:26 +0200 Subject: [PATCH 1/5] Add the Loader boot barrier and the load path boot() wires conflict-free hook trampolines: the load loop at plugins_loaded priority 2, and notice rendering under is_admin() only. Booting too late is detected rather than ignored -- WordPress accepts a callback added at a priority the running dispatch has already reached and then never fires it -- so the window is reported through _doing_it_wrong() and the load runs inline. load() gates in order of cost and decisiveness: enabled, guard constant, dependencies, a readable file, then the should_load filter. The guard constant comes before the dependency check because it alone means "already running", and warning about unmet requirements for a plugin the admin can see working would be worse than useless. Loader_State grows the boot flag and takes the hooks back off. Clearing the flag alone would leave a Loader that reports itself unbooted while its callbacks are still attached, so the next boot() would wire nothing and still look right. --- README.md | 27 +- docs/configuration.md | 18 + docs/conflict-handling.md | 14 +- docs/filters.md | 17 + .../plans/2026-07-31-plugin-absorber.md | 639 +++++++++--------- src/Loader.php | 229 +++++++ tests/_support/Loader_State.php | 24 +- tests/unit/LoaderBootTest.php | 271 ++++++++ tests/unit/LoaderLoadTest.php | 603 +++++++++++++++++ 9 files changed, 1491 insertions(+), 351 deletions(-) create mode 100644 tests/unit/LoaderBootTest.php create mode 100644 tests/unit/LoaderLoadTest.php diff --git a/README.md b/README.md index 18d0bc0..e969227 100644 --- a/README.md +++ b/README.md @@ -19,22 +19,27 @@ plugins shipping different versions of this library will collide otherwise. See ```php use Nexcess\PluginAbsorber\Config; +use Nexcess\PluginAbsorber\Loader; -Config::set_hook_prefix( 'give' ); // required — keys the hooks and options -Config::set_container( give()->container ); // optional — lets you rebind collaborators -``` +add_action( 'plugins_loaded', function () { + Config::set_hook_prefix( 'give' ); // required — keys the hooks and options + Config::set_container( give()->container ); // optional — lets you rebind collaborators -Each sub-plugin is then described by a config array: + Loader::register( [ + 'slug' => 'give-stripe', + 'bundled_plugin_file' => __DIR__ . '/sub-plugins/give-stripe/give-stripe.php', + 'plugin_loaded_constant' => 'GIVE_STRIPE_VERSION', + 'standalone_plugin_basename' => 'give-stripe/give-stripe.php', + ] ); -```php -[ - 'slug' => 'give-stripe', - 'bundled_plugin_file' => __DIR__ . '/sub-plugins/give-stripe/give-stripe.php', - 'plugin_loaded_constant' => 'GIVE_STRIPE_VERSION', - 'standalone_plugin_basename' => 'give-stripe/give-stripe.php', -] + Loader::boot(); +}, 0 ); ``` +Keep the `, 0`: `boot()` wires the load at `plugins_loaded` priority 2, and WordPress silently +ignores a callback added at or past the priority it is already dispatching. Booting later is +reported through `_doing_it_wrong()` and loaded inline, but the ordering guarantees are weaker. + ## Docs - [Installing](docs/installing.md) — Composer, Strauss, and the constants Strauss must leave alone. diff --git a/docs/configuration.md b/docs/configuration.md index 2922713..b3c75fb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -32,6 +32,7 @@ Config::set_container( $container ); | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | +| `Notices\Contracts\Queue_Interface` | `Notices\Queue` | Queues and renders the admin notices. | `set_container()` is a configuration call like `set_hook_prefix()`, and order does not matter: it may come before or after your `Loader::register()` calls, so long as it comes before boot. Registering @@ -75,6 +76,23 @@ at the second `register()` call; a config array the library cannot use is still Register unconditionally and put anything you cannot decide up front — a licence that may not be active, a setting the site owner can change — in `enabled`, which is re-evaluated on every load. +## The bundled file is included from a function, not from global scope + +WordPress includes plugins from `wp-settings.php` at global scope; this library includes them from +inside a method. Variables assigned at the top level of the bundled file are therefore function-local +and do not become globals: + +```php +// In the bundled plugin's main file. +$my_plugin = new My_Plugin(); // Not a global. `global $my_plugin;` elsewhere sees null. +$GLOBALS['my_plugin'] = new My_Plugin(); // Works. +``` + +Everything else — function and class declarations, `define()`, hook registration, `__FILE__` — is +unaffected. Bundle a plugin that publishes its instance through `$GLOBALS`, a singleton or a +container, which is what plugins written in the last decade do anyway. No amount of wrapping on this +side can hand a required file the global scope it would have had. + ## Messages are callables, never strings Your config array is built at plugin load — before `init`, and before your textdomain. Calling diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index d56bca9..9491364 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -18,9 +18,17 @@ Set one per sub-plugin with the `conflict_policy` key, or decide it at runtime w Before loading a bundled plugin, the library checks whether `plugin_loaded_constant` is already defined. `defined()` ⇒ skip, which is what prevents the re-declaration fatal. -**The constant must be defined at file scope.** A standalone that defines it from a bootstrap -hooked at `plugins_loaded` or later has not defined it yet at the moment the guard is read, and the -bundled copy would load on top of it. +**The constant must be defined at file scope**, inside a `defined()` check so whichever copy loads +first wins: + +```php +if ( ! defined( 'GIVE_RECURRING_VERSION' ) ) { + define( 'GIVE_RECURRING_VERSION', '2.4.0' ); +} +``` + +A standalone that defines it from a bootstrap hooked at `plugins_loaded` or later has not defined it +yet at the moment the guard is read, and the bundled copy would load on top of it. ## What the guard cannot do diff --git a/docs/filters.md b/docs/filters.md index e081f58..5e79d04 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -13,3 +13,20 @@ asked for rather than when the sub-plugin is registered, they are also the place by then the textdomain is loaded. A filter returning a non-scalar yields an empty string rather than a fatal cast. + +## The load gate + +| Filter | Arguments | Purpose | +|---|---|---| +| `{prefix}/plugin_absorber/should_load` | `bool $should_load`, `Sub_Plugin $sub_plugin` | Last word before `require_once`. | + +```php +add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_plugin ) { + return $sub_plugin->get_slug() === 'give-recurring' ? false : $should_load; +}, 10, 2 ); +``` + +It is consulted only for a sub-plugin that would otherwise have loaded — after the enabled check, +the guard constant, the dependency check and the file check, in that order. So returning `true` +cannot force a load past the guard constant: nothing overrides that. Anything other than a truthy +return skips the load, which is the safe direction. diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index e1523d3..ad93eef 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -659,24 +659,39 @@ that throws, a binding of the wrong type, and the registration order cases. Not ## Task 11: `Loader` boot and load path -**PR 11** · branch `11-loader-load-path` from `10-notices-queue` · 2 source files +**PR 11** · branch `11-loader-load-path` from `10-notices-queue` · 1 source file **Files:** -- Modify: `src/Loader.php`, `tests/_support/Loader_State.php` (the new `$booted` property needs a default), `README.md` -- Create: `tests/unit/LoaderLoadTest.php`, `tests/unit/LoaderBootTest.php` +- Modify: `src/Loader.php`, `tests/_support/Loader_State.php` (the new `$booted` property needs a + default, and the hooks `boot()` added need unwiring), `README.md`, `docs/filters.md`, + `docs/configuration.md`, `docs/conflict-handling.md` +- Create: `tests/unit/LoaderBootTest.php`, `tests/unit/LoaderLoadTest.php` **Interfaces:** -- Consumes: `Sub_Plugin` predicates (Task 7), `Loader::notices()` (Task 10), `Config::get_hook_name()` (Task 4). +- Consumes: `Sub_Plugin` predicates (Task 7), `Loader::all()` and its buffer drain (Task 9), + `Loader::notices()` → `Notices\Contracts\Queue_Interface`, default `Notices\Queue` (Task 10), + `Config::get_hook_name()` and `Config::get_hook_prefix()` (Task 4). - Produces: - `Loader::boot(): void` — idempotent - `Loader::load_all(): void` - `Loader::render_notices(): void` - `Loader::load( Sub_Plugin ): void` — private + - `Loader::wiring_window_has_closed(): bool` — private + - `Loader::has_hook_prefix(): bool` — private + - `Loader::LOAD_PRIORITY` — private const, the `plugins_loaded` priority the load loop runs at - the `"{$prefix}/plugin_absorber/should_load"` filter, args `(bool $should_load, Sub_Plugin $sub_plugin)` Task 12 adds the `plugins_loaded` @1 hook to `boot()`; Task 13 adds the activation call to `load()`. -**Design note:** `boot()` wires only the @2 load hook and `all_admin_notices` in this PR. The @1 conflict-resolution hook arrives in Task 12 with the resolver it delegates to — wiring a trampoline to a collaborator that does not exist yet would not run. +**Design note:** `boot()` wires only the @2 load hook and `all_admin_notices` in this PR. The @1 +conflict-resolution hook arrives in Task 12 with the resolver it delegates to — wiring a trampoline +to a collaborator that does not exist yet would not run. + +**The boot barrier needs no flush of its own.** `register()` buffers and `all()` flushes, which +landed in Task 9, so `load_all()` just iterates `self::all()` and the drain happens transparently on +the first read. That read is at `plugins_loaded` priority 2, which is after the host's own bootstrap +at priority 0 — so the container is set before anything resolves, and a registration made before +`Config::set_container()` still reaches the bound registrar. - [ ] **Step 1: Cut the branch** @@ -684,264 +699,46 @@ that throws, a binding of the wrong type, and the registration order cases. Not git checkout 10-notices-queue && git checkout -b 11-loader-load-path ``` -- [ ] **Step 2: Write the failing load-path test** - -Each test writes its own fixture file. `require_once` caches by resolved path for the whole PHP process, so a shared fixture would make the second test in the run silently pass. - -```php - - */ - private $fixtures = []; - - public function setUp(): void { - parent::setUp(); - - Config::set_hook_prefix( 'give' ); - $GLOBALS['absorber_loads'] = 0; - } - - public function tearDown(): void { - foreach ( $this->fixtures as $fixture ) { - if ( file_exists( $fixture ) ) { - unlink( $fixture ); - } - } - $this->fixtures = []; - - unset( $GLOBALS['absorber_loads'] ); - delete_transient( 'give_plugin_absorber_notices' ); - Loader_State::reset(); - Config::reset(); - parent::tearDown(); - } - - /** - * Write a throwaway bundled plugin that counts its own loads and defines its guard constant. - * - * A unique path per test is required: require_once caches by resolved path for the lifetime of - * the PHP process, so a shared fixture would make later tests pass without loading anything. - * - * @param string $constant Guard constant to define. - */ - private function make_fixture( string $constant ): string { - $path = sys_get_temp_dir() . '/absorber-' . uniqid( '', true ) . '.php'; - - file_put_contents( - $path, - 'fixtures[] = $path; - - return $path; - } - - /** - * @param array $overrides Config overrides. - */ - private function register( array $overrides = [], ?string $constant = null ): string { - $constant = $constant ?? 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); - $path = $this->make_fixture( $constant ); - - Loader::register( - array_merge( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => $constant, - ], - $overrides - ) - ); - - return $constant; - } - - public function test_it_requires_the_bundled_file(): void { - $constant = $this->register(); - - Loader::load_all(); - - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); - $this->assertTrue( defined( $constant ) ); - } - - public function test_it_requires_the_bundled_file_exactly_once(): void { - $this->register(); - - Loader::load_all(); - Loader::load_all(); - - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); - } - - public function test_it_skips_a_disabled_sub_plugin(): void { - $this->register( [ 'enabled' => false ] ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): void { - $this->register( [ 'dependency_check' => static fn() => false ] ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - $this->assertArrayHasKey( - 'give-recurring:dependency', - get_transient( 'give_plugin_absorber_notices' ) - ); - } - - public function test_it_skips_when_the_guard_constant_is_already_defined(): void { - define( 'ABSORBER_ALREADY_LOADED_GUARD', '1.0.0' ); - - $this->register( [], 'ABSORBER_ALREADY_LOADED_GUARD' ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A defined constant means the code is already present.' ); - } - - public function test_it_skips_when_the_bundled_file_is_missing(): void { - Loader::register( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', - 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_GUARD', - ] - ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_the_should_load_filter_can_veto_the_load(): void { - $this->register(); - - add_filter( 'give/plugin_absorber/should_load', '__return_false' ); - - Loader::load_all(); - - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - } - - public function test_the_should_load_filter_receives_the_sub_plugin(): void { - $this->register(); - - $received = null; - add_filter( - 'give/plugin_absorber/should_load', - static function ( $should_load, $sub_plugin ) use ( &$received ) { - $received = $sub_plugin; - - return $should_load; - }, - 10, - 2 - ); - - Loader::load_all(); - - $this->assertInstanceOf( \Nexcess\PluginAbsorber\Sub_Plugin::class, $received ); - $this->assertSame( 'give-recurring', $received->get_slug() ); - } - - public function test_it_loads_every_registered_sub_plugin(): void { - $this->register( [ 'slug' => 'give-recurring' ] ); - $this->register( [ 'slug' => 'give-fee-recovery' ] ); - - Loader::load_all(); - - $this->assertSame( 2, $GLOBALS['absorber_loads'] ); - } -} -``` - -- [ ] **Step 3: Write the failing boot test** - -```php -assertSame( - 2, - has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) - ); - } - - public function test_booting_twice_wires_the_hook_only_once(): void { - Loader::boot(); - Loader::boot(); - - $callbacks = $GLOBALS['wp_filter']['plugins_loaded']->callbacks[2] ?? []; - - $this->assertCount( 1, $callbacks, 'boot() must be idempotent.' ); - } - - public function test_it_wires_the_admin_notices_hook_in_the_admin(): void { - set_current_screen( 'dashboard' ); - - Loader::boot(); - - $this->assertNotFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); - - set_current_screen( 'front' ); - } -} -``` +- [ ] **Step 2: Write the failing boot test** + +`tests/unit/LoaderBootTest.php`, a `WPTestCase` that calls `Loader_State::reset()` and +`Config_State::reset()` in `tearDown()`. Behaviours to cover: + +- `boot()` wires `load_all` to `plugins_loaded` at `LOAD_PRIORITY`, and `render_notices` to + `all_admin_notices` — the latter only under `is_admin()`, so both branches need a case. Asserting + the front-end branch does *not* wire needs a working recorder, per the testing rules: assert the + hook is absent, then wire it by hand and assert the same read finds it. +- Calling `boot()` twice wires each hook exactly once — read + `$GLOBALS['wp_filter']['plugins_loaded']->callbacks` at the priority and count, since `has_action()` + cannot tell one callback from two. +- Booting too late is reported and recovered from. With `plugins_loaded` already dispatched, or + dispatching at a priority at or past `LOAD_PRIORITY`, `boot()` triggers `_doing_it_wrong()` and + loads inline instead of wiring a hook that would never fire. The inclusive comparison — booting + *from* `plugins_loaded` @2 — is its own case, because that is the near miss a host actually hits. +- Booting from `plugins_loaded` at a priority *before* `LOAD_PRIORITY` still wires normally. + +- [ ] **Step 3: Write the failing load-path test** + +`tests/unit/LoaderLoadTest.php`. Each test writes its own fixture file: `require_once` caches by +resolved path for the whole PHP process, so a shared fixture would make the second test in the run +silently pass. A fixture counts its own loads into a global and defines its guard constant inside a +`defined()` check; `tearDown()` unlinks the fixtures, drops the global, deletes the notices option +and resets both facades. Behaviours to cover: + +- A registered, enabled sub-plugin with a readable file is required exactly once, and its guard + constant is defined afterwards. Calling `load_all()` twice still loads once. +- Every registered sub-plugin loads, in registration order. +- Each gate skips: disabled; guard constant already defined; dependencies unmet — which also queues + the dependency notice through `Loader::notices()`; bundled file missing, unreadable, or a + directory, which reports through `_doing_it_wrong()` and queues *nothing*. +- The `should_load` filter is applied under the name `Config::get_hook_name( 'should_load' )`, + receives the `Sub_Plugin` as its second argument, and vetoes the load when it returns falsy. +- The gate order is itself asserted: with the guard constant defined *and* an unmet dependency, no + dependency notice is queued — the already-loaded check runs first. +- `load_all()` and `render_notices()` with no hook prefix set report through `_doing_it_wrong()` and + return, rather than throwing out of a core action. +- A registrar bound in the container that returns a non-`Sub_Plugin` entry is skipped rather than + fataling. - [ ] **Step 4: Run both to verify they fail** @@ -950,16 +747,30 @@ Expected: FAIL — `Call to undefined method Nexcess\PluginAbsorber\Loader::boot - [ ] **Step 5: Add boot and the load path to `src/Loader.php`** -Add the `$booted` property beside `$resolved`: +Add the priority constant and the `$booted` property beside `$resolved` and `$pending`: ```php /** + * plugins_loaded priority the load loop runs at. + * + * Ahead of the default priority, so a bundled plugin is in memory before the plugins that + * expect it start their own work, and low enough to leave room for earlier wiring. + * + * @since 1.0.0 + * + * @var int + */ + private const LOAD_PRIORITY = 2; + + /** + * Whether the hooks have been wired. + * * @var bool */ private static $booted = false; ``` -Append these methods: +Then the public methods: ```php /** @@ -980,8 +791,6 @@ Append these methods: self::$booted = true; - add_action( 'plugins_loaded', [ self::class, 'load_all' ], 2 ); - if ( is_admin() ) { // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, // network_admin_notices and user_admin_notices as mutually exclusive branches, so a @@ -989,6 +798,24 @@ Append these methods: // deactivation gets noticed -- would never see the queue rendered. add_action( 'all_admin_notices', [ self::class, 'render_notices' ] ); } + + // Adding an action at a priority the current dispatch has already passed is accepted and + // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- + // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with + // no warning and a site that looks entirely healthy. + if ( self::wiring_window_has_closed() ) { + _doing_it_wrong( + __METHOD__, + 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + '1.0.0' + ); + + self::load_all(); + + return; + } + + add_action( 'plugins_loaded', [ self::class, 'load_all' ], self::LOAD_PRIORITY ); } /** @@ -997,6 +824,13 @@ Append these methods: * @return void */ public static function load_all(): void { + // The load path needs the prefix for the should_load filter and for the notice store. + // Throwing out of a core action would take the whole site down over a bootstrap mistake, + // so it is reported where a developer will see it and the load is abandoned instead. + if ( ! self::has_hook_prefix() ) { + return; + } + foreach ( self::all() as $sub_plugin ) { // Registrar_Interface::all() only declares `array`. A host binding its own registrar // that returns anything else would otherwise fatal inside plugins_loaded on the first @@ -1015,16 +849,27 @@ Append these methods: * @return void */ public static function render_notices(): void { + if ( ! self::has_hook_prefix() ) { + return; + } + self::notices()->render(); } +``` +And the private ones, below `flush()` and `resolve()` — public, then private, and no helper above +the API it serves: + +```php /** - * Load one sub-plugin, in the order the checks are cheapest and most decisive. + * Load one sub-plugin, cheapest and most decisive check first. * * @since 1.0.0 * * @param Sub_Plugin $sub_plugin Sub-plugin to load. * + * @throws Config_Exception When a collaborator binding is unusable. + * * @return void */ private static function load( Sub_Plugin $sub_plugin ): void { @@ -1032,131 +877,253 @@ Append these methods: return; } + // Ahead of the dependency check, which calls an arbitrary host callable. This is one + // defined(), it carries the whole re-declaration guarantee, and it is the only gate that + // means "the plugin is already running" -- warning that requirements are unmet for a + // plugin the admin can see working would be worse than useless. + if ( $sub_plugin->is_already_loaded() ) { + return; + } + if ( ! $sub_plugin->are_dependencies_met() ) { self::notices()->queue_dependency_notice( $sub_plugin ); return; } - // The constant is defined => the code is already present, from either copy. Loading the - // bundled file now would be a re-declaration fatal. - if ( $sub_plugin->is_already_loaded() ) { - return; - } + // Not file_exists(): that is true for a directory and for a file with no read permission, + // and require_once fatals on both. A missing file is a broken build in the host plugin + // rather than anything a site owner can act on, so it goes to the developer instead of + // into the notice queue, where it would have displayed the host's own + // dependency_notice_message and sent the owner after the wrong problem entirely. + $file = $sub_plugin->get_bundled_plugin_file(); + + if ( ! is_file( $file ) || ! is_readable( $file ) ) { + _doing_it_wrong( + 'Nexcess\PluginAbsorber\Loader', + sprintf( + 'The bundled plugin file for "%s" is missing or unreadable: %s', + $sub_plugin->get_slug(), + $file + ), + '1.0.0' + ); - if ( ! file_exists( $sub_plugin->get_bundled_plugin_file() ) ) { return; } - $should_load = apply_filters( - Config::get_hook_name( 'should_load' ), - true, - $sub_plugin - ); + // No type guard on the return, unlike the conflict_policy filter: there is no cast here, + // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the + // load, which is the safe direction. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); if ( ! $should_load ) { return; } - require_once $sub_plugin->get_bundled_plugin_file(); + // An include takes the scope of the line it sits on, and this one is inside a method, where + // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled + // file are function-local as a result -- documented for hosts, because no amount of + // wrapping here can hand a required file the global scope it would have had. + require_once $file; + } + + /** + * Whether it is already too late to wire the load hook. + * + * The comparison is inclusive. A callback added to the priority currently being dispatched is + * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` + * with a by-value foreach, so the append lands on an array the running loop has already copied. + * Booting from plugins_loaded at priority 2 is the case a host is likeliest to hit by accident, + * and an exclusive comparison would let exactly that one through unreported. + * + * @since 1.0.0 + * + * @return bool + */ + private static function wiring_window_has_closed(): bool { + if ( ! did_action( 'plugins_loaded' ) ) { + return false; + } + + if ( ! doing_action( 'plugins_loaded' ) ) { + return true; + } + + $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; + + return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + } + + /** + * Whether a hook prefix has been set, reporting to the developer when it has not. + * + * @since 1.0.0 + * + * @return bool + */ + private static function has_hook_prefix(): bool { + try { + Config::get_hook_prefix(); + } catch ( Config_Exception $exception ) { + _doing_it_wrong( 'Nexcess\PluginAbsorber\Loader', $exception->getMessage(), '1.0.0' ); + + return false; + } + + return true; } ``` -- [ ] **Step 6: Teach `tests/_support/Loader_State.php` about the boot flag** +The filter name is built with `Config::get_hook_name( 'should_load' )` and never by concatenating +`Config::get_hook_prefix()` with the rest: `Config` owns the segment between the host's prefix and +the key's own name, and nothing else assembles it. + +- [ ] **Step 6: Teach `tests/_support/Loader_State.php` about the boot flag and the hooks** + +`Loader_State::reset()` walks `Loader`'s static properties and refuses one it has no default for, so +until `$booted` is listed every test that resets throws a `LogicException` naming it. That is the +helper doing its job: a boot flag left standing would wire the hooks once and then let every later +test's `boot()` no-op. -`Loader_State::reset()` walks `Loader`'s static properties and refuses one it has no default for, -so until `$booted` is listed every test that resets throws a `LogicException` naming it. That is -the helper doing its job: a boot flag left standing would wire the hooks once and then let every -later test's `boot()` no-op. +Clearing the flag is not enough on its own. A `Loader` that reports itself unbooted while its +callbacks are still attached is the worse of the two states: the next `boot()` wires nothing and +still looks like it worked, and the stranded callback goes on loading sub-plugins into tests that +never registered any. So the helper unwires both hooks as well — and reads `LOAD_PRIORITY` off the +class by reflection rather than restating `2`, so it cannot go on removing a hook from a priority the +`Loader` no longer wires. ```php protected const DEFAULTS = [ 'resolved' => [], + 'pending' => [], 'booted' => false, ]; -``` -- [ ] **Step 7: Run the tests to verify they pass** + public static function reset(): void { + $reflection = new ReflectionClass( Loader::class ); -Run: `slic run unit` -Expected: PASS — 9 load tests + 3 boot tests. + // Read rather than restated, so the helper cannot go on removing a hook from a priority + // the Loader no longer wires -- which would leave the real callback attached and every + // later test loading sub-plugins it never registered. + $load_priority = $reflection->getConstant( 'LOAD_PRIORITY' ); -- [ ] **Step 8: Confirm static analysis is still clean** + remove_action( 'plugins_loaded', [ Loader::class, 'load_all' ], (int) $load_priority ); + remove_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); -Run: `composer test:analysis` + // ... the property walk from Task 9, unchanged. + } +``` -- [ ] **Step 9: Append to the README** +There is still no `Loader::reset()`, and nothing in this PR adds one. The seam stays in +`tests/_support/`. -```markdown -### Bootstrap +- [ ] **Step 7: Run the tests to verify they pass** -```php -use Nexcess\PluginAbsorber\Config; -use Nexcess\PluginAbsorber\Loader; +Run: `slic run unit`, then `slic run unit --env multisite`. -add_action( 'plugins_loaded', function () { - Config::set_hook_prefix( 'give' ); +- [ ] **Step 8: Confirm static analysis is still clean** - Loader::register( [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => GIVE_PLUGIN_DIR . 'subs/give-recurring/give-recurring.php', - 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION', - ] ); +Run: `composer test:analysis` +Expected: `[OK] No errors`. - Loader::boot(); -}, 0 ); // priority 0 — before the absorber's own @1 and @2 hooks fire -``` +- [ ] **Step 9: Document it, in four places and not in one** -The bundled file must define its guard constant inside a `defined()` check: +The human docs are split out of the README and are not to grow back, so each piece goes where its +subject already lives. The README gets only the bootstrap, short: -```php -if ( ! defined( 'GIVE_RECURRING_VERSION' ) ) { - define( 'GIVE_RECURRING_VERSION', '2.4.0' ); -} +```markdown +Loader::register( [ ... ] ); +Loader::boot(); ``` -### Load gate +wrapped in `add_action( 'plugins_loaded', ..., 0 )`, with a line on why the `, 0` matters: `boot()` +wires the load at priority 2, WordPress silently ignores a callback added at or past the priority it +is already dispatching, and booting later is reported and loaded inline but with weaker ordering +guarantees. -Applied immediately before `require_once`: +`docs/filters.md` gets the load gate — the filter's arguments, the `add_filter()` snippet, and the +order it sits in: consulted only for a sub-plugin that would otherwise have loaded, so returning +`true` cannot force a load past the guard constant. -```php -add_filter( 'give/plugin_absorber/should_load', function ( $should_load, $sub_plugin ) { - return $sub_plugin->get_slug() === 'give-recurring' ? false : $should_load; -}, 10, 2 ); -``` +`docs/configuration.md` gets two things: the `Notices\Contracts\Queue_Interface` → `Notices\Queue` +row in the collaborator table, and the global-scope caveat — top-level assignments in a bundled file +are function-local, `$GLOBALS['my_plugin'] = ...` works, and declarations, `define()`, hooks and +`__FILE__` are unaffected. -A sub-plugin is skipped when it is disabled, its dependencies are unmet, its guard constant is -already defined, its bundled file is missing, or this filter returns false. -``` +`docs/conflict-handling.md` gets the `defined()` snippet the bundled file must wrap its guard +constant in, next to the paragraph that already says the constant must be defined at file scope. + +> **Design notes.** +> +> *The already-loaded check moved ahead of the dependency check.* The first sketch had the two the +> other way round, cheapest-first. `is_already_loaded()` is one `defined()` and the dependency check +> calls an arbitrary host callable, so cheapest-first argues the same way — but the deciding reason +> is what each gate means. A defined guard constant means the plugin is running right now; telling a +> site owner its requirements are unmet, for a plugin they can see working, sends them after a +> problem that does not exist. +> +> *The file check is `is_file()` and `is_readable()`, not `file_exists()`.* `file_exists()` is true +> for a directory and for a file the process cannot read, and `require_once` fatals on both — which +> is the exact failure this library exists to prevent. A missing bundled file is a broken build in +> the host plugin, so it is reported through `_doing_it_wrong()` and queues nothing: the notice queue +> would have shown the host's own `dependency_notice_message` and sent a site owner after the wrong +> problem. +> +> *Booting too late is reported and recovered from, not ignored.* `add_action()` accepts a callback +> at a priority the current dispatch has already passed and then never fires it, so a host that boots +> from `plugins_loaded` at the default priority would load nothing at all, silently, on a site that +> looks healthy. `wiring_window_has_closed()` compares inclusively, because booting from +> `plugins_loaded` @2 is the near miss a host actually hits and an exclusive comparison lets exactly +> that one through unreported. The recovery is an inline `load_all()`: weaker ordering, but the +> sub-plugins load. +> +> *A missing hook prefix returns instead of throwing.* Both `load_all()` and `render_notices()` run +> from core actions, and an exception out of `plugins_loaded` takes the whole site down over a +> bootstrap mistake. `_doing_it_wrong()` puts it in front of the developer who made it and the load +> is abandoned. +> +> *`Loader` still has no `reset()`.* `boot()` adds a third piece of static state and the temptation +> with it, and the answer has not changed: a public reset is API the library supports forever, and a +> host that called it mid-request would drop the registrations the load loop is about to read. +> `Tests\Support\Loader_State::reset()` clears the memo, the buffer and the boot flag by reflection, +> and unwires the two hooks in the same pass — a reset that cleared the flag alone would leave a +> `Loader` that reports itself unbooted with its callbacks still attached. - [ ] **Step 10: Commit, push, open the PR** ```bash -git add src/Loader.php tests/_support/Loader_State.php tests/unit/LoaderLoadTest.php tests/unit/LoaderBootTest.php README.md +git add src/Loader.php tests/_support/Loader_State.php tests/unit/LoaderLoadTest.php tests/unit/LoaderBootTest.php README.md docs/filters.md docs/configuration.md docs/conflict-handling.md git commit -m "Add Loader boot and the load path" git push -u origin 11-loader-load-path gh pr create --base 10-notices-queue --title "Loader boot and load path" --body 'What: `boot()`, `load_all()`, the five-gate load path, and the `should_load` filter. Usage: - Loader::register( [ ... ] ); - Loader::boot(); // wires plugins_loaded @2 and all_admin_notices + add_action( "plugins_loaded", function () { + Config::set_hook_prefix( "give" ); + Loader::register( [ ... ] ); + Loader::boot(); // wires plugins_loaded @2 and all_admin_notices + }, 0 ); add_filter( "give/plugin_absorber/should_load", function ( $should_load, $sub_plugin ) { return $should_load; }, 10, 2 ); -Why this way: gate order is deliberate — `is_enabled()` and `are_dependencies_met()` are cheap -config checks, `is_already_loaded()` is the one that actually prevents the fatal, and the filter -runs last so a host override cannot accidentally re-introduce a re-declaration. The -already-loaded check sits before `file_exists()` because it is both cheaper and more important. - -`boot()` wires only the @2 hook here; the @1 conflict-resolution hook lands with the resolver it -delegates to, since a trampoline pointing at a collaborator that does not exist yet would not run. - -Verify: `slic run unit` — 12 tests. Each writes its own fixture file, because `require_once` caches -by resolved path for the whole PHP process and a shared fixture would make later tests pass without -loading anything.' +Why this way: the guard constant is checked before the dependency check rather than after it. A +defined constant means the plugin is running right now, and warning that requirements are unmet for +a plugin the admin can see working sends them after a problem that does not exist. The file check is +`is_file()` plus `is_readable()`, not `file_exists()`, which is true for a directory and for an +unreadable file and lets `require_once` fatal on both. Booting past `plugins_loaded` @2 is reported +through `_doing_it_wrong()` and loaded inline instead of wiring a hook that would never fire — +against silently loading nothing on a site that looks healthy. `boot()` wires only the @2 hook here; +the @1 conflict-resolution hook lands with the resolver it delegates to. + +Verify: `slic run unit` on both envs, and `composer test:analysis`. Covered: every gate and its +order, the boot window including the inclusive near miss at @2, and the missing-prefix path. Each +load test writes its own fixture, because `require_once` caches by resolved path for the whole PHP +process. Not covered here: conflict resolution and the activation callback, which land in Tasks 12 +and 13.' ``` --- diff --git a/src/Loader.php b/src/Loader.php index 80aa63c..7f1509d 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -7,7 +7,10 @@ use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; +use Nexcess\PluginAbsorber\Notices\Queue; use Throwable; +use WP_Hook; /** * Static facade: collaborator resolution, registration, hook wiring, and the load loop. @@ -15,6 +18,18 @@ * @since 1.0.0 */ class Loader { + /** + * plugins_loaded priority the load loop runs at. + * + * Ahead of the default priority, so a bundled plugin is in memory before the plugins that + * expect it start their own work, and low enough to leave room for earlier wiring. + * + * @since 1.0.0 + * + * @var int + */ + private const LOAD_PRIORITY = 2; + /** * Resolved collaborators, memoized by interface name. * @@ -29,6 +44,13 @@ class Loader { */ private static $pending = []; + /** + * Whether the hooks have been wired. + * + * @var bool + */ + private static $booted = false; + /** * @since 1.0.0 * @@ -40,6 +62,17 @@ public static function registrar(): Registrar_Interface { return self::resolve( Registrar_Interface::class, Registrar::class ); } + /** + * @since 1.0.0 + * + * @throws Config_Exception When the container cannot produce a usable instance. + * + * @return Queue_Interface + */ + public static function notices(): Queue_Interface { + return self::resolve( Queue_Interface::class, Queue::class ); + } + /** * Register one bundled sub-plugin. Call once per sub-plugin, before boot(). * @@ -80,6 +113,89 @@ public static function all(): array { return self::registrar()->all(); } + /** + * Wire the WordPress hooks. Idempotent — safe to call from more than one code path. + * + * Hooks are plain static trampolines rather than container callbacks, which is what keeps the + * container optional. Each trampoline delegates to the resolved collaborator, so rebinding + * still takes effect. + * + * @since 1.0.0 + * + * @return void + */ + public static function boot(): void { + if ( self::$booted ) { + return; + } + + self::$booted = true; + + if ( is_admin() ) { + // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, + // network_admin_notices and user_admin_notices as mutually exclusive branches, so a + // superadmin working in the network admin -- exactly where a network-wide + // deactivation gets noticed -- would never see the queue rendered. + add_action( 'all_admin_notices', [ self::class, 'render_notices' ] ); + } + + // Adding an action at a priority the current dispatch has already passed is accepted and + // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- + // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with + // no warning and a site that looks entirely healthy. + if ( self::wiring_window_has_closed() ) { + _doing_it_wrong( + __METHOD__, + 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + '1.0.0' + ); + + self::load_all(); + + return; + } + + add_action( 'plugins_loaded', [ self::class, 'load_all' ], self::LOAD_PRIORITY ); + } + + /** + * @since 1.0.0 + * + * @return void + */ + public static function load_all(): void { + // The load path needs the prefix for the should_load filter and for the notice store. + // Throwing out of a core action would take the whole site down over a bootstrap mistake, + // so it is reported where a developer will see it and the load is abandoned instead. + if ( ! self::has_hook_prefix() ) { + return; + } + + foreach ( self::all() as $sub_plugin ) { + // Registrar_Interface::all() only declares `array`. A host binding its own registrar + // that returns anything else would otherwise fatal inside plugins_loaded on the first + // predicate call -- the exact failure this library exists to prevent. + if ( ! $sub_plugin instanceof Sub_Plugin ) { + continue; + } + + self::load( $sub_plugin ); + } + } + + /** + * @since 1.0.0 + * + * @return void + */ + public static function render_notices(): void { + if ( ! self::has_hook_prefix() ) { + return; + } + + self::notices()->render(); + } + /** * Hand every buffered registration to the registrar. * @@ -190,4 +306,117 @@ private static function resolve( string $interface, string $default_class ): obj return self::$resolved[ $interface ]; } + + /** + * Load one sub-plugin, cheapest and most decisive check first. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin to load. + * + * @throws Config_Exception When a collaborator binding is unusable. + * + * @return void + */ + private static function load( Sub_Plugin $sub_plugin ): void { + if ( ! $sub_plugin->is_enabled() ) { + return; + } + + // Ahead of the dependency check, which calls an arbitrary host callable. This is one + // defined(), it carries the whole re-declaration guarantee, and it is the only gate that + // means "the plugin is already running" -- warning that requirements are unmet for a + // plugin the admin can see working would be worse than useless. + if ( $sub_plugin->is_already_loaded() ) { + return; + } + + if ( ! $sub_plugin->are_dependencies_met() ) { + self::notices()->queue_dependency_notice( $sub_plugin ); + + return; + } + + // Not file_exists(): that is true for a directory and for a file with no read permission, + // and require_once fatals on both. A missing file is a broken build in the host plugin + // rather than anything a site owner can act on, so it goes to the developer instead of + // into the notice queue, where it would have displayed the host's own + // dependency_notice_message and sent the owner after the wrong problem entirely. + $file = $sub_plugin->get_bundled_plugin_file(); + + if ( ! is_file( $file ) || ! is_readable( $file ) ) { + _doing_it_wrong( + 'Nexcess\PluginAbsorber\Loader', + sprintf( + 'The bundled plugin file for "%s" is missing or unreadable: %s', + $sub_plugin->get_slug(), + $file + ), + '1.0.0' + ); + + return; + } + + // No type guard on the return, unlike the conflict_policy filter: there is no cast here, + // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the + // load, which is the safe direction. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); + + if ( ! $should_load ) { + return; + } + + // An include takes the scope of the line it sits on, and this one is inside a method, where + // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled + // file are function-local as a result -- documented for hosts, because no amount of + // wrapping here can hand a required file the global scope it would have had. + require_once $file; + } + + /** + * Whether it is already too late to wire the load hook. + * + * The comparison is inclusive. A callback added to the priority currently being dispatched is + * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` + * with a by-value foreach, so the append lands on an array the running loop has already copied. + * Booting from plugins_loaded at priority 2 is the case a host is likeliest to hit by accident, + * and an exclusive comparison would let exactly that one through unreported. + * + * @since 1.0.0 + * + * @return bool + */ + private static function wiring_window_has_closed(): bool { + if ( ! did_action( 'plugins_loaded' ) ) { + return false; + } + + if ( ! doing_action( 'plugins_loaded' ) ) { + return true; + } + + $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; + + return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + } + + /** + * Whether a hook prefix has been set, reporting to the developer when it has not. + * + * @since 1.0.0 + * + * @return bool + */ + private static function has_hook_prefix(): bool { + try { + Config::get_hook_prefix(); + } catch ( Config_Exception $exception ) { + _doing_it_wrong( 'Nexcess\PluginAbsorber\Loader', $exception->getMessage(), '1.0.0' ); + + return false; + } + + return true; + } } diff --git a/tests/_support/Loader_State.php b/tests/_support/Loader_State.php index 67cc000..b7163f2 100644 --- a/tests/_support/Loader_State.php +++ b/tests/_support/Loader_State.php @@ -35,10 +35,15 @@ class Loader_State { protected const DEFAULTS = [ 'resolved' => [], 'pending' => [], + 'booted' => false, ]; /** - * Return every static property of `Loader` to its default. + * Return every static property of `Loader` to its default, and unwire the hooks it added. + * + * Clearing the boot flag without unwiring would leave a `Loader` that reports itself unbooted + * while its callbacks are still attached: the next `boot()` would wire nothing, and still look + * like it had worked. * * @throws LogicException When `Loader` has grown a static property this helper does not know * about, rather than leaving it to leak between tests. @@ -48,6 +53,23 @@ class Loader_State { public static function reset(): void { $reflection = new ReflectionClass( Loader::class ); + // Read rather than restated, so the helper cannot go on removing a hook from a priority + // the Loader no longer wires -- which would leave the real callback attached and every + // later test loading sub-plugins it never registered. + $load_priority = $reflection->getConstant( 'LOAD_PRIORITY' ); + + // Refused rather than coerced. A default priority here would take the load hook off some + // other number, leave the real callback attached, and load sub-plugins into every later + // test in the process. + if ( ! is_int( $load_priority ) ) { + throw new LogicException( + sprintf( 'Loader::LOAD_PRIORITY must be an int for %s to unwire the load hook.', self::class ) + ); + } + + remove_action( 'plugins_loaded', [ Loader::class, 'load_all' ], $load_priority ); + remove_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); + foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { $name = $property->getName(); diff --git a/tests/unit/LoaderBootTest.php b/tests/unit/LoaderBootTest.php new file mode 100644 index 0000000..b4a185f --- /dev/null +++ b/tests/unit/LoaderBootTest.php @@ -0,0 +1,271 @@ + + */ + private $added_actions = []; + + /** + * @var string[] + */ + private $fixtures = []; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + + // The harness has to boot WordPress before it can run anything, so plugins_loaded has + // already fired by the time any test starts — and boot() would rightly report that it is + // too late to wire a hook at priority 2. Rewind the counter so the wiring tests see the + // timing a real host bootstrap sees. The tests that exercise a late boot set it back. + $this->plugins_loaded_count = did_action( 'plugins_loaded' ); + unset( $GLOBALS['wp_actions']['plugins_loaded'] ); + + $GLOBALS[ self::LOADED ] = false; + } + + public function tearDown(): void { + $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + + // In tearDown rather than at the end of the test body: a failing assertion would otherwise + // leak an admin screen into every test that runs after it, since is_admin() checks the + // current screen before WP_ADMIN. + set_current_screen( 'front' ); + + // Only what these tests added by hand. The two hooks boot() wires come off in + // Loader_State::reset(), which reads the load priority from the Loader rather than + // restating it. + foreach ( $this->added_actions as [ $hook, $callback, $priority ] ) { + remove_action( $hook, $callback, $priority ); + } + $this->added_actions = []; + + foreach ( $this->fixtures as $fixture ) { + if ( file_exists( $fixture ) ) { + unlink( $fixture ); + } + } + $this->fixtures = []; + + unset( $GLOBALS[ self::LOADED ] ); + Loader_State::reset(); + Config_State::reset(); + parent::tearDown(); + } + + public function test_it_wires_the_load_hook_at_priority_two(): void { + Loader::boot(); + + $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); + } + + public function test_booting_twice_wires_the_hook_only_once(): void { + Loader::boot(); + Loader::boot(); + + $callbacks = $GLOBALS['wp_filter']['plugins_loaded']->callbacks[2] ?? []; + + $this->assertCount( 1, $callbacks, 'boot() must be idempotent.' ); + } + + public function test_it_wires_the_admin_notices_hook_in_the_admin(): void { + set_current_screen( 'dashboard' ); + + Loader::boot(); + + $this->assertNotFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); + } + + /** + * Nothing renders a notice on the front end, and the queue must survive until an admin load + * consumes it. + */ + public function test_it_does_not_wire_the_notice_hook_on_the_front_end(): void { + set_current_screen( 'front' ); + + Loader::boot(); + + $this->assertFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); + } + + /** + * Adding an action at a priority the running dispatch has already passed is accepted and then + * never fires. Booting from plugins_loaded at the default priority instead of 0 would + * otherwise load nothing at all, on a site that looks completely healthy. + * + * Priority 2 is the boundary case: a callback added to the priority currently being dispatched + * is never reached either, because the dispatch loop walks a by-value copy of that priority's + * callback array. + * + * @dataProvider late_boot_priorities + * + * @param int $priority plugins_loaded priority the host boots from. + */ + public function test_booting_too_late_in_plugins_loaded_loads_inline_instead( int $priority ): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); + + $path = $this->make_fixture(); + + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( $path, $priority ) { + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => 'ABSORBER_LATE_BOOT_GUARD_' . $priority, + ] + ); + + Loader::boot(); + }, + $priority + ); + + do_action( 'plugins_loaded' ); + + $this->assertTrue( $GLOBALS[ self::LOADED ], 'A late boot must still load.' ); + } + + /** + * @return Generator + */ + public static function late_boot_priorities(): Generator { + yield 'at the load priority' => [ 2 ]; + yield 'one past it' => [ 3 ]; + yield 'the default a host omits' => [ 10 ]; + } + + public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); + + $path = $this->make_fixture(); + + do_action( 'plugins_loaded' ); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => 'ABSORBER_AFTER_BOOT_GUARD', + ] + ); + Loader::boot(); + + $this->assertTrue( $GLOBALS[ self::LOADED ] ); + } + + /** + * The suite's state helper stands in for the reset() the Loader deliberately does not ship. + * Clearing the boot flag without unwiring would leave a Loader that reports itself unbooted + * while its callbacks are still attached — and every later test would load sub-plugins it + * never registered. + */ + public function test_the_state_helper_unwires_the_hooks_boot_added(): void { + set_current_screen( 'dashboard' ); + + Loader::boot(); + Loader_State::reset(); + + $this->assertFalse( has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); + $this->assertFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); + } + + /** + * The hooks are gone by the time this boots again, so the assertion is about the second boot + * rather than a leftover from the first. + */ + public function test_the_state_helper_allows_booting_again(): void { + Loader::boot(); + Loader_State::reset(); + + Loader::boot(); + + $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); + } + + /** + * boot() runs before anything reads the prefix, so it must not be the thing that throws when + * a host forgot to set one — the load path reports that at a point the host can see. + */ + public function test_boot_does_not_need_a_hook_prefix(): void { + Config_State::reset(); + + Loader::boot(); + + $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); + } + + /** + * Write a throwaway bundled plugin that records its own load. + * + * A unique path per test is required: require_once caches by resolved path for the lifetime of + * the PHP process, so a shared fixture would let a later test pass without loading anything. + * + * @return string + */ + private function make_fixture(): string { + $path = sys_get_temp_dir() . '/absorber-boot-' . uniqid( '', true ) . '.php'; + + file_put_contents( + $path, + 'fixtures[] = $path; + + return $path; + } + + /** + * Add an action tearDown can take back by identity rather than by clearing the whole hook. + * + * @param string $hook Hook to add to. + * @param callable $callback Callback to add. + * @param int $priority Priority to add it at. + * + * @return void + */ + private function add_tracked_action( string $hook, callable $callback, int $priority = 10 ): void { + $this->added_actions[] = [ $hook, $callback, $priority ]; + + add_action( $hook, $callback, $priority ); + } +} diff --git a/tests/unit/LoaderLoadTest.php b/tests/unit/LoaderLoadTest.php new file mode 100644 index 0000000..732f3f6 --- /dev/null +++ b/tests/unit/LoaderLoadTest.php @@ -0,0 +1,603 @@ + + */ + private $fixtures = []; + + /** + * Guard constants a test defined through uopz. + * + * @var string[] + */ + private $constants = []; + + /** + * The plugins_loaded count as the harness left it, for the one test that rewinds it. + * + * @var int|null + */ + private $plugins_loaded_count = null; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + $this->clear_notices(); + $GLOBALS['absorber_loads'] = 0; + } + + public function tearDown(): void { + foreach ( $this->fixtures as $fixture ) { + if ( file_exists( $fixture ) ) { + unlink( $fixture ); + } + } + $this->fixtures = []; + + // In tearDown rather than at the end of the test body: a failing assertion would otherwise + // strand the constant for the rest of the process, and every later test would read it as a + // sub-plugin whose code is already present and skip the load it was written to exercise. + foreach ( $this->constants as $constant ) { + $this->unsetConstant( $constant ); + } + $this->constants = []; + + if ( $this->plugins_loaded_count !== null ) { + $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + $this->plugins_loaded_count = null; + } + + unset( $GLOBALS['absorber_loads'] ); + $this->clear_notices(); + Loader_State::reset(); + Config_State::reset(); + parent::tearDown(); + } + + public function test_it_requires_the_bundled_file(): void { + $constant = $this->register(); + + Loader::load_all(); + + $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + $this->assertTrue( defined( $constant ) ); + } + + public function test_it_requires_the_bundled_file_exactly_once(): void { + $this->register(); + + Loader::load_all(); + Loader::load_all(); + + $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + } + + public function test_it_skips_a_disabled_sub_plugin(): void { + $this->register( [ 'enabled' => false ] ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + } + + public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): void { + $this->register( [ 'dependency_check' => static fn() => false ] ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertArrayHasKey( 'give-recurring:dependency', $this->notice_queue() ); + } + + public function test_it_skips_when_the_guard_constant_is_already_defined(): void { + $constant = $this->define_guard( 'ABSORBER_ALREADY_LOADED_GUARD' ); + + $this->register( [], $constant ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A defined constant means the code is already present.' ); + } + + public function test_it_skips_when_the_bundled_file_is_missing(): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', + 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_GUARD', + ] + ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + } + + /** + * A broken build is a developer problem, not a site-owner one. It must not reach the notice + * queue, where it would render the host's own dependency_notice_message and send the owner + * after a dependency that is perfectly fine. + * + * The message is configured as a callable because that is the only shape the key takes: a + * string is refused outright, so that a host's __() cannot run while it builds its config array. + */ + public function test_a_missing_bundled_file_reports_to_the_developer_not_the_site_owner(): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', + 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_NOTICE_GUARD', + 'dependency_notice_message' => static fn() => 'GiveWP 3.0 or later is required.', + ] + ); + + Loader::load_all(); + + $this->assertSame( [], $this->notice_queue() ); + } + + /** + * file_exists() is true for a directory, and require_once fatals on one. + */ + public function test_it_skips_when_the_bundled_path_is_a_directory(): void { + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => sys_get_temp_dir(), + 'plugin_loaded_constant' => 'ABSORBER_DIRECTORY_GUARD', + ] + ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + } + + /** + * file_exists() is also true for a file the process cannot read, and require_once fatals. + */ + public function test_it_skips_when_the_bundled_file_is_unreadable(): void { + $path = $this->make_fixture( 'ABSORBER_UNREADABLE_GUARD' ); + chmod( $path, 0000 ); + + if ( is_readable( $path ) ) { + $this->markTestSkipped( 'Running as a user that can read a 0000 file.' ); + } + + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => 'ABSORBER_UNREADABLE_GUARD', + ] + ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + + chmod( $path, 0644 ); + } + + /** + * The dependency check calls an arbitrary host callable, so it must not run for a sub-plugin + * whose code is already present — and must not warn that requirements are unmet for a plugin + * the admin can see running. + */ + public function test_an_already_loaded_sub_plugin_is_not_dependency_checked(): void { + $constant = $this->define_guard( 'ABSORBER_LOADED_BEFORE_DEPS_GUARD' ); + + $checked = 0; + $this->register( + [ + 'dependency_check' => static function () use ( &$checked ) { + ++$checked; + + return false; + }, + ], + $constant + ); + + Loader::load_all(); + + $this->assertSame( 0, $checked ); + $this->assertSame( [], $this->notice_queue(), 'No notice for a plugin that is already running.' ); + } + + public function test_the_should_load_filter_can_veto_the_load(): void { + $this->register(); + + add_filter( 'give/plugin_absorber/should_load', '__return_false' ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + } + + public function test_the_should_load_filter_receives_the_sub_plugin(): void { + $this->register(); + + $received = null; + add_filter( + 'give/plugin_absorber/should_load', + static function ( $should_load, $sub_plugin ) use ( &$received ) { + $received = $sub_plugin; + + return $should_load; + }, + 10, + 2 + ); + + Loader::load_all(); + + $this->assertInstanceOf( Sub_Plugin::class, $received ); + $this->assertSame( 'give-recurring', $received->get_slug() ); + } + + /** + * The filter is the last gate before require_once, so it must not be consulted for a + * sub-plugin that was already going to be skipped — a host counting its invocations would + * otherwise see calls for loads that never happen. + */ + public function test_the_should_load_filter_is_not_consulted_for_a_disabled_sub_plugin(): void { + $this->register( [ 'enabled' => false ] ); + + $calls = []; + add_filter( + 'give/plugin_absorber/should_load', + static function ( $should_load ) use ( &$calls ) { + $calls[] = $should_load; + + return $should_load; + } + ); + + Loader::load_all(); + + $this->assertSame( [], $calls ); + + // The recorder has to be shown to work. Without this, a filter that never attached — a + // mistyped hook name, an add_filter() that ran too late — leaves the array empty for a + // reason that has nothing to do with the sub-plugin being disabled, and the assertion above + // passes having proved nothing at all. + apply_filters( 'give/plugin_absorber/should_load', true ); + + $this->assertSame( [ true ], $calls, 'The recorder must catch a call that really happened.' ); + } + + public function test_it_loads_every_registered_sub_plugin(): void { + $this->register( [ 'slug' => 'give-recurring' ] ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + Loader::load_all(); + + $this->assertSame( 2, $GLOBALS['absorber_loads'] ); + } + + /** + * One sub-plugin failing its checks must not stop the rest, or the failure order would decide + * which plugins a site gets. + */ + public function test_a_skipped_sub_plugin_does_not_stop_the_others(): void { + $this->register( [ 'slug' => 'give-recurring', 'enabled' => false ] ); + $this->register( [ 'slug' => 'give-fee-recovery' ] ); + + Loader::load_all(); + + $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + } + + /** + * Registrar_Interface::all() can only declare `array`, so a host implementation is free to + * return anything. The default Registrar cannot produce this state — only a bound one can. + */ + public function test_load_all_ignores_entries_that_are_not_sub_plugins(): void { + $constant = 'ABSORBER_MIXED_REGISTRY_GUARD'; + $path = $this->make_fixture( $constant ); + $sub_plugin = new Sub_Plugin( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + $registrar = new class( $sub_plugin ) implements Registrar_Interface { + /** + * @var array + */ + private $entries; + + public function __construct( Sub_Plugin $sub_plugin ) { + $this->entries = [ + 'junk' => 'not-a-sub-plugin', + 'nope' => 42, + 'real' => $sub_plugin, + ]; + } + + public function register( Sub_Plugin $sub_plugin ): void { + } + + public function all(): array { + // The interface can only declare `array`, and its docblock is a promise a host is + // free to break — which is exactly what this double is here to do. Restating the + // promised shape is what lets the analyser check every other implementation + // strictly while this one hands back the junk the Loader has to survive. + /** @var array $entries */ + $entries = $this->entries; + + return $entries; + } + }; + + $container = new Test_Container(); + $container->singleton( Registrar_Interface::class, static fn() => $registrar ); + Config::set_container( $container ); + + Loader::load_all(); + + $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + } + + /** + * Buffering registration is what lets the container arrive after the sub-plugins do, and the + * registry is only half of that: the load path resolves the notice queue as well. A + * collaborator pinned by an eager resolve inside register() would leave the host's binding + * bound and never used. + */ + public function test_a_container_set_after_register_reaches_the_load_path(): void { + // The harness boots WordPress before any test runs, so plugins_loaded has already fired and + // boot() would rightly report that wiring at priority 2 is too late and load inline + // instead. Rewind it so boot() takes the path a real host bootstrap takes; tearDown puts + // the count back. + $this->plugins_loaded_count = did_action( 'plugins_loaded' ); + unset( $GLOBALS['wp_actions']['plugins_loaded'] ); + + $this->register( [ 'dependency_check' => static fn() => false ] ); + + $notices = new class() implements Queue_Interface { + /** + * Slugs handed to queue_dependency_notice(), in order. + * + * @var string[] + */ + public $dependency_notices = []; + + public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { + } + + public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + } + + public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + $this->dependency_notices[] = $sub_plugin->get_slug(); + } + + public function render(): void { + } + }; + + // The registrar is bound as well as the queue. Without it the sub-plugin would sit in the + // default registrar either way and the notice would arrive however register() behaved, so + // the test would pass without the buffer existing at all. + $registrar = new Spy_Registrar(); + $container = new Test_Container(); + $container->singleton( Registrar_Interface::class, static fn() => $registrar ); + $container->singleton( Queue_Interface::class, static fn() => $notices ); + Config::set_container( $container ); + + Loader::boot(); + + $this->assertSame( + 2, + has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ), + 'boot() must have wired the load hook rather than loading inline.' + ); + + Loader::load_all(); + + $this->assertSame( + [ 'give-recurring' ], + $notices->dependency_notices, + 'A binding made after register() has to reach the load path.' + ); + $this->assertSame( + [], + $this->notice_queue(), + 'The default queue must not have been resolved alongside it.' + ); + } + + /** + * require_once dedupes by resolved path, so one file behind two registrations executes once + * even when the second one's guard constant never gets defined. + */ + public function test_one_bundled_file_behind_two_registrations_loads_once(): void { + $path = sys_get_temp_dir() . '/absorber-shared-' . uniqid( '', true ) . '.php'; + file_put_contents( + $path, + 'fixtures[] = $path; + + foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $index => $slug ) { + Loader::register( + [ + 'slug' => $slug, + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => 'ABSORBER_SHARED_FILE_GUARD_' . $index, + ] + ); + } + + Loader::load_all(); + + $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + } + + public function test_load_all_does_nothing_without_a_hook_prefix(): void { + $this->register(); + + Config_State::reset(); + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + Loader::load_all(); + + $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A bootstrap mistake must not fatal the site.' ); + } + + public function test_render_notices_delegates_to_the_bound_collaborator(): void { + $notices = new class() implements Queue_Interface { + /** + * @var int + */ + public $render_calls = 0; + + public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { + } + + public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + } + + public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + } + + public function render(): void { + ++$this->render_calls; + } + }; + + $container = new Test_Container(); + $container->singleton( Queue_Interface::class, static fn() => $notices ); + Config::set_container( $container ); + + Loader::render_notices(); + + $this->assertSame( 1, $notices->render_calls ); + } + + public function test_render_notices_does_nothing_without_a_hook_prefix(): void { + Config_State::reset(); + $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + + ob_start(); + Loader::render_notices(); + + $this->assertSame( '', (string) ob_get_clean() ); + } + + private function clear_notices(): void { + delete_site_option( 'give_plugin_absorber_notices' ); + } + + /** + * The queue is stored as a site option on every install — on single site that call falls through + * to the plain option table — so there is one place to read it from. + * + * @return array + */ + private function notice_queue(): array { + $queue = get_site_option( 'give_plugin_absorber_notices', [] ); + + return is_array( $queue ) ? $queue : []; + } + + /** + * Define a guard constant for the duration of one test, undone in tearDown. + * + * uopz is what makes this reversible: a plain define() lasts for the whole PHP process, and a + * guard left standing makes every later test read its sub-plugin as already loaded. + * + * @param string $constant Constant to define. + * + * @return string + */ + private function define_guard( string $constant ): string { + $this->constants[] = $constant; + + $this->setConstant( $constant, '1.0.0' ); + + return $constant; + } + + /** + * Write a throwaway bundled plugin that counts its own loads and defines its guard constant. + * + * A unique path per test is required: require_once caches by resolved path for the lifetime of + * the PHP process, so a shared fixture would make later tests pass without loading anything. + * + * @param string $constant Guard constant to define. + */ + private function make_fixture( string $constant ): string { + $path = sys_get_temp_dir() . '/absorber-' . uniqid( '', true ) . '.php'; + + file_put_contents( + $path, + 'fixtures[] = $path; + + return $path; + } + + /** + * @param array $overrides Config overrides. + */ + private function register( array $overrides = [], ?string $constant = null ): string { + $constant = $constant ?? 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); + $path = $this->make_fixture( $constant ); + + Loader::register( + array_merge( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ], + $overrides + ) + ); + + return $constant; + } +} From 2a4e7eff03ed5fa05067259b8ecaa2d104b9f5e2 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 11:58:53 +0200 Subject: [PATCH 2/5] Update PR body format and guidelines in CLAUDE.md and related documentation - Changed the PR body format from four parts to three, removing the 'Verify' section as it is now covered in CLAUDE.md. - Clarified the structure of the 'What' and 'Why this way' sections to enhance clarity and focus for reviewers. - Updated related documentation in plugin-absorber design and plans to reflect these changes, ensuring consistency across all documents. --- CLAUDE.md | 18 ++- .../plans/2026-07-31-plugin-absorber.md | 119 ++++++++++-------- .../2026-07-31-plugin-absorber-design.md | 17 ++- 3 files changed, 93 insertions(+), 61 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f07b852..bb58a7d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -261,19 +261,27 @@ branch exists. `main` is releasable after every merge. - **PR size cap:** ≤10 files, tests and test infrastructure excluded. No logic-bearing PR exceeds 4 source files. - **Commits: no co-author trailers, ever.** -- **PR body is exactly four parts, nothing else** — no boilerplate headings, no restating the diff, - no checklists: +- **PR body is exactly three parts, nothing else** — no boilerplate headings, no restating the diff, + no checklists, and no "Verify" section: the commands are in this file and the coverage is in the + diff, so restating them per PR is filler a reviewer learns to scroll past. ``` - What: one line. + What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. - Why this way: the trade-off taken, and against what. + Why this way: - Verify: the command, and what is deliberately not covered. + **The claim, in bold.** One or two sentences: the trade-off taken, and against what. + + **The next claim.** Same again. ``` + `What` is one line but not a narrow one — a PR that wires two hooks names both, or the second goes + unreviewed. `Why this way` is one bold-led block per decision, never a single paragraph running + several arguments together: a reviewer reads the bold leads, stops at the one they doubt, and the + rest costs them nothing. Cut the connective throat-clearing between claims, never the claims. + - The README grows section-by-section with each PR so it is never ahead of what has shipped. Target ~120 lines: if it runs materially longer, cut rather than add. - New dev-only files belong in `.gitattributes` as `export-ignore` so they stay out of consumer diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index ad93eef..56e287f 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -20,16 +20,19 @@ Every task's requirements implicitly include this section. - **Storage keys:** option `"{$option_prefix}_plugin_absorber_activations"`, option `"{$option_prefix}_plugin_absorber_notices"`. Both are assembled by `Config::get_option_name( string $name )` and nowhere else, alongside `Config::get_hook_name()` for filters. **Amended 2026-08-11:** `{$option_prefix}` is the hook prefix lowercased with hyphens folded to underscores, so `Give-Core` yields the option `give_core_plugin_absorber_notices` while still yielding the filter `Give-Core/plugin_absorber/should_load` — the prefix validator admits `A-Z` and `-`, and a hook-naming value should not reach a storage key verbatim. The two normalisations stay separate: folding case into the hook side would silently rename the host's own filters. **Amended 2026-08-03 (PR 10 review):** the notice queue was specified as a *transient* and is now an option. `set_transient()` returns before touching the database whenever an external object cache is present, so on any Redis or Memcached site the queue would live only in the cache — where a routine `wp_cache_flush()` from a deploy script or a "purge cache" button destroys it. The merge notice is raised exactly once and never re-queued, so losing it means a site owner is never told their plugin was deactivated. On multisite both this and the activation option are network options, because the resolver deactivates network-wide. - **Production dependencies:** `stellarwp/container-contract` only. `lucatume/di52` is dev-only. No other StellarWP library. - **PR size cap:** ≤10 files per PR, tests and test infrastructure excluded. No logic-bearing PR exceeds 4 source files. -- **PR body format** — exactly four parts, nothing else. No boilerplate headings, no restating the diff, no checklists: +- **PR body format** — exactly three parts, nothing else. No boilerplate headings, no restating the diff, no checklists, and no `Verify` section (dropped 2026-08-12: the commands live in `CLAUDE.md` and the coverage is in the diff, so restating them per PR is filler a reviewer learns to scroll past): ``` - What: one line. + What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. - Why this way: the trade-off taken, and against what. + Why this way: - Verify: the command, and what is deliberately not covered. + **The claim, in bold.** One or two sentences: the trade-off taken, and against what. + + **The next claim.** Same again. ``` + `Why this way` is one bold-led block per decision, never a single paragraph running several arguments together — a reviewer reads the bold leads and stops at the one they doubt. Cut the connective throat-clearing between claims, never the claims. - **Branching:** stacked. Each branch cuts from the previous branch, and merges to `main` in order. Never open PR N+1 before PR N's branch exists. - **Commits:** no co-author trailers, ever. - **Every source file** carries a file-level docblock with `@package Nexcess\PluginAbsorber` and every method a docblock with `@since 1.0.0`. This binds `src/` only. Test classes and test support classes keep the file-level docblock, but their methods do not need `@since` — the test code in this plan's own tasks is written that way deliberately (ruled 2026-07-31). @@ -1724,22 +1727,27 @@ Usage: : Conflict_Policy::DEACTIVATE; }, -Why this way: `deactivate_plugins()` receives `$network_wide` — a change from the engineering plan, -which detects network activation and then drops the flag. Without it the call silently no-ops -against a network-activated plugin, so every admin request deactivates nothing and redirects again. -That is an infinite redirect loop on multisite, and it is exactly what the plan is own E2E -criterion ("reloading the plugins page does not loop") was meant to catch. +Why this way: + +**`deactivate_plugins()` receives `$network_wide`** — a change from the engineering plan, which +detects network activation and then drops the flag. Without it the call silently no-ops against a +network-activated plugin, so every admin request deactivates nothing and redirects again: an +infinite redirect loop on multisite, and exactly what the plan is own E2E criterion ("reloading the +plugins page does not loop") was meant to catch. + +**An unknown policy is its own case, never a `default:` fallthrough.** A typo like `defered` would +otherwise land on the deactivate branch and turn off a plugin the site owner deliberately enabled. -`redirect_destination()` returns false on a plugins.php referrer so an inline update is never -interrupted, and rewrites update.php / update-core.php referrers so the user is not bounced back +**`redirect_destination()` returns false on a plugins.php referrer**, so an inline update is never +interrupted. update.php and update-core.php referrers are rewritten so the user is not bounced back into an update screen. -Known limitation, deliberate: `resolve_all()` runs on front-end requests too, matching both -reference implementations. Tracked as issue B in the spec. +**`exit` is never mocked.** The stubbed `wp_safe_redirect()` throws `TestException`, halting the +resolver exactly where production calls `exit` while leaving a failing test free to report as +failing. -Verify: `slic run unit` and `slic run unit --env multisite` — 15 tests. `exit` is never mocked: the -stubbed `wp_safe_redirect()` throws `TestException`, which halts the resolver exactly where -production calls `exit` while leaving a failing test free to report as failing.' +**Known limitation, deliberate:** `resolve_all()` runs on front-end requests too, matching both +reference implementations. Tracked as issue B in the spec.' ``` --- @@ -2092,7 +2100,8 @@ idempotent, version-gated migrations — not here. git add src/Activation.php src/Contracts/Activation_Interface.php src/Loader.php tests/unit/ActivationTest.php tests/unit/LoaderLoadTest.php README.md git commit -m "Add run-once activation tracking" git push -u origin 13-activation -gh pr create --base 12-conflict-resolver --title "Activation" --body 'What: run-once-ever activation tracking, wired into the load path. +gh pr create --base 12-conflict-resolver --title "Activation" --body 'What: `Activation` and `Activation_Interface`, reachable as `Loader::activation()`, wired into the +load path as the last step after a successful require. Usage: @@ -2100,19 +2109,20 @@ Usage: \Give\Recurring\Install::create_tables(); }, -Why this way: `register_activation_hook()` never fires for a `require_once`d file, so a plugin -absorbed into a host would never run its original install routine. One option holds a per-slug -flag, and the callback fires after a successful require — never when the load was skipped, which +Why this way: + +**`register_activation_hook()` never fires for a `require_once`d file**, so a plugin absorbed into +a host would never run its original install routine. One option holds a per-slug flag instead. + +**The callback fires only after a successful require** — never when the load was skipped, which would otherwise create tables for code that is not loaded. -A single option rather than one per slug keeps this to one autoloaded row no matter how many +**A single option rather than one per slug** keeps this to one autoloaded row no matter how many sub-plugins a host bundles. -Known limitation, deliberate: read-then-write is not atomic, so two simultaneous first requests can -both run the callback. Tracked as issue E in the spec; `add_option()` as a claim would close it. - -Verify: `slic run unit` — 11 tests, including a corrupted-option recovery and hook-prefix -namespacing.' +**Known limitation, deliberate:** read-then-write is not atomic, so two simultaneous first requests +can both run the callback. Tracked as issue E in the spec; `add_option()` as a claim would close +it.' ``` --- @@ -2467,29 +2477,28 @@ Requires WordPress 6.4+ for the `wp_admin_notice_markup` filter. git add src/Notices/Queue.php src/Notices/Contracts/Queue_Interface.php src/Loader.php tests/unit/Notices/QueueActivationErrorTest.php README.md git commit -m "Replace WordPress fatal-activation text for absorbed standalones" git push -u origin 14-activation-error-notice -gh pr create --base 13-activation --title "Activation-error rewrite" --body 'What: replaces WordPress generic "triggered a fatal error" notice with the sub-plugin own -explanation when a user re-activates an absorbed standalone. +gh pr create --base 13-activation --title "Activation-error rewrite" --body 'What: the `wp_admin_notice_markup` filter and its Loader trampoline, replacing WordPress generic +"triggered a fatal error" notice with the sub-plugin own explanation when a user re-activates an +absorbed standalone. Usage: "conflict_notice_message" => static fn() => __( "Now bundled with Give.", "give" ), -Why this way: a change from the engineering plan, which specified `ob_start()` on -`admin_head-plugins.php` copied from Kadence. The newer LearnDash reference uses the -`wp_admin_notice_markup` filter — same nonce check, same str_replace, but no output buffering, no -risk of mangling unrelated admin output, and directly unit-testable. The cost is a WordPress 6.4 -floor, which is when that filter landed. +Why this way: -Three gates before touching anything: the plugins screen, a `plugin` parameter matching a +**The `wp_admin_notice_markup` filter, not `ob_start()`.** The engineering plan specified output +buffering on `admin_head-plugins.php`, copied from Kadence. The filter does the same nonce check +and the same str_replace with no buffering, no risk of mangling unrelated admin output, and is +directly unit-testable. The cost is the WordPress 6.4 floor, which is when the filter landed. + +**Three gates before touching the markup:** the plugins screen, a `plugin` parameter matching a registered standalone basename, and a valid `plugin-activation-error_{basename}` nonce. Failing any one returns the markup untouched, as does having no configured message — better WordPress wording than none. -This adds a method to `Notices\Contracts\Queue_Interface`, which shipped in PR 10. Pre-1.0 with no -consumers. - -Verify: `slic run unit` — 8 tests, one per gate plus `wp_kses_post()` sanitising and the Loader -trampoline.' +**This adds a method to `Notices\Contracts\Queue_Interface`,** which shipped in PR 10. Pre-1.0 with +no consumers.' ``` --- @@ -2931,20 +2940,23 @@ gh pr create --base 14-activation-error-notice --title "End-to-end suite" --body Usage: `tests/_data/plugins/absorber-host/absorber-host.php` is the worked consumer example — register, set a policy, supply an activation callback, boot. -Why this way: these drive the real `active_plugins` option and let `deactivate_plugins()` actually -run, rather than stubbing it as the unit tests do. Only `wp_safe_redirect` and `wp_get_referer` are -stubbed; the redirect throws `TestException` so the request halts where production calls `exit`, -without mocking `exit` itself. That makes this a genuine integration check of the load guard, -the three policies, and the run-once activation working together. +Why this way: + +**These drive real WordPress state.** The real `active_plugins` option, a real +`deactivate_plugins()` call, rather than the stubs the unit tests use — a genuine integration check +of the load guard, the three policies, and the run-once activation working together. -Bundled fixtures are generated per test rather than committed: `require_once` caches by resolved +**Only `wp_safe_redirect` and `wp_get_referer` are stubbed.** The redirect throws `TestException`, +so the request halts where production calls `exit` without mocking `exit` itself. + +**Bundled fixtures are generated per test, never committed.** `require_once` caches by resolved path for the whole PHP process, so a committed bundled file would execute once for the entire suite and every later test would pass without loading anything. The generator moved into a shared `WithBundledPlugins` trait — the on-disk counterpart to `WithSubPlugins` from PR 7 — and `LoaderLoadTest` now uses it too. -Verify: `slic run unit` and `slic run unit --env multisite` — 9 end-to-end tests. Not covered: real -HTTP requests and a real browser; the redirect is asserted as a call, not followed.' +**Out of scope:** real HTTP requests and a real browser. The redirect is asserted as a call, not +followed.' ``` --- @@ -3067,12 +3079,15 @@ gh pr create --base 15-e2e-fixtures --title "README pass and 1.0.0" --body 'What Usage: see the complete worked example at the end of the README. -Why this way: the README was built up section by section as each PR landed, so it never drifted -from what actually shipped. This pass removes the duplication that approach leaves behind and -checks the ordering reads for a newcomer rather than in merge order. +Why this way: + +**The README was built up section by section as each PR landed,** so it never drifted from what +actually shipped. This pass removes the duplication that approach leaves behind and checks the +ordering reads for a newcomer rather than in merge order. -Verify: `git archive --format=tar HEAD | tar -t` shows only src/, composer.json, LICENSE, README, -CHANGELOG — no tests, docs, or CI config in a consumer install. Both suites and PHPStan green.' +**`.gitattributes` keeps the dev files out of a consumer install.** `git archive` yields `src/`, +`composer.json`, `LICENSE`, `README.md` and `CHANGELOG.md` — no tests, docs, or CI config reaching +a consumer vendor directory.' ``` - [ ] **Step 9: Merge the stack and tag** diff --git a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md index c6dcc74..5cc9b9a 100644 --- a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md +++ b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md @@ -327,20 +327,29 @@ a method signature already visible in the table. ## 7. PR body template -Four parts, nothing else: +Three parts, nothing else: ``` -What: one line. +What: one line, naming every hook or entry point the PR wires. Usage: the snippet this PR makes possible. -Why this way: the trade-off taken, and against what. +Why this way: -Verify: the command, and what is deliberately not covered. +**The claim, in bold.** One or two sentences: the trade-off taken, and against what. + +**The next claim.** Same again. ``` No boilerplate headings, no restating the diff, no checklists. +There is no `Verify` section (dropped 2026-08-12). The test commands live in `CLAUDE.md` and the +coverage is in the diff; repeating both in every PR is filler a reviewer learns to scroll past. + +`Why this way` is one bold-led block per decision, never a single paragraph running several +arguments together — a reviewer reads the bold leads and stops at the one they doubt. Cut the +connective throat-clearing between claims, never the claims. + --- ## 8. Verification From 7c1bf42d48f749062332cce33ed8eec09970fccf Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 12:44:29 +0200 Subject: [PATCH 3/5] Move collaborator resolution off the Loader Resolution shared nothing with the facade -- no hooks, no registration buffer, no load loop -- and being private static on it meant a collaborator could only reach a peer by depending on the whole Loader. Container\Resolution now owns the memo and the engine; Loader::registrar() and notices() stay as one-line delegations, so the public surface is unchanged and only what a collaborator depends on has moved. boot() stated the plugins_loaded order twice, once as hook priorities and once as inline calls under a comment promising they matched. SEQUENCE states it once and both paths iterate it, which also lets the suite's state helper unwire exactly what boot() wired rather than restating the priority. Loader::all() now narrows to Sub_Plugin itself. A host may bind a registrar returning anything and PHP 7.4 cannot say array in the signature, so the guard belongs where the untrusted value enters rather than copied into every consumer. Loader is final: every member is private static and every internal call is self::, so a subclass could override nothing and change nothing silently. --- CLAUDE.md | 31 +++- .../2026-07-31-plugin-absorber-design.md | 12 +- src/Container/Resolution.php | 138 +++++++++++++++++ src/Loader.php | 139 ++++++------------ tests/_support/Loader_State.php | 37 +++-- tests/_support/Resolution_State.php | 61 ++++++++ 6 files changed, 296 insertions(+), 122 deletions(-) create mode 100644 src/Container/Resolution.php create mode 100644 tests/_support/Resolution_State.php diff --git a/CLAUDE.md b/CLAUDE.md index bb58a7d..88fafd0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,8 +55,10 @@ declarations). ### Public surface A two-class static facade — `Config` (hook prefix + optional container) and `Loader` -(resolve/register/boot/load) — matching the shape of `stellarwp/assets` and -`stellarwp/admin-notices`. Everything else is an implementation detail behind it. +(register/boot/load, plus accessors) — matching the shape of `stellarwp/assets` and +`stellarwp/admin-notices`. Everything else is an implementation detail behind it. `Loader` is +`final`: every member is private static and every internal call is `self::`, so a subclass could +override nothing and would silently change nothing. ### Collaborators @@ -73,11 +75,17 @@ An interface belonging to a folder-scoped concern lives in that folder's `Contra implementation and not in the top-level `src/Contracts/`. `src/Contracts/` is for the interfaces whose implementations sit at the root — `Registrar`, `Plugin_State`, `Activation`. -All four come through one generic helper — `Loader::resolve( string $interface, string +All four come through one generic helper — `Container\Resolution::get( string $interface, string $default_class ): object` — which returns the container binding when `$container->has()`, otherwise `new $default_class()`, memoized either way. Collaborators reach each other through the accessors -(`Loader::registrar()`, `resolver()`, `notices()`, `activation()`), so rebinding one in the host's -container flows everywhere. +(`Resolution::registrar()`, `notices()`, …), so rebinding one in the host's container flows +everywhere. + +**Resolution is its own class, not a private static on `Loader`.** It shares nothing with the +facade — no hooks, no registration buffer, no load loop — and while it lived there it was the only +way for a collaborator to reach a peer, so every collaborator had to depend on the whole facade to +get at one interface. `Loader::registrar()` and `Loader::notices()` remain as one-line delegations, +so the public surface a host sees is unchanged; what changed is what a *collaborator* depends on. The container is **never** used to wire hooks. Hooks stay plain static trampolines, which is what keeps the container genuinely optional. @@ -116,10 +124,19 @@ all_admin_notices → Loader::render_notices() [is_admin() wp_admin_notice_markup → Loader::filter_activation_error_markup() [is_admin() only] ``` -`load_all()` gates each sub-plugin in order, skipping on the first failure: enabled → dependencies -met → not already loaded → file exists → `should_load` filter → `require_once` → activation +`load_all()` gates each sub-plugin in order, skipping on the first failure: enabled → not already +loaded → dependencies met → file exists → `should_load` filter → `require_once` → activation callback (only after a *successful* require). +The guard constant is checked **before** the dependency check, not after. It is one `defined()`, it +carries the whole re-declaration guarantee, and it is the only gate meaning "this plugin is already +running" — warning that requirements are unmet for a plugin the admin can watch working would send +them after the wrong problem. `docs/filters.md` and the spec agree. + +`Loader::all()` narrows to `Sub_Plugin` instances itself, so no caller repeats that guard. A host +may bind a registrar returning anything, and PHP 7.4 cannot express `array` in +the interface signature — so it is filtered once where the untrusted value enters. + `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and `DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. `redirect_destination()` returns `false` when the referrer is already `plugins.php`, so an inline diff --git a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md index 5cc9b9a..c3bd7d2 100644 --- a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md +++ b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md @@ -146,9 +146,9 @@ Hooks wired by boot(): load_all() per sub-plugin, in order: is_enabled()? skip if false - are_dependencies_met()? skip + queue dependency notice is_already_loaded()? skip — constant defined, avoids re-declaration fatal - file_exists( bundled_plugin_file )? skip + are_dependencies_met()? skip + queue dependency notice + is_file() && is_readable()? skip + _doing_it_wrong() …/plugin_absorber/should_load? skip if false require_once bundled_plugin_file activation()->maybe_run( $sub_plugin ) @@ -156,8 +156,8 @@ load_all() per sub-plugin, in order: ### Collaborators -Each is interface-backed and container-resolvable through one `Loader::resolve( $interface, -$default_class )` helper: container binding if present, otherwise `new $default_class()`, memoized +Each is interface-backed and container-resolvable through one `Container\Resolution::get( +$interface, $default_class )` helper: container binding if present, otherwise `new $default_class()`, memoized either way. The container is optional and is **not** used to wire hooks — those stay plain static trampolines, per the `admin-notices` precedent. @@ -222,7 +222,7 @@ Dependency-forced, not arbitrary: - `Loader::load()` calls `notices()->queue_dependency_notice()`, and `Conflict\Resolver` calls `Loader::notices()`. Notices (10) therefore precedes the load path (11) and the Resolver (12). - Each `Contracts\*` interface ships **with** its default implementation and its tests, rather - than all four landing up front. `Loader::resolve()` is generic — `(interface, default_class)` — + than all four landing up front. `Container\Resolution::get()` is generic — `(interface, default_class)` — so it never needs the other interfaces to exist; only the accessors do, and each accessor lands with its pair. - `Conflict_Policy` (6) is split from `Sub_Plugin` (7) so PR 7 is purely predicate logic, which is @@ -266,7 +266,7 @@ message. This is documented in `tests/README.md`. rejecting a string of any kind. - **8 — `Registrar`.** register; `all()`; a duplicate slug throws `Config_Exception` naming both bundled files and leaves the registry untouched; `reset()` clears the guard with the registry. -- **9 — `Loader::resolve()`.** No container → default instance; di52 container binding a custom +- **9 — `Container\Resolution::get()`.** No container → default instance; di52 container binding a custom `Registrar_Interface` → bound instance returned; memoized (identical instance twice); `Loader::reset()` clears both the memo and the registry. - **10 — `Notices\Queue`.** Each queue method writes the option; the same slug and type queued twice diff --git a/src/Container/Resolution.php b/src/Container/Resolution.php new file mode 100644 index 0000000..1a60e5b --- /dev/null +++ b/src/Container/Resolution.php @@ -0,0 +1,138 @@ + + */ + private static $resolved = []; + + /** + * @since 1.0.0 + * + * @throws Config_Exception When the container cannot produce a usable instance. + * + * @return Registrar_Interface + */ + public static function registrar(): Registrar_Interface { + return self::get( Registrar_Interface::class, Registrar::class ); + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When the container cannot produce a usable instance. + * + * @return Queue_Interface + */ + public static function notices(): Queue_Interface { + return self::get( Queue_Interface::class, Queue::class ); + } + + /** + * Resolve an interface from the container when bound, else construct the default. + * + * The container is never required — with none set, every collaborator is a plain `new`, so + * every default class must be constructible with no arguments. Resolution is memoized, and + * nothing resolves until the first read, which is boot: that is what lets a host set its + * container at any point beforehand. Swapping a collaborator after that would be the worse + * behaviour, since anything already holding the old instance would keep it. + * + * Generic rather than one method per collaborator, so each accessor can land with the PR that + * needs it without this one being rewritten each time. + * + * @since 1.0.0 + * + * @template T of object + * + * @param class-string $interface Interface to resolve. + * @param class-string $default_class Concrete class to build when nothing is bound. + * + * @throws Config_Exception When the container throws while building the binding, or returns + * something that does not implement the interface it was asked for. + * + * @return T + */ + public static function get( string $interface, string $default_class ): object { + if ( isset( self::$resolved[ $interface ] ) ) { + // The map holds a different type per key, so it cannot be typed as T as a whole. The + // entry found here still implements $interface: a container binding is only memoized + // once it passes the instanceof below, and the fallback is built from the class-string. + /** @var T $memoized */ + $memoized = self::$resolved[ $interface ]; + + return $memoized; + } + + $container = Config::get_container(); + + if ( $container !== null && $container->has( $interface ) ) { + // has() true only promises the binding exists, not that it can be built: a host factory + // closure is free to throw, and a container asked for a class with an unsatisfiable + // dependency throws its own exception type. Uncaught, either one leaves the host's + // plugins_loaded with a fatal from a vendor namespace that names neither this library + // nor the binding at fault, so both are reported the same way as a binding of the wrong + // type. The original is kept as the previous exception; nothing is memoized. + try { + $instance = $container->get( $interface ); + } catch ( Throwable $thrown ) { + throw new Config_Exception( + sprintf( + 'The container failed to build the binding for %s: %s', + $interface, + $thrown->getMessage() + ), + 0, + $thrown + ); + } + + // Checked before it is memoized. Without this the bad instance is cached, and every + // accessor throws a TypeError blaming this library rather than the binding. + if ( ! $instance instanceof $interface ) { + throw new Config_Exception( + sprintf( + 'The container binding for %s must implement it. Got %s.', + $interface, + is_object( $instance ) ? get_class( $instance ) : gettype( $instance ) + ) + ); + } + + self::$resolved[ $interface ] = $instance; + + return $instance; + } + + self::$resolved[ $interface ] = new $default_class(); + + return self::$resolved[ $interface ]; + } +} diff --git a/src/Loader.php b/src/Loader.php index 7f1509d..03943aa 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -5,19 +5,25 @@ namespace Nexcess\PluginAbsorber; +use Nexcess\PluginAbsorber\Container\Resolution; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; -use Nexcess\PluginAbsorber\Notices\Queue; -use Throwable; use WP_Hook; /** - * Static facade: collaborator resolution, registration, hook wiring, and the load loop. + * Static facade: registration, hook wiring, and the load loop. + * + * Resolving an interface to its implementation lives in `Container\Resolution`; the accessors here + * are one-line delegations, so a host calls `Loader::notices()` and never has to know that. + * + * `final` because it cannot usefully be extended: every member is private static and every internal + * call is `self::`, so a subclass would inherit the API, be unable to override any of it, and change + * nothing — which is the silent no-op this class reports on everywhere else. * * @since 1.0.0 */ -class Loader { +final class Loader { /** * plugins_loaded priority the load loop runs at. * @@ -31,11 +37,20 @@ class Loader { private const LOAD_PRIORITY = 2; /** - * Resolved collaborators, memoized by interface name. + * The plugins_loaded steps, in run order, as [ method, priority ] pairs. + * + * Stated once because boot() 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. + * + * @since 1.0.0 * - * @var array + * @var array */ - private static $resolved = []; + private const SEQUENCE = [ + 'load_all' => self::LOAD_PRIORITY, + ]; /** * Sub-plugins registered but not yet handed to the registrar. @@ -59,7 +74,7 @@ class Loader { * @return Registrar_Interface */ public static function registrar(): Registrar_Interface { - return self::resolve( Registrar_Interface::class, Registrar::class ); + return Resolution::registrar(); } /** @@ -70,7 +85,7 @@ public static function registrar(): Registrar_Interface { * @return Queue_Interface */ public static function notices(): Queue_Interface { - return self::resolve( Queue_Interface::class, Queue::class ); + return Resolution::notices(); } /** @@ -110,7 +125,18 @@ public static function register( array $config ): void { public static function all(): array { self::flush(); - return self::registrar()->all(); + // Registrar_Interface::all() can only declare `array` — PHP 7.4 has no way to say + // array in a signature — so a host binding its own registrar may return + // anything at all. Narrowed once here, where the untrusted value crosses into the library, + // rather than at each call site: a consumer that forgot the check would fatal inside + // plugins_loaded on its first predicate call, which is the exact failure this library + // exists to prevent, and every future consumer would have to remember it too. + return array_filter( + self::registrar()->all(), + static function ( $sub_plugin ): bool { + return $sub_plugin instanceof Sub_Plugin; + } + ); } /** @@ -150,12 +176,17 @@ public static function boot(): void { '1.0.0' ); - self::load_all(); + // In the order the hooks would have run them. + foreach ( array_keys( self::SEQUENCE ) as $method ) { + call_user_func( [ self::class, $method ] ); + } return; } - add_action( 'plugins_loaded', [ self::class, 'load_all' ], self::LOAD_PRIORITY ); + foreach ( self::SEQUENCE as $method => $priority ) { + add_action( 'plugins_loaded', [ self::class, $method ], $priority ); + } } /** @@ -172,13 +203,6 @@ public static function load_all(): void { } foreach ( self::all() as $sub_plugin ) { - // Registrar_Interface::all() only declares `array`. A host binding its own registrar - // that returns anything else would otherwise fatal inside plugins_loaded on the first - // predicate call -- the exact failure this library exists to prevent. - if ( ! $sub_plugin instanceof Sub_Plugin ) { - continue; - } - self::load( $sub_plugin ); } } @@ -230,83 +254,6 @@ private static function flush(): void { } } - /** - * Resolve an interface from the container when bound, else construct the default. - * - * The container is never required — with none set, every collaborator is a plain `new`, so - * every default class must be constructible with no arguments. Resolution is memoized, and - * nothing resolves until the first read, which is boot: that is what lets a host set its - * container at any point beforehand. Swapping a collaborator after that would be the worse - * behaviour, since anything already holding the old instance would keep it. - * - * @since 1.0.0 - * - * @template T of object - * - * @param class-string $interface Interface to resolve. - * @param class-string $default_class Concrete class to build when nothing is bound. - * - * @throws Config_Exception When the container throws while building the binding, or returns - * something that does not implement the interface it was asked for. - * - * @return T - */ - private static function resolve( string $interface, string $default_class ): object { - if ( isset( self::$resolved[ $interface ] ) ) { - // The map holds a different type per key, so it cannot be typed as T as a whole. The - // entry found here still implements $interface: a container binding is only memoized - // once it passes the instanceof below, and the fallback is built from the class-string. - /** @var T $memoized */ - $memoized = self::$resolved[ $interface ]; - - return $memoized; - } - - $container = Config::get_container(); - - if ( $container !== null && $container->has( $interface ) ) { - // has() true only promises the binding exists, not that it can be built: a host factory - // closure is free to throw, and a container asked for a class with an unsatisfiable - // dependency throws its own exception type. Uncaught, either one leaves the host's - // plugins_loaded with a fatal from a vendor namespace that names neither this library - // nor the binding at fault, so both are reported the same way as a binding of the wrong - // type. The original is kept as the previous exception; nothing is memoized. - try { - $instance = $container->get( $interface ); - } catch ( Throwable $thrown ) { - throw new Config_Exception( - sprintf( - 'The container failed to build the binding for %s: %s', - $interface, - $thrown->getMessage() - ), - 0, - $thrown - ); - } - - // Checked before it is memoized. Without this the bad instance is cached, and every - // accessor throws a TypeError blaming this library rather than the binding. - if ( ! $instance instanceof $interface ) { - throw new Config_Exception( - sprintf( - 'The container binding for %s must implement it. Got %s.', - $interface, - is_object( $instance ) ? get_class( $instance ) : gettype( $instance ) - ) - ); - } - - self::$resolved[ $interface ] = $instance; - - return $instance; - } - - self::$resolved[ $interface ] = new $default_class(); - - return self::$resolved[ $interface ]; - } - /** * Load one sub-plugin, cheapest and most decisive check first. * diff --git a/tests/_support/Loader_State.php b/tests/_support/Loader_State.php index b7163f2..65a4b44 100644 --- a/tests/_support/Loader_State.php +++ b/tests/_support/Loader_State.php @@ -33,9 +33,8 @@ class Loader_State { * @var array */ protected const DEFAULTS = [ - 'resolved' => [], - 'pending' => [], - 'booted' => false, + 'pending' => [], + 'booted' => false, ]; /** @@ -53,21 +52,33 @@ class Loader_State { public static function reset(): void { $reflection = new ReflectionClass( Loader::class ); - // Read rather than restated, so the helper cannot go on removing a hook from a priority - // the Loader no longer wires -- which would leave the real callback attached and every - // later test loading sub-plugins it never registered. - $load_priority = $reflection->getConstant( 'LOAD_PRIORITY' ); + // The memo lives on Container\Resolution now, but every test in the suite resets through + // this one call, so it is dropped from here rather than making each of them reset twice. + Resolution_State::reset(); - // Refused rather than coerced. A default priority here would take the load hook off some - // other number, leave the real callback attached, and load sub-plugins into every later - // test in the process. - if ( ! is_int( $load_priority ) ) { + // Read rather than restated, so the helper unwires exactly what boot() wired -- including + // any step the Loader grows later. Restating it would go on removing a callback from a + // priority the Loader no longer uses, leaving the real one attached and every later test + // loading sub-plugins it never registered. + $sequence = $reflection->getConstant( 'SEQUENCE' ); + + // Refused rather than coerced, for the same reason. + if ( ! is_array( $sequence ) ) { throw new LogicException( - sprintf( 'Loader::LOAD_PRIORITY must be an int for %s to unwire the load hook.', self::class ) + sprintf( 'Loader::SEQUENCE must be an array for %s to unwire the hooks boot() added.', self::class ) ); } - remove_action( 'plugins_loaded', [ Loader::class, 'load_all' ], $load_priority ); + foreach ( $sequence as $method => $priority ) { + if ( ! is_string( $method ) || ! is_int( $priority ) ) { + throw new LogicException( + sprintf( 'Loader::SEQUENCE must map method names to int priorities for %s to unwire them.', self::class ) + ); + } + + remove_action( 'plugins_loaded', [ Loader::class, $method ], $priority ); + } + remove_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { diff --git a/tests/_support/Resolution_State.php b/tests/_support/Resolution_State.php new file mode 100644 index 0000000..7c915db --- /dev/null +++ b/tests/_support/Resolution_State.php @@ -0,0 +1,61 @@ + + */ + protected const DEFAULTS = [ + 'resolved' => [], + ]; + + /** + * Return every static property of `Resolution` to its default. + * + * @throws LogicException When `Resolution` has grown a static property this helper does not know + * about, rather than leaving it to leak between tests. + * + * @return void + */ + public static function reset(): void { + $reflection = new ReflectionClass( Resolution::class ); + + foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { + $name = $property->getName(); + + if ( ! array_key_exists( $name, self::DEFAULTS ) ) { + throw new LogicException( + sprintf( 'Resolution::$%s has no default in %s. Add one.', $name, self::class ) + ); + } + + $property->setAccessible( true ); + $property->setValue( null, self::DEFAULTS[ $name ] ); + } + } +} From c7760efe20ee1372cea3631041125b2eb9fc8ed0 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 14:57:50 +0200 Subject: [PATCH 4/5] Require a container and split boot timing off the Loader The container was optional, and that single choice is what forced everything around it: Container\Resolution existed to answer "binding when bound, new $default_class otherwise", which required every default to be constructible with no arguments, which is what put ?Peer $peer = null on the collaborators. It is also the outlier -- across 19 vendored StellarWP packages, 10 have no container at all and 7 require one and throw; only admin-notices does what we were doing. Every host that will absorb a plugin already hands a contract container to Telemetry, Validation or Harbor. So Config::get_container() throws, Resolution is deleted, and Provider teaches the host's container to build every collaborator. It binds only when the host has not, so a host that bound first keeps what it bound, and it binds the container under its own contract because di52 answers has() true for any existing class -- without that line a container which autowires cannot build the two collaborators that take one. Boot\Scheduler takes over when the work happens and Load\Runner takes over the load pass, leaving Loader with registration and the public surface. The pending buffer stays: registration still resolves nothing, which is what lets a host register before it sets a container, and LearnDash builds a container lazily and replaces it at plugins_loaded 0, so anything resolved earlier holds an orphan. Plugin_State becomes Plugin_Checker and Plugin_Deactivator. It took a basename and mutated, so it was never the fact its name claimed, and a host correcting detection -- LearnDash filters option_active_plugins to inject a synthetic path -- can now rebind that half alone. --- CLAUDE.md | 178 +++++-- README.md | 27 +- docs/configuration.md | 41 +- docs/conflict-handling.md | 11 + docs/filters.md | 5 + docs/notices.md | 30 +- .../2026-08-12-container-required-rework.md | 305 ++++++++++++ src/Boot/Scheduler.php | 153 ++++++ src/Config.php | 19 +- src/Container/Resolution.php | 138 ------ src/Contracts/Plugin_Checker_Interface.php | 37 ++ .../Plugin_Deactivator_Interface.php | 32 ++ src/Contracts/Plugin_State_Interface.php | 47 -- src/Contracts/Provider_Interface.php | 29 ++ src/Load/Runner.php | 133 +++++ src/Loader.php | 275 +++-------- src/Notices/Queue.php | 15 +- src/Plugin_Checker.php | 34 ++ ...lugin_State.php => Plugin_Deactivator.php} | 43 +- src/Provider.php | 119 +++++ src/Sub_Plugin.php | 2 +- src/Traits/Guards_Hook_Prefix.php | 41 ++ src/Traits/Loads_Plugin_Functions.php | 35 ++ tests/README.md | 141 ++++-- tests/_support/Loader_State.php | 150 ++++-- tests/_support/Resolution_State.php | 61 --- tests/_support/Spy_Queue.php | 85 ++++ tests/_support/Traits/WithBundledPlugins.php | 123 +++++ tests/_support/Traits/WithContainer.php | 122 +++++ tests/_support/Traits/WithHaltedRedirects.php | 89 ++++ tests/_support/Traits/WithIncorrectUsage.php | 105 ++++ tests/unit/Boot/SchedulerTest.php | 421 ++++++++++++++++ tests/unit/ConfigTest.php | 36 +- .../RunnerTest.php} | 457 +++++++++--------- tests/unit/LoaderBootTest.php | 271 ----------- tests/unit/LoaderResolveTest.php | 403 --------------- tests/unit/LoaderTest.php | 402 +++++++++++++++ tests/unit/Notices/QueueTest.php | 72 +-- tests/unit/PluginCheckerTest.php | 106 ++++ tests/unit/PluginDeactivatorTest.php | 62 +++ tests/unit/PluginStateTest.php | 128 ----- tests/unit/ProviderTest.php | 172 +++++++ tests/unit/SmokeTest.php | 26 + 43 files changed, 3478 insertions(+), 1703 deletions(-) create mode 100644 docs/superpowers/plans/2026-08-12-container-required-rework.md create mode 100644 src/Boot/Scheduler.php delete mode 100644 src/Container/Resolution.php create mode 100644 src/Contracts/Plugin_Checker_Interface.php create mode 100644 src/Contracts/Plugin_Deactivator_Interface.php delete mode 100644 src/Contracts/Plugin_State_Interface.php create mode 100644 src/Contracts/Provider_Interface.php create mode 100644 src/Load/Runner.php create mode 100644 src/Plugin_Checker.php rename src/{Plugin_State.php => Plugin_Deactivator.php} (50%) create mode 100644 src/Provider.php create mode 100644 src/Traits/Guards_Hook_Prefix.php create mode 100644 src/Traits/Loads_Plugin_Functions.php delete mode 100644 tests/_support/Resolution_State.php create mode 100644 tests/_support/Spy_Queue.php create mode 100644 tests/_support/Traits/WithBundledPlugins.php create mode 100644 tests/_support/Traits/WithContainer.php create mode 100644 tests/_support/Traits/WithHaltedRedirects.php create mode 100644 tests/_support/Traits/WithIncorrectUsage.php create mode 100644 tests/unit/Boot/SchedulerTest.php rename tests/unit/{LoaderLoadTest.php => Load/RunnerTest.php} (53%) delete mode 100644 tests/unit/LoaderBootTest.php delete mode 100644 tests/unit/LoaderResolveTest.php create mode 100644 tests/unit/LoaderTest.php create mode 100644 tests/unit/PluginCheckerTest.php create mode 100644 tests/unit/PluginDeactivatorTest.php delete mode 100644 tests/unit/PluginStateTest.php create mode 100644 tests/unit/ProviderTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 88fafd0..1143f48 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,76 +54,129 @@ declarations). ### Public surface -A two-class static facade — `Config` (hook prefix + optional container) and `Loader` -(register/boot/load, plus accessors) — matching the shape of `stellarwp/assets` and -`stellarwp/admin-notices`. Everything else is an implementation detail behind it. `Loader` is +A two-class static facade — `Config` (hook prefix + container) and `Loader` (register/boot, plus +accessors) — matching the shape of `stellarwp/assets` and `stellarwp/admin-notices`. Everything +else is an implementation detail behind it. `Loader` is `final`: every member is private static and every internal call is `self::`, so a subclass could override nothing and would silently change nothing. ### Collaborators -Four interface-backed collaborators, each with a default implementation: +Every collaborator is constructed by the container and injected. The interface-backed ones are the +seams a host may rebind: -| Interface | Default | Responsibility | +| Interface | Bound to | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | | `Notices\Contracts\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | | `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | standalone detection, deactivation, redirect | -| `Contracts\Activation_Interface` | `Activation` | run-once activation-callback tracking | +| `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | deactivates the standalone, network-aware | +| `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | answers whether a plugin is active | +| `Contracts\Activator_Interface` | `Activator` | run-once activation-callback tracking | + +The rest — `Boot\Scheduler`, `Load\Runner`, `Conflict\Gatekeeper`, `Conflict\Redirector`, +`Notices\Store`, `Notices\Renderer` — are bound as concrete classes. A host that wants one of them +different rebinds the class name; there is no interface because nothing in the library dispatches on +one. An interface belonging to a folder-scoped concern lives in that folder's `Contracts\`, not beside its implementation and not in the top-level `src/Contracts/`. `src/Contracts/` is for the interfaces whose -implementations sit at the root — `Registrar`, `Plugin_State`, `Activation`. - -All four come through one generic helper — `Container\Resolution::get( string $interface, string -$default_class ): object` — which returns the container binding when `$container->has()`, otherwise -`new $default_class()`, memoized either way. Collaborators reach each other through the accessors -(`Resolution::registrar()`, `notices()`, …), so rebinding one in the host's container flows -everywhere. - -**Resolution is its own class, not a private static on `Loader`.** It shares nothing with the -facade — no hooks, no registration buffer, no load loop — and while it lived there it was the only -way for a collaborator to reach a peer, so every collaborator had to depend on the whole facade to -get at one interface. `Loader::registrar()` and `Loader::notices()` remain as one-line delegations, -so the public surface a host sees is unchanged; what changed is what a *collaborator* depends on. - -The container is **never** used to wire hooks. Hooks stay plain static trampolines, which is what -keeps the container genuinely optional. - -`Sub_Plugin` is a value object holding the per-sub-plugin predicates that its *configuration alone* -decides (`is_enabled()`, `is_already_loaded()`, `has_standalone_plugin()`, `get_conflict_policy()`, -…). Questions about the site rather than the config — whether the standalone is actually active — -belong to `Plugin_State_Interface`; `Sub_Plugin` only names the plugin to ask about. Keeping the -config predicates there is what lets collaborators stay thin and lets them be tested without hooks. +implementations sit at the root — `Registrar`, `Plugin_Deactivator`, `Plugin_Checker`, `Activator` — +plus `Provider_Interface`. + +**The container is required.** `Config::get_container()` throws `Config_Exception` when unset, which +is what `uplink`, `telemetry`, `schema` and `harbor` all do; `has_container()` stays as the probe. +Optional was the outlier — of nineteen vendored StellarWP packages exactly one falls back to `new`, +and we had modelled ourselves on it. One requirement is what that outlier costs. "Container binding +when bound, `new $default_class` otherwise" forces every default class to be constructible with no +arguments, which forces `?Peer $peer = null` constructor parameters, which forces a `protected` +accessor per peer falling back to a static. `Container\Resolution` was the class holding that chain +together, and the chain is deleted with it. The hosts qualify: `learndash-core` ships a container +implementing this very contract, exposes it as `App::container()`, and already hands it to Telemetry, +Validation and Harbor. The plugins with no container are the add-ons being absorbed, not the hosts +doing the absorbing. + +**One provider, one method.** `Provider::register()` performs every binding, behind a +`Contracts\Provider_Interface` declaring `register(): void` and nothing else — Harbor's shape, not +`stellarwp/foundation`'s. Foundation floors at PHP 8.3 against our 7.4, hard-depends on +`lucatume/di52` against our container-contract-only rule, and its `deferred`/`provides()` API is a +fatal `TypeError` in practice, because di52 type-hints its own `ServiceProvider` base class which +Foundation's abstract does not extend. + +**`Provider` never overwrites a binding.** It binds only what the container does not already have, so +a host that bound its own implementation wins, and the order in which the host calls +`set_container()` and `boot()` stops deciding which implementation it gets. Everything is a +singleton: each binding is either a registry whose contents are the point — a second `Registrar` +would hold a second, emptier list — or a stateless worker with nothing to gain from a second copy. + +The container is **never** used to wire hooks, and the reason is no longer that it is optional. +`Boot\Scheduler` wires callbacks that resolve *inside* the callback — a closure over the container, +or a static trampoline reading `Loader::notices()` — so wiring instantiates nothing, a host may +rebind right up until the hook fires, and a request that reaches none of them builds none of them. +`$container->callback()` reads better and is what the hand-rolled copies in `learndash-core` use, but +it is di52-only: `stellarwp/container-contract` declares `bind`, `get`, `has` and `singleton`, and +nothing else. `[ $resolved_object, 'method' ]` is the other wrong answer — it forces every +collaborator to be built at boot. + +`Loader` keeps the public surface. `registrar()` and `notices()` are one-line delegations to +`$container->get()`, so what a host calls is unchanged; what changed is that a *collaborator* now +depends on the peer it was handed rather than on the facade. + +`Sub_Plugin` is a value object answering the per-sub-plugin questions it can answer **without a +container-bound collaborator** (`is_enabled()`, `is_already_loaded()`, `has_standalone_plugin()`, +`get_conflict_policy()`, …). Note that this is not the same as "config alone": `is_already_loaded()` +reads the global constant table and `is_enabled()` may invoke a host callable that queries anything +it likes. The line is about *dependency direction* — anything needing `Plugin_Checker_Interface` or +the notice queue would drag a container resolution into `Loader::register()`, which deliberately +resolves nothing so the container can arrive at any point before boot. So `Sub_Plugin` only *names* +the plugin to ask about, and the collaborator does the asking. ### What exists today -`Loader`, `Conflict\Resolver` and `Activation` are not built yet. Currently: +`src/Conflict/` — `Resolver`, `Gatekeeper`, `Redirector` — and `Activator` are not built yet. +Currently: | Path | What | |---|---| -| `src/Config.php` | Static facade: hook prefix + optional container. | -| `src/Sub_Plugin.php` | Value object; validates config and answers everything config alone decides. | +| `src/Config.php` | Static facade: hook prefix + container. | +| `src/Loader.php` | Static facade: the registration buffer, `boot()`, and the accessors. | +| `src/Provider.php` | Binds every collaborator; the only file that names a default implementation. | +| `src/Boot/Scheduler.php` | Hook wiring and boot timing: the sequence, the priorities, and the fallback for a host that boots too late. | +| `src/Load/Runner.php` | The load pass: the gate chain, the `require_once`, the activation callback. | +| `src/Sub_Plugin.php` | Value object; validates config and answers what it can without a container-bound collaborator. | | `src/Conflict_Policy.php` | The three policy constants, `default()`, `is_valid()`. | -| `src/Plugin_State.php` | The only file that touches WordPress plugin functions. | +| `src/Plugin_Deactivator.php`, `src/Plugin_Checker.php` | The only files that touch WordPress plugin functions, through `Traits\Loads_Plugin_Functions`. | | `src/Registrar.php` | Holds registered `Sub_Plugin` objects. | +| `src/Traits/` | `Loads_Plugin_Functions` (pulls in `wp-admin/includes/plugin.php`), `Guards_Hook_Prefix` (a missing prefix warns and stands down rather than throwing). | | `src/Notices/` | `Queue` (what a notice says, who may consume it), `Store` (keeps it), `Renderer` (draws it), `Contracts\Queue_Interface`. | -| `src/Contracts/`, `src/Exceptions/` | `Plugin_State_Interface`, `Registrar_Interface`, `Config_Exception`. | +| `src/Contracts/`, `src/Exceptions/` | `Provider_Interface`, `Registrar_Interface`, `Plugin_Deactivator_Interface`, `Plugin_Checker_Interface`, `Config_Exception`. | ### Boot lifecycle ``` Config::set_hook_prefix( 'give' ); -Config::set_container( $container ); // optional +Config::set_container( $container ); // required Loader::register( [ …config… ] ); // once per sub-plugin; a duplicate slug throws Loader::boot(); // idempotent + → Provider::register() // every binding + → Boot\Scheduler // every hook, as a closure over the container -plugins_loaded @1 → Conflict\Resolver::resolve_all() -plugins_loaded @2 → Loader::load_all() -all_admin_notices → Loader::render_notices() [is_admin() only] +plugins_loaded @1 → Conflict\Resolver::resolve_all() [gated by Conflict\Gatekeeper] +plugins_loaded @2 → Load\Runner::load_all() +all_admin_notices → Loader::render_notices() [is_admin() only] wp_admin_notice_markup → Loader::filter_activation_error_markup() [is_admin() only] ``` +**A host calls `Config::set_container()` at `plugins_loaded` priority 0, from its own container +block and not from a service provider.** Priority, because conflict resolution runs at priority 1 and +WordPress silently ignores a callback added at or past the priority it is already dispatching — +LearnDash and MemberDash both wire Harbor's `set_container()` at priority 1, so a host copying that +habit races us. Its own block, because LearnDash's `App::container()` builds a container lazily when +none is set and the plugin then *replaces* it at priority 0: anything that grabbed the container +earlier holds an orphan whose bindings are discarded. This is also why `Loader::register()` buffers +and resolves nothing — registration at plugin-file scope, which the spec sanctions, would otherwise +register into the throwaway. + `load_all()` gates each sub-plugin in order, skipping on the first failure: enabled → not already loaded → dependencies met → file exists → `should_load` filter → `require_once` → activation callback (only after a *successful* require). @@ -139,8 +192,19 @@ the interface signature — so it is filtered once where the untrusted value ent `Conflict\Resolver` switches on the policy: `DEFER` no-ops, `NOTICE_ONLY` queues a notice, and `DEACTIVATE` (the default) deactivates network-aware, queues a merge notice, and redirects. -`redirect_destination()` returns `false` when the referrer is already `plugins.php`, so an inline -update is never interrupted. +`Conflict\Redirector` decides where to; it returns `false` when the referrer is already `plugins.php`, +so an inline update is never interrupted. It decides and never navigates — `wp_safe_redirect()` and +`exit` stay in the resolver, so the policy action and the admin-URL knowledge change for separate +reasons. + +**Who may have a conflict resolved is `Conflict\Gatekeeper`'s business, not the resolver's.** It +gates on an interactive admin `GET` (`plugins_loaded` fires on every request) *and* on +`current_user_can( 'activate_plugins' )` (`plugins_loaded` runs before `auth_redirect()`, so an +unauthenticated GET of an admin URL gets that far). The hook resolves the gatekeeper rather than the +resolver, so a host binding its own `Resolver_Interface` cannot drop either gate by omission. The +capability gate covers every policy, not just the destructive one, and that is free: the other +branches only queue a notice, and `Notices\Queue::render()` refuses to render *or clear* for a user +without the same capability, so queuing earlier would only park it until a capable admin arrives. An unknown policy must be handled as its own case via `Conflict_Policy::is_valid()`, never left to a `default:` fallthrough — a typo like `'defered'` would otherwise deactivate a plugin the site @@ -216,7 +280,8 @@ treatment. Any older sketch showing `Config::reset()` or `Loader::reset()` means - **Asserting a call did *not* happen needs a working recorder.** Record calls into an array, assert it is empty, then invoke the stubbed function once and assert the recorder caught it — otherwise a hook that failed to install passes the test for the wrong reason. -- **Container tests must use `Tests\Support\Test_Container`.** `lucatume\DI52\Container` implements +- **Every test that touches a collaborator sets a container**, since there is no longer a fallback to + fall back to — and it must be `Tests\Support\Test_Container`. `lucatume\DI52\Container` implements PSR-11's `ContainerInterface`, not StellarWP's, so passing it to `Config::set_container()` is a `TypeError`. - **Each load-path test writes its own bundled fixture file.** `require_once` caches by resolved @@ -264,10 +329,33 @@ treatment. Any older sketch showing `Config::reset()` or `Loader::reset()` means - **`deactivate_plugins()` is called silent, with no `$network_wide` argument.** Silent because a `flush_rewrite_rules()` in the standalone's deactivation hook at `plugins_loaded` 404s the site. The `null` default takes both the network and blog branches; a computed `true` strands an entry. -- **`Plugin_State::load_plugin_functions()` guards on `deactivate_plugins()`**, not - `is_plugin_active()` — the latter is a common third-party shim. +- **`Traits\Loads_Plugin_Functions` guards on `deactivate_plugins()`**, not `is_plugin_active()` — + the latter is a common third-party shim. - **Strauss must not rewrite `plugin_loaded_constant` values.** They are shared runtime constants; prefixing them defeats the entire mechanism. +- **Never write a literal guard-constant *name* in `src/`.** Hosts run Strauss with a + `constant_prefix` — `learndash-core` uses `LEARNDASH_`, with an empty exclude list — so a literal + `'GIVE_RECURRING_VERSION'` in our source is rewritten at build time and the `defined()` check then + matches nothing, silently, on the one path whose whole job is preventing a fatal. A constant name + arriving as a config *value* is a string in the host's array and is safe, which is why the design + holds: the library only ever receives these names, never spells one. +- **Collaborators come from the container; value objects and exceptions are constructed inline.** No + `?Peer $peer = null` constructor parameter and no `?? new X()` fallback — a nullable dependency + instantiating its own default is a service locator wearing a constructor signature, and it is what + made every default class owe a no-argument constructor. `new Sub_Plugin( $config )` in + `Loader::register()` and `throw new Config_Exception( … )` stay: one value object per host-supplied + array, built in the call the host can see in its own stack trace, which is what lets registration + validate eagerly while resolving nothing. A `Sub_Plugin_Factory` would put a container resolution + in front of the one method that deliberately performs none. +- **Naming: the suffix names the collaborator's role in the wiring, not whether it causes a side + effect.** A class that returns a URL from a filter still earns `-or` — `Redirector` alongside + `Registrar` and `Granter`, none of which need to perform anything for the name to be right. A class + stays bare while nothing in its folder does the same job (`Conflict\Resolver`, not + `Conflict\Standalone_Resolver`, even once `Gatekeeper` and `Redirector` sit beside it) and takes a + qualifier only when a second class of the same kind lands — `Scheduler` beside a later + `Retry_Scheduler`, never a qualifier bought in advance. An abstract + `-ion`/`-ance` noun is a directory name over agent nouns — `Activation/` holding `Activator` — and + never a class name; a census of 3,343 classes across eight Nexcess/StellarWP codebases found zero. ## Branch and PR workflow @@ -311,7 +399,11 @@ branch exists. `main` is releasable after every merge. still carrying the old `stellarwp/sub-plugin-loader` package name, the `Nexcess\SubPluginLoader\` namespace, a `Config::set_version()` that was removed, and an `ob_start()` approach replaced by the `wp_admin_notice_markup` filter. `docs/superpowers/plans/2026-07-31-plugin-absorber.md` holds the -task-by-task breakdown. +task-by-task breakdown, and +`docs/superpowers/plans/2026-08-12-container-required-rework.md` supersedes it wherever the two +disagree about the container, the collaborator seams or the class names — that plan is what branches +11 through 16 now implement, and the older plan still describes the optional-container design it +replaced. Once a task's PR merges to `main`, delete that task's section from the plan in the next branch that touches the file; git history keeps it. A shipped task's plan describes code that already exists in diff --git a/README.md b/README.md index e969227..7a9ac63 100644 --- a/README.md +++ b/README.md @@ -22,23 +22,32 @@ use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Loader; add_action( 'plugins_loaded', function () { - Config::set_hook_prefix( 'give' ); // required — keys the hooks and options - Config::set_container( give()->container ); // optional — lets you rebind collaborators + Config::set_hook_prefix( 'give' ); // required — keys the hooks and options + Config::set_container( give()->container ); // required — every collaborator resolves from it Loader::register( [ - 'slug' => 'give-stripe', - 'bundled_plugin_file' => __DIR__ . '/sub-plugins/give-stripe/give-stripe.php', - 'plugin_loaded_constant' => 'GIVE_STRIPE_VERSION', - 'standalone_plugin_basename' => 'give-stripe/give-stripe.php', + 'slug' => 'give-recurring', + 'bundled_plugin_file' => __DIR__ . '/sub-plugins/recurring/give-recurring.php', + 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION', + 'standalone_plugin_basename' => 'give-recurring/give-recurring.php', ] ); Loader::boot(); }, 0 ); ``` -Keep the `, 0`: `boot()` wires the load at `plugins_loaded` priority 2, and WordPress silently -ignores a callback added at or past the priority it is already dispatching. Booting later is -reported through `_doing_it_wrong()` and loaded inline, but the ordering guarantees are weaker. +The container is required — any StellarWP `ContainerInterface` implementation, the one you already +hand to Telemetry or Uplink. Every collaborator comes from it. + +Keep the `, 0`. `boot()` wires the load at `plugins_loaded` priority 2, and WordPress silently +ignores a callback added at or past the priority it is already dispatching — so configuring the +library from a provider that itself runs at priority 2 or later races the library it is configuring. +Booting later is reported through `_doing_it_wrong()` and loaded inline, but the ordering guarantees +are weaker. + +Put this in the block that owns your container, not in a service provider, and pass the container you +intend to keep: a host that builds one lazily and replaces it later leaves us holding an orphan whose +bindings were discarded. ## Docs diff --git a/docs/configuration.md b/docs/configuration.md index b3c75fb..c5ab7f1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -6,7 +6,7 @@ use Nexcess\PluginAbsorber\Config; Config::set_hook_prefix( 'give' ); // required — keys hooks and options -Config::set_container( give()->container ); // optional — lets you rebind collaborators +Config::set_container( give()->container ); // required — every collaborator resolves from it ``` The hook prefix accepts letters, numbers, hyphens, and underscores. Anything else throws @@ -14,36 +14,55 @@ The hook prefix accepts letters, numbers, hyphens, and underscores. Anything els option names lowercase it and turn hyphens into underscores, so `Give-Core` hooks `Give-Core/plugin_absorber/should_load` and stores `give_core_plugin_absorber_notices`. -The container is optional. Without one, the library instantiates its own collaborators; with one, -a host can rebind them. +## The container + +Both calls are required, and both belong at `plugins_loaded` priority 0, in your own container +block rather than in a service provider. + +Any implementation of StellarWP's `ContainerInterface` will do — the one your plugin already hands +to Telemetry, Uplink or Harbor. `Config::get_container()` throws `Config_Exception` when none is +set; `Config::has_container()` is the probe if you need to ask. + +Priority matters twice. Conflict resolution runs at `plugins_loaded` priority 1 and the load at +priority 2, and WordPress silently ignores a callback added at or past the priority it is already +dispatching — so configuring us from a provider that itself runs at priority 1 races us. And a host +that builds its container lazily may *replace* it at priority 0; hand us the container before that +happens and we hold an orphan whose bindings were discarded. ## Rebinding a collaborator -Every collaborator is interface-backed. With a container set, bind one to override the library -globally; with no container, the defaults are used and nothing is required. +`Loader::boot()` binds the defaults, and skips any id your container already has — so your binding +wins whether you make it before boot or after, and nothing is resolved until `plugins_loaded` +priority 1 in any case: ```php use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; $container->singleton( Registrar_Interface::class, My_Registrar::class ); -Config::set_container( $container ); ``` | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | | `Notices\Contracts\Queue_Interface` | `Notices\Queue` | Queues and renders the admin notices. | +| `Contracts\Plugin_Deactivator_Interface` | `Plugin_Deactivator` | Deactivates the standalone. | +| `Contracts\Plugin_Checker_Interface` | `Plugin_Checker` | Answers whether a plugin is active. | + +`Plugin_Checker_Interface` is the seam to rebind when your plugin filters `option_active_plugins` or +`site_option_active_sitewide_plugins` — LearnDash injects and then strips a synthetic path — because +`is_plugin_active()` then does not report what is in the database. -`set_container()` is a configuration call like `set_hook_prefix()`, and order does not matter: it may -come before or after your `Loader::register()` calls, so long as it comes before boot. Registering -buffers the sub-plugin and resolves nothing, so nothing is decided until the first read. +`set_container()` is a configuration call like `set_hook_prefix()`, and order does not matter among +the configuration calls: it may come before or after your `Loader::register()` calls, so long as it +comes before boot. Registering buffers the sub-plugin and resolves nothing, so nothing is decided +until the first read. A binding that does not implement the interface it is bound to throws `Config_Exception` when it is resolved, rather than being cached and failing later somewhere less obvious. So does a binding whose factory throws — with the original failure kept as the previous exception. -The container is **not** used to wire hooks — those stay plain static callbacks, so the container -stays genuinely optional. +The container is **not** used to wire hooks. Those are closures that resolve when they fire, so +registering them instantiates nothing and a request that triggers none builds none. ## Sub-plugin keys diff --git a/docs/conflict-handling.md b/docs/conflict-handling.md index 9491364..43cd75b 100644 --- a/docs/conflict-handling.md +++ b/docs/conflict-handling.md @@ -30,6 +30,17 @@ 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. +## What is deliberately out of scope + +**Version negotiation.** The library never compares versions, so it will not spare a standalone that +is newer than the bundled copy. Express that yourself: check the version and return +`Conflict_Policy::DEFER` from the `conflict_policy` [filter](filters.md), which has the final say. + +**Renamed standalone directories.** `standalone_plugin_basename` is the path as installed. A site +that renamed the standalone's directory is not detected, and there is no fallback that derives the +path from the load guard: one key is the guard and the other is the path, and no constant does both +jobs. The cost is a missed detection; the alternative costs the guarantee the guard exists for. + ## What the guard cannot do The guard cannot help on the request that *activates* the standalone: WordPress includes it after diff --git a/docs/filters.md b/docs/filters.md index 5e79d04..4063f16 100644 --- a/docs/filters.md +++ b/docs/filters.md @@ -30,3 +30,8 @@ It is consulted only for a sub-plugin that would otherwise have loaded — after the guard constant, the dependency check and the file check, in that order. So returning `true` cannot force a load past the guard constant: nothing overrides that. Anything other than a truthy return skips the load, which is the safe direction. + +**Watch the polarity when you wire an existing gate to this one.** `should_load` is true means *do +load*. A host filter named for the opposite — LearnDash's `learndash_module_{x}_disabled`, where true +means *do not load* — inverts the gate if it is passed through unnegated, and the failure is silent +in the direction that loads a plugin the site turned off. diff --git a/docs/notices.md b/docs/notices.md index b91d438..a1e73f0 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -30,7 +30,31 @@ example — and the messages may contain markup; the default rendering passes th stripped. Paragraphs come from `wpautop()`, so send the message unwrapped and let a blank line break it — a `

` of your own is left as it is rather than nested inside another. +```php +use Nexcess\PluginAbsorber\Notices\Queue; + +add_action( 'admin_init', function () { + $notices = get_site_option( Queue::option_name(), [] ); + + if ( ! is_array( $notices ) || ! $notices ) { + return; + } + + foreach ( $notices as $key => $message ) { + my_plugin_enqueue_notice( $key, $message ); + } + + delete_site_option( Queue::option_name() ); +} ); +``` + +`admin_init` runs before `all_admin_notices`, where the built-in rendering happens, so deleting the +option there leaves ours nothing to draw and the notice is shown once, by you. Do the deleting: a +notice read and not cleared is shown on every request forever. And keep the capability check — +reading the option directly skips the `activate_plugins` gate `Queue::render()` applies, and a +consumed notice is gone for the administrator who could have acted on it. + The queue is three classes: `Notices\Queue` decides what a notice says and who may consume it, -`Notices\Store` keeps it, `Notices\Renderer` draws it. Both collaborators are constructor arguments, -so `new Queue( null, $renderer )` keeps the queue and replaces only the markup, and -`new Queue( $store )` does the reverse. Replacing either one leaves the other alone. +`Notices\Store` keeps it, `Notices\Renderer` draws it. `Queue` takes both as constructor arguments +and all three are bound in the container, so rebinding `Notices\Renderer` replaces the markup and +leaves the storage alone, and rebinding `Notices\Store` does the reverse. diff --git a/docs/superpowers/plans/2026-08-12-container-required-rework.md b/docs/superpowers/plans/2026-08-12-container-required-rework.md new file mode 100644 index 0000000..ae092fa --- /dev/null +++ b/docs/superpowers/plans/2026-08-12-container-required-rework.md @@ -0,0 +1,305 @@ +# Container-Required Rework + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or +> superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the container mandatory, delete the machinery that existed only to make it optional, and +land the naming and responsibility corrections that the optional container was hiding. + +**Status:** planned 2026-08-12. Nothing implemented. Branches `11-loader-load-path` … `16-readme-release` +are pushed with PRs open (11, 12, 14, 15, 16, 17) and will all be rewritten. + +## Why + +`Container\Resolution` exists to answer "container binding when bound, `new $default_class` otherwise". +That single requirement is what forces every collaborator to be constructible with no arguments, which is +what forces the `?Peer $peer = null` constructors and their `?? Resolution::peer()` accessors, which is +what makes a class named for an abstract noun feel necessary in the first place. Remove the optional +container and the whole chain goes with it. + +Three surveys of the ecosystem, run 2026-08-12, say this is the normal shape and that it costs our hosts +nothing: + +- **Optional is the outlier.** Across 19 vendored StellarWP packages: 10 have no container at all, 7 + require one and throw (`uplink`, `telemetry`, `schema`, `shepherd`, `harbor`, `validation`, + `foundation`), and exactly one — `admin-notices` — is optional with a `new` fallback. We modelled + `Resolution` on the single outlier. +- **The hosts qualify.** `learndash-core` ships `LearnDash\Core\Container extends DI52Container implements + ContainerInterface`, already requires `stellarwp/container-contract` in production, exposes it as + `App::container()`, and already hands it to Telemetry, Validation and Harbor. Give, Event Tickets and + MemberDash are the same story. The plugins that lack a container are the *add-ons being absorbed*, not + the hosts doing the absorbing. +- **The naming was unsupported.** A census of 3,343 classes across eight Nexcess/StellarWP codebases found + zero classes named `Resolution`, zero named `Destination`, and zero named `Locator`. Abstract `-ion` + nouns are folder names over agent-nouned classes — `Activation/` containing `Activator` and + `Deactivator` — never class names themselves. + +`learndash-core` also settles the question of whether this library is worth shipping: it already contains +**four hand-rolled copies** of it — ProPanel (`@since 4.17.0`), Hub (`4.18.0`), Course Grid (`4.21.4`), +Course Reviews (`4.25.1`) — 987 lines with no shared abstraction, wired on exactly our hooks: + +```php +add_action( 'plugins_loaded', $this->container->callback( Loader::class, 'deactivate' ), 1 ); +add_action( 'plugins_loaded', $this->container->callback( Loader::class, 'load' ), 2 ); +add_filter( 'wp_admin_notice_markup', $this->container->callback( Loader::class, 'update_legacy_plugin_activation_notice' ) ); +``` + +A fifth absorption is a fifth copy. Those four modules are the migration target, and what they get wrong +is the case for the library: all four call `deactivate_plugins()` **non-silent**, none has a capability or +interactive-request gate (deactivation runs on anonymous front-end requests), one stores its merge notice +in a **transient**, and all four derive the standalone basename by stripping `WP_PLUGIN_DIR` off the +**load-guard constant** — the double duty our invariants forbid. + +## Decisions + +### D1 — The container is required + +`Config::get_container()` throws `Config_Exception` when unset, matching `Uplink\Config.php:64`, +`Telemetry\Config.php:71`, `Schema\Config.php:27`, `harbor/src/Harbor/Config.php:86`. `has_container()` +stays as the probe. `Config::set_container()` keeps its name — snake_case is the majority spelling in the +corpus — and keeps type-hinting `StellarWP\ContainerContract\ContainerInterface`. + +Type-hinting the contract is safe under Strauss: the host prefixes both its own container and our copy of +the contract into one namespace, which is why Harbor's identical hint works in LearnDash, Give and +MemberDash today (`sfwd-lms/vendor-prefixed/stellarwp/harbor/src/Harbor/Config.php:7` and +`sfwd-lms/src/Core/Container.php:13` name the same class). + +### D2 — `Container\Resolution` is deleted + +Nothing replaces it. Collaborators are bound by a provider and read with `$container->get()`. +`Loader::registrar()` and `Loader::notices()` remain as public accessors, now reading from the container. +`tests/_support/Resolution_State.php` is deleted with it. + +### D3 — One provider, our own one-method contract + +`Contracts\Provider_Interface` declares `register(): void` and nothing else — Harbor's shape +(`harbor/src/Harbor/Contracts/Provider_Interface.php:5`), not Foundation's. + +Foundation is rejected on three counts: a PHP **8.3** floor against our 7.4, a hard `lucatume/di52` +runtime dependency against our container-contract-only rule, and dead API — its `deferred`/`provides()` +are a fatal `TypeError` in practice, because di52 type-hints its own `ServiceProvider` base class which +Foundation's abstract does not extend. + +### D4 — Hooks wire as closures over the container + +`$container->callback()` is di52-specific and absent from `stellarwp/container-contract`, which declares +only `bind`, `get`, `has`, `singleton`. There are zero uses of `callback()` anywhere in the library corpus. +Harbor's idiom is what we copy: + +```php +add_action( 'plugins_loaded', function () use ( $container ) { + $container->get( Runner::class )->load_all(); +}, 2 ); +``` + +This keeps resolution lazy — registering hooks instantiates nothing — where `[ $resolved_object, 'method' ]` +would force every collaborator to be built at boot, as Telemetry does. + +### D5 — No fallback `new` in constructors + +Every collaborator takes required constructor arguments and is bound by the provider. The +`?Peer $peer = null` parameters and the `protected` accessors falling back to `Resolution::` are deleted. +This is platform-cloud's `AGENTS.md:97` rule verbatim: *"Do not make service dependencies nullable to +instantiate defaults inside constructors. The container should provide collaborators."* + +**The line:** collaborators come from the container; value objects are constructed inline. +`Loader::register()` keeps `new Sub_Plugin( $config )` — one value object per host-supplied config array, +built at the call the host can see in its own stack trace, which is what lets registration validate +eagerly while resolving nothing. `throw new Config_Exception( … )` likewise. No `Sub_Plugin_Factory`: it +would put a container resolution in front of the one method that deliberately performs none. + +### D6 — Names + +| Was | Becomes | Reason | +|---|---|---| +| `Container\Resolution` | *deleted* | zero `-ion` class names in 3,343 | +| `Conflict\Destination` | `Conflict\Redirector` | `Login_Redirector::redirect_after_login(): string` is direct precedent for an agent noun that returns a URL and never navigates | +| `Plugin_State` | `Plugin_Deactivator` + `Plugin_Checker` | it takes an argument and mutates; fact nouns in this corpus report ambient state with no arguments | +| `Contracts\Plugin_State_Interface` | `Contracts\Plugin_Deactivator_Interface` + `Contracts\Plugin_Checker_Interface` | follows the split | +| `Activation` (unbuilt) | `Activator` | `Activation/` is a folder name over `Activator`/`Deactivator` | + +Unchanged: `Config`, `Loader`, `Sub_Plugin`, `Conflict_Policy`, `Registrar`, `Conflict\Resolver`, +`Notices\Queue`, `Notices\Store`, `Notices\Renderer`. + +**Naming rules this establishes**, for `CLAUDE.md`: + +- The suffix names the collaborator's **role in the wiring**, not whether it causes a side effect. Six of + ten agent-nouned classes in platform-cloud return a value rather than calling an API — that is the shape + of a WordPress filter — and are still named `Redirector`, `Registrar`, `Granter`, `Highlighter`. +- **Bare until a sibling doing the same job lands.** `Conflict\Resolver` stays bare beside `Gatekeeper` and + `Redirector`, because none of the three does the other's job; the qualifier arrives only with a second + implementation of the *same* role, as LearnDash's `Scheduler` sits bare beside `Retry_Scheduler`. + LearnDash repeats a namespace segment in only 10% of its classes. +- An abstract `-ion`/`-ance` noun is a **directory**, never a class. + +### D7 — The load path splits out of `Loader` + +`Loader` keeps the public API, the registration buffer, and one-line trampolines. Out go: + +- **`Boot\Scheduler`** — `SEQUENCE`, `LOAD_PRIORITY`, `wiring_window_has_closed()`, and the inline + fallback for a host that boots too late. +- **the load loop** — the gate chain (`is_enabled` → `is_already_loaded` → dependencies → `is_file` → + `should_load` → `require_once` → activation callback). +- **`Conflict\Gatekeeper`** — `is_interactive_admin_request()` and `can_resolve_conflicts()`, which today + sit on `Loader` so a host binding its own `Resolver_Interface` cannot drop either by omission. As a + separately bound collaborator the same guarantee holds, because the trampoline resolves the gatekeeper + rather than the resolver. + +### D8 — The pending-registration buffer stays + +`Loader::$pending` and `flush()` survive the container becoming mandatory, so `register()` still resolves +nothing and registration stays order-free. + +Deleting them would be actively unsafe on our primary host: `App::container()` builds a container lazily +when none is set, and `sfwd_lms.php:116` then **replaces** it at `plugins_loaded` 0. Anything that touched +the container before that point holds an orphan whose bindings are discarded. A host registering at +plugin-file scope — which the spec sanctions — would register into the throwaway container and silently +load nothing. + +### D9 — No admin-notices adapter + +`AdminNotices::show()` does not persist anything: the host calls it every request and admin-notices draws +on `admin_notices`, with dismissal in per-user meta. Our merge notice is raised once, at deactivation, and +must survive to a later request — so the storage half cannot be delegated. + +Worse, the obvious adapter is silently broken. Core fires `admin_notices` **before** `all_admin_notices`, +which is where we render; a renderer calling `AdminNotices::show()` registers one hook too late to be +drawn, and `Queue::render()` has already cleared the store. A once-only notice, lost. + +So: no `Renderer_Interface`, no adapter class, no `class_exists()` guard, no `suggest` entry. `Queue::option_name()` +is already public for exactly this purpose, and `docs/notices.md` grows the fifteen lines a host needs to +read the option at `admin_init` and render it however it likes. + +`Notices\Renderer` and `Notices\Store` still become container-bound, because **everything** does (D5) — +not as a special case for this. + +### D10 — The exit rule stays; the boilerplate moves into a helper + +We do **not** adopt platform-cloud's `platform_cloud_exit()` function seam. It is a test-only seam in +`src/`, which `CLAUDE.md` bans outright, and on PHP 7.4 we cannot write `never`, so it would need a +docblock-only `@phpstan-return never` plus a `files` autoload entry loaded on every request of every host. + +We keep stubbing `wp_safe_redirect` and throwing `TestException`, but the discipline moves into one shared +trait so no test hand-rolls it and no test can silently drop the assertion that the request halted: + +```php +protected function capture_redirect( callable $action ): string { + // stub wp_safe_redirect -> record $location, throw TestException + try { + $action(); + $this->fail( 'Expected the action to redirect and terminate.' ); + } catch ( TestException $exception ) { + // Terminated as expected. + } + + return $location; +} +``` + +## What the host must now do + +Documented in `README.md` and `docs/configuration.md`: + +```php +add_action( 'plugins_loaded', function () { + Config::set_hook_prefix( 'learndash' ); + Config::set_container( App::container() ); + + Loader::register( [ /* … */ ] ); + Loader::boot(); +}, 0 ); +``` + +**Priority 0, in the host's own container block — not from a service provider.** LearnDash and MemberDash +both wire Harbor's `set_container()` at `plugins_loaded` **1**, which is where our conflict resolution +runs; a host copying that habit races us. Priority 0 is also after LearnDash replaces its container, so +the binding is the real one. + +## Invariants to add or amend in `CLAUDE.md` + +- **New:** never write a literal guard-constant name in `src/`. `learndash-core` runs Strauss with + `constant_prefix: "LEARNDASH_"` and an empty exclude list, so a literal would be rewritten and the guard + would stop matching. Constant names arriving as config *values* are safe, which is why the design holds. +- **New:** the naming rules from D6. +- **New:** collaborators from the container, value objects inline (D5). +- **Amend:** the architecture section's account of `Container\Resolution`, the four-collaborator table, and + "the container is genuinely optional" throughout. +- **Amend:** `Notices\Store`, `Notices\Renderer` and `Conflict\Redirector` are no longer "defaulted eagerly + with `?? new X()` because there is nothing to wait for" — that reasoning dies with D1. + +## Documented losses and clashes + +Called out in `docs/conflict-handling.md` and `docs/filters.md` so a migrating host is not surprised: + +- **Version negotiation.** ProPanel deliberately does not deactivate a standalone at or above + `3.0.0-dev`, using `get_plugin_data()` and `version_compare`. We keep this out of scope; the host + expresses it by returning `Conflict_Policy::DEFER` from the `conflict_policy` filter after its own + version check. +- **Renamed standalone directories.** Course Grid finds a standalone in a renamed directory by stripping + `WP_PLUGIN_DIR` off its guard constant. Our string `standalone_plugin_basename` cannot, and the invariant + forbidding one key from doing both jobs is why. +- **Filter polarity.** LearnDash gates modules on `learndash_module_{x}_disabled` where true means *do not + load*; our `should_load` is true means *do load*. Wiring one to the other inverts the gate. +- **Detection is host-filterable.** `learndash-core` installs `option_active_plugins` and + `site_option_active_sitewide_plugins` filters that inject and then strip a synthetic path + (`includes/class-ld-lms.php:175`, `:5173`), so `is_plugin_active()` does not report what is in the + database. `Plugin_Checker` is the seam a host rebinds to correct that. + +## Branch surgery + +Every branch from `11-loader-load-path` up is rewritten, so all six open PRs change. + +1. Snapshot every remote tip first: `git branch -f backup/ origin/` for 11 … 16. +2. Rework `11-loader-load-path`, then `gh stack rebase --committer-date-is-author-date` upward. +3. On a conflict, continue with plain `git rebase --continue` — `gh stack rebase --continue` forwards the + flag and exits 129 — then re-run the stack rebase for the branches above. +4. Force-push, then update each PR body to match what its branch now does. + +## Tasks, in rebase order + +Inventory figures below are measured, not estimated: 176 test methods exist across 12 classes; 31 break +under the mandatory container, 26 under the non-nullable constructors. PHPStan runs over `tests/` at +level 9, so most breakage surfaces from `composer test:analysis` without standing up WordPress. + +### Task 11 — `11-loader-load-path` + +- [ ] `Config::get_container()` throws; `has_container()` kept (it has no `src/` callers, but it is the probe a host uses). +- [ ] Delete `src/Container/Resolution.php` and `src/Container/`. Its only `Config::get_container()` call site goes with it. +- [ ] Add `Contracts\Provider_Interface` and `Provider`; bind each default only when `! $container->has( $id )`. +- [ ] Add `Boot\Scheduler` (`LOAD_PRIORITY`, `SEQUENCE`, `wiring_window_has_closed()`, inline fallback, hook wiring as closures). +- [ ] Add `Load\Runner` (`load_all()`, the gate chain, `has_hook_prefix()`). +- [ ] Split `Plugin_State` → `Plugin_Deactivator` + `Plugin_Checker` + `Traits\Loads_Plugin_Functions`; split the interface. **Note:** these files landed on branch 10, so the split appears in PR 11's diff. +- [ ] `Notices\Queue::__construct( Store $store, Renderer $renderer )` — required. +- [ ] Slim `Loader` to accessors, `register()`, `all()`, `flush()`, `boot()`, `render_notices()`. +- [ ] Tests: delete `Resolution_State`, drop its call from `Loader_State::reset():57`, add a `WithContainer` trait, move load/boot coverage into `Load\RunnerTest` and `Boot\SchedulerTest`, add `ProviderTest`, add `WithHaltedRedirects`. +- [ ] Delete, do not fix: `LoaderResolveTest::test_it_falls_back_to_the_default_registrar_without_a_container`, `::test_it_ignores_a_container_with_no_binding`, `PluginStateTest::test_it_constructs_without_arguments`. +- [ ] `Config_State::reset()` still resets the container to `null`; every test class sets one in `setUp()` instead. +- [ ] Docs on this branch: `CLAUDE.md`, `README.md:26`, `docs/configuration.md:17-18` and its "Rebinding a collaborator" section. + +### Task 12 — `12-conflict-resolver` + +- [ ] `Conflict\Destination` → `Conflict\Redirector`; bind it in `Provider` (it stops being a plain `new`). +- [ ] `Conflict\Resolver::__construct()` takes required `Plugin_Checker_Interface`, `Plugin_Deactivator_Interface`, `Queue_Interface`, `Redirector`; delete the `?? Resolution::` accessors. +- [ ] Move `is_interactive_admin_request()` / `can_resolve_conflicts()` into `Conflict\Gatekeeper`; the trampoline resolves the gatekeeper, so a host binding its own `Resolver_Interface` still cannot drop them. +- [ ] `Loader::plugin_state()` splits or drops; `Loader::resolver()` reads from the container. +- [ ] Tests: `ResolverTest` (29 methods, sets no container today), `Exposed_Resolver` (constructed zero-arg), `DestinationTest` → `RedirectorTest`. +- [ ] Fix the documented contradiction at `docs/configuration.md:39-42`, which says the capability gate lives in the resolver while the code and `docs/conflict-handling.md` say `Loader`. + +### Task 13 — `13-activation` + +- [ ] `Activation` → `Activator`, `Activation_Interface` → `Activator_Interface`; constructor injection; bind in `Provider`. + +### Task 14 — `14-activation-error-rewrite` + +- [ ] Rebase; de-null any collaborator constructor the branch adds; keep `wp_admin_notice_markup` wiring in `Boot\Scheduler`. + +### Task 15 — `15-e2e-suite` + +- [ ] The end-to-end bootstrap now builds a container and calls `Provider::register()` before `Loader::boot()`. + +### Task 16 — `16-readme-release` + +- [ ] Final docs pass. Container-optional survivors to rewrite: `CLAUDE.md` L69, L121, L124-125, L142, L158; `docs/configuration.md` L7, L21-22, L40-41, L125. +- [ ] README gains the priority-0 `Config::set_container()` call the branch previously deleted from its snippet. +- [ ] `docs/notices.md` grows the fifteen-line `admin_init` example for a host rendering through its own notice system. +- [ ] Keep the branch's deletion of the spec and the old plan; delete this plan file too once the stack lands. diff --git a/src/Boot/Scheduler.php b/src/Boot/Scheduler.php new file mode 100644 index 0000000..3c69b4c --- /dev/null +++ b/src/Boot/Scheduler.php @@ -0,0 +1,153 @@ +container = $container; + } + + /** + * Wire the WordPress hooks. + * + * Nothing is resolved here. Each step is a closure over the container that asks for its + * collaborator when the hook fires, so a host may still rebind one after boot() and up until + * plugins_loaded, and a binding nothing reaches is never built at all. + * + * @since 1.0.0 + * + * @return void + */ + public function wire(): void { + if ( is_admin() ) { + // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, + // network_admin_notices and user_admin_notices as mutually exclusive branches, so a + // superadmin working in the network admin -- exactly where a network-wide + // deactivation gets noticed -- would never see the queue rendered. + add_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); + } + + // Adding an action at a priority the current dispatch has already passed is accepted and + // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- + // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with + // no warning and a site that looks entirely healthy. + if ( $this->wiring_window_has_closed() ) { + _doing_it_wrong( + Loader::class . '::boot', + 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', + '1.0.0' + ); + + // In the order the hooks would have run them. + foreach ( $this->sequence() as $step ) { + $run = $step['run']; + + $run(); + } + + return; + } + + foreach ( $this->sequence() as $step ) { + add_action( 'plugins_loaded', $step['run'], $step['priority'] ); + } + } + + /** + * The plugins_loaded steps, in run order, as priority and callback. + * + * Stated once because wire() expresses this order twice — as hook priorities when it can still + * wire, and as straight calls when it is too late to and has to run them inline. Those two are + * the same sequence, and a comment is the only thing that could hold them in agreement. + * Iterating one list cannot drift. + * + * A method rather than a constant because a step is a closure now: naming the step and giving + * its priority in two separate lists would put the drift straight back, one list deep. + * + * @since 1.0.0 + * + * @return array + */ + private function sequence(): array { + $container = $this->container; + + return [ + [ + 'priority' => self::LOAD_PRIORITY, + 'run' => static function () use ( $container ): void { + $container->get( Runner::class )->load_all(); + }, + ], + ]; + } + + /** + * Whether it is already too late to wire the load hook. + * + * The comparison is inclusive. A callback added to the priority currently being dispatched is + * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` + * with a by-value foreach, so the append lands on an array the running loop has already copied. + * Booting from plugins_loaded at priority 2 is the case a host is likeliest to hit by accident, + * and an exclusive comparison would let exactly that one through unreported. + * + * @since 1.0.0 + * + * @return bool + */ + private function wiring_window_has_closed(): bool { + if ( ! did_action( 'plugins_loaded' ) ) { + return false; + } + + if ( ! doing_action( 'plugins_loaded' ) ) { + return true; + } + + $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; + + return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; + } +} diff --git a/src/Config.php b/src/Config.php index 30ee51a..abc73b5 100644 --- a/src/Config.php +++ b/src/Config.php @@ -115,9 +115,12 @@ public static function get_option_name( string $name ): string { } /** - * Share the host's container so collaborators become bindable. + * Share the host's container. Required, and required before boot(). * - * Entirely optional — with no container the library instantiates its own defaults. + * Every collaborator this library uses is resolved from it, which is what makes each of them + * replaceable by binding an interface. There is no second, container-less path to keep working + * beside that one: two ways to reach a collaborator means two sets of behaviour to reason + * about, and the one nobody runs is the one that rots. * * @since 1.0.0 * @@ -132,9 +135,17 @@ public static function set_container( ContainerInterface $container ): void { /** * @since 1.0.0 * - * @return ContainerInterface|null + * @throws Config_Exception When no container has been set. + * + * @return ContainerInterface */ - public static function get_container(): ?ContainerInterface { + public static function get_container(): ContainerInterface { + if ( self::$container === null ) { + throw new Config_Exception( + 'You must call Config::set_container() before booting the Plugin Absorber.' + ); + } + return self::$container; } diff --git a/src/Container/Resolution.php b/src/Container/Resolution.php deleted file mode 100644 index 1a60e5b..0000000 --- a/src/Container/Resolution.php +++ /dev/null @@ -1,138 +0,0 @@ - - */ - private static $resolved = []; - - /** - * @since 1.0.0 - * - * @throws Config_Exception When the container cannot produce a usable instance. - * - * @return Registrar_Interface - */ - public static function registrar(): Registrar_Interface { - return self::get( Registrar_Interface::class, Registrar::class ); - } - - /** - * @since 1.0.0 - * - * @throws Config_Exception When the container cannot produce a usable instance. - * - * @return Queue_Interface - */ - public static function notices(): Queue_Interface { - return self::get( Queue_Interface::class, Queue::class ); - } - - /** - * Resolve an interface from the container when bound, else construct the default. - * - * The container is never required — with none set, every collaborator is a plain `new`, so - * every default class must be constructible with no arguments. Resolution is memoized, and - * nothing resolves until the first read, which is boot: that is what lets a host set its - * container at any point beforehand. Swapping a collaborator after that would be the worse - * behaviour, since anything already holding the old instance would keep it. - * - * Generic rather than one method per collaborator, so each accessor can land with the PR that - * needs it without this one being rewritten each time. - * - * @since 1.0.0 - * - * @template T of object - * - * @param class-string $interface Interface to resolve. - * @param class-string $default_class Concrete class to build when nothing is bound. - * - * @throws Config_Exception When the container throws while building the binding, or returns - * something that does not implement the interface it was asked for. - * - * @return T - */ - public static function get( string $interface, string $default_class ): object { - if ( isset( self::$resolved[ $interface ] ) ) { - // The map holds a different type per key, so it cannot be typed as T as a whole. The - // entry found here still implements $interface: a container binding is only memoized - // once it passes the instanceof below, and the fallback is built from the class-string. - /** @var T $memoized */ - $memoized = self::$resolved[ $interface ]; - - return $memoized; - } - - $container = Config::get_container(); - - if ( $container !== null && $container->has( $interface ) ) { - // has() true only promises the binding exists, not that it can be built: a host factory - // closure is free to throw, and a container asked for a class with an unsatisfiable - // dependency throws its own exception type. Uncaught, either one leaves the host's - // plugins_loaded with a fatal from a vendor namespace that names neither this library - // nor the binding at fault, so both are reported the same way as a binding of the wrong - // type. The original is kept as the previous exception; nothing is memoized. - try { - $instance = $container->get( $interface ); - } catch ( Throwable $thrown ) { - throw new Config_Exception( - sprintf( - 'The container failed to build the binding for %s: %s', - $interface, - $thrown->getMessage() - ), - 0, - $thrown - ); - } - - // Checked before it is memoized. Without this the bad instance is cached, and every - // accessor throws a TypeError blaming this library rather than the binding. - if ( ! $instance instanceof $interface ) { - throw new Config_Exception( - sprintf( - 'The container binding for %s must implement it. Got %s.', - $interface, - is_object( $instance ) ? get_class( $instance ) : gettype( $instance ) - ) - ); - } - - self::$resolved[ $interface ] = $instance; - - return $instance; - } - - self::$resolved[ $interface ] = new $default_class(); - - return self::$resolved[ $interface ]; - } -} diff --git a/src/Contracts/Plugin_Checker_Interface.php b/src/Contracts/Plugin_Checker_Interface.php new file mode 100644 index 0000000..0b45244 --- /dev/null +++ b/src/Contracts/Plugin_Checker_Interface.php @@ -0,0 +1,37 @@ +notices = $notices; + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When the container cannot produce a usable registrar, or two + * sub-plugins were registered under one slug. + * + * @return void + */ + public function load_all(): void { + // The load path needs the prefix for the should_load filter and for the notice store. + // Throwing out of a core action would take the whole site down over a bootstrap mistake, + // so it is reported where a developer will see it and the load is abandoned instead. + if ( ! self::has_hook_prefix() ) { + return; + } + + // Loader::all() rather than the registrar directly: it flushes the registrations still + // buffered on the facade before it reads, and a registrar asked on its own would miss + // anything registered since the last read. + foreach ( Loader::all() as $sub_plugin ) { + $this->load( $sub_plugin ); + } + } + + /** + * Load one sub-plugin, cheapest and most decisive check first. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin to load. + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + private function load( Sub_Plugin $sub_plugin ): void { + if ( ! $sub_plugin->is_enabled() ) { + return; + } + + // Ahead of the dependency check, which calls an arbitrary host callable. This is one + // defined(), it carries the whole re-declaration guarantee, and it is the only gate that + // means "the plugin is already running" -- warning that requirements are unmet for a + // plugin the admin can see working would be worse than useless. + if ( $sub_plugin->is_already_loaded() ) { + return; + } + + if ( ! $sub_plugin->are_dependencies_met() ) { + $this->notices->queue_dependency_notice( $sub_plugin ); + + return; + } + + // Not file_exists(): that is true for a directory and for a file with no read permission, + // and require_once fatals on both. A missing file is a broken build in the host plugin + // rather than anything a site owner can act on, so it goes to the developer instead of + // into the notice queue, where it would have displayed the host's own + // dependency_notice_message and sent the owner after the wrong problem entirely. + $file = $sub_plugin->get_bundled_plugin_file(); + + if ( ! is_file( $file ) || ! is_readable( $file ) ) { + _doing_it_wrong( + self::class, + sprintf( + 'The bundled plugin file for "%s" is missing or unreadable: %s', + $sub_plugin->get_slug(), + $file + ), + '1.0.0' + ); + + return; + } + + // No type guard on the return, unlike the conflict_policy filter: there is no cast here, + // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the + // load, which is the safe direction. + $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); + + if ( ! $should_load ) { + return; + } + + // An include takes the scope of the line it sits on, and this one is inside a method, where + // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled + // file are function-local as a result -- documented for hosts, because no amount of + // wrapping here can hand a required file the global scope it would have had. + require_once $file; + } +} diff --git a/src/Loader.php b/src/Loader.php index 03943aa..d2216bf 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -5,17 +5,19 @@ namespace Nexcess\PluginAbsorber; -use Nexcess\PluginAbsorber\Container\Resolution; +use Nexcess\PluginAbsorber\Boot\Scheduler; +use Nexcess\PluginAbsorber\Contracts\Provider_Interface; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; -use WP_Hook; +use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; /** - * Static facade: registration, hook wiring, and the load loop. + * Static facade: registration, and the one call that starts everything. * - * Resolving an interface to its implementation lives in `Container\Resolution`; the accessors here - * are one-line delegations, so a host calls `Loader::notices()` and never has to know that. + * What a host touches, and deliberately little else. How collaborators are built belongs to + * `Provider`, when they run to `Boot\Scheduler`, and the load pass itself to `Load\Runner` — so + * the only reason to open this file is to change what a host may say to the library. * * `final` because it cannot usefully be extended: every member is private static and every internal * call is `self::`, so a subclass would inherit the API, be unable to override any of it, and change @@ -24,33 +26,7 @@ * @since 1.0.0 */ final class Loader { - /** - * plugins_loaded priority the load loop runs at. - * - * Ahead of the default priority, so a bundled plugin is in memory before the plugins that - * expect it start their own work, and low enough to leave room for earlier wiring. - * - * @since 1.0.0 - * - * @var int - */ - private const LOAD_PRIORITY = 2; - - /** - * The plugins_loaded steps, in run order, as [ method, priority ] pairs. - * - * Stated once because boot() 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. - * - * @since 1.0.0 - * - * @var array - */ - private const SEQUENCE = [ - 'load_all' => self::LOAD_PRIORITY, - ]; + use Guards_Hook_Prefix; /** * Sub-plugins registered but not yet handed to the registrar. @@ -69,36 +45,38 @@ final class Loader { /** * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable instance. + * @throws Config_Exception When no container has been set, or its binding is unusable. * * @return Registrar_Interface */ public static function registrar(): Registrar_Interface { - return Resolution::registrar(); + return self::collaborator( Registrar_Interface::class ); } /** * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable instance. + * @throws Config_Exception When no container has been set, or its binding is unusable. * * @return Queue_Interface */ public static function notices(): Queue_Interface { - return Resolution::notices(); + return self::collaborator( Queue_Interface::class ); } /** * Register one bundled sub-plugin. Call once per sub-plugin, before boot(). * * The sub-plugin is buffered rather than handed straight to the registrar, so that registering - * resolves nothing. Resolution needs the container, and a host that registers before it calls - * Config::set_container() would otherwise pin the default registrar and silently ignore the - * binding. Buffering is what lets the container arrive at any point before boot, like every - * other configuration call. + * resolves nothing. Reaching the registrar needs the container, and a host that registers before + * it calls Config::set_container() would otherwise fail on a call that has nothing to do with + * the container. Buffering is what lets the container arrive at any point before boot, like + * every other configuration call. * * The configuration is still validated here: building the Sub_Plugin is what rejects it, and - * that happens at the call the host can see in its own stack trace. + * that happens at the call the host can see in its own stack trace. It is built rather than + * resolved because it is a value object — a container asked for one would need the config + * passed through it, and there is nothing about it to rebind. * * @since 1.0.0 * @@ -117,8 +95,8 @@ public static function register( array $config ): void { * * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable registrar, or two - * sub-plugins were registered under one slug. + * @throws Config_Exception When no container has been set, or two sub-plugins were registered + * under one slug. * * @return array */ @@ -140,14 +118,18 @@ static function ( $sub_plugin ): bool { } /** - * Wire the WordPress hooks. Idempotent — safe to call from more than one code path. + * Bind the collaborators, then let the scheduler decide when they run. Idempotent — safe to + * call from more than one code path. * - * Hooks are plain static trampolines rather than container callbacks, which is what keeps the - * container optional. Each trampoline delegates to the resolved collaborator, so rebinding - * still takes effect. + * The provider is constructed rather than resolved: it is what teaches the container about this + * library, so the container cannot be asked to build it first. It is bound afterwards, and only + * when nothing answers to `Provider_Interface` already, so a host may replace the whole set of + * bindings with one of its own. * * @since 1.0.0 * + * @throws Config_Exception When no container has been set. + * * @return void */ public static function boot(): void { @@ -155,69 +137,71 @@ public static function boot(): void { return; } - self::$booted = true; + $container = Config::get_container(); - if ( is_admin() ) { - // all_admin_notices, not admin_notices. WordPress dispatches admin_notices, - // network_admin_notices and user_admin_notices as mutually exclusive branches, so a - // superadmin working in the network admin -- exactly where a network-wide - // deactivation gets noticed -- would never see the queue rendered. - add_action( 'all_admin_notices', [ self::class, 'render_notices' ] ); + if ( ! $container->has( Provider_Interface::class ) ) { + $container->singleton( Provider_Interface::class, new Provider( $container ) ); } - // Adding an action at a priority the current dispatch has already passed is accepted and - // then never fires. Booting from plugins_loaded at the default priority instead of 0 -- - // the commonest hook mistake there is -- would otherwise mean nothing loads at all, with - // no warning and a site that looks entirely healthy. - if ( self::wiring_window_has_closed() ) { - _doing_it_wrong( - __METHOD__, - 'Loader::boot() must run before plugins_loaded priority 2. Loading inline instead.', - '1.0.0' - ); - - // In the order the hooks would have run them. - foreach ( array_keys( self::SEQUENCE ) as $method ) { - call_user_func( [ self::class, $method ] ); - } - - return; - } + $container->get( Provider_Interface::class )->register(); + $container->get( Scheduler::class )->wire(); - foreach ( self::SEQUENCE as $method => $priority ) { - add_action( 'plugins_loaded', [ self::class, $method ], $priority ); - } + // Last, not first. A boot that threw on its way through -- no container, a binding that + // cannot be built -- has wired nothing, and a host that fixes the mistake and calls again + // should get a working library rather than a silent no-op. + self::$booted = true; } /** * @since 1.0.0 * + * @throws Config_Exception When no container has been set. + * * @return void */ - public static function 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. + public static function render_notices(): void { if ( ! self::has_hook_prefix() ) { return; } - foreach ( self::all() as $sub_plugin ) { - self::load( $sub_plugin ); - } + self::notices()->render(); } /** + * 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. + * * @since 1.0.0 * - * @return void + * @template T of object + * + * @param class-string $interface Collaborator interface to resolve. + * + * @throws Config_Exception When no container has been set, or its binding does not implement + * the interface it was bound to. + * + * @return T */ - public static function render_notices(): void { - if ( ! self::has_hook_prefix() ) { - return; + private static function collaborator( string $interface ): object { + $collaborator = Config::get_container()->get( $interface ); + + if ( ! $collaborator instanceof $interface ) { + throw new Config_Exception( + sprintf( + 'The container binding for %s returned %s, which does not implement it.', + $interface, + is_object( $collaborator ) ? get_class( $collaborator ) : gettype( $collaborator ) + ) + ); } - self::notices()->render(); + return $collaborator; } /** @@ -234,8 +218,8 @@ public static function render_notices(): void { * * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable registrar, or two - * sub-plugins were registered under one slug. + * @throws Config_Exception When no container has been set, or two sub-plugins were registered + * under one slug. * * @return void */ @@ -253,117 +237,4 @@ private static function flush(): void { $registrar->register( $sub_plugin ); } } - - /** - * Load one sub-plugin, cheapest and most decisive check first. - * - * @since 1.0.0 - * - * @param Sub_Plugin $sub_plugin Sub-plugin to load. - * - * @throws Config_Exception When a collaborator binding is unusable. - * - * @return void - */ - private static function load( Sub_Plugin $sub_plugin ): void { - if ( ! $sub_plugin->is_enabled() ) { - return; - } - - // Ahead of the dependency check, which calls an arbitrary host callable. This is one - // defined(), it carries the whole re-declaration guarantee, and it is the only gate that - // means "the plugin is already running" -- warning that requirements are unmet for a - // plugin the admin can see working would be worse than useless. - if ( $sub_plugin->is_already_loaded() ) { - return; - } - - if ( ! $sub_plugin->are_dependencies_met() ) { - self::notices()->queue_dependency_notice( $sub_plugin ); - - return; - } - - // Not file_exists(): that is true for a directory and for a file with no read permission, - // and require_once fatals on both. A missing file is a broken build in the host plugin - // rather than anything a site owner can act on, so it goes to the developer instead of - // into the notice queue, where it would have displayed the host's own - // dependency_notice_message and sent the owner after the wrong problem entirely. - $file = $sub_plugin->get_bundled_plugin_file(); - - if ( ! is_file( $file ) || ! is_readable( $file ) ) { - _doing_it_wrong( - 'Nexcess\PluginAbsorber\Loader', - sprintf( - 'The bundled plugin file for "%s" is missing or unreadable: %s', - $sub_plugin->get_slug(), - $file - ), - '1.0.0' - ); - - return; - } - - // No type guard on the return, unlike the conflict_policy filter: there is no cast here, - // and every unexpected value is falsy-or-truthy without fataling. Anything odd skips the - // load, which is the safe direction. - $should_load = apply_filters( Config::get_hook_name( 'should_load' ), true, $sub_plugin ); - - if ( ! $should_load ) { - return; - } - - // An include takes the scope of the line it sits on, and this one is inside a method, where - // wp-settings.php includes plugins at global scope. Top-level assignments in the bundled - // file are function-local as a result -- documented for hosts, because no amount of - // wrapping here can hand a required file the global scope it would have had. - require_once $file; - } - - /** - * Whether it is already too late to wire the load hook. - * - * The comparison is inclusive. A callback added to the priority currently being dispatched is - * accepted and never reached either: WP_Hook::apply_filters() walks `$this->callbacks[$priority]` - * with a by-value foreach, so the append lands on an array the running loop has already copied. - * Booting from plugins_loaded at priority 2 is the case a host is likeliest to hit by accident, - * and an exclusive comparison would let exactly that one through unreported. - * - * @since 1.0.0 - * - * @return bool - */ - private static function wiring_window_has_closed(): bool { - if ( ! did_action( 'plugins_loaded' ) ) { - return false; - } - - if ( ! doing_action( 'plugins_loaded' ) ) { - return true; - } - - $hook = $GLOBALS['wp_filter']['plugins_loaded'] ?? null; - - return $hook instanceof WP_Hook && $hook->current_priority() >= self::LOAD_PRIORITY; - } - - /** - * Whether a hook prefix has been set, reporting to the developer when it has not. - * - * @since 1.0.0 - * - * @return bool - */ - private static function has_hook_prefix(): bool { - try { - Config::get_hook_prefix(); - } catch ( Config_Exception $exception ) { - _doing_it_wrong( 'Nexcess\PluginAbsorber\Loader', $exception->getMessage(), '1.0.0' ); - - return false; - } - - return true; - } } diff --git a/src/Notices/Queue.php b/src/Notices/Queue.php index 83d202d..dd34180 100644 --- a/src/Notices/Queue.php +++ b/src/Notices/Queue.php @@ -16,8 +16,9 @@ * queue is kept is Store's job and how it is drawn is Renderer's, so a host can replace either one * without inheriting the other, and neither has to be understood to reword a message. * - * Both collaborators are constructor arguments with defaults, so `new Queue()` still gives the - * standard behaviour — which is what `Loader::resolve()` builds when the container has no binding. + * Both collaborators are required constructor arguments, and `Provider` is what hands them over. + * No defaults: a class that can build its own dependencies has a second way to be constructed that + * bypasses every binding a host made, and it is the one a test or a stray `new` reaches for. * * A host already using stellarwp/admin-notices can bind its own implementation of Queue_Interface * and read the same option, whose name is `self::option_name()`. @@ -79,12 +80,12 @@ class Queue implements Queue_Interface { /** * @since 1.0.0 * - * @param Store|null $store Where the queue is kept. - * @param Renderer|null $renderer How a queued notice is drawn. + * @param Store $store Where the queue is kept. + * @param Renderer $renderer How a queued notice is drawn. */ - public function __construct( ?Store $store = null, ?Renderer $renderer = null ) { - $this->store = $store ?? new Store(); - $this->renderer = $renderer ?? new Renderer(); + public function __construct( Store $store, Renderer $renderer ) { + $this->store = $store; + $this->renderer = $renderer; } /** diff --git a/src/Plugin_Checker.php b/src/Plugin_Checker.php new file mode 100644 index 0000000..e48a1b5 --- /dev/null +++ b/src/Plugin_Checker.php @@ -0,0 +1,34 @@ +load_plugin_functions(); + + // WordPress's own is_plugin_active() already ORs in the network check, so asking + // is_plugin_active_for_network() as well would only buy a second get_site_option() per + // sub-plugin per request. + return is_plugin_active( $basename ); + } +} diff --git a/src/Plugin_State.php b/src/Plugin_Deactivator.php similarity index 50% rename from src/Plugin_State.php rename to src/Plugin_Deactivator.php index e320aa3..ba46b72 100644 --- a/src/Plugin_State.php +++ b/src/Plugin_Deactivator.php @@ -5,32 +5,16 @@ namespace Nexcess\PluginAbsorber; -use Nexcess\PluginAbsorber\Contracts\Plugin_State_Interface; +use Nexcess\PluginAbsorber\Contracts\Plugin_Deactivator_Interface; +use Nexcess\PluginAbsorber\Traits\Loads_Plugin_Functions; /** - * Plugin state, straight from WordPress. - * - * The single place in this library that touches WordPress's plugin functions, so it is also the - * single place that has to load them. + * Turns a plugin off, the way WordPress's own unattended paths do. * * @since 1.0.0 */ -class Plugin_State implements Plugin_State_Interface { - /** - * @since 1.0.0 - * - * @param string $basename Plugin basename. - * - * @return bool - */ - public function is_active( string $basename ): bool { - $this->load_plugin_functions(); - - // WordPress's own is_plugin_active() already ORs in the network check, so asking - // is_plugin_active_for_network() as well would only buy a second get_site_option() per - // sub-plugin per request. - return is_plugin_active( $basename ); - } +class Plugin_Deactivator implements Plugin_Deactivator_Interface { + use Loads_Plugin_Functions; /** * @since 1.0.0 @@ -58,21 +42,4 @@ public function deactivate( string $basename ): void { // a second request and a second deactivation hook to clear. deactivate_plugins( $basename, true ); } - - /** - * WordPress only loads these in the admin, and we run at plugins_loaded on every request. - * - * Guarded on deactivate_plugins() rather than is_plugin_active(), because the latter is a - * common third-party shim: something else defining it would short-circuit this and leave the - * rest of the file unloaded, so the first call that needs a function nobody shimmed fatals. - * - * @since 1.0.0 - * - * @return void - */ - private function load_plugin_functions(): void { - if ( ! function_exists( 'deactivate_plugins' ) ) { - require_once ABSPATH . 'wp-admin/includes/plugin.php'; - } - } } diff --git a/src/Provider.php b/src/Provider.php new file mode 100644 index 0000000..31a200d --- /dev/null +++ b/src/Provider.php @@ -0,0 +1,119 @@ +container = $container; + } + + /** + * @since 1.0.0 + * + * @return void + */ + public function register(): void { + $container = $this->container; + + // The container under its own contract, so that a container which builds unbound classes + // reflectively can still satisfy the two collaborators that take one. Bound first because + // everything below it may be resolved that way. + $this->bind_once( ContainerInterface::class, $container ); + + $this->bind_once( Registrar_Interface::class, Registrar::class ); + $this->bind_once( Plugin_Checker_Interface::class, Plugin_Checker::class ); + $this->bind_once( Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ); + $this->bind_once( Store::class ); + $this->bind_once( Renderer::class ); + + // Explicit factories rather than a class name for everything with a constructor argument: + // container-contract promises `bind`, `get`, `has` and `singleton` and nothing about + // autowiring, so a container that resolves nothing by reflection has to be told. + $this->bind_once( + Queue_Interface::class, + static function () use ( $container ): Queue { + return new Queue( $container->get( Store::class ), $container->get( Renderer::class ) ); + } + ); + + $this->bind_once( + Runner::class, + static function () use ( $container ): Runner { + return new Runner( $container->get( Queue_Interface::class ) ); + } + ); + + $this->bind_once( + Scheduler::class, + static function () use ( $container ): Scheduler { + return new Scheduler( $container ); + } + ); + } + + /** + * Bind as a singleton, unless the host already bound something. + * + * The host binds first and wins: this library's defaults are what a container has when nobody + * said otherwise, and a provider that overwrote a binding would make the order in which a host + * calls `set_container()` and `boot()` decide which implementation it gets. + * + * Singletons throughout. Every one of these is either a registry whose contents are the point + * or a stateless worker, and a second registrar would hold a second, emptier list of + * sub-plugins. + * + * @since 1.0.0 + * + * @param string $id Interface or class to bind. + * @param mixed $implementation Class name, instance or factory closure; `null` to have the + * container build `$id` itself. + * + * @return void + */ + private function bind_once( string $id, $implementation = null ): void { + if ( $this->container->has( $id ) ) { + return; + } + + $this->container->singleton( $id, $implementation ); + } +} diff --git a/src/Sub_Plugin.php b/src/Sub_Plugin.php index 875e199..27f7e75 100644 --- a/src/Sub_Plugin.php +++ b/src/Sub_Plugin.php @@ -12,7 +12,7 @@ * * Deliberately not a window onto WordPress. Asking whether the standalone counterpart is active is * a question about the site rather than about this configuration, and it belongs to - * Plugin_State_Interface; this object only names the plugin to ask about. + * Plugin_Checker_Interface; this object only names the plugin to ask about. * * @since 1.0.0 * diff --git a/src/Traits/Guards_Hook_Prefix.php b/src/Traits/Guards_Hook_Prefix.php new file mode 100644 index 0000000..c269aa7 --- /dev/null +++ b/src/Traits/Guards_Hook_Prefix.php @@ -0,0 +1,41 @@ +getMessage(), '1.0.0' ); + + return false; + } + + return true; + } +} diff --git a/src/Traits/Loads_Plugin_Functions.php b/src/Traits/Loads_Plugin_Functions.php new file mode 100644 index 0000000..fc602ac --- /dev/null +++ b/src/Traits/Loads_Plugin_Functions.php @@ -0,0 +1,35 @@ +set_up_container(); +} +``` + +A test about a *rebinding* host binds its own implementation first and passes +the container in — the provider only binds what nothing else has, which is the +guarantee those tests exist to pin: + +```php +$container = new Test_Container(); +$container->singleton( Queue_Interface::class, static fn() => $notices ); + +$this->set_up_container( $container ); +``` + +Bind interfaces, not concrete classes: DI52 reports `has()` true for any class +name that exists, bound or not, so binding `Notices\Store` first cannot +demonstrate anything the provider does. + +`$this->resolve( Some::class )` is the typed read back out, and +`$this->container()` the container itself. Call `tear_down_container()` from +tearDown, alongside `Config_State::reset()`. + +Container tests must use `Tests\Support\Test_Container`. +`lucatume\DI52\Container` implements PSR-11's `ContainerInterface`, not +StellarWP's, so passing it to `Config::set_container()` is a `TypeError`. + ## Sub-plugin fixtures `WithSubPlugins` builds a well-formed `Sub_Plugin`, so a test states only the @@ -58,6 +100,26 @@ $this->make_sub_plugin( [ 'plugin_loaded_constant' => 'ABSORBER_TEST_LOADED_CONS Overrides are merged last, so a deliberately unusable value still reaches the constructor — that is how the tests for rejected config work. +## Bundled plugin fixtures + +`WithBundledPlugins` writes the file the load path requires: + +```php +$constant = $this->make_guard_constant(); +$path = $this->make_bundled_plugin_file( $constant ); + +// … register, load … + +$this->assertSame( 1, $this->bundled_plugin_loads() ); +``` + +Every call writes a *new* file under a unique name, and every guard constant is +unique too. Neither is tidiness: `require_once` dedupes by resolved path for +the lifetime of the PHP process, so a shared fixture lets a later test pass +without loading anything, and the fixture defines its constant for real, so a +reused name makes a later sub-plugin read as already loaded. Call +`remove_bundled_plugin_files()` from tearDown. + A fixture helper cannot be called `make()`, `makeEmpty()`, `construct()`, or `constructEmpty()`: those are public methods on `Codeception\Test\Unit`, which `WPTestCase` extends, and redeclaring one with narrower visibility is a fatal at @@ -156,52 +218,59 @@ class Deactivator { } ``` -Assert it like this: +Assert it through `WithHaltedRedirects`, which owns the whole shape: ```php -use Nexcess\PluginAbsorber\Tests\Support\TestException; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithHaltedRedirects; public function test_redirects_back(): void { - $redirects = []; - - $this->setFunctionReturn( - 'wp_safe_redirect', - static function ( $location ) use ( &$redirects ) { - $redirects[] = $location; - - throw new TestException( 'Halted where production calls exit().' ); - }, - true - ); - $subject = new Deactivator(); - $halted = false; - try { - $subject->redirect_back( 'https://example.test/wp-admin/plugins.php' ); - } catch ( TestException $e ) { - $halted = true; - - $this->assertSame( 'Halted where production calls exit().', $e->getMessage() ); - } + $location = $this->capture_redirect( + static function () use ( $subject ): void { + $subject->redirect_back( 'https://example.test/wp-admin/plugins.php' ); + } + ); - $this->assertTrue( $halted, 'The redirect must halt where production calls exit().' ); - $this->assertSame( [ 'https://example.test/wp-admin/plugins.php' ], $redirects ); + $this->assertSame( 'https://example.test/wp-admin/plugins.php', $location ); } ``` -The `$halted` flag is the part that cannot be dropped. Catching the exception -without asserting that it actually arrived turns "the code under test never -redirected at all" into a silent pass — the same class of failure this section +Two parts of that are easy to drop by hand and silent when dropped, which is +why they live in the trait rather than in each test body. The `fail()` on the +line after the action is what turns "the code under test never redirected at +all" into a failure instead of a pass — the same class of failure this section opens by warning about, moved out of `preventExit()` and into the test body. -Matching on the message as well as the class keeps an unrelated `TestException` -thrown earlier from satisfying the catch for the wrong reason. +Matching on the exception's message as well as its class keeps an unrelated +`TestException` thrown earlier from satisfying the catch for the wrong reason. + +The trait needs `UopzFunctions` on the same class, for the stub. + +`tests/unit/SmokeTest.php` covers both the mechanism and the trait, with +`test_a_stub_can_throw_to_halt_a_code_path` and +`test_the_shared_helper_captures_a_halted_redirect` — the executable proof that +a stub really can stop a code path before it reaches `exit`, and that the shared +helper reports it when one does not. + +## Expecting `_doing_it_wrong()` + +`setExpectedIncorrectUsage()` matches the first argument exactly, which for a +report made with `__METHOD__` means restating a private method name in the test. +That name is an implementation detail of where a gate happens to live — moving +the inline-boot fallback from `Loader` to `Boot\Scheduler` changed it without +changing anything a host can observe. -A bare `expectException( TestException::class )` is fine when the test only -cares that the halt happened and asserts nothing about state afterwards. The -try/catch shape exists so assertions can run after the halt; there is no reason -to use both mechanisms in one test. +`WithIncorrectUsage` registers the expectation from the report itself and +asserts over what was reported instead: + +```php +$this->expect_incorrect_usage(); + +$runner->load_all(); + +$this->assert_the_library_reported_incorrect_usage(); +``` -`tests/unit/SmokeTest.php` covers this with -`test_a_stub_can_throw_to_halt_a_code_path`, which is the executable proof that -a stub really can throw to stop a code path before it reaches `exit`. +An unexpected report still fails the test, because everything the listener sees +is recorded and asserted to belong to this library. Call +`stop_expecting_incorrect_usage()` from tearDown. diff --git a/tests/_support/Loader_State.php b/tests/_support/Loader_State.php index 65a4b44..7e5d639 100644 --- a/tests/_support/Loader_State.php +++ b/tests/_support/Loader_State.php @@ -5,22 +5,24 @@ namespace Nexcess\PluginAbsorber\Tests\Support; +use Closure; use LogicException; use Nexcess\PluginAbsorber\Loader; use ReflectionClass; +use ReflectionFunction; use ReflectionProperty; +use WP_Hook; /** - * Restores `Loader`'s static state between tests. + * Restores `Loader`'s static state between tests, and unwires the hooks boot() added. * * `Loader` has no public way to clear itself, and deliberately so: a reset method would be API the - * library then has to support forever for the sake of its own test suite, and a host that reached - * for it mid-request would discard the registrations the load loop is about to read. Reflection - * keeps that seam on this side of the fence. + * library then has to support forever for the sake of its own test suite, and a host that reached for + * it mid-request would discard the registrations the load loop is about to read. Reflection keeps that + * seam on this side of the fence. * - * Dropping the memo is enough for the default collaborators, which are built per resolve. A - * collaborator bound into a container as a singleton comes back populated on the next resolve, so - * a test that binds one must build a fresh instance rather than expect this to empty it. + * Collaborators are the container's now, so there is no memo here to drop: a test gets a fresh set by + * standing up a fresh container, which `Traits\WithContainer` does in one line. */ class Loader_State { /** @@ -37,6 +39,24 @@ class Loader_State { 'booted' => false, ]; + /** + * The hooks boot() reaches, directly or through `Boot\Scheduler`. + * + * @var string[] + */ + protected const HOOKS = [ + 'plugins_loaded', + 'all_admin_notices', + 'wp_admin_notice_markup', + ]; + + /** + * Namespace every callback this library wires belongs to. + * + * @var string + */ + protected const NAMESPACE_PREFIX = 'Nexcess\\PluginAbsorber\\'; + /** * Return every static property of `Loader` to its default, and unwire the hooks it added. * @@ -50,36 +70,9 @@ class Loader_State { * @return void */ public static function reset(): void { - $reflection = new ReflectionClass( Loader::class ); - - // The memo lives on Container\Resolution now, but every test in the suite resets through - // this one call, so it is dropped from here rather than making each of them reset twice. - Resolution_State::reset(); - - // Read rather than restated, so the helper unwires exactly what boot() wired -- including - // any step the Loader grows later. Restating it would go on removing a callback from a - // priority the Loader no longer uses, leaving the real one attached and every later test - // loading sub-plugins it never registered. - $sequence = $reflection->getConstant( 'SEQUENCE' ); - - // Refused rather than coerced, for the same reason. - if ( ! is_array( $sequence ) ) { - throw new LogicException( - sprintf( 'Loader::SEQUENCE must be an array for %s to unwire the hooks boot() added.', self::class ) - ); - } - - foreach ( $sequence as $method => $priority ) { - if ( ! is_string( $method ) || ! is_int( $priority ) ) { - throw new LogicException( - sprintf( 'Loader::SEQUENCE must map method names to int priorities for %s to unwire them.', self::class ) - ); - } - - remove_action( 'plugins_loaded', [ Loader::class, $method ], $priority ); - } + self::unwire(); - remove_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ); + $reflection = new ReflectionClass( Loader::class ); foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { $name = $property->getName(); @@ -94,4 +87,89 @@ public static function reset(): void { $property->setValue( null, self::DEFAULTS[ $name ] ); } } + + /** + * Take back every callback this library put on the boot hooks. + * + * Identified by where the callback comes from rather than by restating what boot() wires. The + * steps are closures over the container now, so there is no name to match on and no + * `has_action( $hook, [ Loader::class, 'load_all' ] )` to remove them by; and + * `remove_all_actions()` would strip the hook bare, discarding every callback WordPress and the + * rest of the suite have on it for the remainder of the process. + * + * Reading `Boot\Scheduler::SEQUENCE` for its priorities would be the narrower sweep, but it would + * also miss a step wired at a priority the constant no longer names — which is exactly the drift + * an unwire helper exists to survive. + * + * @return void + */ + protected static function unwire(): void { + foreach ( self::HOOKS as $hook ) { + $wp_hook = $GLOBALS['wp_filter'][ $hook ] ?? null; + + if ( ! $wp_hook instanceof WP_Hook ) { + continue; + } + + // Iterating a by-value copy, so removing as we go cannot disturb the walk. + foreach ( $wp_hook->callbacks as $priority => $callbacks ) { + if ( ! is_int( $priority ) || ! is_array( $callbacks ) ) { + continue; + } + + foreach ( $callbacks as $registered ) { + $callback = is_array( $registered ) ? ( $registered['function'] ?? null ) : null; + + if ( $callback !== null && is_callable( $callback ) && self::belongs_to_the_library( $callback ) ) { + remove_action( $hook, $callback, $priority ); + } + } + } + } + } + + /** + * Whether a registered callback came out of this library. + * + * Covers both shapes a hook callback can take here: a closure written in `src/`, and a static or + * instance method on one of the library's own classes. + * + * @param callable $callback Callback registered on one of the boot hooks. + * + * @return bool + */ + protected static function belongs_to_the_library( callable $callback ): bool { + if ( $callback instanceof Closure ) { + $file = ( new ReflectionFunction( $callback ) )->getFileName(); + + return is_string( $file ) && strpos( $file, self::source_directory() ) === 0; + } + + if ( is_array( $callback ) ) { + $target = $callback[0] ?? null; + $class = is_object( $target ) ? get_class( $target ) : $target; + + return is_string( $class ) && strpos( $class, self::NAMESPACE_PREFIX ) === 0; + } + + return is_string( $callback ) && strpos( $callback, self::NAMESPACE_PREFIX ) === 0; + } + + /** + * Where the library's own source lives, read off a class rather than assumed from this file. + * + * @throws LogicException When the path cannot be read, rather than matching every closure on + * an empty prefix and unwiring the whole site. + * + * @return string + */ + protected static function source_directory(): string { + $file = ( new ReflectionClass( Loader::class ) )->getFileName(); + + if ( ! is_string( $file ) || $file === '' ) { + throw new LogicException( sprintf( 'Could not locate the library source from %s.', self::class ) ); + } + + return dirname( $file ) . DIRECTORY_SEPARATOR; + } } diff --git a/tests/_support/Resolution_State.php b/tests/_support/Resolution_State.php deleted file mode 100644 index 7c915db..0000000 --- a/tests/_support/Resolution_State.php +++ /dev/null @@ -1,61 +0,0 @@ - - */ - protected const DEFAULTS = [ - 'resolved' => [], - ]; - - /** - * Return every static property of `Resolution` to its default. - * - * @throws LogicException When `Resolution` has grown a static property this helper does not know - * about, rather than leaving it to leak between tests. - * - * @return void - */ - public static function reset(): void { - $reflection = new ReflectionClass( Resolution::class ); - - foreach ( $reflection->getProperties( ReflectionProperty::IS_STATIC ) as $property ) { - $name = $property->getName(); - - if ( ! array_key_exists( $name, self::DEFAULTS ) ) { - throw new LogicException( - sprintf( 'Resolution::$%s has no default in %s. Add one.', $name, self::class ) - ); - } - - $property->setAccessible( true ); - $property->setValue( null, self::DEFAULTS[ $name ] ); - } - } -} diff --git a/tests/_support/Spy_Queue.php b/tests/_support/Spy_Queue.php new file mode 100644 index 0000000..2b42400 --- /dev/null +++ b/tests/_support/Spy_Queue.php @@ -0,0 +1,85 @@ +render_calls` off a value typed as + * `Queue_Interface` is reading a property the interface does not declare, and static analysis rightly + * rejects it. Named, the spy's own type carries the counters. + * + * It stores nothing, which is the point — a test that binds this one proves the default queue was + * never resolved by asserting the option is still absent. + * + * @since 1.0.0 + */ +class Spy_Queue implements Queue_Interface { + /** + * Slugs handed to queue_merge_notice(), in order. + * + * @var string[] + */ + public $merge_notices = []; + + /** + * Slugs handed to queue_conflict_notice(), in order. + * + * @var string[] + */ + public $conflict_notices = []; + + /** + * Slugs handed to queue_dependency_notice(), in order. + * + * @var string[] + */ + public $dependency_notices = []; + + /** + * How many times render() was called. + * + * @var int + */ + public $render_calls = 0; + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { + $this->merge_notices[] = $sub_plugin->get_slug(); + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + $this->conflict_notices[] = $sub_plugin->get_slug(); + } + + /** + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + $this->dependency_notices[] = $sub_plugin->get_slug(); + } + + /** + * @return void + */ + public function render(): void { + ++$this->render_calls; + } +} diff --git a/tests/_support/Traits/WithBundledPlugins.php b/tests/_support/Traits/WithBundledPlugins.php new file mode 100644 index 0000000..6d96ef2 --- /dev/null +++ b/tests/_support/Traits/WithBundledPlugins.php @@ -0,0 +1,123 @@ +bundled_plugin_files[] = $path; + + return $path; + } + + /** + * A guard constant name no other test can collide with. + * + * The fixture defines its constant for real, and a `define()` lasts for the whole PHP process: a + * name reused by a later test would make its sub-plugin read as already loaded and skip the load + * it was written to exercise. + * + * @since 1.0.0 + * + * @return string + */ + protected function make_guard_constant(): string { + return 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); + } + + /** + * A path no bundled plugin was ever written to. + * + * @since 1.0.0 + * + * @return string + */ + protected function missing_bundled_plugin_file(): string { + return sys_get_temp_dir() . '/absorber-does-not-exist-' . uniqid( '', true ) . '.php'; + } + + /** + * How many times a bundled fixture has been executed this test. + * + * @since 1.0.0 + * + * @return int + */ + protected function bundled_plugin_loads(): int { + $loads = $GLOBALS['absorber_loads'] ?? 0; + + return is_int( $loads ) ? $loads : 0; + } + + /** + * @since 1.0.0 + * + * @return void + */ + protected function reset_bundled_plugin_loads(): void { + $GLOBALS['absorber_loads'] = 0; + } + + /** + * Remove every fixture this test wrote. Call from tearDown. + * + * @since 1.0.0 + * + * @return void + */ + protected function remove_bundled_plugin_files(): void { + foreach ( $this->bundled_plugin_files as $path ) { + if ( file_exists( $path ) ) { + // A test that made a file unreadable to exercise that gate cannot unlink it until the + // permissions come back. + chmod( $path, 0644 ); + unlink( $path ); + } + } + + $this->bundled_plugin_files = []; + + unset( $GLOBALS['absorber_loads'] ); + } +} diff --git a/tests/_support/Traits/WithContainer.php b/tests/_support/Traits/WithContainer.php new file mode 100644 index 0000000..5b15964 --- /dev/null +++ b/tests/_support/Traits/WithContainer.php @@ -0,0 +1,122 @@ +has( ContainerInterface::class ) ) { + $container->singleton( + ContainerInterface::class, + static function () use ( $container ): ContainerInterface { + return $container; + } + ); + } + + ( new Provider( $container ) )->register(); + + Config::set_container( $container ); + + $this->absorber_container = $container; + + return $container; + } + + /** + * The container this test is running against. + * + * @since 1.0.0 + * + * @throws LogicException When the test never stood one up, rather than failing later on a null. + * + * @return Test_Container + */ + protected function container(): Test_Container { + if ( $this->absorber_container === null ) { + throw new LogicException( 'Call set_up_container() before reading the container.' ); + } + + return $this->absorber_container; + } + + /** + * Resolve one id, refusing anything that is not what was asked for. + * + * The contract's `get()` can only declare an untyped return, so every call site would otherwise + * repeat the same narrowing to say what it got back. + * + * @since 1.0.0 + * + * @template T of object + * + * @param class-string $id Id to resolve. + * + * @throws LogicException When the container builds something else. + * + * @return T + */ + protected function resolve( string $id ): object { + $instance = $this->container()->get( $id ); + + if ( ! $instance instanceof $id ) { + throw new LogicException( sprintf( 'The container did not build a %s.', $id ) ); + } + + /** @var T $instance */ + return $instance; + } + + /** + * Forget the container between tests. + * + * Call from tearDown alongside `Config_State::reset()`, which clears the library's own reference. + * + * @since 1.0.0 + * + * @return void + */ + protected function tear_down_container(): void { + $this->absorber_container = null; + } +} diff --git a/tests/_support/Traits/WithHaltedRedirects.php b/tests/_support/Traits/WithHaltedRedirects.php new file mode 100644 index 0000000..8f7ad55 --- /dev/null +++ b/tests/_support/Traits/WithHaltedRedirects.php @@ -0,0 +1,89 @@ +setFunctionReturn( + 'wp_safe_redirect', + static function ( $to ) use ( &$location, $message ) { + $location = is_string( $to ) ? $to : ''; + + throw new TestException( $message ); + }, + true + ); + + try { + $action(); + + // Reached only when the action returned instead of halting, which is the failure this + // helper exists to make impossible to miss. + $this->fail( 'Expected the action to redirect and terminate.' ); + } catch ( TestException $exception ) { + $this->assertSame( + $message, + $exception->getMessage(), + 'The halt has to come from the stubbed redirect, not from something thrown earlier.' + ); + } finally { + // In a finally block so a failed assertion cannot strand the stub for the rest of the + // process, where a later test's redirect would throw for no reason it can see. + $this->unsetFunctionReturn( 'wp_safe_redirect' ); + } + + $this->assertNotSame( '', $location, 'The action must redirect somewhere.' ); + + return $location; + } + + /** + * The message the stubbed redirect throws. + * + * A method rather than a constant: PHP 7.4 has no constants in traits. + * + * @since 1.0.0 + * + * @return string + */ + protected static function halted_at_exit_message(): string { + return 'Halted where production calls exit().'; + } +} diff --git a/tests/_support/Traits/WithIncorrectUsage.php b/tests/_support/Traits/WithIncorrectUsage.php new file mode 100644 index 0000000..6233cb2 --- /dev/null +++ b/tests/_support/Traits/WithIncorrectUsage.php @@ -0,0 +1,105 @@ +incorrect_usage_reports; + + $listener = function ( $function_name ) use ( &$reports ): void { + if ( ! is_string( $function_name ) ) { + return; + } + + $reports[] = $function_name; + + $this->setExpectedIncorrectUsage( $function_name ); + }; + + $this->incorrect_usage_listener = $listener; + + add_action( 'doing_it_wrong_run', $listener ); + } + + /** + * @since 1.0.0 + * + * @return void + */ + protected function assert_the_library_reported_incorrect_usage(): void { + $this->assertNotSame( + [], + $this->incorrect_usage_reports, + 'The library has to report a developer mistake where a developer will see it.' + ); + + foreach ( $this->incorrect_usage_reports as $report ) { + $this->assertStringStartsWith( + 'Nexcess\\PluginAbsorber\\', + $report, + 'The report has to name this library, or the host goes looking in WordPress.' + ); + } + } + + /** + * Take the listener back off. Call from tearDown. + * + * Removed by identity rather than by clearing the hook, which WordPress and the rest of the suite + * are also on. + * + * @since 1.0.0 + * + * @return void + */ + protected function stop_expecting_incorrect_usage(): void { + if ( $this->incorrect_usage_listener !== null ) { + remove_action( 'doing_it_wrong_run', $this->incorrect_usage_listener ); + + $this->incorrect_usage_listener = null; + } + + $this->incorrect_usage_reports = []; + } +} diff --git a/tests/unit/Boot/SchedulerTest.php b/tests/unit/Boot/SchedulerTest.php new file mode 100644 index 0000000..993f54c --- /dev/null +++ b/tests/unit/Boot/SchedulerTest.php @@ -0,0 +1,421 @@ + + */ + private $added_actions = []; + + public function setUp(): void { + parent::setUp(); + + Loader_State::reset(); + Config_State::reset(); + Config::set_hook_prefix( 'give' ); + $this->set_up_container(); + $this->reset_bundled_plugin_loads(); + + // The harness has to boot WordPress before it can run anything, so plugins_loaded has already + // fired by the time any test starts — and boot() would rightly report that it is too late to + // wire a hook at the load priority. Rewind the counter so the wiring tests see the timing a + // real host bootstrap sees. The tests that exercise a late boot set it back. + $this->plugins_loaded_count = did_action( 'plugins_loaded' ); + unset( $GLOBALS['wp_actions']['plugins_loaded'] ); + } + + public function tearDown(): void { + $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; + + // In tearDown rather than at the end of the test body: a failing assertion would otherwise + // leak an admin screen into every test that runs after it, since is_admin() checks the + // current screen before WP_ADMIN. + set_current_screen( 'front' ); + + // Only what these tests added by hand. What boot() wired comes off in Loader_State::reset(). + foreach ( $this->added_actions as [ $hook, $callback, $priority ] ) { + remove_action( $hook, $callback, $priority ); + } + $this->added_actions = []; + + $this->stop_expecting_incorrect_usage(); + $this->remove_bundled_plugin_files(); + Loader_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + /** + * Ahead of the default priority, so a bundled plugin is in memory before the plugins that expect + * it start their own work, and low enough to leave room for earlier wiring — conflict resolution + * runs at 1. The number is documented, so it is part of the contract rather than an internal. + */ + public function test_the_load_step_runs_early_in_plugins_loaded(): void { + $this->assertSame( 2, $this->load_priority() ); + } + + public function test_it_wires_the_load_step_at_the_load_priority(): void { + $this->register_sub_plugin(); + + $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + + Loader::boot(); + + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', $this->load_priority() ), + 'boot() must wire the load step rather than run it.' + ); + $this->assertSame( 0, $this->bundled_plugin_loads(), 'Wiring must not load anything yet.' ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + public function test_booting_twice_wires_the_load_step_only_once(): void { + $this->register_sub_plugin(); + + $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + + Loader::boot(); + Loader::boot(); + + $this->assertSame( + $before + 1, + $this->callbacks_at( 'plugins_loaded', $this->load_priority() ), + 'boot() must be idempotent.' + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + } + + /** + * The reason the load step runs at all early: a plugin hooking plugins_loaded at the default + * priority — which is nearly all of them — has to find the bundled code already in memory. + */ + public function test_a_plugin_hooking_at_the_default_priority_finds_the_bundled_code(): void { + $this->register_sub_plugin(); + + Loader::boot(); + + $loads_seen = null; + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( &$loads_seen ): void { + $loads_seen = $GLOBALS['absorber_loads'] ?? 0; + } + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $loads_seen ); + } + + public function test_it_wires_the_notice_step_in_the_admin(): void { + set_current_screen( 'dashboard' ); + + $notices = $this->bind_spy_queue(); + + Loader::boot(); + + do_action( 'all_admin_notices' ); + + $this->assertSame( 1, $notices->render_calls ); + } + + /** + * Nothing renders a notice on the front end, and the queue must survive until an admin load + * consumes it — rendering is what clears it. + */ + public function test_it_does_not_wire_the_notice_step_on_the_front_end(): void { + set_current_screen( 'front' ); + + $notices = $this->bind_spy_queue(); + + Loader::boot(); + + // The recorder has to be shown to work: without it, a do_action() that fired nothing at all + // would satisfy the assertion below for a reason that has nothing to do with the front end. + $fired = false; + $this->add_tracked_action( + 'all_admin_notices', + static function () use ( &$fired ): void { + $fired = true; + } + ); + + do_action( 'all_admin_notices' ); + + $this->assertTrue( $fired, 'The hook must really have been dispatched.' ); + $this->assertSame( 0, $notices->render_calls ); + } + + /** + * Adding an action at a priority the running dispatch has already passed is accepted and then + * never fires. Booting from plugins_loaded at the default priority instead of 0 would otherwise + * load nothing at all, on a site that looks completely healthy. + * + * The load priority itself is the boundary case: a callback added to the priority currently being + * dispatched is never reached either, because the dispatch loop walks a by-value copy of that + * priority's callback array. + * + * @dataProvider late_boot_priorities + * + * @param int $offset How far past the load priority the host boots from. + */ + public function test_booting_too_late_in_plugins_loaded_loads_inline_instead( int $offset ): void { + $this->expect_incorrect_usage(); + + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); + + $this->add_tracked_action( + 'plugins_loaded', + static function () use ( $path, $constant ): void { + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $path, + 'plugin_loaded_constant' => $constant, + ] + ); + + Loader::boot(); + }, + $this->load_priority() + $offset + ); + + do_action( 'plugins_loaded' ); + + $this->assertSame( 1, $this->bundled_plugin_loads(), 'A late boot must still load.' ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * @return Generator + */ + public static function late_boot_priorities(): Generator { + yield 'at the load priority' => [ 0 ]; + yield 'one past it' => [ 1 ]; + yield 'the default a host omits' => [ 8 ]; + } + + public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { + $this->expect_incorrect_usage(); + + do_action( 'plugins_loaded' ); + + $this->register_sub_plugin(); + Loader::boot(); + + $this->assertSame( 1, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * The suite's state helper stands in for the reset() the Loader deliberately does not ship. + * Clearing the boot flag without unwiring would leave a Loader that reports itself unbooted while + * its callbacks are still attached — and every later test would load sub-plugins it never + * registered. + */ + public function test_the_state_helper_unwires_the_hooks_boot_added(): void { + set_current_screen( 'dashboard' ); + + $load_step = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + $notice_step = $this->callbacks_at( 'all_admin_notices' ); + + Loader::boot(); + Loader_State::reset(); + + $this->assertSame( $load_step, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + $this->assertSame( $notice_step, $this->callbacks_at( 'all_admin_notices' ) ); + } + + /** + * The hooks are gone by the time this boots again, so the assertion is about the second boot + * rather than a leftover from the first. + */ + public function test_the_state_helper_allows_booting_again(): void { + $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + + Loader::boot(); + Loader_State::reset(); + + Loader::boot(); + + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + } + + /** + * boot() runs before anything reads the prefix, so it must not be the thing that throws when a + * host forgot to set one — the load path reports that at a point the host can see. + */ + public function test_boot_does_not_need_a_hook_prefix(): void { + $container = $this->container(); + + Config_State::reset(); + Config::set_container( $container ); + + $before = $this->callbacks_at( 'plugins_loaded', $this->load_priority() ); + + Loader::boot(); + + $this->assertSame( $before + 1, $this->callbacks_at( 'plugins_loaded', $this->load_priority() ) ); + } + + /** + * The priority the load step is wired at, read from the scheduler rather than restated. + * + * @throws LogicException When the constant is missing or not an int, rather than counting + * callbacks at priority zero and passing for the wrong reason. + * + * @return int + */ + private function load_priority(): int { + $priority = ( new ReflectionClass( Scheduler::class ) )->getConstant( 'LOAD_PRIORITY' ); + + if ( ! is_int( $priority ) ) { + throw new LogicException( 'Boot\Scheduler::LOAD_PRIORITY must be an int.' ); + } + + return $priority; + } + + /** + * How many callbacks are on a hook, at one priority or in total. + * + * @param string $hook Hook to count. + * @param int|null $priority Priority to count at, or null for every priority. + * + * @return int + */ + private function callbacks_at( string $hook, ?int $priority = null ): int { + $wp_hook = $GLOBALS['wp_filter'][ $hook ] ?? null; + + if ( ! $wp_hook instanceof WP_Hook ) { + return 0; + } + + $total = 0; + + foreach ( $wp_hook->callbacks as $registered_priority => $callbacks ) { + if ( ! is_array( $callbacks ) ) { + continue; + } + + if ( $priority === null || $registered_priority === $priority ) { + $total += count( $callbacks ); + } + } + + return $total; + } + + /** + * Bind a recording queue in place of the default one. + * + * Bound before the provider runs, which is the only order that leaves it bound. + * + * @return Spy_Queue + */ + private function bind_spy_queue(): Spy_Queue { + $notices = new Spy_Queue(); + $container = new Test_Container(); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Spy_Queue { + return $notices; + } + ); + + $this->set_up_container( $container ); + + return $notices; + } + + /** + * Register a sub-plugin whose bundled file records that it was loaded. + * + * @return void + */ + private function register_sub_plugin(): void { + $constant = $this->make_guard_constant(); + + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => $this->make_bundled_plugin_file( $constant ), + 'plugin_loaded_constant' => $constant, + ] + ); + } + + /** + * Add an action tearDown can take back by identity rather than by clearing the whole hook. + * + * @param string $hook Hook to add to. + * @param callable $callback Callback to add. + * @param int $priority Priority to add it at. + * + * @return void + */ + private function add_tracked_action( string $hook, callable $callback, int $priority = 10 ): void { + $this->added_actions[] = [ $hook, $callback, $priority ]; + + add_action( $hook, $callback, $priority ); + } +} diff --git a/tests/unit/ConfigTest.php b/tests/unit/ConfigTest.php index 169526d..e7dd4e1 100644 --- a/tests/unit/ConfigTest.php +++ b/tests/unit/ConfigTest.php @@ -152,7 +152,30 @@ public function test_an_option_name_needs_a_prefix(): void { public function test_it_reports_no_container_by_default(): void { $this->assertFalse( Config::has_container() ); - $this->assertNull( Config::get_container() ); + } + + /** + * The container is required, so reading it without one is a configuration error rather than a + * null every caller then has to test for. Every collaborator comes from the container now: a + * silent null would surface as a TypeError from somewhere inside plugins_loaded, naming this + * library rather than the bootstrap that skipped a step. + */ + public function test_reading_a_container_that_was_never_set_is_a_configuration_error(): void { + $this->expectException( Config_Exception::class ); + + Config::get_container(); + } + + /** + * The probe stays, and answers without throwing — it is how a host asks whether it has already + * configured the library. + */ + public function test_the_probe_answers_without_throwing(): void { + $this->assertFalse( Config::has_container() ); + + Config::set_container( new Test_Container() ); + + $this->assertTrue( Config::has_container() ); } public function test_it_stores_and_returns_a_container(): void { @@ -171,9 +194,14 @@ public function test_the_state_helper_clears_every_value(): void { Config_State::reset(); $this->assertFalse( Config::has_container() ); - $this->assertNull( Config::get_container() ); - $this->expectException( Config_Exception::class ); - Config::get_hook_prefix(); + foreach ( [ 'get_container', 'get_hook_prefix' ] as $accessor ) { + try { + Config::{$accessor}(); + $this->fail( sprintf( 'Config::%s() must throw once the state helper has run.', $accessor ) ); + } catch ( Config_Exception $exception ) { + $this->assertNotSame( '', $exception->getMessage(), 'The host has to be told what is missing.' ); + } + } } } diff --git a/tests/unit/LoaderLoadTest.php b/tests/unit/Load/RunnerTest.php similarity index 53% rename from tests/unit/LoaderLoadTest.php rename to tests/unit/Load/RunnerTest.php index 732f3f6..1f89644 100644 --- a/tests/unit/LoaderLoadTest.php +++ b/tests/unit/Load/RunnerTest.php @@ -3,30 +3,39 @@ * @package Nexcess\PluginAbsorber */ -namespace Nexcess\PluginAbsorber\Tests\Unit; +namespace Nexcess\PluginAbsorber\Tests\Unit\Load; use Codeception\TestCase\WPTestCase; use lucatume\WPBrowser\Traits\UopzFunctions; use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Contracts\Registrar_Interface; +use Nexcess\PluginAbsorber\Load\Runner; use Nexcess\PluginAbsorber\Loader; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Loader_State; +use Nexcess\PluginAbsorber\Tests\Support\Spy_Queue; use Nexcess\PluginAbsorber\Tests\Support\Spy_Registrar; use Nexcess\PluginAbsorber\Tests\Support\Test_Container; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithBundledPlugins; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithContainer; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithIncorrectUsage; /** + * The load loop and its gate chain. + * + * The gates run cheapest and most decisive first — enabled, then already loaded, then dependencies, + * then the file itself, then the `should_load` filter — and each one is asserted both by what it + * skips and by what it stops the later gates from doing. + * * @since 1.0.0 */ -class LoaderLoadTest extends WPTestCase { +class RunnerTest extends WPTestCase { use UopzFunctions; - - /** - * @var array - */ - private $fixtures = []; + use WithBundledPlugins; + use WithContainer; + use WithIncorrectUsage; /** * Guard constants a test defined through uopz. @@ -36,11 +45,14 @@ class LoaderLoadTest extends WPTestCase { private $constants = []; /** - * The plugins_loaded count as the harness left it, for the one test that rewinds it. + * Every value the should_load filter was called with. * - * @var int|null + * A property rather than a local, because the recorder is a closure the filter keeps: a local + * handed back from the helper that installs it would be a copy taken before the first call. + * + * @var array */ - private $plugins_loaded_count = null; + private $should_load_calls = []; public function setUp(): void { parent::setUp(); @@ -48,17 +60,14 @@ public function setUp(): void { Loader_State::reset(); Config_State::reset(); Config::set_hook_prefix( 'give' ); + $this->set_up_container(); $this->clear_notices(); - $GLOBALS['absorber_loads'] = 0; + $this->reset_bundled_plugin_loads(); + $this->should_load_calls = []; } public function tearDown(): void { - foreach ( $this->fixtures as $fixture ) { - if ( file_exists( $fixture ) ) { - unlink( $fixture ); - } - } - $this->fixtures = []; + $this->remove_bundled_plugin_files(); // In tearDown rather than at the end of the test body: a failing assertion would otherwise // strand the constant for the rest of the process, and every later test would read it as a @@ -68,50 +77,46 @@ public function tearDown(): void { } $this->constants = []; - if ( $this->plugins_loaded_count !== null ) { - $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; - $this->plugins_loaded_count = null; - } - - unset( $GLOBALS['absorber_loads'] ); + $this->stop_expecting_incorrect_usage(); $this->clear_notices(); Loader_State::reset(); Config_State::reset(); + $this->tear_down_container(); parent::tearDown(); } public function test_it_requires_the_bundled_file(): void { $constant = $this->register(); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); $this->assertTrue( defined( $constant ) ); } public function test_it_requires_the_bundled_file_exactly_once(): void { $this->register(); - Loader::load_all(); - Loader::load_all(); + $this->runner()->load_all(); + $this->runner()->load_all(); - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); } public function test_it_skips_a_disabled_sub_plugin(): void { $this->register( [ 'enabled' => false ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); } public function test_it_skips_when_dependencies_are_unmet_and_queues_a_notice(): void { $this->register( [ 'dependency_check' => static fn() => false ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); $this->assertArrayHasKey( 'give-recurring:dependency', $this->notice_queue() ); } @@ -120,103 +125,110 @@ public function test_it_skips_when_the_guard_constant_is_already_defined(): void $this->register( [], $constant ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A defined constant means the code is already present.' ); + $this->assertSame( + 0, + $this->bundled_plugin_loads(), + 'A defined constant means the code is already present.' + ); } public function test_it_skips_when_the_bundled_file_is_missing(): void { - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + $this->expect_incorrect_usage(); Loader::register( [ 'slug' => 'give-recurring', - 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', - 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_GUARD', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); } /** * A broken build is a developer problem, not a site-owner one. It must not reach the notice - * queue, where it would render the host's own dependency_notice_message and send the owner - * after a dependency that is perfectly fine. + * queue, where it would render the host's own dependency_notice_message and send the owner after + * a dependency that is perfectly fine. * - * The message is configured as a callable because that is the only shape the key takes: a - * string is refused outright, so that a host's __() cannot run while it builds its config array. + * The message is configured as a callable because that is the only shape the key takes: a string + * is refused outright, so that a host's __() cannot run while it builds its config array. */ public function test_a_missing_bundled_file_reports_to_the_developer_not_the_site_owner(): void { - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + $this->expect_incorrect_usage(); Loader::register( [ 'slug' => 'give-recurring', - 'bundled_plugin_file' => '/tmp/absorber-does-not-exist-' . uniqid( '', true ) . '.php', - 'plugin_loaded_constant' => 'ABSORBER_MISSING_FILE_NOTICE_GUARD', + 'bundled_plugin_file' => $this->missing_bundled_plugin_file(), + 'plugin_loaded_constant' => $this->make_guard_constant(), 'dependency_notice_message' => static fn() => 'GiveWP 3.0 or later is required.', ] ); - Loader::load_all(); + $this->runner()->load_all(); $this->assertSame( [], $this->notice_queue() ); + $this->assert_the_library_reported_incorrect_usage(); } /** * file_exists() is true for a directory, and require_once fatals on one. */ public function test_it_skips_when_the_bundled_path_is_a_directory(): void { - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + $this->expect_incorrect_usage(); Loader::register( [ 'slug' => 'give-recurring', 'bundled_plugin_file' => sys_get_temp_dir(), - 'plugin_loaded_constant' => 'ABSORBER_DIRECTORY_GUARD', + 'plugin_loaded_constant' => $this->make_guard_constant(), ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); } /** * file_exists() is also true for a file the process cannot read, and require_once fatals. */ public function test_it_skips_when_the_bundled_file_is_unreadable(): void { - $path = $this->make_fixture( 'ABSORBER_UNREADABLE_GUARD' ); + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); chmod( $path, 0000 ); if ( is_readable( $path ) ) { $this->markTestSkipped( 'Running as a user that can read a 0000 file.' ); } - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + $this->expect_incorrect_usage(); Loader::register( [ 'slug' => 'give-recurring', 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => 'ABSORBER_UNREADABLE_GUARD', + 'plugin_loaded_constant' => $constant, ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); - - chmod( $path, 0644 ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); + $this->assert_the_library_reported_incorrect_usage(); } /** * The dependency check calls an arbitrary host callable, so it must not run for a sub-plugin - * whose code is already present — and must not warn that requirements are unmet for a plugin - * the admin can see running. + * whose code is already present — and must not warn that requirements are unmet for a plugin the + * admin can see running. */ public function test_an_already_loaded_sub_plugin_is_not_dependency_checked(): void { $constant = $this->define_guard( 'ABSORBER_LOADED_BEFORE_DEPS_GUARD' ); @@ -233,7 +245,7 @@ public function test_an_already_loaded_sub_plugin_is_not_dependency_checked(): v $constant ); - Loader::load_all(); + $this->runner()->load_all(); $this->assertSame( 0, $checked ); $this->assertSame( [], $this->notice_queue(), 'No notice for a plugin that is already running.' ); @@ -244,9 +256,9 @@ public function test_the_should_load_filter_can_veto_the_load(): void { add_filter( 'give/plugin_absorber/should_load', '__return_false' ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'] ); + $this->assertSame( 0, $this->bundled_plugin_loads() ); } public function test_the_should_load_filter_receives_the_sub_plugin(): void { @@ -264,50 +276,53 @@ static function ( $should_load, $sub_plugin ) use ( &$received ) { 2 ); - Loader::load_all(); + $this->runner()->load_all(); $this->assertInstanceOf( Sub_Plugin::class, $received ); $this->assertSame( 'give-recurring', $received->get_slug() ); } /** - * The filter is the last gate before require_once, so it must not be consulted for a - * sub-plugin that was already going to be skipped — a host counting its invocations would - * otherwise see calls for loads that never happen. + * The filter is the last gate before require_once, so it must not be consulted for a sub-plugin + * that was already going to be skipped — a host counting its invocations would otherwise see calls + * for loads that never happen. */ public function test_the_should_load_filter_is_not_consulted_for_a_disabled_sub_plugin(): void { $this->register( [ 'enabled' => false ] ); - $calls = []; - add_filter( - 'give/plugin_absorber/should_load', - static function ( $should_load ) use ( &$calls ) { - $calls[] = $should_load; + $this->record_should_load_calls(); - return $should_load; - } - ); + $this->runner()->load_all(); - Loader::load_all(); + $this->assertSame( [], $this->should_load_calls ); - $this->assertSame( [], $calls ); + $this->assert_the_should_load_recorder_works(); + } - // The recorder has to be shown to work. Without this, a filter that never attached — a - // mistyped hook name, an add_filter() that ran too late — leaves the array empty for a - // reason that has nothing to do with the sub-plugin being disabled, and the assertion above - // passes having proved nothing at all. - apply_filters( 'give/plugin_absorber/should_load', true ); + /** + * The same guarantee at the far end of the chain: a sub-plugin that failed the dependency check + * has already earned its notice, and asking the filter as well would offer a host a veto over a + * load that was never going to happen. + */ + public function test_the_should_load_filter_is_not_consulted_when_dependencies_are_unmet(): void { + $this->register( [ 'dependency_check' => static fn() => false ] ); + + $this->record_should_load_calls(); + + $this->runner()->load_all(); - $this->assertSame( [ true ], $calls, 'The recorder must catch a call that really happened.' ); + $this->assertSame( [], $this->should_load_calls ); + + $this->assert_the_should_load_recorder_works(); } public function test_it_loads_every_registered_sub_plugin(): void { $this->register( [ 'slug' => 'give-recurring' ] ); $this->register( [ 'slug' => 'give-fee-recovery' ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 2, $GLOBALS['absorber_loads'] ); + $this->assertSame( 2, $this->bundled_plugin_loads() ); } /** @@ -318,18 +333,18 @@ public function test_a_skipped_sub_plugin_does_not_stop_the_others(): void { $this->register( [ 'slug' => 'give-recurring', 'enabled' => false ] ); $this->register( [ 'slug' => 'give-fee-recovery' ] ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); } /** - * Registrar_Interface::all() can only declare `array`, so a host implementation is free to - * return anything. The default Registrar cannot produce this state — only a bound one can. + * Registrar_Interface::all() can only declare `array`, so a host implementation is free to return + * anything. The default Registrar cannot produce this state — only a bound one can. */ - public function test_load_all_ignores_entries_that_are_not_sub_plugins(): void { - $constant = 'ABSORBER_MIXED_REGISTRY_GUARD'; - $path = $this->make_fixture( $constant ); + public function test_it_ignores_entries_that_are_not_sub_plugins(): void { + $constant = $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); $sub_plugin = new Sub_Plugin( [ 'slug' => 'give-recurring', @@ -356,10 +371,10 @@ public function register( Sub_Plugin $sub_plugin ): void { } public function all(): array { - // The interface can only declare `array`, and its docblock is a promise a host is - // free to break — which is exactly what this double is here to do. Restating the - // promised shape is what lets the analyser check every other implementation - // strictly while this one hands back the junk the Loader has to survive. + // The interface can only declare `array`, and its docblock is a promise a host is free + // to break — which is exactly what this double is here to do. Restating the promised + // shape is what lets the analyser check every other implementation strictly while this + // one hands back the junk the load path has to survive. /** @var array $entries */ $entries = $this->entries; @@ -368,159 +383,155 @@ public function all(): array { }; $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $registrar ); - Config::set_container( $container ); - - Loader::load_all(); - - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); - } - - /** - * Buffering registration is what lets the container arrive after the sub-plugins do, and the - * registry is only half of that: the load path resolves the notice queue as well. A - * collaborator pinned by an eager resolve inside register() would leave the host's binding - * bound and never used. - */ - public function test_a_container_set_after_register_reaches_the_load_path(): void { - // The harness boots WordPress before any test runs, so plugins_loaded has already fired and - // boot() would rightly report that wiring at priority 2 is too late and load inline - // instead. Rewind it so boot() takes the path a real host bootstrap takes; tearDown puts - // the count back. - $this->plugins_loaded_count = did_action( 'plugins_loaded' ); - unset( $GLOBALS['wp_actions']['plugins_loaded'] ); - - $this->register( [ 'dependency_check' => static fn() => false ] ); - - $notices = new class() implements Queue_Interface { - /** - * Slugs handed to queue_dependency_notice(), in order. - * - * @var string[] - */ - public $dependency_notices = []; - - public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { - } - - public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + $container->singleton( + Registrar_Interface::class, + static function () use ( $registrar ): Registrar_Interface { + return $registrar; } - - public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { - $this->dependency_notices[] = $sub_plugin->get_slug(); - } - - public function render(): void { - } - }; - - // The registrar is bound as well as the queue. Without it the sub-plugin would sit in the - // default registrar either way and the notice would arrive however register() behaved, so - // the test would pass without the buffer existing at all. - $registrar = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $registrar ); - $container->singleton( Queue_Interface::class, static fn() => $notices ); - Config::set_container( $container ); - - Loader::boot(); - - $this->assertSame( - 2, - has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ), - 'boot() must have wired the load hook rather than loading inline.' ); + $this->set_up_container( $container ); - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( - [ 'give-recurring' ], - $notices->dependency_notices, - 'A binding made after register() has to reach the load path.' - ); - $this->assertSame( - [], - $this->notice_queue(), - 'The default queue must not have been resolved alongside it.' - ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); } /** - * require_once dedupes by resolved path, so one file behind two registrations executes once - * even when the second one's guard constant never gets defined. + * require_once dedupes by resolved path, so one file behind two registrations executes once even + * when the second one's guard constant never gets defined. */ public function test_one_bundled_file_behind_two_registrations_loads_once(): void { - $path = sys_get_temp_dir() . '/absorber-shared-' . uniqid( '', true ) . '.php'; - file_put_contents( - $path, - 'fixtures[] = $path; + $path = $this->make_bundled_plugin_file( $this->make_guard_constant() ); - foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $index => $slug ) { + foreach ( [ 'give-recurring', 'give-fee-recovery' ] as $slug ) { Loader::register( [ 'slug' => $slug, 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => 'ABSORBER_SHARED_FILE_GUARD_' . $index, + 'plugin_loaded_constant' => $this->make_guard_constant(), ] ); } - Loader::load_all(); + $this->runner()->load_all(); - $this->assertSame( 1, $GLOBALS['absorber_loads'] ); + $this->assertSame( 1, $this->bundled_plugin_loads() ); } + /** + * The load path needs the prefix for the should_load filter and for the notice store. Throwing + * out of a core action would take the whole site down over a bootstrap mistake, so it is reported + * where a developer will see it and the load is abandoned instead. + */ public function test_load_all_does_nothing_without_a_hook_prefix(): void { $this->register(); + $runner = $this->runner(); + $container = $this->container(); + + // The prefix goes, the container stays: this is about the missing prefix, and a library that + // reached the container first would fail this test for the other reason. Config_State::reset(); - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + Config::set_container( $container ); + $this->expect_incorrect_usage(); - Loader::load_all(); + $runner->load_all(); - $this->assertSame( 0, $GLOBALS['absorber_loads'], 'A bootstrap mistake must not fatal the site.' ); + $this->assertSame( 0, $this->bundled_plugin_loads(), 'A bootstrap mistake must not fatal the site.' ); + $this->assert_the_library_reported_incorrect_usage(); } - public function test_render_notices_delegates_to_the_bound_collaborator(): void { - $notices = new class() implements Queue_Interface { - /** - * @var int - */ - public $render_calls = 0; + /** + * Registration is buffered, which is what lets the container arrive after the sub-plugins do — + * and the registry is only half of that: the load path resolves the notice queue as well. A + * collaborator pinned by an eager resolve inside register() would leave the host's binding bound + * and never used. + */ + public function test_a_container_set_after_register_reaches_the_load_path(): void { + Config_State::reset(); + Config::set_hook_prefix( 'give' ); - public function queue_merge_notice( Sub_Plugin $sub_plugin ): void { - } + $this->register( [ 'dependency_check' => static fn() => false ] ); - public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { - } + $notices = new Spy_Queue(); - public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + // The registrar is bound as well as the queue. Without it the sub-plugin would sit in the + // default registrar either way and the notice would arrive however register() behaved, so the + // test would pass without the buffer existing at all. + $registrar = new Spy_Registrar(); + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $registrar ): Registrar_Interface { + return $registrar; } - - public function render(): void { - ++$this->render_calls; + ); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Queue_Interface { + return $notices; } - }; + ); + $this->set_up_container( $container ); - $container = new Test_Container(); - $container->singleton( Queue_Interface::class, static fn() => $notices ); - Config::set_container( $container ); + $this->runner()->load_all(); - Loader::render_notices(); + $this->assertSame( + [ 'give-recurring' ], + $notices->dependency_notices, + 'A binding made after register() has to reach the load path.' + ); + $this->assertSame( + [], + $this->notice_queue(), + 'The default queue must not have been resolved alongside it.' + ); + } - $this->assertSame( 1, $notices->render_calls ); + /** + * The runner as the container builds it, which is how the scheduler reaches it too. + * + * @return Runner + */ + private function runner(): Runner { + return $this->resolve( Runner::class ); } - public function test_render_notices_does_nothing_without_a_hook_prefix(): void { - Config_State::reset(); - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader' ); + /** + * Record every should_load call, so a test can assert there were none. + * + * @return void + */ + private function record_should_load_calls(): void { + $calls = &$this->should_load_calls; - ob_start(); - Loader::render_notices(); + add_filter( + 'give/plugin_absorber/should_load', + static function ( $should_load ) use ( &$calls ) { + $calls[] = $should_load; + + return $should_load; + } + ); + } + + /** + * Show the recorder works, having just asserted it caught nothing. + * + * Without this, a filter that never attached — a mistyped hook name, an add_filter() that ran too + * late — leaves the array empty for a reason that has nothing to do with the gate under test, and + * the assertion passes having proved nothing at all. + * + * @return void + */ + private function assert_the_should_load_recorder_works(): void { + apply_filters( 'give/plugin_absorber/should_load', true ); - $this->assertSame( '', (string) ob_get_clean() ); + $this->assertSame( + [ true ], + $this->should_load_calls, + 'The recorder must catch a call that really happened.' + ); } private function clear_notices(): void { @@ -557,35 +568,15 @@ private function define_guard( string $constant ): string { return $constant; } - /** - * Write a throwaway bundled plugin that counts its own loads and defines its guard constant. - * - * A unique path per test is required: require_once caches by resolved path for the lifetime of - * the PHP process, so a shared fixture would make later tests pass without loading anything. - * - * @param string $constant Guard constant to define. - */ - private function make_fixture( string $constant ): string { - $path = sys_get_temp_dir() . '/absorber-' . uniqid( '', true ) . '.php'; - - file_put_contents( - $path, - 'fixtures[] = $path; - - return $path; - } - /** * @param array $overrides Config overrides. + * @param string|null $constant Guard constant to use, or a fresh one. + * + * @return string */ private function register( array $overrides = [], ?string $constant = null ): string { - $constant = $constant ?? 'ABSORBER_FIXTURE_' . strtoupper( bin2hex( random_bytes( 4 ) ) ); - $path = $this->make_fixture( $constant ); + $constant = $constant ?? $this->make_guard_constant(); + $path = $this->make_bundled_plugin_file( $constant ); Loader::register( array_merge( diff --git a/tests/unit/LoaderBootTest.php b/tests/unit/LoaderBootTest.php deleted file mode 100644 index b4a185f..0000000 --- a/tests/unit/LoaderBootTest.php +++ /dev/null @@ -1,271 +0,0 @@ - - */ - private $added_actions = []; - - /** - * @var string[] - */ - private $fixtures = []; - - public function setUp(): void { - parent::setUp(); - - Loader_State::reset(); - Config_State::reset(); - Config::set_hook_prefix( 'give' ); - - // The harness has to boot WordPress before it can run anything, so plugins_loaded has - // already fired by the time any test starts — and boot() would rightly report that it is - // too late to wire a hook at priority 2. Rewind the counter so the wiring tests see the - // timing a real host bootstrap sees. The tests that exercise a late boot set it back. - $this->plugins_loaded_count = did_action( 'plugins_loaded' ); - unset( $GLOBALS['wp_actions']['plugins_loaded'] ); - - $GLOBALS[ self::LOADED ] = false; - } - - public function tearDown(): void { - $GLOBALS['wp_actions']['plugins_loaded'] = $this->plugins_loaded_count; - - // In tearDown rather than at the end of the test body: a failing assertion would otherwise - // leak an admin screen into every test that runs after it, since is_admin() checks the - // current screen before WP_ADMIN. - set_current_screen( 'front' ); - - // Only what these tests added by hand. The two hooks boot() wires come off in - // Loader_State::reset(), which reads the load priority from the Loader rather than - // restating it. - foreach ( $this->added_actions as [ $hook, $callback, $priority ] ) { - remove_action( $hook, $callback, $priority ); - } - $this->added_actions = []; - - foreach ( $this->fixtures as $fixture ) { - if ( file_exists( $fixture ) ) { - unlink( $fixture ); - } - } - $this->fixtures = []; - - unset( $GLOBALS[ self::LOADED ] ); - Loader_State::reset(); - Config_State::reset(); - parent::tearDown(); - } - - public function test_it_wires_the_load_hook_at_priority_two(): void { - Loader::boot(); - - $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); - } - - public function test_booting_twice_wires_the_hook_only_once(): void { - Loader::boot(); - Loader::boot(); - - $callbacks = $GLOBALS['wp_filter']['plugins_loaded']->callbacks[2] ?? []; - - $this->assertCount( 1, $callbacks, 'boot() must be idempotent.' ); - } - - public function test_it_wires_the_admin_notices_hook_in_the_admin(): void { - set_current_screen( 'dashboard' ); - - Loader::boot(); - - $this->assertNotFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); - } - - /** - * Nothing renders a notice on the front end, and the queue must survive until an admin load - * consumes it. - */ - public function test_it_does_not_wire_the_notice_hook_on_the_front_end(): void { - set_current_screen( 'front' ); - - Loader::boot(); - - $this->assertFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); - } - - /** - * Adding an action at a priority the running dispatch has already passed is accepted and then - * never fires. Booting from plugins_loaded at the default priority instead of 0 would - * otherwise load nothing at all, on a site that looks completely healthy. - * - * Priority 2 is the boundary case: a callback added to the priority currently being dispatched - * is never reached either, because the dispatch loop walks a by-value copy of that priority's - * callback array. - * - * @dataProvider late_boot_priorities - * - * @param int $priority plugins_loaded priority the host boots from. - */ - public function test_booting_too_late_in_plugins_loaded_loads_inline_instead( int $priority ): void { - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); - - $path = $this->make_fixture(); - - $this->add_tracked_action( - 'plugins_loaded', - static function () use ( $path, $priority ) { - Loader::register( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => 'ABSORBER_LATE_BOOT_GUARD_' . $priority, - ] - ); - - Loader::boot(); - }, - $priority - ); - - do_action( 'plugins_loaded' ); - - $this->assertTrue( $GLOBALS[ self::LOADED ], 'A late boot must still load.' ); - } - - /** - * @return Generator - */ - public static function late_boot_priorities(): Generator { - yield 'at the load priority' => [ 2 ]; - yield 'one past it' => [ 3 ]; - yield 'the default a host omits' => [ 10 ]; - } - - public function test_booting_after_plugins_loaded_has_finished_loads_inline(): void { - $this->setExpectedIncorrectUsage( 'Nexcess\PluginAbsorber\Loader::boot' ); - - $path = $this->make_fixture(); - - do_action( 'plugins_loaded' ); - - Loader::register( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => $path, - 'plugin_loaded_constant' => 'ABSORBER_AFTER_BOOT_GUARD', - ] - ); - Loader::boot(); - - $this->assertTrue( $GLOBALS[ self::LOADED ] ); - } - - /** - * The suite's state helper stands in for the reset() the Loader deliberately does not ship. - * Clearing the boot flag without unwiring would leave a Loader that reports itself unbooted - * while its callbacks are still attached — and every later test would load sub-plugins it - * never registered. - */ - public function test_the_state_helper_unwires_the_hooks_boot_added(): void { - set_current_screen( 'dashboard' ); - - Loader::boot(); - Loader_State::reset(); - - $this->assertFalse( has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); - $this->assertFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); - } - - /** - * The hooks are gone by the time this boots again, so the assertion is about the second boot - * rather than a leftover from the first. - */ - public function test_the_state_helper_allows_booting_again(): void { - Loader::boot(); - Loader_State::reset(); - - Loader::boot(); - - $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); - } - - /** - * boot() runs before anything reads the prefix, so it must not be the thing that throws when - * a host forgot to set one — the load path reports that at a point the host can see. - */ - public function test_boot_does_not_need_a_hook_prefix(): void { - Config_State::reset(); - - Loader::boot(); - - $this->assertSame( 2, has_action( 'plugins_loaded', [ Loader::class, 'load_all' ] ) ); - } - - /** - * Write a throwaway bundled plugin that records its own load. - * - * A unique path per test is required: require_once caches by resolved path for the lifetime of - * the PHP process, so a shared fixture would let a later test pass without loading anything. - * - * @return string - */ - private function make_fixture(): string { - $path = sys_get_temp_dir() . '/absorber-boot-' . uniqid( '', true ) . '.php'; - - file_put_contents( - $path, - 'fixtures[] = $path; - - return $path; - } - - /** - * Add an action tearDown can take back by identity rather than by clearing the whole hook. - * - * @param string $hook Hook to add to. - * @param callable $callback Callback to add. - * @param int $priority Priority to add it at. - * - * @return void - */ - private function add_tracked_action( string $hook, callable $callback, int $priority = 10 ): void { - $this->added_actions[] = [ $hook, $callback, $priority ]; - - add_action( $hook, $callback, $priority ); - } -} diff --git a/tests/unit/LoaderResolveTest.php b/tests/unit/LoaderResolveTest.php deleted file mode 100644 index 2f7b83f..0000000 --- a/tests/unit/LoaderResolveTest.php +++ /dev/null @@ -1,403 +0,0 @@ -assertInstanceOf( Registrar::class, Loader::registrar() ); - } - - public function test_it_memoizes_the_resolved_collaborator(): void { - $this->assertSame( Loader::registrar(), Loader::registrar() ); - } - - /** - * Both binding styles arrive at the same instance: the memo is what makes even a bind(), which - * the container would otherwise rebuild on every call, resolve exactly once. - * - * @dataProvider container_binding_methods - * - * @param string $binding_method Container method the host bound the registrar with. - */ - public function test_it_resolves_a_bound_registrar_from_the_container( string $binding_method ): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->{$binding_method}( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - $this->assertSame( $bound, Loader::registrar() ); - $this->assertSame( $bound, Loader::registrar(), 'The binding must be resolved only once.' ); - } - - /** - * @return Generator - */ - public static function container_binding_methods(): Generator { - yield 'singleton' => [ 'singleton' ]; - yield 'bind' => [ 'bind' ]; - } - - public function test_it_ignores_a_container_with_no_binding(): void { - $container = new Test_Container(); - - $this->assertFalse( - $container->has( Registrar_Interface::class ), - 'DI52 reports has() true for any existing class name; this must stay an interface.' - ); - - Config::set_container( $container ); - - $this->assertInstanceOf( Registrar::class, Loader::registrar() ); - } - - /** - * Checked before the instance is memoized. Caching it would make every accessor throw a - * TypeError blaming the library, with no way back. - * - * @dataProvider unusable_bindings - * - * @param mixed $bound Whatever the host's factory hands back. - * @param string $reported_type How the rejection is expected to name it. - */ - public function test_it_rejects_a_binding_that_does_not_implement_the_interface( - $bound, - string $reported_type - ): void { - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - try { - Loader::registrar(); - $this->fail( 'Expected a Config_Exception.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( Registrar_Interface::class, $exception->getMessage() ); - $this->assertStringContainsString( - $reported_type, - $exception->getMessage(), - 'The message has to name what came back, or the host cannot tell which binding is wrong.' - ); - } - - Config_State::reset(); - - $this->assertInstanceOf( - Registrar::class, - Loader::registrar(), - 'The bad instance must not have been cached, or there would be no way back.' - ); - } - - /** - * A factory is free to return anything at all, and the two non-object cases are the ones that - * reach the branch reporting a type name rather than a class name. - * - * @return Generator - */ - public static function unusable_bindings(): Generator { - yield 'an instance of the wrong class' => [ new stdClass(), 'stdClass' ]; - yield 'the class name instead of an instance' => [ Registrar::class, 'string' ]; - yield 'nothing at all' => [ null, 'NULL' ]; - } - - /** - * A bound factory may throw, and a container asked for something it cannot build throws its own - * exception type; the contract is explicit that has() true does not promise get() succeeds. - * Left uncaught, the host gets a fatal from a vendor namespace at plugins_loaded that names - * neither this library nor the binding at fault. - */ - public function test_it_reports_a_container_that_throws_as_a_configuration_error(): void { - $failure = new RuntimeException( 'the host factory needed a database connection' ); - $container = new Test_Container(); - $container->singleton( - Registrar_Interface::class, - static function () use ( $failure ): Registrar_Interface { - throw $failure; - } - ); - Config::set_container( $container ); - - try { - Loader::registrar(); - $this->fail( 'Expected a Config_Exception.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( - Registrar_Interface::class, - $exception->getMessage(), - 'The message has to name the binding that could not be built.' - ); - $this->assertContains( - $failure, - $this->previous_chain( $exception ), - 'The original failure has to stay reachable, or the real cause is lost.' - ); - } - - Config_State::reset(); - - $this->assertInstanceOf( - Registrar::class, - Loader::registrar(), - 'A binding that could not be built must not have been memoized.' - ); - } - - /** - * A binding that could not be built leaves the registrations buffered, so the read that comes - * after the host fixes its container still has them. Emptying the buffer before the registrar - * resolved would drop them silently and load nothing. - */ - public function test_registrations_survive_a_container_that_throws(): void { - $container = new Test_Container(); - $container->singleton( - Registrar_Interface::class, - static function (): Registrar_Interface { - throw new RuntimeException( 'not today' ); - } - ); - Config::set_container( $container ); - - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - try { - Loader::all(); - $this->fail( 'Expected a Config_Exception.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( Registrar_Interface::class, $exception->getMessage() ); - } - - Config_State::reset(); - - $this->assertArrayHasKey( - 'give-recurring', - Loader::all(), - 'The buffered registration must still be there once the container is usable.' - ); - } - - /** - * The memo is what makes the container optional without paying for a lookup per call. Reading a - * collaborator is a boot-time act, so a host that has done it has finished configuring. - */ - public function test_a_container_set_after_the_first_resolve_does_not_change_the_memo(): void { - $default = Loader::registrar(); - - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - $this->assertSame( - $default, - Loader::registrar(), - 'Swapping a collaborator mid-request would strand whatever already holds the old one.' - ); - } - - public function test_register_builds_a_sub_plugin_and_stores_it(): void { - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - $all = Loader::all(); - - $this->assertCount( 1, $all ); - $this->assertInstanceOf( Sub_Plugin::class, $all['give-recurring'] ); - $this->assertSame( 'give-recurring', $all['give-recurring']->get_slug() ); - } - - public function test_register_rejects_an_invalid_config(): void { - $this->expectException( Config_Exception::class ); - - Loader::register( [ 'slug' => 'give-recurring' ] ); - } - - /** - * Registering must not resolve anything, or the first register() call would pin the default - * registrar for the whole request. - */ - public function test_register_resolves_nothing_until_the_first_read(): void { - $builds = 0; - $container = new Test_Container(); - $container->singleton( - Registrar_Interface::class, - static function () use ( &$builds ): Registrar_Interface { - ++$builds; - - return new Spy_Registrar(); - } - ); - Config::set_container( $container ); - - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - $this->assertSame( 0, $builds, 'register() must not reach the container.' ); - - Loader::all(); - - $this->assertSame( 1, $builds, 'The first read is what resolves the registrar.' ); - } - - /** - * The headline of deferring registration: the container is a configuration call like any other, - * so it may arrive after the sub-plugins do. Resolving inside register() dropped it silently. - */ - public function test_a_container_set_after_register_takes_effect(): void { - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - Loader::all(); - - $this->assertArrayHasKey( - 'give-recurring', - $bound->sub_plugins, - 'The bound registrar must receive registrations made before it was bound.' - ); - } - - public function test_register_delegates_to_a_bound_registrar(): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - Loader::all(); - - $this->assertArrayHasKey( 'give-recurring', $bound->sub_plugins ); - } - - /** - * The buffer is emptied as it drains. Left full, the second read hands the registrar a slug it - * already holds and the duplicate guard fires on a registration the host only made once. - */ - public function test_reading_twice_does_not_register_twice(): void { - $bound = new Spy_Registrar(); - $container = new Test_Container(); - $container->singleton( Registrar_Interface::class, static fn() => $bound ); - Config::set_container( $container ); - - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - Loader::all(); - Loader::all(); - - $this->assertSame( 1, $bound->register_calls ); - } - - /** - * Deferring registration moves the duplicate-slug report from the second register() call to the - * first read. It still names both bundled files, which is what the host needs to find them. - */ - public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - Loader::register( - [ - 'slug' => 'give-recurring', - 'bundled_plugin_file' => '/tmp/other/other.php', - 'plugin_loaded_constant' => 'OTHER_VERSION_FIXTURE', - ] - ); - - try { - Loader::all(); - $this->fail( 'Expected a Config_Exception.' ); - } catch ( Config_Exception $exception ) { - $this->assertStringContainsString( 'give-recurring', $exception->getMessage() ); - $this->assertStringContainsString( '/tmp/other/other.php', $exception->getMessage() ); - } - } - - public function test_all_is_empty_before_anything_is_registered(): void { - $this->assertSame( [], Loader::all() ); - } - - /** - * The buffer is static state like the memo, so the suite's reset helper has to reach it too. - * A registration left buffered by one test drains into the next test's registrar. - */ - public function test_the_state_helper_clears_buffered_registrations(): void { - Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); - - Loader_State::reset(); - - $this->assertSame( [], Loader::all() ); - } - - /** - * Build the raw configuration array a host writes. - * - * The shared WithSubPlugins trait builds `Sub_Plugin` objects; `Loader::register()` takes the - * array instead, because building that object is the part of registration under test here. The - * derived values follow the trait's: a per-slug bundled file, and a guard constant carrying the - * `_FIXTURE` suffix nothing ever defines, since a define() lasts for the whole PHP process. - * - * @param string $slug Sub-plugin slug. - * - * @return array - */ - private function sub_plugin_config( string $slug ): array { - return [ - 'slug' => $slug, - 'bundled_plugin_file' => "/tmp/{$slug}/{$slug}.php", - 'plugin_loaded_constant' => strtoupper( str_replace( '-', '_', $slug ) ) . '_VERSION_FIXTURE', - ]; - } - - /** - * Every exception behind the given one, nearest first. - * - * Walked rather than read a single level deep: a container is entitled to wrap what a factory - * threw in its own exception type before it reaches us, and it does. - * - * @param Throwable $exception Exception to walk. - * - * @return Throwable[] - */ - private function previous_chain( Throwable $exception ): array { - $chain = []; - - for ( $previous = $exception->getPrevious(); $previous !== null; $previous = $previous->getPrevious() ) { - $chain[] = $previous; - } - - return $chain; - } -} diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php new file mode 100644 index 0000000..ebe2fc6 --- /dev/null +++ b/tests/unit/LoaderTest.php @@ -0,0 +1,402 @@ +stop_expecting_incorrect_usage(); + Loader_State::reset(); + Config_State::reset(); + $this->tear_down_container(); + parent::tearDown(); + } + + /** + * @dataProvider collaborator_accessors + * + * @param string $accessor Static method on Loader. + * @param class-string $expected Class the provider's default binding builds. + */ + public function test_the_accessors_read_from_the_container( string $accessor, string $expected ): void { + $this->set_up_container(); + + $this->assertInstanceOf( $expected, Loader::{$accessor}() ); + } + + /** + * A host binds the wrong class far more easily than it binds none at all, and the container + * hands back whatever it was told to without checking. Left to PHP's own return-type check the + * failure is a TypeError naming this library's method, raised inside plugins_loaded, which + * reads as a bug here rather than as the typo it is. + */ + public function test_a_binding_that_does_not_implement_its_interface_is_reported(): void { + $container = new Test_Container(); + + $container->singleton( Registrar_Interface::class, static function (): object { + return new class() { + // Anything at all, so long as it is not a registrar. + }; + } ); + + Config::set_container( $container ); + + $this->expectException( Config_Exception::class ); + $this->expectExceptionMessage( Registrar_Interface::class ); + + Loader::registrar(); + } + + /** + * The accessors are the host's own way in, so each one has to survive the container becoming + * required rather than only the paths the library happens to take. + * + * @return Generator + */ + public static function collaborator_accessors(): Generator { + yield 'the registrar' => [ 'registrar', Registrar::class ]; + yield 'the notice queue' => [ 'notices', Queue::class ]; + } + + /** + * Nothing falls back to `new` any more. A host that never set a container gets a + * Config_Exception naming the mistake, not a default collaborator that quietly ignores the + * bindings it was going to make. + * + * @dataProvider accessor_names + * + * @param string $accessor Static method on Loader. + */ + public function test_an_accessor_without_a_container_is_a_configuration_error( string $accessor ): void { + $this->expectException( Config_Exception::class ); + + Loader::{$accessor}(); + } + + /** + * @return Generator + */ + public static function accessor_names(): Generator { + yield 'the registrar' => [ 'registrar' ]; + yield 'the notice queue' => [ 'notices' ]; + } + + /** + * Whichever way the host bound it. `bind()` rebuilds per call where `singleton()` does not, and + * the accessor hands back the host's object either way — a library that cached the first resolve + * itself would make the difference invisible and the rebinding untestable. + * + * @dataProvider container_binding_methods + * + * @param string $binding_method Container method the host bound the registrar with. + */ + public function test_it_resolves_a_bound_registrar_from_the_container( string $binding_method ): void { + $bound = new Spy_Registrar(); + $container = new Test_Container(); + $container->{$binding_method}( + Registrar_Interface::class, + static function () use ( $bound ): Registrar_Interface { + return $bound; + } + ); + $this->set_up_container( $container ); + + $this->assertSame( $bound, Loader::registrar() ); + $this->assertSame( $bound, Loader::registrar() ); + } + + /** + * @return Generator + */ + public static function container_binding_methods(): Generator { + yield 'singleton' => [ 'singleton' ]; + yield 'bind' => [ 'bind' ]; + } + + public function test_register_builds_a_sub_plugin_and_stores_it(): void { + $this->set_up_container(); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $all = Loader::all(); + + $this->assertCount( 1, $all ); + $this->assertInstanceOf( Sub_Plugin::class, $all['give-recurring'] ); + $this->assertSame( 'give-recurring', $all['give-recurring']->get_slug() ); + } + + public function test_register_rejects_an_invalid_config(): void { + $this->set_up_container(); + + $this->expectException( Config_Exception::class ); + + Loader::register( [ 'slug' => 'give-recurring' ] ); + } + + /** + * Registering must resolve nothing at all — not even the container. The host container LearnDash + * hands us is *replaced* at plugins_loaded 0, so anything that touched a container before that + * point holds an orphan whose bindings were thrown away. + */ + public function test_register_needs_no_container(): void { + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $this->set_up_container(); + + $this->assertArrayHasKey( 'give-recurring', Loader::all() ); + } + + /** + * The other half of the same guarantee: with a container set, the first register() must still not + * reach into it, or it would pin the registrar before the host finished binding. + */ + public function test_register_resolves_nothing_until_the_first_read(): void { + $builds = 0; + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( &$builds ): Registrar_Interface { + ++$builds; + + return new Spy_Registrar(); + } + ); + $this->set_up_container( $container ); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $this->assertSame( 0, $builds, 'register() must not reach the container.' ); + + Loader::all(); + + $this->assertSame( 1, $builds, 'The first read is what resolves the registrar.' ); + } + + public function test_register_delegates_to_a_bound_registrar(): void { + $bound = $this->bind_registrar(); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + Loader::all(); + + $this->assertArrayHasKey( 'give-recurring', $bound->sub_plugins ); + } + + /** + * The buffer is emptied as it drains. Left full, the second read hands the registrar a slug it + * already holds and the duplicate guard fires on a registration the host only made once. + */ + public function test_reading_twice_does_not_register_twice(): void { + $bound = $this->bind_registrar(); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + Loader::all(); + Loader::all(); + + $this->assertSame( 1, $bound->register_calls ); + } + + /** + * Deferring registration moves the duplicate-slug report from the second register() call to the + * first read. It still names both bundled files, which is what the host needs to find them. + */ + public function test_a_duplicate_slug_is_refused_at_the_first_read(): void { + $this->set_up_container(); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + Loader::register( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/other/other.php', + 'plugin_loaded_constant' => 'OTHER_VERSION_FIXTURE', + ] + ); + + try { + Loader::all(); + $this->fail( 'Expected a Config_Exception.' ); + } catch ( Config_Exception $exception ) { + $this->assertStringContainsString( 'give-recurring', $exception->getMessage() ); + $this->assertStringContainsString( '/tmp/other/other.php', $exception->getMessage() ); + } + } + + public function test_all_is_empty_before_anything_is_registered(): void { + $this->set_up_container(); + + $this->assertSame( [], Loader::all() ); + } + + /** + * A binding that cannot be built leaves the registrations buffered, so the read that comes after + * the host fixes its container still has them. Emptying the buffer before the registrar resolved + * would drop them silently and load nothing. + * + * What the container throws is the container's business — the contract is explicit that has() + * true does not promise get() succeeds — so this catches Throwable rather than pinning a type + * this library does not own. + */ + public function test_registrations_survive_a_registrar_the_container_cannot_build(): void { + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function (): Registrar_Interface { + throw new RuntimeException( 'the host factory needed a database connection' ); + } + ); + $this->set_up_container( $container ); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + $failed = false; + + try { + Loader::all(); + } catch ( Throwable $exception ) { + $failed = true; + } + + $this->assertTrue( $failed, 'A registrar that cannot be built has to surface, not be swallowed.' ); + + $this->set_up_container(); + + $this->assertArrayHasKey( + 'give-recurring', + Loader::all(), + 'The buffered registration must still be there once the container is usable.' + ); + } + + /** + * The buffer is static state, so the suite's reset helper has to reach it: a registration left + * buffered by one test drains into the next test's registrar. + */ + public function test_the_state_helper_clears_buffered_registrations(): void { + $this->set_up_container(); + + Loader::register( $this->sub_plugin_config( 'give-recurring' ) ); + + Loader_State::reset(); + + $this->assertSame( [], Loader::all() ); + } + + public function test_render_notices_delegates_to_the_bound_queue(): void { + $notices = new Spy_Queue(); + $container = new Test_Container(); + $container->singleton( + Queue_Interface::class, + static function () use ( $notices ): Queue_Interface { + return $notices; + } + ); + $this->set_up_container( $container ); + + Loader::render_notices(); + + $this->assertSame( 1, $notices->render_calls ); + } + + public function test_render_notices_does_nothing_without_a_hook_prefix(): void { + $container = $this->set_up_container(); + + Config_State::reset(); + Config::set_container( $container ); + $this->expect_incorrect_usage(); + + ob_start(); + + try { + Loader::render_notices(); + } finally { + // In a finally block so a throw cannot leave the suite's own output trapped in an + // abandoned buffer. + $output = (string) ob_get_clean(); + } + + $this->assertSame( '', $output ); + $this->assert_the_library_reported_incorrect_usage(); + } + + /** + * Bind a recording registrar, in the order a host binds one: before the provider fills in what is + * missing. + * + * @return Spy_Registrar + */ + private function bind_registrar(): Spy_Registrar { + $bound = new Spy_Registrar(); + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $bound ): Registrar_Interface { + return $bound; + } + ); + + $this->set_up_container( $container ); + + return $bound; + } + + /** + * Build the raw configuration array a host writes. + * + * The shared WithSubPlugins trait builds `Sub_Plugin` objects; `Loader::register()` takes the + * array instead, because building that object is the part of registration under test here. The + * derived values follow the trait's: a per-slug bundled file, and a guard constant carrying the + * `_FIXTURE` suffix nothing ever defines, since a define() lasts for the whole PHP process. + * + * @param string $slug Sub-plugin slug. + * + * @return array + */ + private function sub_plugin_config( string $slug ): array { + return [ + 'slug' => $slug, + 'bundled_plugin_file' => "/tmp/{$slug}/{$slug}.php", + 'plugin_loaded_constant' => strtoupper( str_replace( '-', '_', $slug ) ) . '_VERSION_FIXTURE', + ]; + } +} diff --git a/tests/unit/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php index b8dd041..8551bd4 100644 --- a/tests/unit/Notices/QueueTest.php +++ b/tests/unit/Notices/QueueTest.php @@ -59,7 +59,7 @@ public function tearDown(): void { } public function test_the_default_notices_satisfy_the_contract(): void { - $this->assertInstanceOf( Queue_Interface::class, new Queue() ); + $this->assertInstanceOf( Queue_Interface::class, $this->make_queue() ); } /** @@ -78,7 +78,7 @@ public function test_it_queues_a_notice( string $expected, bool $exact ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->{$method}( $this->make_sub_plugin( $overrides ) ); $queue = $this->queue(); @@ -159,7 +159,7 @@ public static function queued_notices(): Generator { * already happened, the other asks the user to do it. Sharing a default would be wrong. */ public function test_the_merge_and_conflict_defaults_differ(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_conflict_notice( $this->make_sub_plugin() ); @@ -169,7 +169,7 @@ public function test_the_merge_and_conflict_defaults_differ(): void { } public function test_a_configured_message_is_used_for_both_conflict_types(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Ours.' ] ) ); @@ -184,7 +184,7 @@ public function test_a_configured_message_is_used_for_both_conflict_types(): voi } public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_merge_notice( $this->make_sub_plugin() ); @@ -192,7 +192,7 @@ public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): } public function test_one_slug_can_hold_notices_of_different_types(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_dependency_notice( $this->make_sub_plugin() ); @@ -200,7 +200,7 @@ public function test_one_slug_can_hold_notices_of_different_types(): void { } public function test_different_slugs_do_not_collide(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'slug' => 'give-fee-recovery' ] ) ); @@ -212,7 +212,7 @@ public function test_different_slugs_do_not_collide(): void { } public function test_render_outputs_dismissible_markup(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -230,7 +230,7 @@ public function test_render_outputs_dismissible_markup(): void { * @param string $class Expected `notice-*` class. */ public function test_render_uses_the_severity_of_the_notice_type( string $method, string $class ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->{$method}( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Something happened.' ] ) ); @@ -251,7 +251,7 @@ public static function notice_severities(): Generator { } public function test_render_strips_a_script_from_the_message(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Careful.' ] ) ); @@ -270,7 +270,7 @@ public function test_render_strips_a_script_from_the_message(): void { * typically — while the event handler a message must never be able to ship is stripped. */ public function test_render_keeps_a_link_but_not_an_event_handler(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'See the docs.' ] @@ -284,7 +284,7 @@ public function test_render_keeps_a_link_but_not_an_event_handler(): void { } public function test_render_clears_the_queue(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->render_to_string( $notices ); @@ -294,7 +294,7 @@ public function test_render_clears_the_queue(): void { } public function test_render_outputs_every_queued_notice(): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'First.' ] ) ); @@ -309,13 +309,13 @@ public function test_render_outputs_every_queued_notice(): void { } public function test_render_outputs_nothing_when_the_queue_is_empty(): void { - $this->assertSame( '', $this->render_to_string( new Queue() ) ); + $this->assertSame( '', $this->render_to_string( $this->make_queue() ) ); } /** * Where notices are kept is a constructor argument, so a host can move the queue somewhere else - * without also taking on how notices are worded or drawn. Both arguments default, so - * `new Queue()` — which is what Loader::resolve() builds — is unaffected. + * without also taking on how notices are worded or drawn. Both arguments are required and both + * are bound by `Provider`, so a host rebinds the store rather than subclassing the queue. */ public function test_a_replacement_store_is_used_instead_of_the_option(): void { $store = new class() extends Store { @@ -349,7 +349,7 @@ public function clear(): void { } }; - ( new Queue( $store ) )->queue_merge_notice( + $this->make_queue( $store )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -372,7 +372,7 @@ public function render( array $queue ): void { } }; - $notices = new Queue( null, $renderer ); + $notices = $this->make_queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( '

1

', $this->render_to_string( $notices ) ); @@ -400,7 +400,7 @@ public function render( array $queue ): void { } }; - $notices = new Queue( null, $renderer ); + $notices = $this->make_queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); wp_set_current_user( $this->create_user( 'subscriber' ) ); @@ -420,7 +420,7 @@ public function render( array $queue ): void { * @param string|null $role Role to render as, or null for a logged-out visitor. */ public function test_render_does_nothing_for_a_user_who_cannot_activate_plugins( ?string $role ): void { - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -450,7 +450,7 @@ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue( $this->markTestSkipped( 'Outside multisite an administrator simply has activate_plugins.' ); } - $notices = new Queue(); + $notices = $this->make_queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -469,7 +469,7 @@ public function test_a_site_administrator_on_multisite_cannot_consume_the_queue( * transient-backed design this class exists to avoid. */ public function test_the_queue_is_a_durable_database_row(): void { - ( new Queue() )->queue_merge_notice( + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] ) ); @@ -481,7 +481,7 @@ public function test_the_queue_is_a_durable_database_row(): void { wp_cache_flush(); - $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( new Queue() ) ); + $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( $this->make_queue() ) ); } /** @@ -495,7 +495,7 @@ public function test_the_queue_is_a_durable_database_row(): void { public function test_render_ignores_anything_that_is_not_a_message( $stored, ?string $present, array $absent ): void { $this->seed_queue( $stored ); - $output = $this->render_to_string( new Queue() ); + $output = $this->render_to_string( $this->make_queue() ); if ( $present === null ) { $this->assertSame( '', $output ); @@ -544,7 +544,7 @@ public static function malformed_queues(): Generator { public function test_a_corrupted_queue_heals_on_the_next_write(): void { $this->seed_queue( [ 'a:merge' => [ 'nested' ] ] ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); } @@ -555,7 +555,7 @@ public function test_the_option_is_keyed_by_the_hook_prefix(): void { $this->assertSame( self::OPTION_FOR_OTHER_PREFIX, Queue::option_name() ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertIsArray( get_site_option( self::OPTION_FOR_OTHER_PREFIX, false ) ); $this->assertFalse( $this->queue_exists() ); @@ -566,7 +566,7 @@ public function test_queueing_needs_a_hook_prefix(): void { $this->expectException( Config_Exception::class ); - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); } /** @@ -578,7 +578,7 @@ public function test_the_queue_is_not_autoloaded(): void { $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); } - ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); + $this->make_queue()->queue_merge_notice( $this->make_sub_plugin() ); $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); } @@ -675,6 +675,22 @@ private function create_user( string $role ): int { return $user_id; } + /** + * The queue as the container builds it, or with one half replaced. + * + * Both collaborators are required arguments — nothing in `src/` defaults a collaborator to a + * `new` of its own any more — so the standard pair is spelled out once here rather than in every + * test that only cares about what a notice says. + * + * @param Store|null $store Where the queue is kept. + * @param Renderer|null $renderer How a queued notice is drawn. + * + * @return Queue + */ + private function make_queue( ?Store $store = null, ?Renderer $renderer = null ): Queue { + return new Queue( $store ?? new Store(), $renderer ?? new Renderer() ); + } + private function render_to_string( Queue $notices ): string { ob_start(); diff --git a/tests/unit/PluginCheckerTest.php b/tests/unit/PluginCheckerTest.php new file mode 100644 index 0000000..a7c16a8 --- /dev/null +++ b/tests/unit/PluginCheckerTest.php @@ -0,0 +1,106 @@ +checker = new Plugin_Checker(); + } + + public function test_it_implements_the_contract(): void { + $this->assertInstanceOf( Plugin_Checker_Interface::class, $this->checker ); + } + + public function test_it_reports_an_active_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', true ); + + $this->assertTrue( $this->checker->is_active( 'give-recurring/give-recurring.php' ) ); + } + + public function test_it_reports_an_inactive_plugin(): void { + $this->setFunctionReturn( 'is_plugin_active', false ); + + $this->assertFalse( $this->checker->is_active( 'give-recurring/give-recurring.php' ) ); + } + + /** + * The basename is what reaches deactivate_plugins() next, so asserting only the return value + * would let the wrong plugin be turned off unnoticed. + */ + public function test_it_passes_the_basename_through_to_wordpress(): void { + $received = null; + + $this->setFunctionReturn( + 'is_plugin_active', + static function ( $basename ) use ( &$received ) { + $received = $basename; + + return true; + }, + true + ); + + $this->checker->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 'give-recurring/give-recurring.php', $received ); + } + + /** + * is_plugin_active() already ORs in the network check, so one call answers both scopes. A second + * get_site_option() per sub-plugin per request would buy nothing. + */ + public function test_the_active_check_costs_one_call(): void { + $calls = 0; + + $this->setFunctionReturn( + 'is_plugin_active', + static function () use ( &$calls ) { + ++$calls; + + return true; + }, + true + ); + $this->setFunctionReturn( + 'is_plugin_active_for_network', + static function () { + throw new LogicException( 'The network check is redundant and must not be called.' ); + }, + true + ); + + $this->checker->is_active( 'give-recurring/give-recurring.php' ); + + $this->assertSame( 1, $calls ); + } +} diff --git a/tests/unit/PluginDeactivatorTest.php b/tests/unit/PluginDeactivatorTest.php new file mode 100644 index 0000000..94e25b3 --- /dev/null +++ b/tests/unit/PluginDeactivatorTest.php @@ -0,0 +1,62 @@ +deactivator = new Plugin_Deactivator(); + } + + public function test_it_implements_the_contract(): void { + $this->assertInstanceOf( Plugin_Deactivator_Interface::class, $this->deactivator ); + } + + /** + * Silent, and with no third argument. A noisy deactivation runs the standalone's own deactivation + * hook at plugins_loaded — where a routine flush_rewrite_rules() in it 404s every custom permalink + * on the site — and a computed $network_wide would skip one of the two scopes core's null default + * covers, stranding an entry for a plugin active in both. + */ + public function test_it_deactivates_silently_in_every_scope(): void { + $received = []; + + $this->setFunctionReturn( + 'deactivate_plugins', + static function ( ...$arguments ) use ( &$received ): void { + $received = $arguments; + }, + true + ); + + $this->deactivator->deactivate( 'give-recurring/give-recurring.php' ); + + $this->assertSame( [ 'give-recurring/give-recurring.php', true ], $received ); + } +} diff --git a/tests/unit/PluginStateTest.php b/tests/unit/PluginStateTest.php deleted file mode 100644 index 817423f..0000000 --- a/tests/unit/PluginStateTest.php +++ /dev/null @@ -1,128 +0,0 @@ -plugin_state = new Plugin_State(); - } - - public function test_it_implements_the_contract(): void { - $this->assertInstanceOf( Plugin_State_Interface::class, $this->plugin_state ); - } - - /** - * The loader builds every unbound collaborator with a bare `new`, so a constructor that grew a - * required argument would fatal at plugins_loaded rather than here. - */ - public function test_it_constructs_without_arguments(): void { - $this->assertInstanceOf( Plugin_State::class, new Plugin_State() ); - } - - public function test_it_reports_an_active_plugin(): void { - $this->setFunctionReturn( 'is_plugin_active', true ); - - $this->assertTrue( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); - } - - public function test_it_reports_an_inactive_plugin(): void { - $this->setFunctionReturn( 'is_plugin_active', false ); - - $this->assertFalse( $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ) ); - } - - /** - * The basename is what reaches deactivate_plugins(), so asserting only the return value would - * let the wrong plugin be turned off unnoticed. - */ - public function test_it_passes_the_basename_through_to_wordpress(): void { - $received = null; - - $this->setFunctionReturn( - 'is_plugin_active', - static function ( $basename ) use ( &$received ) { - $received = $basename; - - return true; - }, - true - ); - - $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); - - $this->assertSame( 'give-recurring/give-recurring.php', $received ); - } - - /** - * is_plugin_active() already ORs in the network check, so one call answers both scopes. A - * second get_site_option() per sub-plugin per request would buy nothing. - */ - public function test_the_active_check_costs_one_call(): void { - $calls = 0; - - $this->setFunctionReturn( - 'is_plugin_active', - static function () use ( &$calls ) { - ++$calls; - - return true; - }, - true - ); - $this->setFunctionReturn( - 'is_plugin_active_for_network', - static function () { - throw new \LogicException( 'The network check is redundant and must not be called.' ); - }, - true - ); - - $this->plugin_state->is_active( 'give-recurring/give-recurring.php' ); - - $this->assertSame( 1, $calls ); - } - - /** - * Silent, and with no third argument. A noisy deactivation runs the standalone's own - * deactivation hook at plugins_loaded, and a computed $network_wide would skip one of the two - * scopes core's null default covers. - */ - public function test_it_deactivates_silently_in_every_scope(): void { - $received = []; - - $this->setFunctionReturn( - 'deactivate_plugins', - static function ( ...$arguments ) use ( &$received ) { - $received = $arguments; - }, - true - ); - - $this->plugin_state->deactivate( 'give-recurring/give-recurring.php' ); - - $this->assertSame( [ 'give-recurring/give-recurring.php', true ], $received ); - } -} diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php new file mode 100644 index 0000000..c898287 --- /dev/null +++ b/tests/unit/ProviderTest.php @@ -0,0 +1,172 @@ +assertInstanceOf( Provider_Interface::class, new Provider( new Test_Container() ) ); + } + + /** + * @dataProvider default_bindings + * + * @param string $id Id the library resolves. + * @param class-string $expected Class the container must build for it. + */ + public function test_it_binds_every_default( string $id, string $expected ): void { + $container = $this->registered_container(); + + $this->assertInstanceOf( $expected, $container->get( $id ) ); + } + + /** + * Every id the library asks the container for. A collaborator added without a line here is one + * the container cannot build, which is a fatal at plugins_loaded rather than a missing default. + * + * @return Generator + */ + public static function default_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class, Registrar::class ]; + yield 'the notice queue' => [ Queue_Interface::class, Queue::class ]; + yield 'the notice store' => [ Store::class, Store::class ]; + yield 'the notice renderer' => [ Renderer::class, Renderer::class ]; + yield 'the plugin checker' => [ Plugin_Checker_Interface::class, Plugin_Checker::class ]; + yield 'the deactivator' => [ Plugin_Deactivator_Interface::class, Plugin_Deactivator::class ]; + yield 'the load runner' => [ Runner::class, Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class, Scheduler::class ]; + } + + /** + * The registrar holds the registrations and the queue holds its store, so a binding rebuilt per + * call would hand the load loop a registry the flush never reached. + * + * @dataProvider stateful_bindings + * + * @param string $id Id that must resolve to one instance per container. + */ + public function test_a_binding_resolves_to_one_instance( string $id ): void { + $container = $this->registered_container(); + + $this->assertSame( $container->get( $id ), $container->get( $id ) ); + } + + /** + * @return Generator + */ + public static function stateful_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class ]; + yield 'the notice queue' => [ Queue_Interface::class ]; + } + + /** + * The `! has()` guard, which is the whole reason a host can rebind anything: the provider runs + * over the host's own container, so binding first has to be binding last. + * + * @dataProvider host_bindings + * + * @param string $id Id the host bound first. + * @param object $bound Implementation the host bound. + */ + public function test_it_leaves_a_binding_the_host_made_first_alone( string $id, object $bound ): void { + $container = new Test_Container(); + $container->singleton( + $id, + static function () use ( $bound ): object { + return $bound; + } + ); + + ( new Provider( $container ) )->register(); + + $this->assertSame( $bound, $container->get( $id ) ); + } + + /** + * The interfaces only. DI52 reports `has()` true for any *class* name that exists, bound or not, + * so a concrete id cannot demonstrate the guard — it can only demonstrate DI52's autowiring. + * + * @return Generator + */ + public static function host_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class, new Spy_Registrar() ]; + yield 'the notice queue' => [ Queue_Interface::class, new Spy_Queue() ]; + } + + /** + * Registering twice is the shape a host lands in by wiring the provider from two entry points. + * The second pass must not replace instances the first pass already handed out. + */ + public function test_registering_twice_keeps_the_first_instances(): void { + $container = $this->registered_container(); + $registrar = $container->get( Registrar_Interface::class ); + + ( new Provider( $container ) )->register(); + + $this->assertSame( $registrar, $container->get( Registrar_Interface::class ) ); + } + + /** + * A container with the defaults registered into it, plus the self-binding a host container + * ordinarily offers. + * + * @return Test_Container + */ + private function registered_container(): Test_Container { + $container = new Test_Container(); + $container->singleton( + ContainerInterface::class, + static function () use ( $container ): ContainerInterface { + return $container; + } + ); + + ( new Provider( $container ) )->register(); + + return $container; + } +} diff --git a/tests/unit/SmokeTest.php b/tests/unit/SmokeTest.php index 11973f1..3704930 100644 --- a/tests/unit/SmokeTest.php +++ b/tests/unit/SmokeTest.php @@ -9,6 +9,7 @@ use Codeception\TestCase\WPTestCase; use Nexcess\PluginAbsorber\Tests\Support\TestException; +use Nexcess\PluginAbsorber\Tests\Support\Traits\WithHaltedRedirects; use lucatume\WPBrowser\Traits\UopzFunctions; /** @@ -16,6 +17,7 @@ */ class SmokeTest extends WPTestCase { use UopzFunctions; + use WithHaltedRedirects; /** * Message carried by the exception that stands in for exit(). @@ -77,6 +79,30 @@ static function () use ( $message ) { $this->assertTrue( $halted, 'A stubbed function must be able to throw in place of exit().' ); } + /** + * The shared form of the same mechanism, proven here rather than on its first real caller. + * + * `capture_redirect()` is what every test of a redirecting branch uses, so a fault in it would + * show up as a failure in whatever code path happened to be under test — and the two parts of it + * that matter most, the fail() when nothing halted and the message match, are exactly the parts + * whose absence is silent. + * + * @since 1.0.0 + */ + public function test_the_shared_helper_captures_a_halted_redirect(): void { + $location = $this->capture_redirect( + static function (): void { + wp_safe_redirect( 'https://example.test/wp-admin/plugins.php' ); + + // Never reached: the stub throws first, which is the whole point. Production really + // does end the request here. + exit; + } + ); + + $this->assertSame( 'https://example.test/wp-admin/plugins.php', $location ); + } + /** * Proves the multisite env is actually a network, on its own tables. * From 4777f458918ba2a2d6eff8bdd393fad735fde1a3 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Wed, 12 Aug 2026 16:34:15 +0200 Subject: [PATCH 5/5] Enhance error handling and documentation in Loader and Provider - Improved error reporting in Loader to catch and handle exceptions during sub-plugin loading, ensuring that configuration errors are communicated clearly without causing fatal errors. - Updated the documentation in configuration.md to clarify the registration process and collision handling for slugs. - Added capability checks in notices.md to prevent non-admin users from consuming notices, preserving important warnings for administrators. - Refined the handling of class bindings in Provider to ensure that class IDs are correctly bound even when they exist in the container, enhancing the reliability of the dependency injection process. - Expanded unit tests in LoaderTest and ProviderTest to cover new error handling scenarios and ensure robustness against configuration issues. --- docs/configuration.md | 6 +- docs/notices.md | 11 +++- src/Load/Runner.php | 28 ++++++++- src/Loader.php | 31 +++++++++- src/Provider.php | 18 +++++- tests/_support/Test_Container.php | 20 ++++++- tests/unit/Load/RunnerTest.php | 98 +++++++++++++++++++++++++++++++ tests/unit/LoaderTest.php | 89 +++++++++++++++++++++++++--- tests/unit/ProviderTest.php | 60 ++++++++++++++++--- 9 files changed, 330 insertions(+), 31 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index c5ab7f1..01cc8ac 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -90,8 +90,10 @@ at include time. Register each slug exactly once. A slug also names the sub-plugin's notices and its once-ever activation record, so a second registration under the same slug is refused with a `Config_Exception` naming both bundled files rather than quietly dropping one of the two from the -load. Because registrations are buffered until boot, that collision is reported at boot rather than -at the second `register()` call; a config array the library cannot use is still rejected on the spot. +load. Registrations are buffered and nothing reads them until the load pass at `plugins_loaded` +priority 2, so that is where the collision surfaces — not at the second `register()` call and not at +`boot()`. It is reported with `_doing_it_wrong()` and that request loads no sub-plugin at all, rather +than thrown out of a core hook. A config array the library cannot use is still rejected on the spot. Register unconditionally and put anything you cannot decide up front — a licence that may not be active, a setting the site owner can change — in `enabled`, which is re-evaluated on every load. diff --git a/docs/notices.md b/docs/notices.md index a1e73f0..304f11b 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -34,6 +34,13 @@ break it — a `

` of your own is left as it is rather than nested inside anot use Nexcess\PluginAbsorber\Notices\Queue; add_action( 'admin_init', function () { + // Gates the read, not just the delete: `admin_init` fires for every logged-in user, and + // draining the queue for one who cannot act on it destroys the only warning an + // administrator was going to get. + if ( ! current_user_can( 'activate_plugins' ) ) { + return; + } + $notices = get_site_option( Queue::option_name(), [] ); if ( ! is_array( $notices ) || ! $notices ) { @@ -50,9 +57,7 @@ add_action( 'admin_init', function () { `admin_init` runs before `all_admin_notices`, where the built-in rendering happens, so deleting the option there leaves ours nothing to draw and the notice is shown once, by you. Do the deleting: a -notice read and not cleared is shown on every request forever. And keep the capability check — -reading the option directly skips the `activate_plugins` gate `Queue::render()` applies, and a -consumed notice is gone for the administrator who could have acted on it. +notice read and not cleared is shown on every request forever. The queue is three classes: `Notices\Queue` decides what a notice says and who may consume it, `Notices\Store` keeps it, `Notices\Renderer` draws it. `Queue` takes both as constructor arguments diff --git a/src/Load/Runner.php b/src/Load/Runner.php index b66f2ee..8470b80 100644 --- a/src/Load/Runner.php +++ b/src/Load/Runner.php @@ -43,8 +43,8 @@ public function __construct( Queue_Interface $notices ) { /** * @since 1.0.0 * - * @throws Config_Exception When the container cannot produce a usable registrar, or two - * sub-plugins were registered under one slug. + * @throws Config_Exception From loading a sub-plugin, which reads the hook prefix the guard + * above has already established is set. * * @return void */ @@ -59,7 +59,29 @@ public function load_all(): void { // Loader::all() rather than the registrar directly: it flushes the registrations still // buffered on the facade before it reads, and a registrar asked on its own would miss // anything registered since the last read. - foreach ( Loader::all() as $sub_plugin ) { + try { + $sub_plugins = Loader::all(); + } catch ( Config_Exception $exception ) { + // The flush is where a duplicate slug is caught, and reading the registrar is where a + // missing container or an unusable binding is. All three are bootstrap mistakes, and + // all three arrive inside plugins_loaded: letting one out would fatal every request, + // front end and admin alike, and lock the developer out of the screen where the + // registration could be corrected. The hook this runs on exists to prevent a fatal, so + // it is the last place that may cause one -- the mistake is reported to the developer + // and the load is abandoned instead. + _doing_it_wrong( + self::class, + sprintf( + 'The registered sub-plugins could not be read, so none were loaded: %s', + $exception->getMessage() + ), + '1.0.0' + ); + + return; + } + + foreach ( $sub_plugins as $sub_plugin ) { $this->load( $sub_plugin ); } } diff --git a/src/Loader.php b/src/Loader.php index d2216bf..98628f4 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -11,6 +11,7 @@ use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Traits\Guards_Hook_Prefix; +use Throwable; /** * Static facade: registration, and the one call that starts everything. @@ -183,13 +184,37 @@ public static function render_notices(): void { * * @param class-string $interface Collaborator interface to resolve. * - * @throws Config_Exception When no container has been set, or its binding does not implement - * the interface it was bound to. + * @throws Config_Exception When no container has been set, when it throws while building the + * binding, or when the binding does not implement the interface it was + * bound to. * * @return T */ private static function collaborator( string $interface ): object { - $collaborator = Config::get_container()->get( $interface ); + // Resolved outside the try: a missing container is this library's own configuration error + // already, reported in its own words, and re-wrapping it would bury that sentence one + // exception deeper for no gain. + $container = Config::get_container(); + + // A host factory closure is free to throw, and a container asked for a binding with an + // unsatisfiable dependency -- or for an interface nothing has bound yet, which is every + // interface before boot() runs the provider -- throws its own exception type. Uncaught, + // either one leaves the host's plugins_loaded with a fatal from a vendor namespace that + // names neither this library nor the binding at fault, so both are reported the same way as + // a binding of the wrong type. The original failure is kept as the previous exception. + try { + $collaborator = $container->get( $interface ); + } catch ( Throwable $thrown ) { + throw new Config_Exception( + sprintf( + 'The container failed to build the binding for %s: %s', + $interface, + $thrown->getMessage() + ), + 0, + $thrown + ); + } if ( ! $collaborator instanceof $interface ) { throw new Config_Exception( diff --git a/src/Provider.php b/src/Provider.php index 31a200d..0dc1353 100644 --- a/src/Provider.php +++ b/src/Provider.php @@ -95,7 +95,21 @@ static function () use ( $container ): Scheduler { * * 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. + * calls `set_container()` and `boot()` decide which implementation it gets. It is also what + * keeps a second `register()` harmless, instead of swapping in a fresh registrar and losing + * every sub-plugin registered so far. + * + * The `class_exists()` half is what makes that question answerable at all. `has()` means "can + * return an entry", not "the host bound this" -- di52 answers it with `isBound() || + * class_exists()`, so for a class id it is true before anything has been bound. Asked alone it + * stands down every binding above whose id is a class, the explicit factories included, leaving + * those collaborators autowired where the container autowires, broken where it does not, and + * singletons nowhere. An interface no container can build unprompted, so there the same call + * answers exactly what is being asked. + * + * What that costs is a host rebinding one of the concrete workers, which has to happen after + * boot: nothing here can tell that binding apart from the container's own willingness to build + * the class. The interface seams -- the ones a host is invited to replace -- are unaffected. * * Singletons throughout. Every one of these is either a registry whose contents are the point * or a stateless worker, and a second registrar would hold a second, emptier list of @@ -110,7 +124,7 @@ static function () use ( $container ): Scheduler { * @return void */ private function bind_once( string $id, $implementation = null ): void { - if ( $this->container->has( $id ) ) { + if ( ! class_exists( $id ) && $this->container->has( $id ) ) { return; } diff --git a/tests/_support/Test_Container.php b/tests/_support/Test_Container.php index 46e7c1f..8a7c872 100644 --- a/tests/_support/Test_Container.php +++ b/tests/_support/Test_Container.php @@ -42,10 +42,13 @@ public function get( string $id ) { } /** - * Reports whether the id is bound. + * Reports whether the container can return an entry for the id. * * Inherits DI52's permissive semantics: any existing *class* name reports true even with * nothing bound, because DI52 falls back to `class_exists()`. Interface names are unaffected. + * `isBound()` below is the narrower question — whether a binding was actually made — and this + * adapter exposes it so that a caller distinguishing the two reaches the same method on a real + * host container. * * @inheritDoc */ @@ -53,6 +56,21 @@ public function has( string $id ) { return $this->container->has( $id ); } + /** + * Whether something was bound to the id, autowirable class names excluded. + * + * Not part of `ContainerInterface`. It is on DI52, and on the adapters hosts wrap DI52 in — the + * example adapter in `stellarwp/container-contract` forwards unknown calls through `__call()` — + * so a collaborator probing for it finds it here as it would in production. + * + * @param string $id Identifier of the entry to look for. + * + * @return bool + */ + public function isBound( string $id ): bool { + return $this->container->isBound( $id ); + } + /** * @inheritDoc */ diff --git a/tests/unit/Load/RunnerTest.php b/tests/unit/Load/RunnerTest.php index 1f89644..b78b4d0 100644 --- a/tests/unit/Load/RunnerTest.php +++ b/tests/unit/Load/RunnerTest.php @@ -44,6 +44,18 @@ class RunnerTest extends WPTestCase { */ private $constants = []; + /** + * Every `_doing_it_wrong()` message seen since the recorder went on. + * + * @var string[] + */ + private $incorrect_usage_messages = []; + + /** + * @var callable|null + */ + private $incorrect_usage_message_listener = null; + /** * Every value the should_load filter was called with. * @@ -78,6 +90,7 @@ public function tearDown(): void { $this->constants = []; $this->stop_expecting_incorrect_usage(); + $this->stop_recording_incorrect_usage_messages(); $this->clear_notices(); Loader_State::reset(); Config_State::reset(); @@ -441,6 +454,36 @@ public function test_load_all_does_nothing_without_a_hook_prefix(): void { $this->assert_the_library_reported_incorrect_usage(); } + /** + * The same guarantee for the read itself. Reading flushes the registration buffer, and the + * registrar refuses a slug it already holds — a throw that arrives inside plugins_loaded, where it + * would take down the front end and wp-admin together and lock the developer out of the screen + * where the duplicate registration could be undone. + */ + public function test_a_duplicate_slug_is_reported_rather_than_fataling_the_request(): void { + // Two registrations of the default slug, each with a bundled fixture of its own — one file + // behind both would load once for the second registration and hide the skip under a dedupe. + $this->register(); + $this->register(); + + $this->expect_incorrect_usage(); + $this->record_incorrect_usage_messages(); + + $this->runner()->load_all(); + + // Reaching this line at all is half of what is under test: load_all() has to return. + $this->assertSame( + 0, + $this->bundled_plugin_loads(), + 'A read that failed has no list to load from, so nothing may load.' + ); + $this->assert_the_library_reported_incorrect_usage(); + $this->assert_a_reported_message_contains( + 'give-recurring', + 'The report has to name the slug, or it could have been raised for any other reason.' + ); + } + /** * Registration is buffered, which is what lets the container arrive after the sub-plugins do — * and the registry is only half of that: the load path resolves the notice queue as well. A @@ -534,6 +577,61 @@ private function assert_the_should_load_recorder_works(): void { ); } + /** + * Keep the *message* of every incorrect-usage report, which the shared trait deliberately does not. + * + * `WithIncorrectUsage` pins that the library reported something against itself, which is all most + * tests need. A test about one particular failure needs more: a report raised for an unrelated + * reason — no hook prefix, no container — would otherwise satisfy it just as well. + * + * @return void + */ + private function record_incorrect_usage_messages(): void { + $messages = &$this->incorrect_usage_messages; + + $listener = static function ( $function_name, $message ) use ( &$messages ): void { + $messages[] = is_string( $message ) ? $message : ''; + }; + + $this->incorrect_usage_message_listener = $listener; + + add_action( 'doing_it_wrong_run', $listener, 10, 2 ); + } + + /** + * @param string $needle Text one report has to carry. + * @param string $message Why it has to. + * + * @return void + */ + private function assert_a_reported_message_contains( string $needle, string $message ): void { + $this->assertNotSame( [], $this->incorrect_usage_messages, 'Nothing was reported at all.' ); + $this->assertStringContainsString( + $needle, + implode( PHP_EOL, $this->incorrect_usage_messages ), + $message + ); + } + + /** + * Take the recorder back off. Call from tearDown, for the same reason the trait's own removal is + * there: a failing assertion would otherwise leave it listening for the rest of the process. + * + * Removed by identity rather than by clearing the hook, which WordPress and the rest of the suite + * are also on. + * + * @return void + */ + private function stop_recording_incorrect_usage_messages(): void { + if ( $this->incorrect_usage_message_listener !== null ) { + remove_action( 'doing_it_wrong_run', $this->incorrect_usage_message_listener ); + + $this->incorrect_usage_message_listener = null; + } + + $this->incorrect_usage_messages = []; + } + private function clear_notices(): void { delete_site_option( 'give_plugin_absorber_notices' ); } diff --git a/tests/unit/LoaderTest.php b/tests/unit/LoaderTest.php index ebe2fc6..01e8436 100644 --- a/tests/unit/LoaderTest.php +++ b/tests/unit/LoaderTest.php @@ -82,10 +82,67 @@ public function test_a_binding_that_does_not_implement_its_interface_is_reported Config::set_container( $container ); - $this->expectException( Config_Exception::class ); - $this->expectExceptionMessage( Registrar_Interface::class ); + try { + Loader::registrar(); + $this->fail( 'Expected a Config_Exception.' ); + } catch ( Config_Exception $exception ) { + $this->assertStringContainsString( Registrar_Interface::class, $exception->getMessage() ); + $this->assertStringContainsString( + 'does not implement', + $exception->getMessage(), + 'A binding that built fine and is simply the wrong type must not be reported as one ' + . 'the container could not build: the two send the host to different files.' + ); + } + } + + /** + * A bound factory may throw, and a container asked for something it cannot build throws its own + * exception type; the contract is explicit that has() true does not promise get() succeeds. + * Left uncaught, the host gets a fatal from a vendor namespace at plugins_loaded that names + * neither this library nor the binding at fault. + */ + public function test_it_reports_a_container_that_throws_as_a_configuration_error(): void { + $failure = new RuntimeException( 'the host factory needed a database connection' ); + $container = new Test_Container(); + $container->singleton( + Registrar_Interface::class, + static function () use ( $failure ): Registrar_Interface { + throw $failure; + } + ); + $this->set_up_container( $container ); - Loader::registrar(); + try { + Loader::registrar(); + $this->fail( 'Expected a Config_Exception.' ); + } catch ( Config_Exception $exception ) { + $this->assertStringContainsString( + Registrar_Interface::class, + $exception->getMessage(), + 'The message has to name the binding that could not be built.' + ); + $this->assertContains( + $failure, + $this->previous_chain( $exception ), + 'The original failure has to stay reachable, or the real cause is lost.' + ); + } + } + + /** + * The missing-container report is this library's own sentence, in its own words. Wrapping it in + * the build-failure message would bury it a level deeper and name an interface the host never + * bound anything to, when what it has to hear is that it set no container at all. + */ + public function test_a_missing_container_is_not_reported_as_a_failed_binding(): void { + try { + Loader::registrar(); + $this->fail( 'Expected a Config_Exception.' ); + } catch ( Config_Exception $exception ) { + $this->assertStringNotContainsString( 'failed to build', $exception->getMessage() ); + $this->assertNull( $exception->getPrevious(), 'There is no earlier failure to point at.' ); + } } /** @@ -272,10 +329,6 @@ public function test_all_is_empty_before_anything_is_registered(): void { * A binding that cannot be built leaves the registrations buffered, so the read that comes after * the host fixes its container still has them. Emptying the buffer before the registrar resolved * would drop them silently and load nothing. - * - * What the container throws is the container's business — the contract is explicit that has() - * true does not promise get() succeeds — so this catches Throwable rather than pinning a type - * this library does not own. */ public function test_registrations_survive_a_registrar_the_container_cannot_build(): void { $container = new Test_Container(); @@ -293,7 +346,7 @@ static function (): Registrar_Interface { try { Loader::all(); - } catch ( Throwable $exception ) { + } catch ( Config_Exception $exception ) { $failed = true; } @@ -399,4 +452,24 @@ private function sub_plugin_config( string $slug ): array { 'plugin_loaded_constant' => strtoupper( str_replace( '-', '_', $slug ) ) . '_VERSION_FIXTURE', ]; } + + /** + * Every exception behind the given one, nearest first. + * + * Walked rather than read a single level deep: a container is entitled to wrap what a factory + * threw in its own exception type before it reaches us, and it does. + * + * @param Throwable $exception Exception to walk. + * + * @return Throwable[] + */ + private function previous_chain( Throwable $exception ): array { + $chain = []; + + for ( $previous = $exception->getPrevious(); $previous !== null; $previous = $previous->getPrevious() ) { + $chain[] = $previous; + } + + return $chain; + } } diff --git a/tests/unit/ProviderTest.php b/tests/unit/ProviderTest.php index c898287..9d07292 100644 --- a/tests/unit/ProviderTest.php +++ b/tests/unit/ProviderTest.php @@ -83,9 +83,11 @@ public static function default_bindings(): Generator { /** * The registrar holds the registrations and the queue holds its store, so a binding rebuilt per - * call would hand the load loop a registry the flush never reached. + * call would hand the load loop a registry the flush never reached. The rest are stateless, and + * are here because a class id that resolves twice over is a class id the provider never bound: + * the container is autowiring it, which means the explicit factory never ran. * - * @dataProvider stateful_bindings + * @dataProvider single_instance_bindings * * @param string $id Id that must resolve to one instance per container. */ @@ -98,14 +100,50 @@ public function test_a_binding_resolves_to_one_instance( string $id ): void { /** * @return Generator */ - public static function stateful_bindings(): Generator { - yield 'the registrar' => [ Registrar_Interface::class ]; - yield 'the notice queue' => [ Queue_Interface::class ]; + public static function single_instance_bindings(): Generator { + yield 'the registrar' => [ Registrar_Interface::class ]; + yield 'the notice queue' => [ Queue_Interface::class ]; + yield 'the notice store' => [ Store::class ]; + yield 'the notice renderer' => [ Renderer::class ]; + yield 'the load runner' => [ Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class ]; } /** - * The `! has()` guard, which is the whole reason a host can rebind anything: the provider runs - * over the host's own container, so binding first has to be binding last. + * A class id is the case the guard gets wrong for free: DI52 answers `has()` with + * `isBound() || class_exists()`, so every id below reports true before anything is bound. A + * provider trusting that reply binds none of them — the explicit factories written for a + * container that does not autowire included — and nothing says so, because a container that + * *does* autowire still hands back an object. + * + * @dataProvider class_id_bindings + * + * @param string $id Class id the provider must bind regardless. + */ + public function test_it_binds_a_class_id_the_container_reports_it_already_has( string $id ): void { + $container = new Test_Container(); + + $this->assertTrue( $container->has( $id ), 'has() is expected to be true for an existing class.' ); + $this->assertFalse( $container->isBound( $id ), 'Nothing has bound the id yet.' ); + + ( new Provider( $container ) )->register(); + + $this->assertTrue( $container->isBound( $id ) ); + } + + /** + * @return Generator + */ + public static function class_id_bindings(): Generator { + yield 'the notice store' => [ Store::class ]; + yield 'the notice renderer' => [ Renderer::class ]; + yield 'the load runner' => [ Runner::class ]; + yield 'the boot scheduler' => [ Scheduler::class ]; + } + + /** + * The guard, which is the whole reason a host can rebind anything: the provider runs over the + * host's own container, so binding first has to be binding last. * * @dataProvider host_bindings * @@ -127,8 +165,12 @@ static function () use ( $bound ): object { } /** - * The interfaces only. DI52 reports `has()` true for any *class* name that exists, bound or not, - * so a concrete id cannot demonstrate the guard — it can only demonstrate DI52's autowiring. + * Interface ids only, which is as far as the guard reaches. Nothing can build an interface + * unprompted, so `has()` is true there only where a binding exists and the host's object + * survives. A class id cannot be covered: di52 answers `has()` for one with the same true + * whether or not anything was bound, so the provider cannot see the host's binding and + * replaces it — a host rebinding `Store`, `Renderer`, `Load\Runner` or `Boot\Scheduler` has to + * do it after boot. * * @return Generator */