diff --git a/projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-reset-overrides-on-switch-blog b/projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-reset-overrides-on-switch-blog new file mode 100644 index 000000000000..cb4829dc434c --- /dev/null +++ b/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. diff --git a/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php b/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php index beb0f90db21f..7bad15b5773e 100644 --- a/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php +++ b/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|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 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; } /** diff --git a/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php b/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php index b3b5ad69fe36..b8ef43ca90da 100644 --- a/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php +++ b/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