Skip to content

WP_Theme_JSON: Prevent implicit coercion in to_ruleset#11236

Open
ramonjd wants to merge 3 commits intoWordPress:trunkfrom
ramonjd:update/theme-json-to-ruleset-coercion
Open

WP_Theme_JSON: Prevent implicit coercion in to_ruleset#11236
ramonjd wants to merge 3 commits intoWordPress:trunkfrom
ramonjd:update/theme-json-to-ruleset-coercion

Conversation

@ramonjd
Copy link
Member

@ramonjd ramonjd commented Mar 12, 2026

Brings over changes from:

to_ruleset used string concatenation ($element['name'] . ': ' . $element['value'] . ';'), so PHP implicitly coerced non-string values (e.g. booleans → '1'/'', arrays → 'Array'). That could emit invalid or misleading CSS.

At the same time, pass a style theme.json path in test_get_styles_with_appearance_tools() to simulate a style node. Before it was settings.

The fix

  • Cast numeric values to string explicitly.
  • Skip declarations whose value is not a string or number (booleans, arrays, objects).

Testing

Tests should pass!

Trac ticket: https://core.trac.wordpress.org/ticket/64848

Use of AI Tools

Diagnostics

@ramonjd ramonjd self-assigned this Mar 12, 2026
@ramonjd ramonjd added the bug label Mar 12, 2026
@github-actions
Copy link

github-actions bot commented Mar 12, 2026

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props ramonopoly, andrewserong, isabel_brison.

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

);

$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;}';
$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;}';
Copy link
Member Author

Choose a reason for hiding this comment

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

The test result is different because we're no longer passing settings as metadata. Settings contains no style data, but supports data (mostly bools as values), but the source code was trying to fetch style data nevertheless. Since the paths to the settings and styles are same, the result was bool instead of real gap values.

It was this that led me to remove the silent coercion in to_ruleset.

@github-actions
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for backporting, this change looks good to me!

ramonjd added 2 commits March 13, 2026 11:33
…es and cast numeric values to strings.

This change ensures that only plain string values are included in the generated CSS ruleset, improving the handling of declarations.
@ramonjd ramonjd force-pushed the update/theme-json-to-ruleset-coercion branch from 7cf6326 to c4acf90 Compare March 13, 2026 00:33
@tellthemachines
Copy link
Contributor

This LGTM but if we're aiming for 7.1 we'll have to wait until the 7.0 release branch is created at RC1 to commit it to trunk.

@tellthemachines
Copy link
Contributor

Also, PHP tests seem to be complaining about your use of setAccessible()

There was 1 error:

1) Tests_Theme_wpThemeJson::test_to_ruleset_skips_non_scalar_values_and_casts_numerics
Method ReflectionMethod::setAccessible() is deprecated since 8.5, as it has no effect since PHP 8.1

/var/www/tests/phpunit/tests/theme/wpThemeJson.php:7066
/var/www/vendor/bin/phpunit:122

@ramonjd
Copy link
Member Author

ramonjd commented Mar 13, 2026

PHP tests seem to be complaining about your use of setAccessible()

Argh, < 8.5 wants it, > 8.5 hates it. 🤣

This LGTM but if we're aiming for 7.1 we'll have to wait until the 7.0 release branch is created at RC1 to commit it to trunk.

Thanks for the heads up. I'll leave it brewing until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants