Skip to content

Theme_JSON: Prevent implicit coercion in to_ruleset #76392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ramonjd merged 8 commits into from
Mar 13, 2026
Merged

Theme_JSON: Prevent implicit coercion in to_ruleset #76392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c2ac2f
Fix CSS value handling in WP_Theme_JSON_Gutenberg to skip boolean fea…
ramonjd Mar 11, 2026
c2953a7
Enhance WP_Theme_JSON_Gutenberg to skip non-string CSS declarations. …
ramonjd Mar 11, 2026
a29fb13
Refactor WP_Theme_JSON_Gutenberg to improve CSS value handling by rem…
ramonjd Mar 12, 2026
284bdc3
Add test for WP_Theme_JSON_Gutenberg::to_ruleset to ensure non-scalar…
ramonjd Mar 12, 2026
738991a
Remove comments
ramonjd Mar 12, 2026
6378006
lintbean
ramonjd Mar 12, 2026
62bb47b
Update test for WP_Theme_JSON_Gutenberg::to_ruleset to set reflection…
ramonjd Mar 12, 2026
a54f8c7
Remove comment, add changelog
ramonjd Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions backport-changelog/7.1/11236.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
https://github.com/WordPress/wordpress-develop/pull/11236

* https://github.com/WordPress/gutenberg/pull/76392
14 changes: 13 additions & 1 deletion lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2090,6 +2090,7 @@ private static function get_feature_selector( array $feature_selectors, string $
* creates the corresponding ruleset.
*
* @since 5.8.0
* @since 7.1.0 Skip declarations whose value is not a plain string (booleans, arrays, objects, etc.).
*
* @param string $selector CSS selector.
* @param array $declarations List of declarations.
Expand All @@ -2103,7 +2104,18 @@ protected static function to_ruleset( $selector, $declarations ) {
$declaration_block = array_reduce(
$declarations,
static function ( $carry, $element ) {
return $carry .= $element['name'] . ': ' . $element['value'] . ';'; },
$value = $element['value'];

if ( is_numeric( $value ) ) {
$value = (string) $value;
}

if ( ! is_string( $value ) ) {
return $carry;
}

return $carry .= $element['name'] . ': ' . $value . ';';
},
Comment on lines +2107 to +2118

@ramonjd ramonjd Mar 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the root issues. $element['value'] could be anything and PHP silently coerces it without the checks.

''
);

Expand Down
40 changes: 38 additions & 2 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3901,11 +3901,11 @@ public function test_get_styles_with_appearance_tools() {
);

$metadata = array(
'path' => array( 'settings' ),
'path' => array( 'styles' ),
'selector' => 'body',
);

$expected = ':where(body) { margin: 0; }.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }:where(.wp-site-blocks) > * { margin-block-start: ; margin-block-end: 0; }:where(.wp-site-blocks) > :first-child { margin-block-start: 0; }:where(.wp-site-blocks) > :last-child { margin-block-end: 0; }:root { --wp--style--block-gap: ; }:root :where(.is-layout-flow) > :first-child{margin-block-start: 0;}:root :where(.is-layout-flow) > :last-child{margin-block-end: 0;}:root :where(.is-layout-flow) > *{margin-block-start: 1;margin-block-end: 0;}:root :where(.is-layout-constrained) > :first-child{margin-block-start: 0;}:root :where(.is-layout-constrained) > :last-child{margin-block-end: 0;}:root :where(.is-layout-constrained) > *{margin-block-start: 1;margin-block-end: 0;}:root :where(.is-layout-flex){gap: 1;}:root :where(.is-layout-grid){gap: 1;}.is-layout-flow > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-flow > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-flow > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-constrained > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-constrained > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)){margin-left: auto !important;margin-right: auto !important;}body .is-layout-flex{display: flex;}.is-layout-flex{flex-wrap: wrap;align-items: center;}.is-layout-flex > :is(*, div){margin: 0;}body .is-layout-grid{display: grid;}.is-layout-grid > :is(*, div){margin: 0;}';

@ramonjd ramonjd Mar 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice --wp--style--block-gap: ; the value was always empty. It was a quirk of passing settings as the path in the test that made the following condition pass because the value was true:

if ( null !== $block_gap_value && false !== $block_gap_value && '' !== $block_gap_value ) {

$expected = ':where(body) { margin: 0; }.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }:where(.wp-site-blocks) > * { margin-block-start: ; margin-block-end: 0; }:where(.wp-site-blocks) > :first-child { margin-block-start: 0; }:where(.wp-site-blocks) > :last-child { margin-block-end: 0; }:root { --wp--style--block-gap: ; }.is-layout-flow > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-flow > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-flow > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}.is-layout-constrained > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}.is-layout-constrained > .aligncenter{margin-left: auto !important;margin-right: auto !important;}.is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)){margin-left: auto !important;margin-right: auto !important;}body .is-layout-flex{display: flex;}.is-layout-flex{flex-wrap: wrap;align-items: center;}.is-layout-flex > :is(*, div){margin: 0;}body .is-layout-grid{display: grid;}.is-layout-grid > :is(*, div){margin: 0;}';

@tellthemachines tellthemachines Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty --wp--style--block-gap: ; is still here though, is that expected?

@ramonjd ramonjd Mar 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

It's because blockGap isn't present: https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-gutenberg.php#L3525-L3531

So it's expected, but not ideal I guess.

We could skip adding those styles if there's no blockGap?

I left that bit alone as I was afraid of blowing things up.

$this->assertSameCSS( $expected, $theme_json->get_root_layout_rules( WP_Theme_JSON_Gutenberg::ROOT_BLOCK_SELECTOR, $metadata ) );
}

Expand Down Expand Up @@ -7257,4 +7257,40 @@ public function test_sanitize_preserves_null_schema_behavior() {
$this->assertSame( 'string-value', $settings['appearanceTools'] );
$this->assertSame( array( 'nested' => 'value' ), $settings['custom'] );
}

/**
* @covers WP_Theme_JSON_Gutenberg::to_ruleset
*/
public function test_to_ruleset_skips_non_scalar_values_and_casts_numerics() {
Comment on lines +7261 to +7264

@andrewserong andrewserong Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I approved before checking the test results 😅

Does this one need a call to $reflection->setAccessible( true );?

The failure is:

  1. WP_Theme_JSON_Gutenberg_Test::test_to_ruleset_skips_non_scalar_values_and_casts_numerics
    ReflectionException: Trying to invoke protected method WP_Theme_JSON_Gutenberg::to_ruleset() from scope ReflectionMethod

@ramonjd ramonjd Mar 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL I deleted it because my local linter said it was deprecated. The tests pass for me. 🙃 I'll add it back it.

Thanks!!

@andrewserong andrewserong Mar 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it might just be the old PHP versions tests where it's needed, that might explain it!

@ramonjd ramonjd Mar 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh because of a PHP version mismatch prolly

$reflection = new ReflectionMethod( WP_Theme_JSON_Gutenberg::class, 'to_ruleset' );
$reflection->setAccessible( true );
$declarations = array(
array(
'name' => 'color',
'value' => 'red',
),
array(
'name' => 'opacity',
'value' => true,
),
array(
'name' => 'margin',
'value' => 0,
),
array(
'name' => 'padding',
'value' => false,
),
array(
'name' => 'gap',
'value' => array(),
),
);
$result = $reflection->invoke( null, '.test', $declarations );
$this->assertStringContainsString( 'color: red;', $result, 'Color declaration should be included' );
$this->assertStringContainsString( 'margin: 0;', $result, 'Numeric value should be cast to string' );
$this->assertStringNotContainsString( 'opacity', $result, 'Boolean value should be skipped' );
$this->assertStringNotContainsString( 'padding', $result, 'Boolean value should be skipped' );
$this->assertStringNotContainsString( 'gap', $result, 'Array value should be skipped' );
}
}
Loading