Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
668c31e
4c02184
a4e24a0
4696b35
97a24e0
719a4bd
9a0e4b4
915f254
2f9b431
3769e79
dc57d38
943d9bb
0a33eb1
35695f2
7599a7f
ca161da
caaeb72
6569a11
d073251
93e3c82
ea23bf0
afe235e
1b583a1
1e93db4
c8cdcc1
5214e4f
6b6d2b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 forWP_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.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
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.See https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-7bab66e2c26e312df49f13ff601b4ab55601e876c21abdcf21cd884def623226R112-R121.
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
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 :)
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.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 withexists. The second doesn't seem to need it. I can add some additional description if that's helpful.// 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.
Might be missing something, but this seems unnecessary considering
$new = $old
, and there's already a test forisset( $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)
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:
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.