-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Backport theme.json version 3 migrations #6616
Backport theme.json version 3 migrations #6616
Conversation
4d0eda8
to
be649bd
Compare
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Hi there, to help folks (like me) who don't have much context, are there any specific things that should be tested in this PR? Or is the assumption to run through the test instructions for each Gutenberg PR? |
d21b5f8
to
d549db2
Compare
af0a2c5
to
969dc48
Compare
The focus for the PHP side of things is more around what gets generated by global styles which is only tested via the UI on the gutenberg PRs. You might need to test the stylesheet generation directly since some of the JS changes aren't included. The dev note summarizes the changes that need to be made to make a v3 theme.json work like a v2 theme.json. But without changing the version, the styles generated for any existing v2 theme.json should remain unchanged from before. If the dev note doesn't explain enough, the original two PRs WordPress/gutenberg#58409 and WordPress/gutenberg#61842 are a good place to find more info if I'm not around. |
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
So far I've been stuck on testing defaultFontSizes. I can confirm that migration does something - I tested that manually using I basically copied the wpThemeJson tests. Shouldn't it be true? But I'm also having trouble manually testing. I've started off with font sizes and am following the instructions over at WordPress/gutenberg#58409 and the dev note I understand the editor UI won't reflect theme.json definitions until the JS packages are migrated, so I've been testing the output of With theme.json version 2 the default font sizes are generated by With theme.json version 3 Logging where $prevent_override is set, I see Just to be sure I also set Even hardcoding a Am I missing something or should I be testing the output of It would be nice to have some steps to test this backport in particular. Sorry I ran out of time to go further. Thanks! |
It should be false whenever there were existing settings, otherwise it should be left empty.
This is the correct behavior. The |
Okay, thanks for the tips. I think there was a bit of a 🤦🏻 moment on my side. It took me a while to grok. Now I'm seeing the following: Theme with
Theme with
I'll continue to test the spacing later - I'm AFK for a few hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a code review, and it looks good. I appreciate all the comments.
I'm not so familiar with the code in the class-wp-theme-json though, so happy for someone else to look through it.
if ( ! isset( $new['settings'] ) ) { | ||
$new['settings'] = array(); | ||
} | ||
if ( ! isset( $new['settings']['typography'] ) ) { | ||
$new['settings']['typography'] = array(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be missing something, but this seems unnecessary considering $new = $old
, and there's already a test for isset( $old['settings']['typography']['fontSizes'] )
above.
Not a big issue though, I don't mind cautious code.
(edit: There's also _wp_array_set
, which would make this more succinct -
https://developer.wordpress.org/reference/functions/_wp_array_set/)
(edit again: Even better, do what Ramon says - https://github.com/WordPress/wordpress-develop/pull/6616/files/f8e247d7812644d67e946a8f98e734e795bcb9c9#r1625538035)
Manual testingI tested by manipulating 2024 theme.json files. Spacing sizes✅ Font sizes✅ As above, but tested with same-slugged I also added a theme.json to a classic theme (2020) and repeated the above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the work on this @ajlende
I'm coming at it pretty fresh mind you, but manual testing is working for me in the block and classic themes I tested. 👍🏻
I left some questions. It would be good to get another round of eyes from a Core committer, or someone who is more familiar with the feature.
Tentatively it's looking good to me.
@@ -39,7 +39,7 @@ class WP_Theme_JSON_Data { | |||
* @param array $data Array following the theme.json specification. | |||
* @param string $origin The origin of the data: default, theme, user. | |||
*/ | |||
public function __construct( $data = array(), $origin = 'theme' ) { | |||
public function __construct( $data = array( 'version' => WP_Theme_JSON::LATEST_SCHEMA ), $origin = 'theme' ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change warrant a @since
annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it change the @since
? It sounds like this is fine to do in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change in default value here is more to avoid confusion than anything. The same was done in the constructor for WP_Theme_JSON
, and the constructor for WP_Theme_JSON
automatically runs the migration to the latest version.
The version
option is required for theme.json data and an omission of it originally signaled that the file was the shape of an experimental-theme.json or theme.json v0 WordPress/gutenberg#36154. When it landed in core, it represented a v1 theme.json, so this probably should have been updated then for clarity https://github.com/WordPress/gutenberg/pull/36155/files#diff-b03597cc3da199e5aa5a94950e8241135904853e7c3b82d758e42744878afae7R315-R335.
It didn't really matter that much at the time because the migrations weren't changing default values. Since the default value is changed now, keeping this as an empty array means migrating from a v1 theme.json and adding 'defaultFontSizes' => false
and 'defaultSpacingSizes' => false
which I don't think is the intention of the default value.
} | ||
if ( ! isset( $theme_support_data['settings']['typography']['fontSizes'] ) ) { | ||
// If the theme does not have any font sizes, we still want to show the core one. | ||
$default_font_sizes = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested, but optional for follow up, these conditions return bools so the return value can be used to set the var, e.g.,
$default_font_sizes = ! isset( $theme_support_data['settings']['typography']['fontSizes'] );
Not really required though, just filling space here... 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole thing could use a refactor. I chose to use the same structure as colors, gradients, and shadows above and below this for now to be clear that it was doing exactly the same thing as those other settings. https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR294-R302
switch ( $theme_json['version'] ) { | ||
case 1: | ||
$theme_json = self::migrate_v1_to_v2( $theme_json ); | ||
// no break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments required for some linting rule? Otherwise I'd remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like them! But they should probably follow coding standards 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a standard, I was just thinking that the comment doesn't add much. There's no break. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it said break deliberately omitted
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could help to stop someone adding a break later on.
When I say following coding standards, I mean it should have a capital first letter and end with a period (if the comment is kept).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it's fine to do in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter made me add them. It required the exact string // no break
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I ran PHPCS in wordpress-develop and it doesn't have the same rule as Gutenberg.
FILE: /Users/ajlende/Documents/gutenberg/lib/class-wp-theme-json-schema-gutenberg.php
--------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------
56 | ERROR | There must be a comment when fall-through is intentional in a non-empty
| | case body (PSR2.ControlStructures.SwitchDeclaration.TerminatingComment)
--------------------------------------------------------------------------------------
EDIT: I saved the GB file, but not the WPdev one when I thought it was only in one place. They both show the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing and it looks like it only matters that the line starts with exists. The second doesn't seem to need it. I can add some additional description if that's helpful.// no break
// Migrate each version in order starting with the current version.
switch ( $theme_json['version'] ) {
case 1:
$theme_json = self::migrate_v1_to_v2( $theme_json );
// Deliberate fall through. Continue on with migrations.
case 2:
$theme_json = self::migrate_v2_to_v3( $theme_json );
}
if ( ! isset( $new['settings']['spacing'] ) ) { | ||
$new['settings']['spacing'] = array(); | ||
} | ||
$new['settings']['spacing']['defaultSpacingSizes'] = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the defaultFontSizes above, in PHP you can set the value of a nested array item:
$new = [];
$new['settings']['spacing']['defaultSpacingSizes'] = false;
var_dump( $new );
/*
Output:
array(1) {
["settings"]=>
array(1) {
["spacing"]=>
array(1) {
["defaultSpacingSizes"]=>
bool(false)
}
}
}*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow-up with this.
@@ -747,8 +756,8 @@ public function __construct( $theme_json = array(), $origin = 'theme' ) { | |||
$valid_block_names = array_keys( static::get_blocks_metadata() ); | |||
$valid_element_names = array_keys( static::ELEMENTS ); | |||
$valid_variations = static::get_valid_block_style_variations(); | |||
$theme_json = static::sanitize( $this->theme_json, $valid_block_names, $valid_element_names, $valid_variations ); | |||
$this->theme_json = static::maybe_opt_in_into_settings( $theme_json ); | |||
$this->theme_json = static::sanitize( $this->theme_json, $valid_block_names, $valid_element_names, $valid_variations ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find anything in the Gutenberg PR about this change.
Any reason to set the class property $this->theme_json
here?
If so, maybe it's worth mentioning in the annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets set again on the next line anyway so I wonder this is just a style/consistency thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's no functional change here. If it's not ok, let's follow-up separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point during development I was using the $theme_json
argument to check the theme.json version and run code conditionally based on that. Turns out the way that we merge files in get_merged_data
(https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR618-R636) means that it isn't a reliable way to check if we're starting with a v2 theme.json.
I didn't switch it back because the code is functionally the same, and I think it reads better this way.
// Pre-generate the spacingSizes from spacingScale. | ||
$scale_path = array( 'settings', 'spacing', 'spacingScale', $origin ); | ||
$spacing_scale = _wp_array_get( $this->theme_json, $scale_path, null ); | ||
if ( isset( $spacing_scale ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional:
There was a recent effort to reduce the use of _wp_array_get
https://core.trac.wordpress.org/ticket/59405
More verbose, but could also be
if ( isset( $this->theme_json['settings']['spacing']['spacingScale'][ $origin ] ) ) {
....
$spacing_scale_sizes = static::compute_spacing_sizes( $this->theme_json['settings']['spacing']['spacingScale'][ $origin ] );
}
might save about 0.000002ms
😄
Not a huge deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using _wp_array_get
is perfectly valid, IMO. The before/after comparisons I've seen report profiler numbers which are skewed for smaller functions (they report bigger differences than it really is) due to how profilers work (they need to set instrumentation for each function). We'd need before/after comparison with production setup (no profiler, env variables set to production, etc.) to really gauge the impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the awareness of the effort. I learned that we can use the ??
operator now that the minimum PHP version is 7 from the discussion. In many places it looks like the code is more readable and more performant than _wp_array_get
, and I'll definitely be using that in the future.
However, I'm skeptical of the performance benefit in this particular case considering the alternative is less readable and more prone to errors. Judging from the discussion on WordPress/gutenberg#51116, They were seeing a ~1-2ms improvement (when taking into account the profiler overhead) from replacing more than 2000 _wp_array_get
calls (WordPress/gutenberg#51116 (comment)).
I think it makes sense to use the null coalescing operator when everything is on one line, but the path here is used in multiple different places, so I'll probably leave this one as-is during my refactors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, thanks for the follow up discussion!
if ( ! isset( $theme_settings['settings']['spacing'] ) ) { | ||
$theme_settings['settings']['spacing'] = array(); | ||
} | ||
$theme_settings['settings']['spacing']['spacingSizes'] = $settings['spacingSizes']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with above, you might get away with setting the value on the array you wish to create, e.g., only $theme_settings['settings']['spacing']['spacingSizes'] = $settings['spacingSizes'];
* | ||
* @return null|void | ||
*/ | ||
public function set_spacing_sizes() { | ||
_deprecated_function( __METHOD__, '6.6.0' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a $replacement
method that users should prefer? It can be in the third argument.
https://developer.wordpress.org/reference/functions/_deprecated_function/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to follow-up with a fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no replacement. This is being called automatically in the constructor and merge now. That being said, I seem to have missed some comments in the backport that explained that. I'll add them in a follow-up.
! isset( $spacing_scale['operator'] ) || | ||
( '+' !== $spacing_scale['operator'] && '*' !== $spacing_scale['operator'] ) || | ||
! isset( $spacing_scale['increment'] ) || | ||
! is_numeric( $spacing_scale['increment'] ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain melted slightly... 😆
Just checking, this has test coverage? It might help folks understand what's going on and also confirm the intentions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/*
* This condition is intentionally missing some checks on ranges for the values in order to
* keep backwards compatibility with the previous implementation.
*/
I tried to get rid of it or at least fix the missing checks, but that would have required a much bigger refactor and API design work. This was already landing rather late in the release cycle, so I opted to keep spacingScale
working the same and just copy the condition from above https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-e12c3008d747d1bec5be9927c5e7b1ced0a88641abe52c278d495da936714817R3792-R3814 without the trigger_error
(which seemed important to keep the public function working the same).
Triggering the error didn't make sense when this happened automatically in the constructor. Partial settings are valid for the theme
and custom
origins https://github.com/wordpress/wordpress-develop/blob/d088e31c73456179502c9bd5354fc43c6267bd7a/src/wp-includes/class-wp-theme-json-resolver.php#L142. It wasn't a problem before because merging the settings happened before set_spacing_sizes
was called.
Just checking, this has test coverage?
The code already had test coverage for each of the checks https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-5b485fe0725d5e02bb5aa976681c29e20b164c67c96250c6144b4b119a067eedR4754, but it didn't have tests for the invalid use cases that didn't trigger an error like "steps": 20
only producing 15 steps. I did add checks in the theme.json JSON schema so it'll at least get flagged there now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the work on this @ajlende
I'm coming at it pretty fresh mind you, but manual testing is working for me in the block and classic themes I tested. 👍🏻
I left some questions. It would be good to get another round of eyes from a Core committer, or someone who is more familiar with the feature.
Tentatively it's looking good to me.
c721b50
to
6b6d2b6
Compare
See #6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. git-svn-id: https://develop.svn.wordpress.org/trunk@58328 602fd350-edb4-49c9-b593-d223f7449a82
See WordPress/wordpress-develop#6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58328 git-svn-id: http://core.svn.wordpress.org/trunk@57785 1a063a9b-81f0-0310-95a4-ce76da25c4cd
See WordPress/wordpress-develop#6616. See also the original Gutenberg PRs: * WordPress/gutenberg#58409 * WordPress/gutenberg#61328 * WordPress/gutenberg#61842 * WordPress/gutenberg#62199 * WordPress/gutenberg#62252 Fixes #61282. Props ajlende, talldanwp, ramonopoly, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58328 git-svn-id: https://core.svn.wordpress.org/trunk@57785 1a063a9b-81f0-0310-95a4-ce76da25c4cd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished the review, but wanted to share what I've seen so far:
- The general direction is ok.
- There's tests for the gist of the changes (v2 to v3 migration).
- One of the tests I've done was to switch back every input dataset to v2 and see the test report, as we don't want to require any change to consumers that still use the v2 version. TLDR: test work as expected. If the theme was a v2, what happened is that the migration added
defaultFontSizes: false
, which is how it works because we changed the prevent override logic to default totrue
— hence this addition in the migration.
@@ -586,7 +618,6 @@ public static function get_merged_data( $origin = 'custom' ) { | |||
$result = new WP_Theme_JSON(); | |||
$result->merge( static::get_core_data() ); | |||
if ( 'default' === $origin ) { | |||
$result->set_spacing_sizes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice change :)
$config = $theme_json->get_data(); | ||
|
||
// Needs to be set for schema migrations of user data. | ||
$config['isGlobalStylesUserThemeJSON'] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the things I haven't been able to test, and want to leave a note to clarifying it. @ajlende can you provide reproduction steps for this? How does it fail without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this also changed recently at #6271 to remove the extra-parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used during the migration to prevent adding "defaultFontSizes": false
and "defaultSpacingSizes: false
to empty user data since those values aren't configurable by the user in the global styles UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also responded on the Gutenberg PR for #6271. See WordPress/gutenberg#61262 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved the follow-up you prepared WordPress/gutenberg#62305 It's a nice one, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backport for that follow-up at #6737
@oandregal I went back and forth in the Gutenberg PR for this if I should fix all the tests that were testing the exact output of the theme.json instead of the specific thing they should have been testing or just update the theme.json version to latest everywhere. The later seemed easier because I could do it with a simple find and replace, so that's what I went with. Would have you preferred the former? I can still update things in the follow-up. |
Thanks again for the help on reviewing folks and for all the work @ajlende Great that this has landed overnight! |
Backport of the following Gutenberg PRs
default-spacing-sizes
anddefault-font-sizes
options for classic themes gutenberg#62252Trac ticket: https://core.trac.wordpress.org/ticket/61282
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.