Skip to content
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.

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

Fix/wp get global styles for custom props returns internal variable #50366

Conversation

samnajian
Copy link
Contributor

@samnajian samnajian commented May 5, 2023

What?

This PR fixes #49693 so that wp_get_global_styles for the following in theme.json

"core/post-terms": {
    "typography": { "fontSize": "var(--wp--preset--font-size--small)" },
    "color":{ "background": "var:preset|color|secondary" }
}

returns:

(
    [typography] => Array( [fontSize] => var(--wp--preset--font-size--small) )
    [color] => Array( [background] => var(--wp--preset--color--secondary) )
)

Why?

#49693
The internal syntax shouldn't leak out.

How?

This PR extract the already existing logic that converts var:preset|color|secondary to var(--wp--preset--font-size--small) to a separate method, then uses the same method to sanitize the output of wp_get_global_styles to only include custom CSS variables and not internal variable syntax.

Testing Instructions

Please refer to the issues for testing instructions, but please note the following:

  1. Make sure the Gutenberg plugin is installed and active
  2. In body of wp_get_global_styles exchange WP_Theme_JSON_Resolver with WP_Theme_JSON_Resolver_Gutenberg
  3. Confirm the output of wp_get_global_styles doesn't include internal variable syntax.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 5, 2023
@github-actions
Copy link

github-actions bot commented May 5, 2023

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @samnajian! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch 2 times, most recently from aa61fb1 to c3ae73e Compare May 5, 2023 12:17
@samnajian samnajian marked this pull request as ready for review May 5, 2023 12:30
@samnajian samnajian requested a review from spacedmonkey as a code owner May 5, 2023 12:30
@oandregal oandregal self-requested a review May 5, 2023 12:34
@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label May 5, 2023
@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch from c3ae73e to e95e75b Compare May 5, 2023 12:40
@oandregal
Copy link
Member

This is a great start, thanks!

In body of wp_get_global_styles exchange WP_Theme_JSON_Resolver with WP_Theme_JSON_Resolver_Gutenberg

The only thing left would be to copy back from core the wp_get_global_styles function and declare it as gutenberg_get_global_styles function in Gutenberg. This is a bit cumbersome, but it's the method we have now to keep core<=>gutenberg in sync.

@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch 5 times, most recently from d0417a4 to adc1de9 Compare May 7, 2023 13:04
@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch 4 times, most recently from 5cc4576 to df27629 Compare May 8, 2023 12:27
@oandregal
Copy link
Member

I've left a few suggestions for improving sanitize_variables implementation. Other than that, this is ready. Thanks for your work here 👏

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved
lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved
lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved
@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch 2 times, most recently from 366ed4a to 2ae9672 Compare May 9, 2023 09:13
The logic that converts internal css variables to custom css
variables is extracted to a function
samnajian and others added 9 commits May 9, 2023 14:36
As of the changes to WP_Theme_JSON_Resolver_Gutenberg,
this function had to get defined to keep in sync with core
Since convert_custom_properties is applied in constructor
the other methods don't need to apply it again
Since the data is already checked against the schema
sanitize_variables doesn't need to know about the schema
anymore
@samnajian samnajian force-pushed the fix/wp_get_global_styles-for-custom-props-returns-internal-variable branch from 05229a0 to 5833401 Compare May 9, 2023 12:36
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

This is great work, thanks for your contributions, Sam!

@oandregal oandregal merged commit ccd529d into WordPress:trunk May 10, 2023
@github-actions
Copy link

Congratulations on your first merged pull request, @samnajian! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

* Gets the styles resulting of merging core, theme, and user data.
*
* @since 5.9.0
* @since 6.3.0 the internal link format "var:preset|color|secondary" is resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Once introduced in WP Core, this modifies the value format for the returned styles from wp_get_global_styles(). As such, it is a backwards-compatibility (BC) break for extenders expecting and using this structure.

The result could mean broken styling experiences for users.

Is this a truly a bugfix, meaning wp_get_global_styles() should always have returned the style value structure? Or is this a new improvement to avoid leaking internal structures?

cc @oandregal

Copy link
Member

@oandregal oandregal May 11, 2023

Choose a reason for hiding this comment

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

This is why I think it's a fine change.

Let say we had the following input:

"core/post-terms": {
    "typography": { "fontSize": "var(--wp--preset--font-size--small)" },
    "color":{ "background": "var:preset|color|secondary" }
}

Before this PR, the output of wp_get_global_styles would be:

(
    [typography] => Array( [fontSize] => var(--wp--preset--font-size--small) )
    [color] => Array( [background] => var:preset|color|secondary )
)

After this PR, the output of wp_get_global_styles would be:

(
    [typography] => Array( [fontSize] => var(--wp--preset--font-size--small) )
    [color] => Array( [background] => var(--wp--preset--color--secondary) )
)

Before this PR, the consumers had to deal with two formats: var(--wp--preset--color--secondary) and var:preset|color|secondary, as a possible representation of the same value.

After this PR, they only have to deal with one format that's been always present: var(--wp--preset--color--secondary). I don't remember when the var:preset|color|secondary was introduced for people to use in theme.json but it was later and added as a convenience. It should have never leaked to consumers, in my view. I did some quick search but didn't find when it was, I can do some archeology if this is important.

An important note is that the format doesn't carry any meaning about where this data comes from. Any origin (default, block, theme, user) can use var:preset|color|secondary. Consumers cannot use that format as a hint of it being data from the user origin.

Does this help?

@oandregal
Copy link
Member

Follow-up to fix PHP docblock at #50527

@oandregal
Copy link
Member

Backport at WordPress/wordpress-develop#4556

@ramonjd ramonjd removed the Needs PHP backport Needs PHP backport to Core label Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wp_get_global_styles: for custom props, it should return the CSS format not the shortened internal one
4 participants