fix(newspack-popups): cache empty popups_for_post() results

plugins/newspack-popups/includes/class-newspack-popups-inserter.php

@@ -17,9 +17,28 @@ final class Newspack_Popups_Inserter {
 	const ADMIN_SCRIPT_HANDLE = 'newspack-popups-admin-bar';
 
 	/**
-	 * The popup objects to display.
+	 * 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
+	 * @var array|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<int, array>
 	 */
 	protected static $popups = [];
 
@@ -154,10 +173,6 @@ public static function disable_prompts( $disabled ) {
 	 * @return array Popup objects.
 	 */
 	public static function popups_for_post() {
-		if ( ! empty( 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() );
@@ -168,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;

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 );
 	}
 

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();