Feature Flags: key the wpcom override cache by blog

projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-reset-overrides-on-switch-blog

@@ -0,0 +1,4 @@
+Significance: patch
+Type: fixed
+
+Feature Flags: read a site's flag overrides per blog, so a flag resolved on WordPress.com's public API answers for the requested site rather than the blog the process started on.

projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php

@@ -81,15 +81,23 @@ class Wpcom_Feature_Flags {
 	const CAPABILITY = 'manage_options';
 
 	/**
-	 * Request-scoped cache of the sanitized override map.
+	 * Cache of one blog's sanitized override map; valid only for $overrides_blog_id.
 	 *
-	 * Null means "not read from the option yet", which is distinct from the empty
-	 * array a site with no overrides legitimately has.
+	 * Keyed by blog rather than dropped on `switch_blog`: WordPress.com's public API
+	 * resolves flags before switching to the requested site, and a hook only helps
+	 * if it fires, and fires before any other callback that resolves a flag.
 	 *
 	 * @var array<array-key, bool>|null
 	 */
 	private static $overrides = null;
 
+	/**
+	 * The blog $overrides was read for. Null means nothing is cached.
+	 *
+	 * @var int|null
+	 */
+	private static $overrides_blog_id = null;
+
 	/**
 	 * Register the hooks this feature needs.
 	 *
@@ -147,7 +155,7 @@ public static function current_user_can_manage() {
 	/**
 	 * Return this site's flag overrides.
 	 *
-	 * Memoized for the request. filter_enabled() runs once per flag resolution,
+	 * Memoized per blog. filter_enabled() runs once per flag resolution,
 	 * and the screen resolves every listed flag to fill its Effective column, so
 	 * without this the option read, the per-entry preg_match(), and the ksort()
 	 * in sanitize_overrides() all repeat for every flag checked. The option
@@ -158,13 +166,16 @@ public static function current_user_can_manage() {
 	 * @return array<array-key, bool> Map of flag name to forced value.
 	 */
 	public static function get_overrides() {
-		if ( null !== self::$overrides ) {
+		$blog_id = get_current_blog_id();
+
+		if ( null !== self::$overrides && self::$overrides_blog_id === $blog_id ) {
 			return self::$overrides;
 		}
 
 		$stored = get_option( self::OVERRIDES_OPTION );
 
-		self::$overrides = is_array( $stored ) ? self::sanitize_overrides( $stored ) : array();
+		self::$overrides         = is_array( $stored ) ? self::sanitize_overrides( $stored ) : array();
+		self::$overrides_blog_id = $blog_id;
 
 		return self::$overrides;
 	}
@@ -178,7 +189,8 @@ public static function get_overrides() {
 	public static function save_overrides( array $overrides ) {
 		$overrides = self::sanitize_overrides( $overrides );
 
-		self::$overrides = null;
+		self::$overrides         = null;
+		self::$overrides_blog_id = null;
 
 		if ( empty( $overrides ) ) {
 			delete_option( self::OVERRIDES_OPTION );
@@ -198,7 +210,8 @@ public static function save_overrides( array $overrides ) {
 	 * @return void
 	 */
 	public static function reset_overrides_cache() {
-		self::$overrides = null;
+		self::$overrides         = null;
+		self::$overrides_blog_id = null;
 	}
 
 	/**

projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php

@@ -38,6 +38,13 @@ class Wpcom_Feature_Flags_Test extends \WorDBless\BaseTestCase {
 	 */
 	private static $bootstrap_wiring = array();
 
+	/**
+	 * The blog id the test started on, restored by tear_down().
+	 *
+	 * @var mixed
+	 */
+	private $original_blog_id;
+
 	/**
 	 * Record the bootstrap's hook registrations before any test disturbs them.
 	 */
@@ -50,6 +57,15 @@ public static function set_up_before_class() {
 		);
 	}
 
+	/**
+	 * Remember the blog context, which the blog-switching test changes.
+	 */
+	public function set_up() {
+		parent::set_up();
+
+		$this->original_blog_id = $GLOBALS['blog_id'];
+	}
+
 	/**
 	 * Reset every piece of global state these tests touch.
 	 */
@@ -63,6 +79,8 @@ public function tear_down() {
 		// These tests write the option directly, which save_overrides() is not there to notice.
 		Wpcom_Feature_Flags::reset_overrides_cache();
 		remove_all_filters( 'jetpack_feature_flag_enabled' );
+		remove_all_filters( 'pre_option_' . Wpcom_Feature_Flags::OVERRIDES_OPTION );
+		$GLOBALS['blog_id'] = $this->original_blog_id;
 		remove_all_filters( 'jetpack_feature_flag_enabled_my-feature' );
 		remove_all_filters( 'wp_die_handler' );
 		Feature_Flags::reset();
@@ -391,6 +409,50 @@ public function test_override_applies_to_unregistered_flag() {
 		$this->assertTrue( Feature_Flags::is_enabled( 'not-registered-anywhere' ) );
 	}
 
+	/**
+	 * WordPress.com's public API resolves flags before it switches to the
+	 * requested site. The suite is single-site, so the blog changes through the
+	 * global get_current_blog_id() reads, without firing `switch_blog`.
+	 */
+	public function test_each_blog_resolves_its_own_overrides() {
+		add_filter(
+			'pre_option_' . Wpcom_Feature_Flags::OVERRIDES_OPTION,
+			function () {
+				return array( 'my-feature' => 1 === get_current_blog_id() );
+			}
+		);
+		Wpcom_Feature_Flags::init();
+
+		$GLOBALS['blog_id'] = 1;
+
+		$this->assertTrue( Feature_Flags::is_enabled( 'my-feature' ) );
+
+		$GLOBALS['blog_id'] = 2;
+		$this->assertSame( array( 'my-feature' => false ), Wpcom_Feature_Flags::get_overrides() );
+
+		$GLOBALS['blog_id'] = 1;
+		$this->assertSame( array( 'my-feature' => true ), Wpcom_Feature_Flags::get_overrides() );
+	}
+
+	/**
+	 * Repeat resolutions on the same blog answer from the cache.
+	 */
+	public function test_overrides_are_read_once_per_blog() {
+		$reads = 0;
+		add_filter(
+			'pre_option_' . Wpcom_Feature_Flags::OVERRIDES_OPTION,
+			function () use ( &$reads ) {
+				++$reads;
+				return array( 'my-feature' => true );
+			}
+		);
+		Wpcom_Feature_Flags::init();
+
+		$this->assertTrue( Feature_Flags::is_enabled( 'my-feature' ) );
+		$this->assertSame( array( 'my-feature' => true ), Wpcom_Feature_Flags::get_overrides() );
+		$this->assertSame( 1, $reads );
+	}
+
 	/**
 	 * Overrides are stored site-wide on purpose: an Automattician flips a flag
 	 * to see how the site behaves, including for logged-out visitors. So