Experiments: Update the gutenberg-experiments setting to allow additional bool properties by sdnunca · Pull Request #83504 · WordPress/gutenberg

sdnunca · 2026-09-24T17:04:46Z

What?

Allow additional boolean properties in gutenberg-experiments.

Why?

Once a previously activated experiment is removed from the properties list, previously stored values start failing the schema validation, resulting in gutenberg-experiments being returned as null.

This causes the Experiments page to display all toggles as OFF and clicking any of the toggles overwrites the setting value.

As all properties in gutenberg-experiments are always boolean, the schema validation should allow additional (unregistered) boolean properties.

How?

Prior art in #82072 tackles recovering from a completely invalid gutenberg-experiments value, but it ends up coercing all input to boolean.

As the experiment properties should always be boolean, this PR extends the validation to allow the current registered properties + any other boolean properties.

Testing Instructions

Set gutenberg-experiments to contain a removed property:

wp option update gutenberg-experiments '{"gutenberg-block-experiments":true,"removed-experiment":true}' --format=json

Open Settings -> Experiments.
Confirm all toggles are OFF.
Apply this PR and confirm the Experimental blocks toggle is now ON.

Testing Instructions for Keyboard

Screenshots or screencast

N/A.

Before	After

Use of AI Tools

No.

…onal bool properties

github-actions · 2026-09-24T17:04:56Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sdnunca <sdnunca@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mcsf <mcsf@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

tyxla

Thanks for working on this @sdnunca!

I think the real problem is in routes/experiments-home/stage.tsx - when the setting comes back as null, the page uses {} and then saves over the whole option. This PR fixes one way to get there, but not all of them.

What do you think about not saving when the value is null? Or we could remove only the bad entries on the server, instead of making the whole setting null perhaps?

tyxla · 2026-09-24T20:09:26Z

