perf: Memoize is_user_admin, is_preview_request and viewing_as_spec per request by jhalitschke · Pull Request #1553 · Automattic/newspack-popups

jhalitschke · 2026-04-28T12:07:51Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Summary

Memoize Newspack_Popups::is_user_admin(), Newspack_Popups::is_preview_request() and Newspack_Popups_View_As::viewing_as_spec() using static variables so each method computes its result only
once per request.

Problem

On archive pages with many posts, these methods are called repeatedly per popup x post combination:

Method	Calls per request	Cost
`is_user_admin()`	132	1.6ms
`is_preview_request()`	120	3.5ms
`viewing_as_spec()`	132	2.4ms

The call chain is:
assess_taxonomy_filter (96x) -> is_preview_request -> viewing_as_spec -> is_user_admin
show_admin_bar (22x) -> is_preview_request -> viewing_as_spec -> is_user_admin
parse_view_as (12x) -> viewing_as_spec -> is_user_admin

is_user_admin() calls apply_filters() + current_user_can() every time. is_preview_request() calls is_customize_preview(), previewed_popup_id(), preset_popup_id() and
viewing_as_spec() every time. None of these results can change within a single HTTP request.

On a test archive page with 48 posts and ~91 evaluated popups, the total overhead is ~5ms. The cost scales linearly with post count x popup count.

Fix

Add static $result memoization to all three methods. The first call computes the result; subsequent calls return the cached value immediately.

This pattern is safe because:

User capabilities don't change within a request
$_GET['view_as'] is constant within a request
previewed_popup_id() / preset_popup_id() read from $_GET which is constant
is_customize_preview() is constant within a request

Test plan

Verify prompts display correctly on archive pages (logged out)
Verify prompts display correctly for admin users (logged in)
Verify "View As" feature still works (?view_as=... parameter)
Verify single popup preview still works
Verify Customizer preview still works

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

Copilot

Pull request overview

This PR aims to improve runtime performance on high-post-count archive pages by memoizing a few frequently-invoked “request-constant” methods in the Newspack Popups plugin.

Changes:

Added local-static memoization to Newspack_Popups::is_preview_request() and Newspack_Popups::is_user_admin().
Added local-static memoization to Newspack_Popups_View_As::viewing_as_spec().
Updated (but introduced issues in) the is_preview_request() docblock sentence.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
includes/class-newspack-popups.php	Memoizes preview/admin checks to reduce repeated capability/query evaluation work.
includes/class-newspack-popups-view-as.php	Memoizes “view as” spec resolution to avoid repeated admin + query param parsing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-28T14:07:53Z


 	/**
-	 * Is it a preview request – a single popup preview or using "view as" feature.
+	 * Is it a preview request ? a single popup preview or using "view as" feature.


The docblock punctuation/grammar was changed to "Is it a preview request ? ..." which reads incorrectly (space before "?" and uses "?" where the sentence continues). Please update it to a grammatically correct form (e.g., "Is it a preview request? ..." or "Is it a preview request — ...").

Suggested change

* Is it a preview request ? a single popup preview or using "view as" feature.

* Is it a preview request? A single popup preview or using the "view as" feature.

Copilot · 2026-04-28T14:07:54Z

 	public static function is_preview_request() {
+		static $result = null;
+		if ( null !== $result ) {
+			return $result;
+		}
 		$is_customizer_preview = is_customize_preview();
 		// Used by the Newspack Plugin's Campaigns Wizard.
 		$is_view_as_preview = false != Newspack_Popups_View_As::viewing_as_spec();
-		return ! empty( self::previewed_popup_id() ) || ! empty( self::preset_popup_id() ) || $is_view_as_preview || $is_customizer_preview;
+		$result = ! empty( self::previewed_popup_id() ) || ! empty( self::preset_popup_id() ) || $is_view_as_preview || $is_customizer_preview;
+		return $result;


Memoizing the preview-request result in a local static can leak across multiple simulated requests in the PHPUnit suite (tests call go_to() repeatedly with different query strings). Once cached, later renders in the same PHP process may get a stale preview/non-preview value. Consider caching per request context (e.g., keyed by relevant $_GET values / request URI) or adding an explicit reset hook used by tests before each go_to()/render.

Copilot · 2026-04-28T14:07:54Z

@@ -971,7 +980,8 @@ public static function is_user_admin() {
 		 * @return string Filtered capability string.
 		 */
 		$capability = apply_filters( 'newspack_popups_admin_user_capability', 'edit_others_pages' );
-		return is_user_logged_in() && current_user_can( $capability );
+		$result     = is_user_logged_in() && current_user_can( $capability );
+		return $result;


is_user_admin() is now memoized with a single static $result, which becomes stale if the current user changes within the same PHP process (the test suite calls wp_set_current_user() mid-test). This will cause admin checks to keep returning the first computed value. Cache should be keyed by get_current_user_id() (or invalidated on the set_current_user action) so it remains correct when user context changes.

Copilot · 2026-04-28T14:07:54Z

 	 * Get the "view as" feature specification.
 	 *
 	 * @return string "View as" specification.
 	 */
 	public static function viewing_as_spec() {
+		static $result = null;
+		if ( null !== $result ) {


The @return docblock for viewing_as_spec() says it returns a string, but the function can return false. Please update the docblock to reflect the actual return type (e.g., string|false) so callers and static analysis don’t get incorrect type information.

Copilot · 2026-04-28T14:07:54Z

 	public static function viewing_as_spec() {
+		static $result = null;
+		if ( null !== $result ) {
+			return $result;
+		}
 		if ( ! Newspack_Popups::is_user_admin() ) {
-			return false;
+			$result = false;
+			return $result;
 		}
 		if ( isset( $_GET['view_as'] ) && $_GET['view_as'] ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized
-			return sanitize_text_field( $_GET['view_as'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+			$result = sanitize_text_field( $_GET['view_as'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
+			return $result;
 		}
+		$result = false;
+		return $result;


viewing_as_spec() now memoizes into a single static $result. In PHPUnit (and in any long-running PHP process), user context and $_GET['view_as'] can change between calls; once cached, this method can keep returning a stale spec/false even after wp_set_current_user() or go_to() updates the request. Prefer caching keyed by current user ID and the raw view_as value, or provide a reset mechanism for tests.

dkoo · 2026-05-29T18:05:51Z

@jhalitschke can you please address the feedback from the Copilot code review? If you disagree with any feedback that's no problem, just respond to the thread with an explanation before you resolve it. We'll do a full code review after all Copilot feedback is resolved.

jhalitschke added 2 commits April 28, 2026 13:58

perf: Optimize preview request and admin user checks

51a3821

perf: Optimize viewing_as_spec method for caching

be8097b

jhalitschke requested a review from a team as a code owner April 28, 2026 12:07

leogermani requested a review from Copilot April 28, 2026 14:02

Copilot started reviewing on behalf of leogermani April 28, 2026 14:03 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Memoize is_user_admin, is_preview_request and viewing_as_spec per request#1553

perf: Memoize is_user_admin, is_preview_request and viewing_as_spec per request#1553
jhalitschke wants to merge 2 commits into
Automattic:trunkfrom
jhalitschke:perf/memoize-checks-per-request

jhalitschke commented Apr 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

dkoo commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	* Is it a preview request ? a single popup preview or using "view as" feature.
	* Is it a preview request? A single popup preview or using the "view as" feature.

Conversation

jhalitschke commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

Summary

Problem

Fix

Test plan

Other information:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

dkoo commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhalitschke commented Apr 28, 2026 •

edited

Loading