Skip to content

fix: incorrect conditional check in WP_Theme_JSON_Gutenberg::convert_variables_to_values() - #68401

Merged
westonruter merged 3 commits into
WordPress:trunkfrom
justlevine:fix/strops-false_convert_variables_to_value
Oct 25, 2026
Merged

fix: incorrect conditional check in WP_Theme_JSON_Gutenberg::convert_variables_to_values()#68401
westonruter merged 3 commits into
WordPress:trunkfrom
justlevine:fix/strops-false_convert_variables_to_value

Conversation

@justlevine

@justlevine justlevine commented Dec 30, 2024

Copy link
Copy Markdown
Contributor

What?

This PR fixes an issue in WP_Theme_JSON_Gutenberg::convert_variables_to_values(), where preg_match_all() is aways run, even if var( is not found in the style.

This is due to the loose 0 <= false comparison.

Why?

While this issue was surfaced by PHPStan (via #66693) it can be remediated independently as part of #66598

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Before After

@github-actions

github-actions Bot commented Dec 30, 2024

Copy link
Copy Markdown

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core, Gutenberg Plugin.
  • Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

@justlevine
justlevine force-pushed the fix/strops-false_convert_variables_to_value branch from e0d6d11 to 4ff09a5 Compare April 18, 2025 23:03
westonruter and others added 2 commits October 24, 2026 21:53
@westonruter @claude
Merge branch 'trunk' into the PR branch
da05515 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@westonruter @claude
Use str_contains() for the var( check
880627a 
This makes `WP_Theme_JSON_Gutenberg::convert_variables_to_value()`
identical to `WP_Theme_JSON::convert_variables_to_value()` in WordPress
core, which made the same fix with `str_contains()` in r63895.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@westonruter westonruter added the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label Oct 25, 2026
@github-actions

github-actions Bot commented Oct 25, 2026 •
edited
Loading

Copy link
Copy Markdown

🤖 PR meta 🤖

🎉 Props

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: justlevine <justlevine@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

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

Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

Updated as activity occurs, without notifying anyone named here. Add the props-bot label to refresh.

@westonruter

westonruter commented Oct 25, 2026

Copy link
Copy Markdown
Member

🤖 Comment by Claude Opus 5.5

Trunk was merged in, and the check was changed from false !== strpos() to str_contains() in 880627a. This makes WP_Theme_JSON_Gutenberg::convert_variables_to_value() identical to core's WP_Theme_JSON::convert_variables_to_value(), which made the same fix in r63895 (Core-66164). Since the change already exists in core, the No Core Sync Required label was added.

There is no change in output: the preg_match_all() inside the condition requires a literal var( to match anything, so the always-true 0 <= strpos() guard only meant the regular expression also ran on style values without a CSS variable. This resolves the smallerOrEqual.alwaysTrue error reported at PHPStan level 5.

@westonruter westonruter added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 25, 2026
@westonruter
westonruter merged commit 5e986c0 into WordPress:trunk Oct 25, 2026
88 checks passed
@github-actions github-actions Bot added this to the Gutenberg 24.4 milestone Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes
@tellthemachines tellthemachines Awaiting requested review from tellthemachines tellthemachines is a code owner

Assignees

No one assigned

Labels

No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Milestone

Gutenberg 24.4

Development

Successfully merging this pull request may close these issues.

2 participants

@justlevine @westonruter