From 64e880fb28abe218e7a2b6fee7f670651112fbf9 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 3 Aug 2026 15:41:03 +0200 Subject: [PATCH 01/10] Add the Notices queue and its interface --- src/Contracts/Notices_Interface.php | 60 +++++++ src/Notices.php | 169 ++++++++++++++++++++ tests/unit/NoticesTest.php | 236 ++++++++++++++++++++++++++++ 3 files changed, 465 insertions(+) create mode 100644 src/Contracts/Notices_Interface.php create mode 100644 src/Notices.php create mode 100644 tests/unit/NoticesTest.php diff --git a/src/Contracts/Notices_Interface.php b/src/Contracts/Notices_Interface.php new file mode 100644 index 0000000..0843884 --- /dev/null +++ b/src/Contracts/Notices_Interface.php @@ -0,0 +1,60 @@ +queue( + $sub_plugin, + self::TYPE_MERGE, + $sub_plugin->get_conflict_notice_message( + sprintf( + '%s has been deactivated because it is now bundled and loaded automatically.', + $sub_plugin->get_slug() + ) + ) + ); + } + + /** + * The default differs from the merge notice's on purpose: this one asks the user to act, + * where that one reports something already done. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_conflict_notice( Sub_Plugin $sub_plugin ): void { + $this->queue( + $sub_plugin, + self::TYPE_CONFLICT, + $sub_plugin->get_conflict_notice_message( + sprintf( + '%s is now bundled and loaded automatically. You can safely deactivate the standalone plugin.', + $sub_plugin->get_slug() + ) + ) + ); + } + + /** + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * + * @return void + */ + public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { + $this->queue( $sub_plugin, self::TYPE_DEPENDENCY, $sub_plugin->get_dependency_notice_message() ); + } + + /** + * @since 1.0.0 + * + * @return void + */ + public function render(): void { + $queue = $this->get_queue(); + + if ( $queue === [] ) { + return; + } + + delete_transient( $this->transient_name() ); + + foreach ( $queue as $message ) { + if ( $message === '' ) { + continue; + } + + printf( + '

%s

', + esc_html( $message ) + ); + } + } + + /** + * Store one notice, keyed by slug and type so different types can coexist. + * + * A sub-plugin can legitimately earn a merge notice while the conflict is resolved and a + * dependency notice while the load is attempted, in the same request. Keying by slug alone + * would silently drop one of them. + * + * @since 1.0.0 + * + * @param Sub_Plugin $sub_plugin Sub-plugin concerned. + * @param string $type Notice type. + * @param string $message Resolved message. + * + * @return void + */ + private function queue( Sub_Plugin $sub_plugin, string $type, string $message ): void { + $queue = $this->get_queue(); + + $queue[ $sub_plugin->get_slug() . ':' . $type ] = $message; + + // No expiry: the queue has to outlive the redirect and wait for the next admin load. + set_transient( $this->transient_name(), $queue, 0 ); + } + + /** + * @since 1.0.0 + * + * @return array + */ + private function get_queue(): array { + $queue = get_transient( $this->transient_name() ); + + if ( ! is_array( $queue ) ) { + return []; + } + + // The store is shared with whatever else can write an option, and the render path prints + // what it finds. Anything that is not a string message is dropped rather than coerced. + return array_filter( $queue, 'is_string' ); + } + + /** + * @since 1.0.0 + * + * @return string + */ + private function transient_name(): string { + return Config::get_hook_prefix() . '_plugin_absorber_notices'; + } +} diff --git a/tests/unit/NoticesTest.php b/tests/unit/NoticesTest.php new file mode 100644 index 0000000..74b900b --- /dev/null +++ b/tests/unit/NoticesTest.php @@ -0,0 +1,236 @@ + $overrides Config overrides. + */ + private function make_sub_plugin( array $overrides = [] ): Sub_Plugin { + return new Sub_Plugin( + array_merge( + [ + 'slug' => 'give-recurring', + 'bundled_plugin_file' => '/tmp/give-recurring.php', + 'plugin_loaded_constant' => 'GIVE_RECURRING_VERSION_NOTICES', + ], + $overrides + ) + ); + } + + private function render_to_string( Notices $notices ): string { + ob_start(); + $notices->render(); + + return (string) ob_get_clean(); + } + + public function test_the_loader_resolves_the_default_notices(): void { + $this->assertInstanceOf( Notices::class, Loader::notices() ); + } + + public function test_the_default_notices_satisfy_the_contract(): void { + $this->assertInstanceOf( Notices_Interface::class, new Notices() ); + } + + public function test_it_queues_a_merge_notice_into_the_transient(): void { + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); + + $queue = get_transient( self::TRANSIENT ); + + $this->assertIsArray( $queue ); + $this->assertArrayHasKey( 'give-recurring:merge', $queue ); + $this->assertSame( 'Bundled now.', $queue['give-recurring:merge'] ); + } + + public function test_the_merge_notice_falls_back_to_a_default_message(): void { + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + + $queue = get_transient( self::TRANSIENT ); + + $this->assertStringContainsString( 'give-recurring', $queue['give-recurring:merge'] ); + $this->assertNotSame( '', $queue['give-recurring:merge'] ); + } + + public function test_it_queues_a_conflict_notice(): void { + ( new Notices() )->queue_conflict_notice( $this->make_sub_plugin() ); + + $this->assertArrayHasKey( 'give-recurring:conflict', get_transient( self::TRANSIENT ) ); + } + + /** + * The two conflict-flavoured notices say opposite things — one reports a deactivation that + * already happened, the other asks the user to do it. Sharing a default would be wrong. + */ + public function test_the_merge_and_conflict_defaults_differ(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin() ); + $notices->queue_conflict_notice( $this->make_sub_plugin() ); + + $queue = get_transient( self::TRANSIENT ); + + $this->assertNotSame( $queue['give-recurring:merge'], $queue['give-recurring:conflict'] ); + } + + public function test_a_configured_message_is_used_for_both_conflict_types(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Ours.' ] ) ); + $notices->queue_conflict_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Ours.' ] ) ); + + $queue = get_transient( self::TRANSIENT ); + + $this->assertSame( 'Ours.', $queue['give-recurring:merge'] ); + $this->assertSame( 'Ours.', $queue['give-recurring:conflict'] ); + } + + public function test_it_queues_a_dependency_notice_using_the_sub_plugin_message(): void { + ( new Notices() )->queue_dependency_notice( $this->make_sub_plugin( [ 'dependency_notice_message' => 'Needs Give.' ] ) ); + + $this->assertSame( 'Needs Give.', get_transient( self::TRANSIENT )['give-recurring:dependency'] ); + } + + public function test_the_dependency_notice_falls_back_to_the_sub_plugin_default(): void { + ( new Notices() )->queue_dependency_notice( $this->make_sub_plugin() ); + + $this->assertSame( + 'give-recurring could not be loaded because its requirements are not met.', + get_transient( self::TRANSIENT )['give-recurring:dependency'] + ); + } + + public function test_queueing_the_same_slug_and_type_twice_does_not_duplicate(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin() ); + $notices->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertCount( 1, get_transient( self::TRANSIENT ) ); + } + + public function test_one_slug_can_hold_notices_of_different_types(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin() ); + $notices->queue_dependency_notice( $this->make_sub_plugin() ); + + $this->assertCount( 2, get_transient( self::TRANSIENT ) ); + } + + public function test_different_slugs_do_not_collide(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin() ); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'slug' => 'give-fee-recovery' ] ) ); + + $queue = get_transient( self::TRANSIENT ); + + $this->assertCount( 2, $queue ); + $this->assertArrayHasKey( 'give-recurring:merge', $queue ); + $this->assertArrayHasKey( 'give-fee-recovery:merge', $queue ); + } + + public function test_render_outputs_dismissible_warning_markup(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); + + $output = $this->render_to_string( $notices ); + + $this->assertStringContainsString( 'notice notice-warning is-dismissible', $output ); + $this->assertStringContainsString( 'Bundled now.', $output ); + } + + public function test_render_escapes_the_message(): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => '' ] ) ); + + $output = $this->render_to_string( $notices ); + + $this->assertStringNotContainsString( '' ] ) ); @@ -178,13 +253,30 @@ public function test_render_escapes_the_message(): void { $this->assertStringContainsString( '<script>', $output ); } + /** + * Escaping is `esc_html()` on purpose, so a message is plain text and a host that ships a + * link gets literal angle brackets. Pinned here because loosening it later is safe and + * tightening it later is not. + */ + public function test_render_does_not_allow_markup_in_a_message(): void { + $notices = new Notices(); + $notices->queue_merge_notice( + $this->make_sub_plugin( [ 'conflict_notice_message' => 'See the docs.' ] ) + ); + + $output = $this->render_to_string( $notices ); + + $this->assertStringNotContainsString( 'assertStringContainsString( '<a href=', $output ); + } + public function test_render_clears_the_queue(): void { $notices = new Notices(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->render_to_string( $notices ); - $this->assertFalse( get_transient( self::TRANSIENT ) ); + $this->assertFalse( $this->queue_exists() ); $this->assertSame( '', $this->render_to_string( $notices ), 'A second render must output nothing.' ); } @@ -203,34 +295,267 @@ public function test_render_outputs_nothing_when_the_queue_is_empty(): void { $this->assertSame( '', $this->render_to_string( new Notices() ) ); } - public function test_the_queue_survives_a_simulated_redirect(): void { + /** + * Rendering consumes the queue, so a user who cannot act on the notice must neither see it + * nor destroy it. The merge notice is raised once and never re-queued. + * + * @dataProvider users_who_cannot_activate_plugins + * + * @param string|null $role Role to render as, or null for a logged-out visitor. + */ + public function test_render_does_nothing_for_a_user_who_cannot_activate_plugins( ?string $role ): void { + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); + + wp_set_current_user( $role === null ? 0 : $this->create_user( $role ) ); + + $this->assertSame( '', $this->render_to_string( $notices ) ); + $this->assertTrue( $this->queue_exists(), 'The queue must survive for someone who can act on it.' ); + } + + /** + * @return Generator + */ + public static function users_who_cannot_activate_plugins(): Generator { + yield 'a subscriber' => [ 'subscriber' ]; + yield 'a logged-out visitor' => [ null ]; + } + + /** + * Surprising but intended: on multisite `activate_plugins` maps through + * `manage_network_plugins`, which only a super admin has unless the network has opened the + * plugins menu to site admins. So the person who installed the plugin on their own site is + * not the person who sees the notice — a network administrator is. + */ + public function test_a_site_administrator_on_multisite_cannot_consume_the_queue(): void { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Outside multisite an administrator simply has activate_plugins.' ); + } + + $notices = new Notices(); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); + + wp_set_current_user( $this->create_user( 'administrator' ) ); + + $this->assertSame( '', $this->render_to_string( $notices ) ); + $this->assertTrue( $this->queue_exists(), 'The queue must survive for the network administrator.' ); + } + + /** + * The resolver redirects, so the queue has to come back off a durable database row rather + * than out of the object cache the redirecting request happened to warm. Asserting the row + * itself, not just that a flush is survivable: on a site with no persistent object cache a + * transient lands in the options table too, so a flush test alone would pass for the + * transient-backed design this class exists to avoid. + */ + public function test_the_queue_is_a_durable_database_row(): void { ( new Notices() )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); - // A redirect ends the request; the next one builds a fresh object against the same store. + $this->assertStringContainsString( + 'Bundled now.', + $this->stored_row(), + 'The queue must be a row in the database, not a cache entry.' + ); + + wp_cache_flush(); + + $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( new Notices() ) ); + } + + /** + * @dataProvider malformed_queues + * + * @param mixed $stored Raw option value to seed. + * @param string|null $present Substring the output must contain, or null when nothing at + * all should be rendered. + * @param array $absent Substrings the output must not contain. + */ + public function test_render_ignores_anything_that_is_not_a_message( $stored, ?string $present, array $absent ): void { + $this->seed_queue( $stored ); + $output = $this->render_to_string( new Notices() ); - $this->assertStringContainsString( 'Bundled now.', $output ); + if ( $present === null ) { + $this->assertSame( '', $output ); + } else { + $this->assertStringContainsString( $present, $output ); + } + + foreach ( $absent as $needle ) { + $this->assertStringNotContainsString( $needle, $output ); + } } /** - * The queue outlives the request that filled it, so it must not expire before the admin load - * that renders it. WordPress stores a no-expiry transient without a timeout option. + * The first two are the likeliest real corruption: another plugin, or a host reading and + * rewriting the option, leaves something behind that is not an array at all. The rest are + * per-entry rubbish, which is dropped without taking the well-formed entries with it. + * + * @return Generator}> */ - public function test_the_queue_is_stored_without_an_expiry(): void { + public static function malformed_queues(): Generator { + yield 'a scalar instead of an array' => [ 'not-a-queue', null, [ 'not-a-queue', 'notice' ] ]; + + yield 'an object instead of an array' => [ + (object) [ 'a:merge' => 'Nope.' ], + null, + [ 'Nope.', 'notice' ], + ]; + + yield 'entries that are not strings' => [ + [ + 'a:merge' => 'Fine.', + 'b:merge' => [ 'nested' ], + 'c:merge' => null, + 'd:merge' => 42, + ], + 'Fine.', + [ 'Array', '42' ], + ]; + + yield 'an empty message' => [ [ 'a:merge' => '' ], null, [ 'notice' ] ]; + + // A message that is only whitespace would otherwise print an empty notice box. + yield 'a whitespace-only message' => [ [ 'a:merge' => " \n\t" ], null, [ 'notice' ] ]; + } + + public function test_a_corrupted_queue_heals_on_the_next_write(): void { + $this->seed_queue( [ 'a:merge' => [ 'nested' ] ] ); + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); - $this->assertFalse( get_option( '_transient_timeout_' . self::TRANSIENT ) ); + $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); } - public function test_the_transient_is_keyed_by_the_hook_prefix(): void { - Config::reset(); + public function test_the_option_is_keyed_by_the_hook_prefix(): void { + Config_State::reset(); Config::set_hook_prefix( 'woo' ); + $this->assertSame( 'woo_plugin_absorber_notices', Notices::option_name() ); + + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + + $this->assertIsArray( get_site_option( 'woo_plugin_absorber_notices', false ) ); + $this->assertFalse( $this->queue_exists() ); + } + + public function test_queueing_needs_a_hook_prefix(): void { + Config_State::reset(); + + $this->expectException( Config_Exception::class ); + + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + } + + /** + * The queue is empty on nearly every request and only ever read in the admin, so it must not + * ride along in the autoloaded bundle on every front-end request. + */ + public function test_the_queue_is_not_autoloaded(): void { + if ( is_multisite() ) { + $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); + } + ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); - $this->assertIsArray( get_transient( 'woo_plugin_absorber_notices' ) ); - $this->assertFalse( get_transient( self::TRANSIENT ) ); + $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); + } + + /** + * The queue as the class stores it. Always an array, so callers can index and count it: use + * queue_exists() to ask whether there is a row at all. + * + * @return array + */ + private function queue(): array { + $queue = get_site_option( self::OPTION, [] ); + + return is_array( $queue ) ? $queue : []; + } + + /** + * Whether the option exists at all, which is what "render cleared the queue" means. + * + * @return bool + */ + private function queue_exists(): bool { + return get_site_option( self::OPTION, false ) !== false; + } + + /** + * The serialized option value straight out of the database, bypassing the object cache. + * + * @return string + */ + private function stored_row(): string { + /** @var wpdb $wpdb */ + global $wpdb; + + if ( is_multisite() ) { + $stored = $wpdb->get_var( + $wpdb->prepare( + "SELECT meta_value FROM {$wpdb->sitemeta} WHERE meta_key = %s AND site_id = %d", + self::OPTION, + get_current_network_id() + ) + ); + } else { + $stored = $wpdb->get_var( + $wpdb->prepare( "SELECT option_value FROM {$wpdb->options} WHERE option_name = %s", self::OPTION ) + ); + } + + $this->assertIsString( $stored, 'The queue option has no row in the database.' ); + + return $stored; + } + + /** + * @param mixed $queue Raw queue contents, well-formed or not. + */ + private function seed_queue( $queue ): void { + update_site_option( self::OPTION, $queue ); + } + + private function clear_queue(): void { + delete_site_option( self::OPTION ); + } + + /** + * @param string $role Role to give the new user. + * + * @throws RuntimeException When the user cannot be created, rather than letting a later + * capability assertion fail for an unrelated reason. + * + * @return int + */ + private function create_user( string $role ): int { + $user_id = wp_insert_user( + [ + 'user_login' => uniqid( 'absorber-' ), + 'user_pass' => wp_generate_password(), + 'role' => $role, + ] + ); + + if ( $user_id instanceof WP_Error ) { + throw new RuntimeException( 'Could not create a ' . $role . ': ' . $user_id->get_error_message() ); + } + + return $user_id; + } + + private function render_to_string( Notices $notices ): string { + ob_start(); + + try { + $notices->render(); + } finally { + // In a finally block so a throw from render() cannot leave the suite's own output + // trapped in an abandoned buffer. + $output = (string) ob_get_clean(); + } - delete_transient( 'woo_plugin_absorber_notices' ); + return $output; } } From 0fa25cd29805a0c31550b57b0ebced27a4e7a9b4 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Mon, 3 Aug 2026 15:54:54 +0200 Subject: [PATCH 03/10] Plan: store notices in an option and render them on all_admin_notices --- .../plans/2026-07-31-plugin-absorber.md | 49 +++++++++++++++---- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index d637a20..9f309ef 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -17,7 +17,7 @@ Every task's requirements implicitly include this section. - **PHP floor:** `>=7.4`. **WordPress floor:** 6.4 (the `wp_admin_notice_markup` filter). Stated in the README only — WordPress is not a Composer dependency, so it is not enforceable in `require`. - **Class naming:** `Snake_Case` (`Sub_Plugin`, `Conflict_Policy`, `Config_Exception`). Methods fully spelled out and readable. Config keys descriptive and WordPress-centric. - **Filter names:** `"{$hook_prefix}/plugin_absorber/should_load"` and `"{$hook_prefix}/plugin_absorber/conflict_policy"`. -- **Storage keys:** option `"{$hook_prefix}_plugin_absorber_activations"`, transient `"{$hook_prefix}_plugin_absorber_notices"`. +- **Storage keys:** option `"{$hook_prefix}_plugin_absorber_activations"`, option `"{$hook_prefix}_plugin_absorber_notices"`. **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: @@ -2911,7 +2911,32 @@ Lands before the load path and the resolver because both call into it. Task 11 calls `queue_dependency_notice()`; Task 12 calls `queue_merge_notice()` and `queue_conflict_notice()`; Task 14 extends this interface. **Design notes:** -- Transient `"{$hook_prefix}_plugin_absorber_notices"`, no expiry, so the queue survives the resolver's `wp_safe_redirect()` and renders on the next admin load. +- Option `"{$hook_prefix}_plugin_absorber_notices"`, so the queue survives the resolver's `wp_safe_redirect()` and renders on the next admin load. + +> **Deviations, deliberate (added 2026-08-03, from the PR 10 review):** +> +> 1. **An option, not a transient.** Verified in core: `set_transient()` short-circuits to +> `wp_cache_set()` and never writes the database when an external object cache is present. On a +> Redis or Memcached site the queue would exist only in the cache, and `wp_cache_flush()` — run +> by deploy scripts and every "purge cache" button — destroys it. The merge notice is raised +> once and never re-queued, so losing it means the site owner is never told. This queue is not +> a cache. See the amended Global Constraint. +> 2. **Network options on multisite.** The resolver passes `$network_wide` to +> `deactivate_plugins()`, which removes the plugin from every site in the network. A per-site +> option would have parked the explanation in whichever site's options table happened to serve +> the request that triggered it — invisible to the superadmin, on one of fifty sites. +> 3. **`render()` checks `activate_plugins` first.** Rendering *consumes* the queue, so without a +> gate any logged-in user loading `profile.php` would silently swallow the one warning an +> administrator was going to get. On multisite this correctly resolves to superadmins, since +> `activate_plugins` maps through `manage_network_plugins` there. +> 4. **`all_admin_notices`, not `admin_notices`** (see Task 11). The three notice hooks are +> mutually exclusive branches in `admin-header.php`, and `admin_notices` does not fire in the +> network admin — exactly where a network-wide deactivation would be noticed. +> 5. **`get_conflict_notice_message( $default )` replaces the planned private `message_or_default()` +> helper**, using the parameter added to `Sub_Plugin` in PR 7. Identical semantics, one less +> duplicated method. +> 6. **`get_queue()` drops non-string entries** rather than printing them, and writes the cleaned +> array back, so a corrupted queue heals on the next write. - Queue entries are keyed `"{$slug}:{$type}"`, not by slug alone. A sub-plugin can legitimately earn a merge notice at `plugins_loaded` @1 and a dependency notice at @2 in the same request; keying by slug alone would silently drop one. - Default messages live **here**, not in `Sub_Plugin`. `get_conflict_notice_message()` returns `''` when unconfigured (Task 7 asserts this), and each notice type supplies its own fallback sentence — so auto-deactivating a plugin can never leave the user with no explanation. @@ -3344,7 +3369,7 @@ gh pr create --base 09-loader-resolve --title "Notice queue" --body 'What: the t Usage: Loader::notices()->queue_merge_notice( $sub_plugin ); - Loader::notices()->render(); // hooked to admin_notices by boot() + Loader::notices()->render(); // hooked to all_admin_notices by boot() // Or supply your own copy: "conflict_notice_message" => static fn() => __( "Now bundled with Give.", "give" ), @@ -3385,7 +3410,7 @@ redirect (queue with one instance, render with another).' 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 `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. - [ ] **Step 1: Cut the branch** @@ -3614,7 +3639,7 @@ class LoaderBootTest extends WPTestCase { public function tearDown(): void { remove_all_actions( 'plugins_loaded' ); - remove_all_actions( 'admin_notices' ); + remove_all_actions( 'all_admin_notices' ); Loader::reset(); Config::reset(); parent::tearDown(); @@ -3643,7 +3668,7 @@ class LoaderBootTest extends WPTestCase { Loader::boot(); - $this->assertNotFalse( has_action( 'admin_notices', [ Loader::class, 'render_notices' ] ) ); + $this->assertNotFalse( has_action( 'all_admin_notices', [ Loader::class, 'render_notices' ] ) ); set_current_screen( 'front' ); } @@ -3690,7 +3715,11 @@ Append these methods, and extend `reset()` as shown at the end: add_action( 'plugins_loaded', [ self::class, 'load_all' ], 2 ); if ( is_admin() ) { - add_action( 'admin_notices', [ self::class, 'render_notices' ] ); + // 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' ] ); } } @@ -3842,7 +3871,7 @@ gh pr create --base 10-notices-queue --title "Loader boot and load path" --body Usage: Loader::register( [ ... ] ); - Loader::boot(); // wires plugins_loaded @2 and admin_notices + Loader::boot(); // wires plugins_loaded @2 and all_admin_notices add_filter( "give/plugin_absorber/should_load", function ( $should_load, $sub_plugin ) { return $should_load; @@ -5135,11 +5164,11 @@ Expected: FAIL — `Call to undefined method Nexcess\PluginAbsorber\Notices::fil } ``` -And inside `boot()`'s `is_admin()` block, beside the existing `admin_notices` line: +And inside `boot()`'s `is_admin()` block, beside the existing `all_admin_notices` line: ```php if ( is_admin() ) { - add_action( 'admin_notices', [ self::class, 'render_notices' ] ); + add_action( 'all_admin_notices', [ self::class, 'render_notices' ] ); add_filter( 'wp_admin_notice_markup', [ self::class, 'filter_activation_error_markup' ] ); } ``` From 5bd6fb096db854faf0ab2e8d6d3fa67a19193edd Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 12:23:30 +0200 Subject: [PATCH 04/10] Address review: document the notice queue and drop the last Loader reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The queue's accessor moved out of this change, so the test suite no longer imports Loader, resets it, or asserts that it resolves the default notices. Nothing here depended on that reset for isolation: the option is deleted in setUp() and tearDown(), and the hook prefix is reset through Config_State, so each test starts from an empty queue and an unset prefix on its own. The two raw queries that read the option row straight out of the database now pass the table through the %i identifier placeholder instead of interpolating it into the SQL. The query text is a literal again, which is what wpdb::prepare() asks for and what static analysis was rejecting. Verified against WordPress core before collapsing the multisite branching: update_site_option() delegates to update_network_option( null, ... ), which outside multisite ends in update_option( $option, $value, false ) — or add_option( $option, $value, '', false ) on the first write. Autoload is off on both paths, which is what the queue needs. get_site_option() and delete_site_option() fall through to get_option() and delete_option() the same way, so one call is correct on either install type. Also gives the queue a README section of its own: the option name, that it is a network option on multisite, that rendering is gated on activate_plugins and consumes the queue, and that option_name() is public so a host can render the queue without replacing anything. --- README.md | 24 ++++++++++++++++++++++++ src/Notices.php | 7 ++++--- tests/unit/NoticesTest.php | 19 ++++++++++--------- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 4fbf058..74c515f 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,30 @@ after the bundled copy has already loaded, so that re-declaration is a real fata it in its activation sandbox, and this library rewrites the resulting error screen into an explanation. +### Notices + +The three notices this library raises — the standalone was deactivated, the standalone is still +active, a dependency check failed — are queued in a single option named +`{prefix}_plugin_absorber_notices`, where `{prefix}` is the hook prefix you set. On multisite it is +a **network** option, so the queue is shared across every site on the network. + +An option and not a transient, on purpose. With a persistent object cache a transient never reaches +the database, so a `wp_cache_flush()` from a deploy script or a "purge cache" button would destroy +the queue. The deactivation notice is raised exactly once and never re-queued, so losing it means +the site owner is never told their plugin was turned off. + +`Notices::render()` prints the queue and then clears it, and it is gated on the `activate_plugins` +capability. Since rendering consumes the queue, a user who cannot act on a notice must not be shown +one — a subscriber loading their profile page would otherwise silently swallow the only warning an +administrator was ever going to get. Note that on multisite `activate_plugins` maps through +`manage_network_plugins`, so it is a network administrator, not the site administrator who installed +the plugin, who sees these. + +`Notices::option_name()` is public so you can render the queue yourself without replacing anything. +The value is an `array` keyed `slug:type` — for example `give-recurring:merge` — and +the messages are plain text; the default rendering escapes them with `esc_html()`, so a link in a +message comes out as literal angle brackets. + ### Filters | Filter | Arguments | Purpose | diff --git a/src/Notices.php b/src/Notices.php index 391cfec..5ff04db 100644 --- a/src/Notices.php +++ b/src/Notices.php @@ -209,9 +209,10 @@ private function queue( Sub_Plugin $sub_plugin, string $type, string $message ): $queue[ $sub_plugin->get_slug() . ':' . $type ] = $message; - // One call covers both install types: outside multisite `update_site_option()` runs - // `update_option( $option, $value, false )`, which is also exactly the autoload=false the - // queue wants — it is empty on almost every request and only the admin ever reads it. + // One call covers both install types. Outside multisite `update_site_option()` ends in + // `update_option( $option, $value, false )`, or `add_option( $option, $value, '', false )` + // the first time — either way autoload is off, which is exactly what this queue wants: it + // is empty on almost every request and only ever read in the admin. update_site_option( self::option_name(), $queue ); } diff --git a/tests/unit/NoticesTest.php b/tests/unit/NoticesTest.php index 73977ed..6cbd82f 100644 --- a/tests/unit/NoticesTest.php +++ b/tests/unit/NoticesTest.php @@ -10,7 +10,6 @@ use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Contracts\Notices_Interface; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Loader; use Nexcess\PluginAbsorber\Notices; use Nexcess\PluginAbsorber\Tests\Support\Config_State; use Nexcess\PluginAbsorber\Tests\Support\Traits\WithSubPlugins; @@ -29,7 +28,6 @@ class NoticesTest extends WPTestCase { public function setUp(): void { parent::setUp(); - Loader::reset(); Config_State::reset(); Config::set_hook_prefix( 'give' ); $this->clear_queue(); @@ -50,15 +48,10 @@ public function setUp(): void { public function tearDown(): void { $this->clear_queue(); delete_site_option( 'woo_plugin_absorber_notices' ); - Loader::reset(); Config_State::reset(); parent::tearDown(); } - public function test_the_loader_resolves_the_default_notices(): void { - $this->assertInstanceOf( Notices::class, Loader::notices() ); - } - public function test_the_default_notices_satisfy_the_contract(): void { $this->assertInstanceOf( Notices_Interface::class, new Notices() ); } @@ -485,6 +478,9 @@ private function queue_exists(): bool { /** * The serialized option value straight out of the database, bypassing the object cache. * + * The table name goes through the `%i` identifier placeholder rather than into the string, so + * the query stays a literal and nothing interpolated ever reaches the parser. + * * @return string */ private function stored_row(): string { @@ -494,14 +490,19 @@ private function stored_row(): string { if ( is_multisite() ) { $stored = $wpdb->get_var( $wpdb->prepare( - "SELECT meta_value FROM {$wpdb->sitemeta} WHERE meta_key = %s AND site_id = %d", + 'SELECT meta_value FROM %i WHERE meta_key = %s AND site_id = %d', + $wpdb->sitemeta, self::OPTION, get_current_network_id() ) ); } else { $stored = $wpdb->get_var( - $wpdb->prepare( "SELECT option_value FROM {$wpdb->options} WHERE option_name = %s", self::OPTION ) + $wpdb->prepare( + 'SELECT option_value FROM %i WHERE option_name = %s', + $wpdb->options, + self::OPTION + ) ); } From 6c7cf6422a4a0db778c0de8ee61409b88fb7c70e Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 13:32:00 +0200 Subject: [PATCH 05/10] Split Notices into a queue, a store and a renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One class decided what a notice said, where it was kept, who could consume it and how it was drawn. Task 14 adds a fifth job to the same class by hanging the activation-error rewrite off it. Notice_Store owns the option, Notice_Renderer owns the markup and the severity map, and Notices keeps the interface, the message defaults and the capability gate. The gate stays with the orchestration because it guards clearing the queue as much as drawing it. Both collaborators are constructor arguments that default to the standard implementations, so new Notices() — what Loader::resolve() builds when the container holds no binding — behaves as before and Notices_Interface is untouched. NoticesTest is unchanged apart from three added tests covering the new seam. --- README.md | 4 + .../plans/2026-07-31-plugin-absorber.md | 25 ++- src/Notice_Renderer.php | 84 ++++++++++ src/Notice_Store.php | 96 +++++++++++ src/Notices.php | 143 +++++----------- tests/unit/NoticeRendererTest.php | 141 ++++++++++++++++ tests/unit/NoticeStoreTest.php | 158 ++++++++++++++++++ tests/unit/NoticesTest.php | 101 +++++++++++ 8 files changed, 650 insertions(+), 102 deletions(-) create mode 100644 src/Notice_Renderer.php create mode 100644 src/Notice_Store.php create mode 100644 tests/unit/NoticeRendererTest.php create mode 100644 tests/unit/NoticeStoreTest.php diff --git a/README.md b/README.md index 74c515f..2031e0c 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,10 @@ The value is an `array` keyed `slug:type` — for example `give-r the messages are plain text; the default rendering escapes them with `esc_html()`, so a link in a message comes out as literal angle brackets. +If you want the queue but not the markup, hand `Notices` your own renderer — `new Notices( null, +$renderer )` — and bind that. Pass a `Notice_Store` in the first argument to keep the queue +somewhere other than the option. Replacing either one leaves the other alone. + ### Filters | Filter | Arguments | Purpose | diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index 9f309ef..7ed862a 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -2899,7 +2899,7 @@ container.' Lands before the load path and the resolver because both call into it. **Files:** -- Create: `src/Contracts/Notices_Interface.php`, `src/Notices.php`, `tests/unit/NoticesTest.php` +- Create: `src/Contracts/Notices_Interface.php`, `src/Notices.php`, `src/Notice_Store.php`, `src/Notice_Renderer.php`, `tests/unit/NoticesTest.php`, `tests/unit/NoticeStoreTest.php`, `tests/unit/NoticeRendererTest.php` - Modify: `src/Loader.php` (add the `notices()` accessor), `README.md` **Interfaces:** @@ -2936,7 +2936,28 @@ Lands before the load path and the resolver because both call into it. > helper**, using the parameter added to `Sub_Plugin` in PR 7. Identical semantics, one less > duplicated method. > 6. **`get_queue()` drops non-string entries** rather than printing them, and writes the cleaned -> array back, so a corrupted queue heals on the next write. +> array back, so a corrupted queue heals on the next write. This moved to `Notice_Store::all()` +> in deviation 7. +> 7. **`Notices` is split into three** (added 2026-08-11): `Notice_Store` owns the option, +> `Notice_Renderer` owns the markup and the severity map, and `Notices` keeps the interface, the +> message defaults and the capability gate. One class had four reasons to change — storage, +> wording, severity, markup — and Task 14 adds a fifth by hanging the activation-error rewrite +> off the same class. Splitting now means a host that wants only different markup replaces +> `Notice_Renderer` instead of reimplementing the queue. +> +> Both are constructor arguments defaulting to the standard implementations, so `new Notices()` +> — what `Loader::resolve()` builds when the container holds no binding — is unchanged, and so +> is `Notices_Interface`. Tasks 11, 12 and 14 are unaffected. +> +> The capability check stays in `Notices::render()` rather than moving into the renderer, +> because it guards clearing the queue as much as drawing it: split apart, a user who may not +> see the queue could still consume it. +> +> Not done, and deliberately: `Notices_Interface` still mixes queueing with rendering, so a host +> replacing one inherits the other, and a fourth notice type is still a breaking interface +> change. Both need a spec amendment and rework in Tasks 11, 12 and 14, and fixing them here +> would put this PR over the four-source-file cap. The same cap is why `Notice_Store` and +> `Notice_Renderer` are concrete rather than interface-backed. - Queue entries are keyed `"{$slug}:{$type}"`, not by slug alone. A sub-plugin can legitimately earn a merge notice at `plugins_loaded` @1 and a dependency notice at @2 in the same request; keying by slug alone would silently drop one. - Default messages live **here**, not in `Sub_Plugin`. `get_conflict_notice_message()` returns `''` when unconfigured (Task 7 asserts this), and each notice type supplies its own fallback sentence — so auto-deactivating a plugin can never leave the user with no explanation. diff --git a/src/Notice_Renderer.php b/src/Notice_Renderer.php new file mode 100644 index 0000000..6e2ddc5 --- /dev/null +++ b/src/Notice_Renderer.php @@ -0,0 +1,84 @@ + + */ + private const CLASSES = [ + Notices::TYPE_MERGE => 'notice-warning', + Notices::TYPE_CONFLICT => 'notice-warning', + Notices::TYPE_DEPENDENCY => 'notice-error', + ]; + + /** + * Print every message in the queue. + * + * Messages are printed through `esc_html()`, so they are plain text: markup a host puts in a + * message renders as literal angle brackets rather than as a link. + * + * @since 1.0.0 + * + * @param array $queue Queue to draw, keyed `slug:type`. + * + * @return void + */ + public function render( array $queue ): void { + foreach ( $queue as $key => $message ) { + $message = trim( $message ); + + // A whitespace-only message would print an empty notice box, which reads as a bug. + if ( $message === '' ) { + continue; + } + + printf( + '

%s

', + esc_attr( $this->notice_class( (string) $key ) ), + esc_html( $message ) + ); + } + } + + /** + * The `notice-*` class for a queue entry, taken from the type half of its `slug:type` key. + * + * @since 1.0.0 + * + * @param string $key Queue key. + * + * @return string + */ + private function notice_class( string $key ): string { + $parts = explode( ':', $key ); + $type = (string) end( $parts ); + + // An entry written by an older version, or by a host reading and rewriting the option, is + // shown rather than dropped: a warning is the safe severity for something unrecognised. + return self::CLASSES[ $type ] ?? 'notice-warning'; + } +} diff --git a/src/Notice_Store.php b/src/Notice_Store.php new file mode 100644 index 0000000..d4ea1d9 --- /dev/null +++ b/src/Notice_Store.php @@ -0,0 +1,96 @@ + + */ + public function all(): array { + // Outside multisite `get_site_option()` is `get_option()`, so this reads back whatever + // put() wrote on either install type. + $queue = get_site_option( self::option_name(), [] ); + + if ( ! is_array( $queue ) ) { + return []; + } + + // Anything that is not a string message is dropped rather than printed. put() writes the + // filtered array back, so a corrupted entry heals itself. + return array_filter( $queue, 'is_string' ); + } + + /** + * Store one message, replacing any it already holds under the same key. + * + * @since 1.0.0 + * + * @param string $key Queue key, `slug:type`. + * @param string $message Resolved message. + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + public function put( string $key, string $message ): void { + $queue = $this->all(); + + $queue[ $key ] = $message; + + // One call covers both install types. Outside multisite `update_site_option()` ends in + // `update_option( $option, $value, false )`, or `add_option( $option, $value, '', false )` + // the first time — either way autoload is off, which is exactly what this queue wants: it + // is empty on almost every request and only ever read in the admin. + update_site_option( self::option_name(), $queue ); + } + + /** + * @since 1.0.0 + * + * @throws Config_Exception When no hook prefix has been set. + * + * @return void + */ + public function clear(): void { + // Outside multisite `delete_site_option()` is `delete_option()`. + delete_site_option( self::option_name() ); + } +} diff --git a/src/Notices.php b/src/Notices.php index 5ff04db..1f1c7b5 100644 --- a/src/Notices.php +++ b/src/Notices.php @@ -11,39 +11,43 @@ /** * Default notices: an option-backed queue that survives the resolver's redirect. * - * An option rather than a transient. With an external object cache, `set_transient()` never - * touches the database at all — the queue would live only in Redis or Memcached, where a routine - * `wp_cache_flush()` from a deploy script or a "purge cache" button destroys it. The merge notice - * is raised once and never again, so losing it means a site owner is never told their plugin was - * deactivated. This queue is not a cache. + * This class decides *what a notice says* and *who is allowed to consume the queue*. Where the + * queue is kept is Notice_Store's job and how it is drawn is Notice_Renderer's, so a host can + * replace either one without inheriting the other, and neither has to be understood to reword a + * message. * - * Deliberately minimal markup so the library stays dependency-free. A host already using - * stellarwp/admin-notices can bind its own implementation and read the same option, whose name is - * `self::option_name()`. + * Both collaborators are constructor arguments with defaults, so `new Notices()` still gives the + * standard behaviour — which is what `Loader::resolve()` builds when the container has no binding. + * + * A host already using stellarwp/admin-notices can bind its own implementation of + * Notices_Interface and read the same option, whose name is `self::option_name()`. * * @since 1.0.0 */ class Notices implements Notices_Interface { /** + * Notice types. Public because they are the second half of a queue key — an entry is stored + * under `slug:type`, and reading the queue yourself means matching against these. + * * @since 1.0.0 * * @var string */ - private const TYPE_MERGE = 'merge'; + public const TYPE_MERGE = 'merge'; /** * @since 1.0.0 * * @var string */ - private const TYPE_CONFLICT = 'conflict'; + public const TYPE_CONFLICT = 'conflict'; /** * @since 1.0.0 * * @var string */ - private const TYPE_DEPENDENCY = 'dependency'; + public const TYPE_DEPENDENCY = 'dependency'; /** * Capability required to see, and thereby consume, the queue. @@ -59,21 +63,29 @@ class Notices implements Notices_Interface { private const CAPABILITY = 'activate_plugins'; /** - * The `notice-*` class each notice type renders with. + * @since 1.0.0 * - * A dependency notice reports a plugin that did not load at all, which is `notice-error` by - * WordPress convention. The other two report a conflict the library has already handled — the - * site works, so they are warnings. + * @var Notice_Store + */ + private $store; + + /** + * @since 1.0.0 * + * @var Notice_Renderer + */ + private $renderer; + + /** * @since 1.0.0 * - * @var array + * @param Notice_Store|null $store Where the queue is kept. + * @param Notice_Renderer|null $renderer How a queued notice is drawn. */ - private const CLASSES = [ - self::TYPE_MERGE => 'notice-warning', - self::TYPE_CONFLICT => 'notice-warning', - self::TYPE_DEPENDENCY => 'notice-error', - ]; + public function __construct( ?Notice_Store $store = null, ?Notice_Renderer $renderer = null ) { + $this->store = $store ?? new Notice_Store(); + $this->renderer = $renderer ?? new Notice_Renderer(); + } /** * @since 1.0.0 @@ -136,8 +148,11 @@ public function queue_dependency_notice( Sub_Plugin $sub_plugin ): void { } /** - * Messages are printed through `esc_html()`, so they are plain text: markup a host puts in a - * message renders as literal angle brackets rather than as a link. + * Draw the queue, then consume it. + * + * The capability check stays here rather than in the renderer because it guards the clearing + * as much as the drawing: the two have to be decided together or a user who may not see the + * queue could still destroy it. * * @since 1.0.0 * @@ -150,28 +165,15 @@ public function render(): void { return; } - $queue = $this->get_queue(); + $queue = $this->store->all(); if ( $queue === [] ) { return; } - foreach ( $queue as $key => $message ) { - $message = trim( $message ); + $this->renderer->render( $queue ); - // A whitespace-only message would print an empty notice box, which reads as a bug. - if ( $message === '' ) { - continue; - } - - printf( - '

%s

', - esc_attr( $this->notice_class( (string) $key ) ), - esc_html( $message ) - ); - } - - $this->clear_queue(); + $this->store->clear(); } /** @@ -184,7 +186,7 @@ public function render(): void { * @return string */ public static function option_name(): string { - return Config::get_hook_prefix() . '_plugin_absorber_notices'; + return Notice_Store::option_name(); } /** @@ -205,65 +207,6 @@ public static function option_name(): string { * @return void */ private function queue( Sub_Plugin $sub_plugin, string $type, string $message ): void { - $queue = $this->get_queue(); - - $queue[ $sub_plugin->get_slug() . ':' . $type ] = $message; - - // One call covers both install types. Outside multisite `update_site_option()` ends in - // `update_option( $option, $value, false )`, or `add_option( $option, $value, '', false )` - // the first time — either way autoload is off, which is exactly what this queue wants: it - // is empty on almost every request and only ever read in the admin. - update_site_option( self::option_name(), $queue ); - } - - /** - * @since 1.0.0 - * - * @throws Config_Exception When no hook prefix has been set. - * - * @return array - */ - private function get_queue(): array { - // Outside multisite `get_site_option()` is `get_option()`, so this reads back whatever - // queue() wrote on either install type. - $queue = get_site_option( self::option_name(), [] ); - - if ( ! is_array( $queue ) ) { - return []; - } - - // Anything that is not a string message is dropped rather than printed. queue() writes - // the filtered array back, so a corrupted entry heals itself. - return array_filter( $queue, 'is_string' ); - } - - /** - * @since 1.0.0 - * - * @throws Config_Exception When no hook prefix has been set. - * - * @return void - */ - private function clear_queue(): void { - // Outside multisite `delete_site_option()` is `delete_option()`. - delete_site_option( self::option_name() ); - } - - /** - * The `notice-*` class for a queue entry, taken from the type half of its `slug:type` key. - * - * @since 1.0.0 - * - * @param string $key Queue key. - * - * @return string - */ - private function notice_class( string $key ): string { - $parts = explode( ':', $key ); - $type = (string) end( $parts ); - - // An entry written by an older version, or by a host reading and rewriting the option, is - // shown rather than dropped: a warning is the safe severity for something unrecognised. - return self::CLASSES[ $type ] ?? 'notice-warning'; + $this->store->put( $sub_plugin->get_slug() . ':' . $type, $message ); } } diff --git a/tests/unit/NoticeRendererTest.php b/tests/unit/NoticeRendererTest.php new file mode 100644 index 0000000..15c51d8 --- /dev/null +++ b/tests/unit/NoticeRendererTest.php @@ -0,0 +1,141 @@ +render( [ 'give-recurring:merge' => 'Bundled now.' ] ); + + $this->assertStringContainsString( 'is-dismissible', $output ); + $this->assertStringContainsString( 'Bundled now.', $output ); + } + + /** + * @dataProvider notice_severities + * + * @param string $key Queue key to render under. + * @param string $class Expected `notice-*` class. + */ + public function test_the_type_half_of_the_key_picks_the_severity( string $key, string $class ): void { + $this->assertStringContainsString( + 'notice ' . $class . ' is-dismissible', + $this->render( [ $key => 'Something happened.' ] ) + ); + } + + /** + * A dependency notice reports a plugin that did not load, which is an error; the conflict pair + * report something the library handled, which is a warning. An unrecognised type is drawn as a + * warning rather than dropped — it may have been written by an older version, or by a host + * reading and rewriting the option. + * + * @return Generator + */ + public static function notice_severities(): Generator { + yield 'merge' => [ 'give-recurring:' . Notices::TYPE_MERGE, 'notice-warning' ]; + yield 'conflict' => [ 'give-recurring:' . Notices::TYPE_CONFLICT, 'notice-warning' ]; + yield 'dependency' => [ 'give-recurring:' . Notices::TYPE_DEPENDENCY, 'notice-error' ]; + yield 'unknown type' => [ 'give-recurring:invented', 'notice-warning' ]; + yield 'no type at all' => [ 'give-recurring', 'notice-warning' ]; + } + + /** + * A slug containing a colon still resolves to the type, because the type is the last segment + * rather than the second. + */ + public function test_the_type_is_the_last_segment_of_the_key(): void { + $this->assertStringContainsString( + 'notice-error', + $this->render( [ 'give:recurring:' . Notices::TYPE_DEPENDENCY => 'Requirements not met.' ] ) + ); + } + + public function test_it_escapes_the_message(): void { + $output = $this->render( [ 'a:merge' => '' ] ); + + $this->assertStringNotContainsString( '' ] ) ); $output = $this->render_to_string( $notices ); @@ -254,7 +254,7 @@ public function test_render_escapes_the_message(): void { * tightening it later is not. */ public function test_render_does_not_allow_markup_in_a_message(): void { - $notices = new Notices(); + $notices = new Queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'See the docs.' ] ) ); @@ -266,7 +266,7 @@ public function test_render_does_not_allow_markup_in_a_message(): void { } public function test_render_clears_the_queue(): void { - $notices = new Notices(); + $notices = new Queue(); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->render_to_string( $notices ); @@ -276,7 +276,7 @@ public function test_render_clears_the_queue(): void { } public function test_render_outputs_every_queued_notice(): void { - $notices = new Notices(); + $notices = new Queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'First.' ] ) ); $notices->queue_dependency_notice( $this->make_sub_plugin( [ 'dependency_notice_message' => 'Second.' ] ) ); @@ -287,16 +287,16 @@ 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 Notices() ) ); + $this->assertSame( '', $this->render_to_string( new 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 Notices()` — which is what Loader::resolve() builds — is unaffected. + * `new Queue()` — which is what Loader::resolve() builds — is unaffected. */ public function test_a_replacement_store_is_used_instead_of_the_option(): void { - $store = new class() extends Notice_Store { + $store = new class() extends Store { /** * @var array */ @@ -327,7 +327,7 @@ public function clear(): void { } }; - ( new Notices( $store ) )->queue_merge_notice( + ( new Queue( $store ) )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); @@ -339,7 +339,7 @@ public function clear(): void { * The other half of the same seam: different markup, same queue and same consumption rules. */ public function test_a_replacement_renderer_draws_the_queue(): void { - $renderer = new class() extends Notice_Renderer { + $renderer = new class() extends Renderer { /** * @param array $queue Queue to draw. * @@ -350,7 +350,7 @@ public function render( array $queue ): void { } }; - $notices = new Notices( null, $renderer ); + $notices = new Queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( '

1

', $this->render_to_string( $notices ) ); @@ -362,7 +362,7 @@ public function render( array $queue ): void { * the renderer rather than inside it: a user who may not see the queue must not destroy it. */ public function test_a_replacement_renderer_is_never_reached_without_the_capability(): void { - $renderer = new class() extends Notice_Renderer { + $renderer = new class() extends Renderer { /** * @var bool */ @@ -378,7 +378,7 @@ public function render( array $queue ): void { } }; - $notices = new Notices( null, $renderer ); + $notices = new Queue( null, $renderer ); $notices->queue_merge_notice( $this->make_sub_plugin() ); wp_set_current_user( $this->create_user( 'subscriber' ) ); @@ -398,7 +398,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 Notices(); + $notices = new Queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); wp_set_current_user( $role === null ? 0 : $this->create_user( $role ) ); @@ -426,7 +426,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 Notices(); + $notices = new Queue(); $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); wp_set_current_user( $this->create_user( 'administrator' ) ); @@ -443,7 +443,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 Notices() )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); + ( new Queue() )->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Bundled now.' ] ) ); $this->assertStringContainsString( 'Bundled now.', @@ -453,7 +453,7 @@ public function test_the_queue_is_a_durable_database_row(): void { wp_cache_flush(); - $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( new Notices() ) ); + $this->assertStringContainsString( 'Bundled now.', $this->render_to_string( new Queue() ) ); } /** @@ -467,7 +467,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 Notices() ); + $output = $this->render_to_string( new Queue() ); if ( $present === null ) { $this->assertSame( '', $output ); @@ -516,7 +516,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 Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); $this->assertSame( [ 'give-recurring:merge' ], array_keys( $this->queue() ) ); } @@ -525,9 +525,9 @@ public function test_the_option_is_keyed_by_the_hook_prefix(): void { Config_State::reset(); Config::set_hook_prefix( 'woo' ); - $this->assertSame( 'woo_plugin_absorber_notices', Notices::option_name() ); + $this->assertSame( 'woo_plugin_absorber_notices', Queue::option_name() ); - ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); $this->assertIsArray( get_site_option( 'woo_plugin_absorber_notices', false ) ); $this->assertFalse( $this->queue_exists() ); @@ -538,7 +538,7 @@ public function test_queueing_needs_a_hook_prefix(): void { $this->expectException( Config_Exception::class ); - ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); } /** @@ -550,7 +550,7 @@ public function test_the_queue_is_not_autoloaded(): void { $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); } - ( new Notices() )->queue_merge_notice( $this->make_sub_plugin() ); + ( new Queue() )->queue_merge_notice( $this->make_sub_plugin() ); $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); } @@ -647,7 +647,7 @@ private function create_user( string $role ): int { return $user_id; } - private function render_to_string( Notices $notices ): string { + private function render_to_string( Queue $notices ): string { ob_start(); try { diff --git a/tests/unit/NoticeRendererTest.php b/tests/unit/Notices/RendererTest.php similarity index 87% rename from tests/unit/NoticeRendererTest.php rename to tests/unit/Notices/RendererTest.php index 15c51d8..2be51c7 100644 --- a/tests/unit/NoticeRendererTest.php +++ b/tests/unit/Notices/RendererTest.php @@ -3,12 +3,12 @@ * @package Nexcess\PluginAbsorber */ -namespace Nexcess\PluginAbsorber\Tests\Unit; +namespace Nexcess\PluginAbsorber\Tests\Unit\Notices; use Codeception\TestCase\WPTestCase; use Generator; -use Nexcess\PluginAbsorber\Notice_Renderer; -use Nexcess\PluginAbsorber\Notices; +use Nexcess\PluginAbsorber\Notices\Queue; +use Nexcess\PluginAbsorber\Notices\Renderer; /** * The drawing half of the queue. @@ -18,7 +18,7 @@ * * @since 1.0.0 */ -class NoticeRendererTest extends WPTestCase { +class RendererTest extends WPTestCase { public function test_it_prints_dismissible_markup(): void { $output = $this->render( [ 'give-recurring:merge' => 'Bundled now.' ] ); @@ -48,9 +48,9 @@ public function test_the_type_half_of_the_key_picks_the_severity( string $key, s * @return Generator */ public static function notice_severities(): Generator { - yield 'merge' => [ 'give-recurring:' . Notices::TYPE_MERGE, 'notice-warning' ]; - yield 'conflict' => [ 'give-recurring:' . Notices::TYPE_CONFLICT, 'notice-warning' ]; - yield 'dependency' => [ 'give-recurring:' . Notices::TYPE_DEPENDENCY, 'notice-error' ]; + yield 'merge' => [ 'give-recurring:' . Queue::TYPE_MERGE, 'notice-warning' ]; + yield 'conflict' => [ 'give-recurring:' . Queue::TYPE_CONFLICT, 'notice-warning' ]; + yield 'dependency' => [ 'give-recurring:' . Queue::TYPE_DEPENDENCY, 'notice-error' ]; yield 'unknown type' => [ 'give-recurring:invented', 'notice-warning' ]; yield 'no type at all' => [ 'give-recurring', 'notice-warning' ]; } @@ -62,7 +62,7 @@ public static function notice_severities(): Generator { public function test_the_type_is_the_last_segment_of_the_key(): void { $this->assertStringContainsString( 'notice-error', - $this->render( [ 'give:recurring:' . Notices::TYPE_DEPENDENCY => 'Requirements not met.' ] ) + $this->render( [ 'give:recurring:' . Queue::TYPE_DEPENDENCY => 'Requirements not met.' ] ) ); } @@ -129,7 +129,7 @@ private function render( array $queue ): string { ob_start(); try { - ( new Notice_Renderer() )->render( $queue ); + ( new Renderer() )->render( $queue ); } finally { // In a finally block so a throw from render() cannot leave the suite's own output // trapped in an abandoned buffer. diff --git a/tests/unit/NoticeStoreTest.php b/tests/unit/Notices/StoreTest.php similarity index 80% rename from tests/unit/NoticeStoreTest.php rename to tests/unit/Notices/StoreTest.php index 94d2cc7..42d9bf4 100644 --- a/tests/unit/NoticeStoreTest.php +++ b/tests/unit/Notices/StoreTest.php @@ -3,25 +3,25 @@ * @package Nexcess\PluginAbsorber */ -namespace Nexcess\PluginAbsorber\Tests\Unit; +namespace Nexcess\PluginAbsorber\Tests\Unit\Notices; use Codeception\TestCase\WPTestCase; use Generator; use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; -use Nexcess\PluginAbsorber\Notice_Store; +use Nexcess\PluginAbsorber\Notices\Store; use Nexcess\PluginAbsorber\Tests\Support\Config_State; /** - * The storage half of the queue, exercised without going through Notices. + * The storage half of the queue, exercised without going through Queue. * - * NoticesTest already covers this ground from the outside; these are the assertions that belong to + * QueueTest already covers this ground from the outside; these are the assertions that belong to * the store itself, so that swapping how notices are worded or drawn cannot quietly take the * storage guarantees with it. * * @since 1.0.0 */ -class NoticeStoreTest extends WPTestCase { +class StoreTest extends WPTestCase { private const OPTION = 'give_plugin_absorber_notices'; public function setUp(): void { @@ -40,9 +40,9 @@ public function tearDown(): void { } public function test_it_stores_a_message_under_the_given_key(): void { - ( new Notice_Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); - $this->assertSame( [ 'give-recurring:merge' => 'Bundled now.' ], ( new Notice_Store() )->all() ); + $this->assertSame( [ 'give-recurring:merge' => 'Bundled now.' ], ( new Store() )->all() ); } /** @@ -51,15 +51,15 @@ public function test_it_stores_a_message_under_the_given_key(): void { * never the writing one. */ public function test_the_queue_outlives_the_instance_that_wrote_it(): void { - ( new Notice_Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); wp_cache_flush(); - $this->assertSame( 'Bundled now.', ( new Notice_Store() )->all()['give-recurring:merge'] ?? '' ); + $this->assertSame( 'Bundled now.', ( new Store() )->all()['give-recurring:merge'] ?? '' ); } public function test_writing_the_same_key_twice_replaces_rather_than_duplicates(): void { - $store = new Notice_Store(); + $store = new Store(); $store->put( 'give-recurring:merge', 'First.' ); $store->put( 'give-recurring:merge', 'Second.' ); @@ -67,7 +67,7 @@ public function test_writing_the_same_key_twice_replaces_rather_than_duplicates( } public function test_different_keys_coexist(): void { - $store = new Notice_Store(); + $store = new Store(); $store->put( 'give-recurring:merge', 'One.' ); $store->put( 'give-recurring:dependency', 'Two.' ); @@ -75,7 +75,7 @@ public function test_different_keys_coexist(): void { } public function test_clear_removes_the_row_entirely(): void { - $store = new Notice_Store(); + $store = new Store(); $store->put( 'give-recurring:merge', 'Bundled now.' ); $store->clear(); @@ -93,7 +93,7 @@ public function test_clear_removes_the_row_entirely(): void { public function test_it_drops_anything_that_is_not_a_message( $stored, array $expected ): void { update_site_option( self::OPTION, $stored ); - $this->assertSame( $expected, ( new Notice_Store() )->all() ); + $this->assertSame( $expected, ( new Store() )->all() ); } /** @@ -122,16 +122,16 @@ public static function malformed_queues(): Generator { public function test_a_corrupted_queue_heals_on_the_next_write(): void { update_site_option( self::OPTION, [ 'a:merge' => [ 'nested' ] ] ); - ( new Notice_Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); - $this->assertSame( [ 'give-recurring:merge' ], array_keys( ( new Notice_Store() )->all() ) ); + $this->assertSame( [ 'give-recurring:merge' ], array_keys( ( new Store() )->all() ) ); } public function test_the_option_is_keyed_by_the_hook_prefix(): void { Config_State::reset(); Config::set_hook_prefix( 'woo' ); - $this->assertSame( 'woo_plugin_absorber_notices', Notice_Store::option_name() ); + $this->assertSame( 'woo_plugin_absorber_notices', Store::option_name() ); } public function test_it_needs_a_hook_prefix(): void { @@ -139,7 +139,7 @@ public function test_it_needs_a_hook_prefix(): void { $this->expectException( Config_Exception::class ); - ( new Notice_Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); } /** @@ -151,7 +151,7 @@ public function test_the_queue_is_not_autoloaded(): void { $this->markTestSkipped( 'Network options are not part of the per-site autoload bundle.' ); } - ( new Notice_Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); $this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) ); } From 9f780bbb908abf05c698f395ed9a25dbf1462d52 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 16:47:11 +0200 Subject: [PATCH 07/10] Refactor notice handling and configuration options - Introduced `Notices\Contracts\Queue_Interface` to define the structure for managing admin notices. - Updated `Config` class to include methods for generating option names with normalized prefixes, ensuring consistent storage keys. - Enhanced `Store` class to utilize the new option name generation method. - Updated documentation to clarify the distinction between hook names and option names, including examples of their usage. - Added unit tests to validate the new functionality and ensure proper handling of hook prefixes in option names. --- CLAUDE.md | 25 ++++++++-- docs/configuration.md | 6 ++- .../2026-07-31-plugin-absorber-design.md | 12 ++--- src/Config.php | 49 ++++++++++++++++++- .../{ => Contracts}/Queue_Interface.php | 2 +- src/Notices/Queue.php | 1 + src/Notices/Store.php | 6 ++- tests/unit/ConfigTest.php | 45 +++++++++++++++++ tests/unit/Notices/StoreTest.php | 28 ++++++++++- 9 files changed, 156 insertions(+), 18 deletions(-) rename src/Notices/{ => Contracts}/Queue_Interface.php (98%) diff --git a/CLAUDE.md b/CLAUDE.md index df7e2d4..b41aa31 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,10 +65,14 @@ Four interface-backed collaborators, each with a default implementation: | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | -| `Notices\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | -| `Conflict\Resolver_Interface` | `Conflict\Resolver` | standalone detection, deactivation, redirect | +| `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 | +An interface belonging to a folder-scoped concern lives in that folder's `Contracts\`, not beside its +implementation and not in the top-level `src/Contracts/`. `src/Contracts/` is for the interfaces whose +implementations sit at the root — `Registrar`, `Plugin_State`, `Activation`. + All four come through one generic helper — `Loader::resolve( string $interface, string $default_class ): object` — which returns the container binding when `$container->has()`, otherwise `new $default_class()`, memoized either way. Collaborators reach each other through the accessors @@ -95,7 +99,7 @@ config predicates there is what lets collaborators stay thin and lets them be te | `src/Conflict_Policy.php` | The three policy constants, `default()`, `is_valid()`. | | `src/Plugin_State.php` | The only file that touches WordPress plugin functions. | | `src/Registrar.php` | Holds registered `Sub_Plugin` objects. | -| `src/Notices/` | `Queue` (what a notice says, who may consume it), `Store` (keeps it), `Renderer` (draws it). | +| `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`. | ### Boot lifecycle @@ -128,7 +132,13 @@ owner deliberately turned on. ### Keys - Filters: `{$hook_prefix}/plugin_absorber/should_load`, `{$hook_prefix}/plugin_absorber/conflict_policy` -- Options: `{$hook_prefix}_plugin_absorber_activations`, `{$hook_prefix}_plugin_absorber_notices` +- Options: `{$option_prefix}_plugin_absorber_activations`, `{$option_prefix}_plugin_absorber_notices` + +Both are built in `Config` — `get_hook_name()` and `get_option_name()` — so nothing else assembles +the segment between the host's prefix and the key's own name. The two differ in one respect: +`{$option_prefix}` is the hook prefix lowercased with hyphens folded to underscores, because the +prefix validator admits `A-Z` and `-` and a hook-naming value should not reach a storage key +verbatim. Hook names keep the host's casing exactly as it passed it. The notice queue is an option, not a transient: with a persistent object cache a transient never reaches the database, so a `wp_cache_flush()` would destroy a merge notice that is raised exactly @@ -203,6 +213,13 @@ treatment. Any older sketch showing `Config::reset()` or `Loader::reset()` means - **The guard constant and the standalone basename are two separate keys.** No constant does double duty as both a load guard and a path resolver. +- **`get_hook_name()` and `get_option_name()` do not share a normalisation.** Folding case into the + hook prefix itself would silently rename the host's filters; leaving the raw prefix in an option + name puts `A-Z` and `-` into a storage key. Only the option side normalises, and collapsing the two + code paths breaks whichever end it is collapsed toward. +- **Notice messages are rendered through `wp_kses_post()`, not escaped.** They come from the host's + own config or filter, never from user input, so a knowledge-base link survives. Tightening this to + `esc_html()` after 1.0 would break every host that shipped one. - **String-only config keys reject callables.** `standalone_plugin_basename`, `conflict_policy`, `conflict_notice_message`, `dependency_notice_message` throw `Config_Exception` on a non-string. A string function name is indistinguishable from a string value, so honouring both would make the diff --git a/docs/configuration.md b/docs/configuration.md index 04ef2b4..af2dceb 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -5,12 +5,14 @@ ```php use Nexcess\PluginAbsorber\Config; -Config::set_hook_prefix( 'give' ); // required — keys hooks, transients, options +Config::set_hook_prefix( 'give' ); // required — keys hooks and options Config::set_container( give()->container ); // optional — lets you rebind collaborators ``` The hook prefix accepts letters, numbers, hyphens, and underscores. Anything else throws -`Config_Exception`, as does reading the prefix before it is set. +`Config_Exception`, as does reading the prefix before it is set. Hook names repeat it verbatim; +option names lowercase it and turn hyphens into underscores, so `Give-Core` hooks +`Give-Core/plugin_absorber/should_load` and stores `give_core_plugin_absorber_notices`. The container is optional. Without one, the library instantiates its own collaborators; with one, a host can rebind them. 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 5366d69..8664a20 100644 --- a/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md +++ b/docs/superpowers/specs/2026-07-31-plugin-absorber-design.md @@ -16,7 +16,7 @@ Where the two disagree, this document wins. | Repository | `github.com/stellarwp/plugin-absorber` | | Root namespace | `Nexcess\PluginAbsorber\` | | Filter segment | `{hook_prefix}/plugin_absorber/…` | -| Option / transient | `{hook_prefix}_plugin_absorber_activations` / `…_notices` | +| Option / transient | `{option_prefix}_plugin_absorber_activations` / `…_notices`, where `{option_prefix}` is the hook prefix lowercased with hyphens folded to underscores | | PR strategy | Stacked branches, merged to `main` in order | | Test strategy | Codeception + `wp-browser`, WPLoader (real WP) + `uopz` for unhookable functions | | PR size cap | ≤ 10 files, tests excluded; no logic-bearing PR exceeds 4 source files | @@ -50,7 +50,7 @@ fatal string. The filter is strictly better: no buffering, no risk of mangling u output, and testable by calling the filter directly. Consequences: -- `Notices\Queue_Interface` does **not** declare `start_buffer()`. +- `Notices\Contracts\Queue_Interface` does **not** declare `start_buffer()`. - `Loader::boot()` does **not** hook `admin_head-plugins.php`. - The library requires **WordPress 6.4+** (when the filter landed). Stated in the README only — not enforceable through Composer, since WordPress is not a Composer dependency. @@ -142,8 +142,8 @@ trampolines, per the `admin-notices` precedent. | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | holds registered `Sub_Plugin` objects | -| `Notices\Queue_Interface` | `Notices\Queue` | notice queue + activation-error rewrite | -| `Conflict\Resolver_Interface` | `Conflict\Resolver` | standalone detection, deactivation, redirect | +| `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 | ### Config schema @@ -182,9 +182,9 @@ from the size cap. | 7 | `07-sub-plugin` | 2 | `Sub_Plugin`, README | | 8 | `08-registrar` | 3 | `Contracts\Registrar_Interface`, `Registrar`, README | | 9 | `09-loader-resolve` | 2 | `Loader`, README | -| 10 | `10-notices-queue` | 4 | `Notices\Queue_Interface`, `Notices\Queue`, `Notices\Store`, `Notices\Renderer`, README | +| 10 | `10-notices-queue` | 4 | `Notices\Contracts\Queue_Interface`, `Notices\Queue`, `Notices\Store`, `Notices\Renderer`, README | | 11 | `11-loader-load-path` | 2 | `Loader` mod, README | -| 12 | `12-conflict-resolver` | 4 | `Conflict\Resolver_Interface`, `Conflict\Resolver`, `Loader` mod, README | +| 12 | `12-conflict-resolver` | 4 | `Conflict\Contracts\Resolver_Interface`, `Conflict\Resolver`, `Loader` mod, README | | 13 | `13-activation` | 4 | `Contracts\Activation_Interface`, `Activation`, `Loader` mod, README | | 14 | `14-activation-error-notice` | 3 | `Notices\Queue` mod, `Loader` mod, README | | 15 | `15-e2e-fixtures` | 1 | README | diff --git a/src/Config.php b/src/Config.php index 9d97e21..30ee51a 100644 --- a/src/Config.php +++ b/src/Config.php @@ -25,7 +25,10 @@ class Config { protected static $container = null; /** - * Set the unique per-host slug that keys hooks, transients, and the activation option. + * Set the unique per-host slug that keys this library's hooks and options. + * + * It is stored exactly as given: hook names repeat it verbatim, and only `get_option_name()` + * folds it. * * @since 1.0.0 * @@ -85,6 +88,32 @@ public static function get_hook_name( string $name ): string { return self::get_hook_prefix() . '/plugin_absorber/' . $name; } + /** + * Build the name of one of this library's options. + * + * The prefix is a hook-naming value: `set_hook_prefix()` takes anything WordPress will accept + * inside a filter name, mixed case and hyphens included, and `get_hook_name()` repeats it byte + * for byte — a host that passed `Give-Core` must be able to hook + * `Give-Core/plugin_absorber/should_load` and have it fire. A storage key answers to a + * narrower convention, so the folding happens here and nowhere else: the same prefix produces + * `give_core_plugin_absorber_notices`. + * + * The `_plugin_absorber_` segment lives here for the reason `get_hook_name()` gives for its + * own: the notice queue is not the only option keyed this way, and a segment each caller + * assembled would have to be found in every one of them if it ever changed. + * + * @since 1.0.0 + * + * @param string $name Option name, less the prefix and this library's namespace. + * + * @throws Config_Exception When no prefix has been set. + * + * @return string + */ + public static function get_option_name( string $name ): string { + return self::get_option_prefix() . '_plugin_absorber_' . $name; + } + /** * Share the host's container so collaborators become bindable. * @@ -117,4 +146,22 @@ public static function get_container(): ?ContainerInterface { public static function has_container(): bool { return self::$container !== null; } + + /** + * The hook prefix folded into the shape a storage key takes. + * + * Only option names are folded, and only on the way out — the stored prefix keeps whatever the + * host passed, because that is what its hook names are made of. Two prefixes differing only in + * case or in hyphens against underscores would land on the same option, which is the price of + * asking a host for one prefix rather than two; no host runs both `Give-Core` and `give_core`. + * + * @since 1.0.0 + * + * @throws Config_Exception When no prefix has been set. + * + * @return string + */ + private static function get_option_prefix(): string { + return strtolower( str_replace( '-', '_', self::get_hook_prefix() ) ); + } } diff --git a/src/Notices/Queue_Interface.php b/src/Notices/Contracts/Queue_Interface.php similarity index 98% rename from src/Notices/Queue_Interface.php rename to src/Notices/Contracts/Queue_Interface.php index fcc3700..d261556 100644 --- a/src/Notices/Queue_Interface.php +++ b/src/Notices/Contracts/Queue_Interface.php @@ -3,7 +3,7 @@ * @package Nexcess\PluginAbsorber */ -namespace Nexcess\PluginAbsorber\Notices; +namespace Nexcess\PluginAbsorber\Notices\Contracts; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; use Nexcess\PluginAbsorber\Sub_Plugin; diff --git a/src/Notices/Queue.php b/src/Notices/Queue.php index 6f03c43..83d202d 100644 --- a/src/Notices/Queue.php +++ b/src/Notices/Queue.php @@ -6,6 +6,7 @@ namespace Nexcess\PluginAbsorber\Notices; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Sub_Plugin; /** diff --git a/src/Notices/Store.php b/src/Notices/Store.php index 857ab7e..23297db 100644 --- a/src/Notices/Store.php +++ b/src/Notices/Store.php @@ -24,7 +24,9 @@ */ class Store { /** - * The option name backing the queue. Read it directly to render these notices yourself. + * The option name backing the queue. Read it directly to render these notices yourself — and + * read it from here rather than composing it, since the hook prefix is normalised on its way + * into a storage key. * * @since 1.0.0 * @@ -33,7 +35,7 @@ class Store { * @return string */ public static function option_name(): string { - return Config::get_hook_prefix() . '_plugin_absorber_notices'; + return Config::get_option_name( 'notices' ); } /** diff --git a/tests/unit/ConfigTest.php b/tests/unit/ConfigTest.php index 8a41a82..169526d 100644 --- a/tests/unit/ConfigTest.php +++ b/tests/unit/ConfigTest.php @@ -105,6 +105,51 @@ public function test_a_hook_name_needs_a_prefix(): void { Config::get_hook_name( 'conflict_policy' ); } + /** + * The assertion that keeps hook names and option names apart. Only the storage key is folded, + * so a host that passed `Give-Core` can still hook the filter name it was given. + */ + public function test_a_hook_name_keeps_the_prefix_verbatim(): void { + Config::set_hook_prefix( 'Give-Core' ); + + $this->assertSame( + 'Give-Core/plugin_absorber/should_load', + Config::get_hook_name( 'should_load' ) + ); + } + + /** + * @dataProvider option_name_prefixes + * + * @param string $prefix Prefix under test. + * @param string $expected Option name it must produce. + */ + public function test_it_builds_an_option_name_from_a_normalised_prefix( + string $prefix, + string $expected + ): void { + Config::set_hook_prefix( $prefix ); + + $this->assertSame( $expected, Config::get_option_name( 'notices' ) ); + } + + /** + * @return Generator + */ + public static function option_name_prefixes(): Generator { + yield 'nothing to fold' => [ 'give', 'give_plugin_absorber_notices' ]; + yield 'underscore is kept' => [ 'give_recurring', 'give_recurring_plugin_absorber_notices' ]; + yield 'mixed case' => [ 'GiveRecurring', 'giverecurring_plugin_absorber_notices' ]; + yield 'hyphen' => [ 'give-recurring', 'give_recurring_plugin_absorber_notices' ]; + yield 'mixed case and hyphen' => [ 'Give-Core', 'give_core_plugin_absorber_notices' ]; + } + + public function test_an_option_name_needs_a_prefix(): void { + $this->expectException( Config_Exception::class ); + + Config::get_option_name( 'notices' ); + } + public function test_it_reports_no_container_by_default(): void { $this->assertFalse( Config::has_container() ); $this->assertNull( Config::get_container() ); diff --git a/tests/unit/Notices/StoreTest.php b/tests/unit/Notices/StoreTest.php index 42d9bf4..464c0eb 100644 --- a/tests/unit/Notices/StoreTest.php +++ b/tests/unit/Notices/StoreTest.php @@ -24,6 +24,10 @@ class StoreTest extends WPTestCase { private const OPTION = 'give_plugin_absorber_notices'; + private const OPTION_WOO = 'woo_plugin_absorber_notices'; + + private const OPTION_NORMALISED = 'give_core_plugin_absorber_notices'; + public function setUp(): void { parent::setUp(); @@ -34,7 +38,8 @@ public function setUp(): void { public function tearDown(): void { delete_site_option( self::OPTION ); - delete_site_option( 'woo_plugin_absorber_notices' ); + delete_site_option( self::OPTION_WOO ); + delete_site_option( self::OPTION_NORMALISED ); Config_State::reset(); parent::tearDown(); } @@ -131,7 +136,26 @@ public function test_the_option_is_keyed_by_the_hook_prefix(): void { Config_State::reset(); Config::set_hook_prefix( 'woo' ); - $this->assertSame( 'woo_plugin_absorber_notices', Store::option_name() ); + $this->assertSame( self::OPTION_WOO, Store::option_name() ); + } + + /** + * The hook prefix is allowed mixed case and hyphens because it names filters. An option name + * is a storage key, so the prefix reaches the database folded rather than raw. + */ + public function test_the_option_name_normalises_the_hook_prefix(): void { + Config_State::reset(); + Config::set_hook_prefix( 'Give-Core' ); + + $this->assertSame( self::OPTION_NORMALISED, Store::option_name() ); + + ( new Store() )->put( 'give-recurring:merge', 'Bundled now.' ); + + $this->assertSame( + [ 'give-recurring:merge' => 'Bundled now.' ], + get_site_option( self::OPTION_NORMALISED ), + 'The queue must be written under the normalised name.' + ); } public function test_it_needs_a_hook_prefix(): void { From d75c41a84133c2f6ebd88c3f8135c4f0226bb03d Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 16:52:05 +0200 Subject: [PATCH 08/10] Enhance notice handling and markup processing - Updated `Notices\Queue` and `Notices\Contracts\Queue_Interface` to allow messages with markup, utilizing `wp_kses_post()` for safe rendering. - Modified `Notices\Renderer` to reflect changes in message processing, ensuring links and formatting are preserved while disallowing scripts and event handlers. - Revised documentation to clarify the handling of notices and the implications of markup in messages. - Enhanced unit tests to validate the new rendering behavior and ensure proper handling of markup in notices. --- docs/notices.md | 5 +- .../plans/2026-07-31-plugin-absorber.md | 145 ++++++++++++------ .../2026-07-31-plugin-absorber-design.md | 2 +- src/Notices/Contracts/Queue_Interface.php | 10 +- src/Notices/Renderer.php | 16 +- tests/unit/Notices/QueueTest.php | 38 +++-- tests/unit/Notices/RendererTest.php | 40 +++-- 7 files changed, 173 insertions(+), 83 deletions(-) diff --git a/docs/notices.md b/docs/notices.md index da816e8..6ef285b 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -25,8 +25,9 @@ administrator, not the site administrator who installed the plugin, who sees the `Notices\Queue::option_name()` is public, so you can render the queue yourself without replacing anything. The value is an `array` keyed `slug:type` — `give-recurring:merge`, for -example — and the messages are plain text; the default rendering escapes them with `esc_html()`, so -a link in a message comes out as literal angle brackets. +example — and the messages may contain markup; the default rendering passes them through +`wp_kses_post()`, so a link, emphasis or a list survives while scripts and event handlers are +stripped. 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, diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index 641feb0..e37a555 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -17,7 +17,7 @@ Every task's requirements implicitly include this section. - **PHP floor:** `>=7.4`. **WordPress floor:** 6.4 (the `wp_admin_notice_markup` filter). Stated in the README only — WordPress is not a Composer dependency, so it is not enforceable in `require`. - **Class naming:** `Snake_Case` (`Sub_Plugin`, `Conflict_Policy`, `Config_Exception`). Methods fully spelled out and readable. Config keys descriptive and WordPress-centric. - **Filter names:** `"{$hook_prefix}/plugin_absorber/should_load"` and `"{$hook_prefix}/plugin_absorber/conflict_policy"`. -- **Storage keys:** option `"{$hook_prefix}_plugin_absorber_activations"`, option `"{$hook_prefix}_plugin_absorber_notices"`. **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. +- **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: @@ -42,21 +42,25 @@ Every task's requirements implicitly include this section. ``` plugin-absorber/ ├── src/ -│ ├── Config.php # static config facade: hook prefix, version, container +│ ├── Config.php # static config facade: hook prefix, container, name building │ ├── Loader.php # static facade: resolve/register/boot/load loop │ ├── Sub_Plugin.php # value object + every per-sub-plugin predicate │ ├── Conflict_Policy.php # three policy string constants +│ ├── Plugin_State.php # the only file touching WordPress plugin functions │ ├── Registrar.php # default slug => Sub_Plugin map │ ├── Activation.php # default run-once activation tracking │ ├── Conflict/ -│ │ ├── Resolver.php # default standalone detection/deactivation/redirect -│ │ └── Resolver_Interface.php -│ ├── Contracts/ -│ │ ├── Registrar_Interface.php -│ │ └── Activation_Interface.php +│ │ ├── Contracts/ +│ │ │ └── Resolver_Interface.php +│ │ └── Resolver.php # default standalone detection/deactivation/redirect +│ ├── Contracts/ # interfaces whose implementations sit at the src/ root +│ │ ├── Activation_Interface.php +│ │ ├── Plugin_State_Interface.php +│ │ └── Registrar_Interface.php │ ├── Notices/ +│ │ ├── Contracts/ +│ │ │ └── Queue_Interface.php │ │ ├── Queue.php # default queue: wording + capability gate -│ │ ├── Queue_Interface.php │ │ ├── Renderer.php # markup and severity │ │ └── Store.php # the option the queue lives in │ └── Exceptions/ @@ -629,7 +633,7 @@ class Sub_Plugin { } return (string) apply_filters( - Config::get_hook_prefix() . '/plugin_absorber/conflict_policy', + Config::get_hook_name( 'conflict_policy' ), $policy, $this ); @@ -1516,8 +1520,8 @@ Config::set_container( $container ); | Interface | Default | Responsibility | |---|---|---| | `Contracts\Registrar_Interface` | `Registrar` | Holds the registered sub-plugins. | -| `Notices\Queue_Interface` | `Notices\Queue` | Notice queue and the activation-error rewrite. | -| `Conflict\Resolver_Interface` | `Conflict\Resolver` | Standalone detection, deactivation, redirect. | +| `Notices\Contracts\Queue_Interface` | `Notices\Queue` | Notice queue and the activation-error rewrite. | +| `Conflict\Contracts\Resolver_Interface` | `Conflict\Resolver` | Standalone detection, deactivation, redirect. | | `Contracts\Activation_Interface` | `Activation` | Run-once activation tracking. | The container is **not** used to wire hooks — those stay plain static callbacks, so the container @@ -1565,19 +1569,19 @@ container.' Lands before the load path and the resolver because both call into it. **Files:** -- Create: `src/Notices/Queue_Interface.php`, `src/Notices/Queue.php`, `src/Notices/Store.php`, `src/Notices/Renderer.php`, `tests/unit/Notices/QueueTest.php`, `tests/unit/Notices/StoreTest.php`, `tests/unit/Notices/RendererTest.php` +- Create: `src/Notices/Contracts/Queue_Interface.php`, `src/Notices/Queue.php`, `src/Notices/Store.php`, `src/Notices/Renderer.php`, `tests/unit/Notices/QueueTest.php`, `tests/unit/Notices/StoreTest.php`, `tests/unit/Notices/RendererTest.php` - Modify: `src/Loader.php` (add the `notices()` accessor), `README.md` **Interfaces:** -- Consumes: `Config::get_hook_prefix()` (Task 4), `Sub_Plugin` message getters (Task 7), the `WithSubPlugins` trait (Task 7) for its fixtures, `Loader::resolve()` (Task 9). +- Consumes: `Config::get_option_name()` (Task 4), `Sub_Plugin` message getters (Task 7), the `WithSubPlugins` trait (Task 7) for its fixtures, `Loader::resolve()` (Task 9). - Produces: - - `Notices\Queue_Interface` with `queue_merge_notice( Sub_Plugin ): void`, `queue_conflict_notice( Sub_Plugin ): void`, `queue_dependency_notice( Sub_Plugin ): void`, `render(): void` - - `Loader::notices(): Notices\Queue_Interface` + - `Notices\Contracts\Queue_Interface` with `queue_merge_notice( Sub_Plugin ): void`, `queue_conflict_notice( Sub_Plugin ): void`, `queue_dependency_notice( Sub_Plugin ): void`, `render(): void` + - `Loader::notices(): Notices\Contracts\Queue_Interface` Task 11 calls `queue_dependency_notice()`; Task 12 calls `queue_merge_notice()` and `queue_conflict_notice()`; Task 14 extends this interface. **Design notes:** -- Option `"{$hook_prefix}_plugin_absorber_notices"`, so the queue survives the resolver's `wp_safe_redirect()` and renders on the next admin load. +- Option `"{$option_prefix}_plugin_absorber_notices"`, built by `Config::get_option_name( 'notices' )`, so the queue survives the resolver's `wp_safe_redirect()` and renders on the next admin load. > **Deviations, deliberate (added 2026-08-03, from the PR 10 review):** > @@ -1614,9 +1618,13 @@ Lands before the load path and the resolver because both call into it. > > `Notices` was a plural bag noun naming the subject rather than the job, which read worst of > the three once the split landed. The folder carries the subject and the class carries the job, -> following `Conflict\Resolver` — which is also why `Queue_Interface` sits beside its -> implementation rather than in `Contracts\`. The rename is free now and a breaking change after -> 1.0. +> following `Conflict\Resolver`. The rename is free now and a breaking change after 1.0. +> +> The interface follows the folder: `Notices\Contracts\Queue_Interface` in +> `src/Notices/Contracts/Queue_Interface.php`. A folder-scoped concern owns its own contract, so +> the top-level `src/Contracts/` is left holding only the interfaces whose implementations sit at +> the `src/` root — `Registrar_Interface`, `Plugin_State_Interface`, `Activation_Interface`. +> Task 12 does the same for `Conflict\Contracts\Resolver_Interface`. > > Both collaborators are constructor arguments defaulting to the standard implementations, so > `new Queue()` — what `Loader::resolve()` builds when the container holds no binding — behaves @@ -1633,9 +1641,23 @@ Lands before the load path and the resolver because both call into it. > would put this PR over the four-source-file cap. The same cap is why `Notices\Store` and > `Notices\Renderer` are concrete rather than interface-backed. > +> 8. **`Notices\Store` builds its key with `Config::get_option_name( 'notices' )`** (added +> 2026-08-11), not by concatenating `Config::get_hook_prefix()`. The prefix validator admits +> `A-Z` and `-`, so `Give-Core` would otherwise put a capitalised, hyphenated segment straight +> into a storage key. `get_option_name()` lowercases and folds hyphens to underscores; +> `get_hook_name()` deliberately does not, because normalising there would silently rename the +> host's own filters. Nothing outside `Config` assembles either kind of name. +> 9. **`Notices\Renderer` prints through `wp_kses_post()`, not `esc_html()`** (added 2026-08-11). +> A message reaches the renderer only from the host's own `conflict_notice_message` / +> `dependency_notice_message` config or from its filter — never from user input — and the +> commonest thing a host wants in a merge notice is a link to its own knowledge-base article. +> `wp_kses_post()` still strips scripts and event handlers, so the XSS surface is unchanged. +> Task 14's activation-error rewrite takes the same message from the same source onto the same +> screen, so the two must not diverge. +> > The Step 4/5 code listings below still show the pre-split, pre-rename shape, exactly as they -> still show the transient that deviation 1 replaced. The deviations are the record of what -> shipped. +> still show the transient that deviation 1 replaced and the hand-built option key that +> deviation 8 replaced. The deviations are the record of what shipped. - Queue entries are keyed `"{$slug}:{$type}"`, not by slug alone. A sub-plugin can legitimately earn a merge notice at `plugins_loaded` @1 and a dependency notice at @2 in the same request; keying by slug alone would silently drop one. - Default messages live **here**, not in `Sub_Plugin`. `get_conflict_notice_message()` returns `''` when unconfigured (Task 7 asserts this), and each notice type supplies its own fallback sentence — so auto-deactivating a plugin can never leave the user with no explanation. @@ -1761,14 +1783,21 @@ class NoticesTest extends WPTestCase { $this->assertStringContainsString( 'Bundled now.', $output ); } - public function test_render_escapes_the_message(): void { + public function test_render_strips_unsafe_markup_but_keeps_a_link(): void { $notices = new Notices(); - $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => '' ] ) ); + $notices->queue_merge_notice( + $this->make_sub_plugin( + [ 'conflict_notice_message' => 'Read more' ] + ) + ); $output = $this->render_to_string( $notices ); $this->assertStringNotContainsString( '' ] ); + public function test_it_strips_unsafe_markup_from_the_replacement_but_keeps_a_link(): void { + $this->register( + [ 'conflict_notice_message' => 'Read more' ] + ); $result = ( new Notices() )->filter_activation_error_markup( $this->wordpress_markup ); $this->assertStringNotContainsString( '' )` is the empty + // string. Either that or a whitespace-only message would print an empty notice box, + // which reads as a bug. + $message = trim( wp_kses_post( $message ) ); - // A whitespace-only message would print an empty notice box, which reads as a bug. if ( $message === '' ) { continue; } @@ -59,7 +63,9 @@ public function render( array $queue ): void { printf( '

%s

', esc_attr( $this->notice_class( (string) $key ) ), - esc_html( $message ) + // Already filtered: escaping it again here would undo the whole point and print a + // link as literal angle brackets. + $message ); } } diff --git a/tests/unit/Notices/QueueTest.php b/tests/unit/Notices/QueueTest.php index 0c46d1c..1ba8b71 100644 --- a/tests/unit/Notices/QueueTest.php +++ b/tests/unit/Notices/QueueTest.php @@ -9,8 +9,8 @@ use Generator; use Nexcess\PluginAbsorber\Config; use Nexcess\PluginAbsorber\Exceptions\Config_Exception; +use Nexcess\PluginAbsorber\Notices\Contracts\Queue_Interface; use Nexcess\PluginAbsorber\Notices\Queue; -use Nexcess\PluginAbsorber\Notices\Queue_Interface; use Nexcess\PluginAbsorber\Notices\Renderer; use Nexcess\PluginAbsorber\Notices\Store; use Nexcess\PluginAbsorber\Tests\Support\Config_State; @@ -27,6 +27,10 @@ class QueueTest extends WPTestCase { private const OPTION = 'give_plugin_absorber_notices'; + // The option the queue moves to once the hook prefix changes, which is what proves the name is + // derived from the prefix rather than fixed. + private const OPTION_FOR_OTHER_PREFIX = 'woo_plugin_absorber_notices'; + public function setUp(): void { parent::setUp(); @@ -49,7 +53,7 @@ public function setUp(): void { public function tearDown(): void { $this->clear_queue(); - delete_site_option( 'woo_plugin_absorber_notices' ); + delete_site_option( self::OPTION_FOR_OTHER_PREFIX ); Config_State::reset(); parent::tearDown(); } @@ -238,31 +242,35 @@ public static function notice_severities(): Generator { yield 'dependency' => [ 'queue_dependency_notice', 'notice-error' ]; } - public function test_render_escapes_the_message(): void { + public function test_render_strips_a_script_from_the_message(): void { $notices = new Queue(); - $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => '' ] ) ); + $notices->queue_merge_notice( $this->make_sub_plugin( [ 'conflict_notice_message' => 'Careful.' ] ) ); $output = $this->render_to_string( $notices ); - $this->assertStringNotContainsString( '' ] ); + /** + * `wp_kses_post()` drops a disallowed tag but keeps the text it wrapped, so the payload lands + * on the page as inert text rather than as markup the browser would run. + */ + public function test_it_strips_a_script_from_a_message(): void { + $output = $this->render( [ 'a:merge' => 'Careful.' ] ); - $this->assertStringNotContainsString( '' ]; } public function test_an_empty_queue_prints_nothing(): void { From e5bbf7fe33085fcc8ae7de503091dc4c19911b13 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 16:52:17 +0200 Subject: [PATCH 09/10] Refactor unit test structure and documentation updates - Renamed the newly created unit test file from `NoticesActivationErrorTest.php` to `QueueActivationErrorTest.php` for better clarity and organization. - Made minor adjustments to the documentation to improve readability and consistency, including formatting changes in the `Notices\Contracts\Queue_Interface` section. - Ensured that the new test file aligns with the updated structure of the Notices classes. --- docs/superpowers/plans/2026-07-31-plugin-absorber.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/plans/2026-07-31-plugin-absorber.md b/docs/superpowers/plans/2026-07-31-plugin-absorber.md index e37a555..88feafc 100644 --- a/docs/superpowers/plans/2026-07-31-plugin-absorber.md +++ b/docs/superpowers/plans/2026-07-31-plugin-absorber.md @@ -3623,7 +3623,7 @@ completely unhelpful. Swap in the sub-plugin's own explanation. **Files:** - Modify: `src/Notices/Contracts/Queue_Interface.php`, `src/Notices/Queue.php`, `src/Loader.php`, `README.md` -- Create: `tests/unit/NoticesActivationErrorTest.php` +- Create: `tests/unit/Notices/QueueActivationErrorTest.php` **Interfaces:** - Consumes: `Loader::all()` (Task 9), `Sub_Plugin::get_standalone_plugin_basename()` / `get_conflict_notice_message()` (Task 7). @@ -3981,7 +3981,8 @@ registered standalone basename, and a valid `plugin-activation-error_{basename}` 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. +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.' @@ -4607,8 +4608,8 @@ Checked against `docs/superpowers/specs/2026-07-31-plugin-absorber-design.md`: (`is_standalone_plugin_network_active()`) + Task 12 (the `$network_wide` argument); D → Task 7 (`get_dependency_notice_message()`) + Task 10 (rendering). Deferred B/E/F are recorded above and in the PR bodies that touch them. Every spec test bullet has a corresponding test method. -- **Interface consistency.** `Notices\Contracts\Queue_Interface` grows one method in Task 14 — flagged in both the - task and the PR body rather than left implicit. `Loader_State::reset()` is written once in Task 9 and +- **Interface consistency.** `Notices\Contracts\Queue_Interface` grows one method in Task 14 — + flagged in both the task and the PR body rather than left implicit. `Loader_State::reset()` is written once in Task 9 and extended once in Task 11 (`$booted`), shown in full both times. `redirect_destination()` is `protected`, matching how the Task 12 tests subclass it. - **Placeholder scan.** No TBD, no "add error handling", no "similar to Task N". Every code step From 0cd7e3a0484a4e8538c988e74d3e36729289e5c2 Mon Sep 17 00:00:00 2001 From: Nikolay Strikhar Date: Tue, 11 Aug 2026 17:05:37 +0200 Subject: [PATCH 10/10] Wrap notice paragraphs with wpautop() instead of a literal

wp_kses_post() allows

, so a host message that already carries one was being nested inside another -- markup the parser resolves by closing the outer tag early and stranding a

, drawing an empty line above the notice. A
    , which the same allowlist preserves, cannot sit in a

    at all. wpautop() wraps only what needs wrapping and turns a blank line into a real paragraph break. --- docs/notices.md | 3 ++- src/Notices/Renderer.php | 15 ++++++++--- tests/unit/Notices/RendererTest.php | 39 +++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/docs/notices.md b/docs/notices.md index 6ef285b..b91d438 100644 --- a/docs/notices.md +++ b/docs/notices.md @@ -27,7 +27,8 @@ administrator, not the site administrator who installed the plugin, who sees the anything. The value is an `array` keyed `slug:type` — `give-recurring:merge`, for example — and the messages may contain markup; the default rendering passes them through `wp_kses_post()`, so a link, emphasis or a list survives while scripts and event handlers are -stripped. +stripped. Paragraphs come from `wpautop()`, so send the message unwrapped and let a blank line +break it — a `

    ` of your own is left as it is rather than nested inside another. The 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, diff --git a/src/Notices/Renderer.php b/src/Notices/Renderer.php index e2f7b35..bc4bb07 100644 --- a/src/Notices/Renderer.php +++ b/src/Notices/Renderer.php @@ -42,6 +42,14 @@ class Renderer { * link to a knowledge-base article, emphasis or a list survives, while a script or an event * handler attribute does not. * + * The paragraph comes from `wpautop()` rather than a literal `

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

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

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

      ` at all, so the list the allowlist just + * preserved would break straight back out of it. `wpautop()` wraps only what needs wrapping, + * and turns the blank line a plain translated string uses as a break into a real one. + * * @since 1.0.0 * * @param array $queue Queue to draw, keyed `slug:type`. @@ -61,11 +69,12 @@ public function render( array $queue ): void { } printf( - '

      %s

      ', + '
      %s
      ', esc_attr( $this->notice_class( (string) $key ) ), // Already filtered: escaping it again here would undo the whole point and print a - // link as literal angle brackets. - $message + // link as literal angle brackets. Trimmed because `wpautop()` leaves a trailing + // newline, which would otherwise sit inside the div on every notice. + trim( wpautop( $message ) ) ); } } diff --git a/tests/unit/Notices/RendererTest.php b/tests/unit/Notices/RendererTest.php index 0927656..47642b9 100644 --- a/tests/unit/Notices/RendererTest.php +++ b/tests/unit/Notices/RendererTest.php @@ -101,6 +101,45 @@ public function test_it_strips_an_event_handler_from_a_link(): void { $this->assertStringContainsString( 'the docs', $output ); } + /** + * A bare message still has to reach the screen as a paragraph: `.notice` styles the `

      ` inside + * it, and text sitting directly in the div loses that spacing. + */ + public function test_it_wraps_a_bare_message_in_a_paragraph(): void { + $this->assertStringContainsString( '

      Bundled now.

      ', $this->render( [ 'a:merge' => 'Bundled now.' ] ) ); + } + + /** + * The same allowlist that keeps a link keeps a `

      `, so a host may well send one. Wrapping that + * in another paragraph is markup no browser can honour: the parser closes the outer `

      ` at the + * inner one and leaves a stray `

      ` behind, so the notice renders with an empty leading line. + */ + public function test_it_does_not_wrap_a_paragraph_the_host_already_wrote(): void { + $output = $this->render( [ 'a:merge' => '

      Bundled now.

      ' ] ); + + $this->assertStringNotContainsString( '

      ', $output ); + $this->assertSame( 1, substr_count( $output, '

      ' ), 'One paragraph in, one paragraph out.' ); + } + + /** + * The class docblock promises a list survives, and a `

        ` inside a `

        ` is not a list that + * survived: the browser closes the paragraph before it, orphaning the closing tag. + */ + public function test_it_keeps_a_list_out_of_the_paragraph(): void { + $output = $this->render( [ 'a:merge' => 'Requires:

        • Give
        ' ] ); + + $this->assertMatchesRegularExpression( '#

        \s*
          #', $output, 'The paragraph must close first.' ); + $this->assertStringContainsString( '
        • Give
          • ', $output ); + } + + /** + * A blank line is the only paragraph break available to a host whose message is a plain + * translated string, so it has to survive as one rather than collapse into running text. + */ + public function test_a_blank_line_starts_a_second_paragraph(): void { + $this->assertSame( 2, substr_count( $this->render( [ 'a:merge' => "One.\n\nTwo." ] ), '

          ' ) ); + } + /** * @dataProvider empty_messages *