+					'properties'           => $properties,
+					// Allow stale keys left behind by removed experiments without invalidating the entire option.
+					'additionalProperties' => array(
+						'type' => 'boolean',


This only helps when the old value can be converted to a boolean. If an old key has null or 'on', the whole setting is still null, and we still get the same bug.

tyxla · 2026-09-24T20:09:59Z

+					// Experiment properties must always be boolean.
+					'properties'           => $properties,
+					// Allow stale keys left behind by removed experiments without invalidating the entire option.
+					'additionalProperties' => array(


With this change, old keys stay in the option forever, and there is no way to remove them from the UI. We have reused experiment IDs before (gutenberg-full-page-client-side-navigation), so an old true could turn on a new experiment without the admin knowing. Should we clean them up somewhere?

tyxla · 2026-09-24T20:10:17Z

+					// Experiment properties must always be boolean.
+					'properties'           => $properties,
+					// Allow stale keys left behind by removed experiments without invalidating the entire option.
+					'additionalProperties' => array(


Before, the REST API returned a 400 for an unknown experiment ID. Now it saves any key. So a typo in setGutenbergExperiments() in e2e tests will not fail early anymore.

tyxla · 2026-09-24T20:10:42Z

-					'type'       => 'object',
-					'properties' => $properties,
+					'type'                 => 'object',
+					// Experiment properties must always be boolean.


Nit: I don't think we need this comment. This line doesn't make the properties boolean, the loop above does.

tyxla · 2026-09-24T20:10:55Z

+	}
+
+	public function tear_down() {
+		delete_option( 'gutenberg-experiments' );


Nit: I think we don't need delete_option() here. WP_UnitTestCase already rolls back the database after each test.

tyxla · 2026-09-24T20:11:14Z

+		$this->assertSame( $experiments, $this->get_experiments_setting() );
+	}
+
+	public function test_unregistered_experiments_do_not_invalidate_the_setting() {


This test only uses true. Can we add a case with a value like null or 'on'? Right now it still breaks the setting.

Also, this test and test_unregistered_experiments_are_preserved check almost the same thing. Maybe we can merge them into one?

tyxla · 2026-09-24T20:11:31Z

+		$this->assertSame( $expected, get_option( 'gutenberg-experiments' ) );
+	}
+
+	public function test_rest_rejects_a_value_that_is_not_a_map() {


Small thing: an empty string ('') passes this check, because rest_is_object( '' ) returns true. Then all experiments get turned off. Maybe worth a test?

tyxla · 2026-09-24T20:13:21Z

+		$this->assertSame( 400, $response->get_status() );
+		$this->assertSame( 'rest_invalid_param', $response->get_data()['code'] );
+		$this->assertSame( $experiments, get_option( 'gutenberg-experiments' ) );


Nit: we could extend WP_Test_REST_TestCase and use assertErrorResponse( 'rest_invalid_param', $response, 400 ) here

sdnunca · 2026-09-24T23:04:35Z

Thanks for working on this @sdnunca!

I think the real problem is in routes/experiments-home/stage.tsx - when the setting comes back as null, the page uses {} and then saves over the whole option. This PR fixes one way to get there, but not all of them.

What do you think about not saving when the value is null? Or we could remove only the bad entries on the server, instead of making the whole setting null perhaps?

Thank you for looking at the changes @tyxla!

I spent some time thinking about this and the front-end handling seems sensible to me. If we ended up with invalid data in gutenberg-experiments and the API returned null, starting over and recreating the array is the best option we have.

Because setSettings spreads the changed values over the response data and not over a list of current experiments, I assumed our intention was to preserve any user data. Being wary of altering user data at all, I discarded approaches where we clean up the array shape.

From what I could tell, the experiments have always been/should always be boolean-ish , so the schema allowing unregistered booleans made sense to me.

While I think the best option here would be to manage existing data when changing the experiments using a migration, I'm not sure how realistic a requirement that is.

If we're open to altering the stored data, I'd say using rest_pre_get_setting and removing any keys not matching the registered ones makes sense. The array would then be updated on first save and would stop growing when experiments are updated. This wouldn't cover the case where we remove and later reintroduce another experiment with the same name, but I don't think we can cover that without some sort of migration logic.

I'll give this approach a try and get back to you.

ramonjd · 2026-09-25T02:29:08Z

Thanks for reviving this!

After the minor suggested fixes in, does this PR fix the primary unhappy path? (In my testing, it does)

Being wary of altering user data at all, I discarded approaches where we clean up the array shape.

Was thinking of follow ups... what if overwriting the unknown state was user-driven?

So, check for null, show a notice and offer an explicit "Reset experiments" action that writes { 'gutenberg-experiments': {} }.

The only benefit I can see is that it does not rewrite stored data behind the user's back.

Maybe I'm missing something.

Ideally, it'd be great to iterate over the stored keys and tell the UI which keys are invalid so we could continue to render toggle buttons with valid values, and report those that don't. But I'm guessing that might involve a new read endpoint or something.

t-hamano · 2026-09-25T04:32:25Z

Personally, since it is an 'experimental setting' as the name suggests, I don't feel we need to worry too much about backward compatibility. I also think it would be fine to implicitly remove old keys without notifying users. Am I missing anything?
Regarding whether to use rest_pre_get_setting() or additionalProperties, I don't have a strong opinion either way.

mcsf · 2026-09-25T08:27:19Z

Personally, since it is an 'experimental setting' as the name suggests, I don't feel we need to worry too much about backward compatibility. I also think it would be fine to implicitly remove old keys without notifying users. Am I missing anything?

AFAIU there's a secondary use case to address: users who routinely change Gutenberg branches back and forth may lose settings without knowing it. The most obvious scenarios concerns contributors themselves, who test different branches all day, but the most compelling scenarios to fix are those that can affect "regular users":

site admin switches to the latest Gutenberg stable, or even an RC
- this new version removes experimental flag F
user interacts with their site
- the system implicitly updates the user's setting to remove F
due to some bugs, admin decides to revert
user gets confused by change in behaviour due to lost experiments settings

There are reports that this could happen in WP.com, where certain experiments are given more visibility so that users opt into them.

So the safest thing right now seems to preserve unrecognised experiments and ensure they don't fail validation.

To avoid accumulating stale experiments forever, maybe we could try a follow-up PR that adds a sanitize_callback to the setting that filters the values against a list of known $deprecated_experiments. We would only add experiments to this constant once enough time had passed since their stabilisation or abandonment.

tyxla · 2026-09-25T09:02:41Z

So the safest thing right now seems to preserve unrecognised experiments and ensure they don't fail validation.

To avoid accumulating stale experiments forever, maybe we could try a follow-up PR that adds a sanitize_callback to the setting that filters the values against a list of known $deprecated_experiments. We would only add experiments to this constant once enough time had passed since their stabilisation or abandonment.

This seems like a good compromise. Without that, it makes me question the existence of validation in the first place.

sdnunca · 2026-09-28T11:42:22Z

Thanks so much for the feedback, much appreciated. I've cleaned up the unit tests.

AFAIU, the PR matches the compromise described above to preserve unrecognized experiments, I can tackle the $deprecated_expriments approach in a follow-up PR.

tyxla

I believe this is looking good. Left a few minor nits for your consideration.

@mcsf I believe this is aligned with your compromise suggestions, but could you have another look just in case?

tyxla · 2026-09-28T14:51:18Z

+	/**
+	 * @dataProvider experiment_values_provider
+	 */
+	public function test_reads_experiment_values( $initial_experiments, $read_experiments ) {


Hmm, the data provider keys (input, output, should_fail_on_save) don't match the argument names here. It works now with the current PHPUnit version, but I believe newer PHPUnit passes string keys as named arguments, so this would break. Maybe we should use the same names in both places?

tyxla · 2026-09-28T14:52:26Z

+	/**
+	 * @return array< string,  array{ 'input': mixed, 'output': array|null, 'should_fail_on_save': bool }>;
+	 */


Could use a newline, and the double space and trailing ; looks unintended:

Suggested change

/**

* @return array< string, array{ 'input': mixed, 'output': array|null, 'should_fail_on_save': bool }>;

*/

/**

* @return array< string, array{ 'input': mixed, 'output': array|null, 'should_fail_on_save': bool }>

*/

tyxla · 2026-09-28T15:03:06Z

+			$this->assertSame( 200, $response->get_status() );
+			$data = $response->get_data();
+			$this->assertArrayHasKey( 'gutenberg-experiments', $data );
+			$this->assertSame(


Nit: the arguments to assertSame() are swapped. The expected value should go first, so the error message is clearer when the test fails.

tyxla · 2026-09-28T15:03:24Z

+	 */
+	public function experiment_values_provider(): array {
+		return array(
+			'unregistered experiment, non bool, fails schema validation and returns null.' =>


Not blocking, but none of these tests cover the original bug, where one toggle turns off all the other experiments. Maybe we can add a test for that?

Agreed.

I'm also tempted to say that we're stretching the usefulness of @dataProvider here. Tests should be as clear to read as possible, and IMO the cleverness of factoring out experiment_values_provider is detrimental. Let's improve the setup:

split the data into passing and failing cases:

data_valid_values, data_invalid_values,

test_saves_experiments_setting, test_rejects_experiments_setting

rename the remaining test to test_reads_experiments_setting for consistency

method signatures:

test_saves_experiments_setting( $input, $expected )

test_rejects_experiments_setting( $input ) — the expected output is always null

dataprovider data shapes adjusted accordingly, e.g. drop the should_fail_on_save property

is it idiomatic to name the keys of each row's data? I don't remember ever seeing it. I'm tempted to suggest dropping the keys, i.e.

public function data_valid_values() { return array( - array( 'input' => '…', 'expected' => '…' ), + array( '…', '…' ),

And improve the test cases themselves:

as @tyxla points out, add cases to cover the original bug

rename dummy value "test-experiment" to "unregistered-experiment"

rename test cases, which are too verbose, e.g.:

"unregistered experiment" is enough, we know from context it's a passing case

"unregistered experiment with invalid value" is fine for the failing case

"all experiments are returned, including bool unregistered ones." -> "registered and unregistered experiments"

"empty string is object like." -> "empty string"

"non-empty string fails schema validation." -> "non-legacy string value" (obvious in the context of that dataprovider)

"object is object like." -> "empty object"

do we need a test case with a non-empty object?

"legacy checkbox values are coerced to bool." -> "legacy string values ('0'|'1')"

tyxla · 2026-09-28T15:03:54Z

+					'type'                 => 'object',
+					'properties'           => $properties,
+					// Allow stale keys left behind by removed experiments without invalidating the entire option.
+					'additionalProperties' => array(


One more thing: I think this now also accepts a list like [ true, false ], because keys 0 and 1 are counted as extra boolean properties. Before, this returned a 400. Can we check this and maybe add a test for it?

Experiments: Update the gutenberg-experiments setting to allow additi…

5ca4a8a

…onal bool properties

sdnunca requested a review from spacedmonkey as a code owner September 24, 2026 17:04

sdnunca mentioned this pull request Sep 24, 2026

wpcomsh: Remove obsolete Gutenberg experiment Automattic/jetpack#52772

Merged

tyxla reviewed Sep 24, 2026

View reviewed changes

ramonjd requested a review from t-hamano September 25, 2026 00:34

ramonjd added [Type] Bug An existing feature does not function as intended Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts labels Sep 25, 2026

ramonjd assigned sdnunca Sep 25, 2026

Clean up unit tests

17960c2

tyxla reviewed Sep 28, 2026

View reviewed changes

jsnajdr mentioned this pull request Oct 13, 2026

Experiments: don't invalidate saved option with a legacy key #84619

Merged

Conversation

sdnunca commented Sep 24, 2026

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Use of AI Tools

Uh oh!

github-actions Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdnunca commented Sep 24, 2026

Uh oh!

ramonjd commented Sep 25, 2026

Uh oh!

t-hamano commented Sep 25, 2026

Uh oh!

mcsf commented Sep 25, 2026

Uh oh!

tyxla commented Sep 25, 2026

Uh oh!

sdnunca commented Sep 28, 2026

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Sep 24, 2026 •

edited

Loading