From 7ba0ef42ef51f5fd46652dd773b59e95859c019c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Halitschke?= Date: Wed, 1 Jul 2026 17:51:53 +0200 Subject: [PATCH 1/2] fix(newspack-popups): cache empty popups_for_post() results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit self::$popups defaulted to [] and the cache guard was `! empty( self::$popups )`, so a legitimate empty result (no popups eligible for the current request) was indistinguishable from "not yet computed". Every call re-ran assess_has_disabled_popups(), parse_view_as(), retrieve_eligible_popups() and a should_display() pass over every campaign — on archive pages this repeats once per post instead of once per request. Switch the memoization guard to null, since an empty array is a valid, cacheable result. Co-Authored-By: Claude Sonnet 5 --- .../includes/class-newspack-popups-inserter.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php b/plugins/newspack-popups/includes/class-newspack-popups-inserter.php index 9b63bed643..cad2703d28 100755 --- a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php +++ b/plugins/newspack-popups/includes/class-newspack-popups-inserter.php @@ -17,11 +17,13 @@ final class Newspack_Popups_Inserter { const ADMIN_SCRIPT_HANDLE = 'newspack-popups-admin-bar'; /** - * The popup objects to display. + * The popup objects to display, memoized per request. + * Null means "not yet computed" for this request; an empty array is a + * valid, cacheable result (no popups eligible for the current post). * - * @var array + * @var array|null */ - protected static $popups = []; + protected static $popups = null; /** * Segments for displayed popups. @@ -154,7 +156,7 @@ public static function disable_prompts( $disabled ) { * @return array Popup objects. */ public static function popups_for_post() { - if ( ! empty( self::$popups ) ) { + if ( null !== self::$popups ) { return self::$popups; } From 40019f37005576ceeb7eccbbe23cefa05a0f95ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Halitschke?= Date: Wed, 1 Jul 2026 18:07:00 +0200 Subject: [PATCH 2/2] fix: key popups_for_post() cache by post ID, not request-wide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot flagged that assess_has_disabled_popups() and should_display() both depend on the current post (post meta, post type, taxonomy terms), so a single request-wide cache — which is what the previous fix in this PR produced — can return a stale result for a post other than the one it was computed for. On an archive page looping over posts with different categories, this could suppress prompts that should display, or show prompts that shouldn't, for every post after the first. Split the cache into two tiers: - self::$eligible_popups: the raw retrieve_eligible_popups() query result, which doesn't depend on the current post, memoized once per request as before (this is the expensive part — a WP_Query). - self::$popups: the post-dependent should_display() filter result, now keyed by post ID instead of a single value, so different posts in the same request get their own cache entry. array_key_exists() (rather than an empty()/null check) distinguishes "not yet computed for this post ID" from "computed and empty", so the original empty-result bug this PR set out to fix stays fixed on a per-post basis. Update the two tests that seed Newspack_Popups_Inserter::$popups directly via reflection (test-block-theme-header-insertion.php, test-overlay-queueing.php) to seed at the current post ID instead of as a flat list, matching the new keyed shape. The existing setValue(null, []) resets in those tests' set_up()/tear_down() are unaffected, since an empty array still correctly means "nothing cached for any post". Co-Authored-By: Claude Sonnet 5 --- .../class-newspack-popups-inserter.php | 73 ++++++++++++++----- .../test-block-theme-header-insertion.php | 6 +- .../tests/test-overlay-queueing.php | 4 +- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php b/plugins/newspack-popups/includes/class-newspack-popups-inserter.php index cad2703d28..1d43a45565 100755 --- a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php +++ b/plugins/newspack-popups/includes/class-newspack-popups-inserter.php @@ -17,13 +17,30 @@ final class Newspack_Popups_Inserter { const ADMIN_SCRIPT_HANDLE = 'newspack-popups-admin-bar'; /** - * The popup objects to display, memoized per request. - * Null means "not yet computed" for this request; an empty array is a - * valid, cacheable result (no popups eligible for the current post). + * Prompts eligible for display in the current request, before the + * per-post should_display() filter is applied. This doesn't depend on + * the current post, so — unlike self::$popups below — it's safe to + * memoize once for the whole request. Null means "not yet computed"; + * an empty array is a valid, cacheable result. * * @var array|null */ - protected static $popups = null; + protected static $eligible_popups = null; + + /** + * The popup objects to display, keyed by post ID and memoized per post + * within the current request. should_display() and + * assess_has_disabled_popups() both depend on the current post (post + * meta, post type, taxonomy terms), so a single request-wide cache can + * return the wrong result for a post other than the one it was computed + * for — e.g. on an archive page looping over posts with different + * categories. An empty array value for a given post ID is a valid, + * cacheable "no popups eligible for this post" result, distinct from + * the key being absent, which means "not yet computed for this post". + * + * @var array + */ + protected static $popups = []; /** * Segments for displayed popups. @@ -156,10 +173,6 @@ public static function disable_prompts( $disabled ) { * @return array Popup objects. */ public static function popups_for_post() { - if ( null !== self::$popups ) { - return self::$popups; - } - // Get the previewed popup and return early. if ( Newspack_Popups::previewed_popup_id() ) { $preview_popup = Newspack_Popups_Model::retrieve_preview_popup( Newspack_Popups::previewed_popup_id() ); @@ -170,27 +183,51 @@ public static function popups_for_post() { return [ $preset_popup ]; } - // Popups disabled for this page. + // Popups disabled for this page. The default filter checks post meta + // via get_the_ID(), so this must be evaluated on every call, not just + // the first one for the request. if ( self::assess_has_disabled_popups() ) { return []; } - $view_as_spec = Newspack_Popups_View_As::parse_view_as(); - $campaign_id = isset( $view_as_spec['campaign'] ) ? $view_as_spec['campaign'] : false; - $include_unpublished = isset( $view_as_spec['show_unpublished'] ) && 'true' === $view_as_spec['show_unpublished'] ? true : false; + // should_display() checks the current post's type and taxonomy terms, + // so the final eligible-popups list can legitimately differ per post + // within the same request (e.g. an archive page looping over posts + // with different categories). Memoize per post ID rather than once + // for the whole request. + $post_id = get_the_ID() ?: 0; + if ( array_key_exists( $post_id, self::$popups ) ) { + return self::$popups[ $post_id ]; + } + + // The eligibility query itself doesn't depend on the current post, so + // — unlike should_display() above — it's safe, and worth it since + // it's the expensive part, to memoize once for the whole request. + if ( null !== self::$eligible_popups ) { + $eligible_popups = self::$eligible_popups; + } else { + $view_as_spec = Newspack_Popups_View_As::parse_view_as(); + $campaign_id = isset( $view_as_spec['campaign'] ) ? $view_as_spec['campaign'] : false; + $include_unpublished = isset( $view_as_spec['show_unpublished'] ) && 'true' === $view_as_spec['show_unpublished'] ? true : false; + + $eligible_popups = Newspack_Popups_Model::retrieve_eligible_popups( $include_unpublished, $campaign_id ); + + if ( ! defined( 'IS_TEST_ENV' ) || ! IS_TEST_ENV ) { + self::$eligible_popups = $eligible_popups; + } + } - // Retrieve all prompts eligible for display. - $popups_to_maybe_display = Newspack_Popups_Model::retrieve_eligible_popups( $include_unpublished, $campaign_id ); - $popups_to_display = array_filter( - $popups_to_maybe_display, + $popups_to_display = array_filter( + $eligible_popups, function( $popup ) { return self::should_display( $popup, true ); } ); - // Cache results so we don't have to query again. + // Cache results so we don't have to re-run should_display() for this + // post again. if ( ! defined( 'IS_TEST_ENV' ) || ! IS_TEST_ENV ) { - self::$popups = $popups_to_display; + self::$popups[ $post_id ] = $popups_to_display; } return $popups_to_display; diff --git a/plugins/newspack-popups/tests/test-block-theme-header-insertion.php b/plugins/newspack-popups/tests/test-block-theme-header-insertion.php index bf1bef2403..267e6c2110 100644 --- a/plugins/newspack-popups/tests/test-block-theme-header-insertion.php +++ b/plugins/newspack-popups/tests/test-block-theme-header-insertion.php @@ -104,10 +104,14 @@ private function get_header_template_part_block() { /** * Prepare inserter cache with popup objects for deterministic tests. * + * self::$popups is now keyed by post ID (see popups_for_post()), so seed + * it at whatever post ID is current when popups_for_post() will actually + * be called, rather than as a flat list. + * * @param array $popups Popup objects. */ private function seed_inserter_popups( $popups ) { - self::$inserter_popups_property->setValue( null, $popups ); + self::$inserter_popups_property->setValue( null, [ get_the_ID() ?: 0 => $popups ] ); self::$header_template_part_has_rendered_property->setValue( null, false ); } diff --git a/plugins/newspack-popups/tests/test-overlay-queueing.php b/plugins/newspack-popups/tests/test-overlay-queueing.php index ac4ade0ca4..0c945b8753 100644 --- a/plugins/newspack-popups/tests/test-overlay-queueing.php +++ b/plugins/newspack-popups/tests/test-overlay-queueing.php @@ -177,11 +177,13 @@ public function test_classic_archive_path_emits_marker_inline_for_scroll_trigger // Force `popups_for_post()` to return our overlay rather than running // the eligibility query, which depends on a fully bootstrapped request. + // self::$popups is keyed by post ID (see popups_for_post()), so seed + // it at the post ID that will be current when it's called below. $reflection_class = new ReflectionClass( 'Newspack_Popups_Inserter' ); $popups_property = $reflection_class->getProperty( 'popups' ); $popups_property->setAccessible( true ); $prior_popups = $popups_property->getValue(); - $popups_property->setValue( null, [ $overlay_popup ] ); + $popups_property->setValue( null, [ get_the_ID() ?: 0 => [ $overlay_popup ] ] ); ob_start(); Newspack_Popups_Inserter::insert_popups_after_header();