diff --git a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php b/plugins/newspack-popups/includes/class-newspack-popups-inserter.php index 9b63bed643..1d43a45565 100755 --- a/plugins/newspack-popups/includes/class-newspack-popups-inserter.php +++ b/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 */ 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; 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();