-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Theme Export: If the theme declares a version number then add a schema #39775
Conversation
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 works well for me when I export a theme!
I've just left one comment about the schema URL.
if ( $theme_json_raw['version'] && in_array( $theme_json_raw['version'], array( "1", "2" ) ) ) { | ||
// Put $schema at the start of the array. | ||
$theme_json_raw = array_reverse( $theme_json_raw ); | ||
$theme_json_raw['$schema'] = "https://json.schemastore.org/theme-v1.json"; |
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.
Would it be better to use this URL: https://schemas.wp.org/trunk/theme.json?
I think this one is always kept up to date with Gutenberg, based on this post.
@@ -99,10 +99,18 @@ function gutenberg_generate_block_templates_export_file() { | |||
// Load theme.json into the zip file. | |||
$tree = WP_Theme_JSON_Resolver_Gutenberg::get_theme_data(); | |||
$tree->merge( WP_Theme_JSON_Resolver_Gutenberg::get_user_data() ); | |||
$theme_json_raw = $tree->get_data(); | |||
// If a version is defined, add a schema. | |||
if ( $theme_json_raw['version'] && in_array( $theme_json_raw['version'], array( "1", "2" ) ) ) { |
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 issue with checking for 1
or 2
is that when the version is upgraded this code needs to as well. Given that worst-case scenario is that the schema is not yet published, hence is ignored by editors, can we use what's in version
? version
is going to be always the latest, and, except for perhaps the dates around a major WordPress release it's going to be already published in the schema store.
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.
Hmmm, but https://json.schemastore.org/theme-v2.json doesn't exist?
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.
Oh, interesting. I just saw Sarah's comment above #39775 (comment) pointing to https://make.wordpress.org/themes/2021/11/30/theme-json-schema/
According to that, it sounds like we need to use different URL depending on whether we run in the plugin or core. I'll ping @ajlende for confirmation but it seems that we need:
https://schemas.wp.org/trunk/theme.json
in the pluginhttps://schemas.wp.org/wp/<version>/theme.json
in core
We need to do something to remember doing the change when this code is ported to core. Perhaps a comment would sufice.
1ee77d1
to
29b0e66
Compare
I updated the approach here to use the URLs above, including the |
There is an edge case, and I wouldn't recommend it, but if for any reason, the active theme has set version: 1 and schema to 5.8, then the export changes this to trunk and version 2:
(as copied from https://make.wordpress.org/themes/2021/11/30/theme-json-schema/) Becomes:
|
$theme_json_version = 'trunk'; | ||
} | ||
$theme_json_raw = array_reverse( $theme_json_raw ); | ||
$theme_json_raw['$schema'] = 'https://schemas.wp.org/wp/' . $theme_json_version . '/theme.json'; |
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.
Can't $schema
already exist? if it's added manually we should respect that.
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 not stored in the tree - we could get it from the file directly, which is the approach I took at first (#39590) but it seems like the recommended approach is to use the data from the class instance rather than getting it from the file, so it's properly sanitised etc.
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.
Should WP_Theme_JSON_Schema_Gutenberg
handle the URL of the schema, since this class is in charge with migrating existing JSON to new versions, then it is the ultimate source of truth for what schema is in use.
cc @oandregal
2d61f24
to
7586a4c
Compare
7586a4c
to
c46de20
Compare
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 this is a good approach considering we want this to only serve as a dev ex improvement.
I wonder if theme.json
will be exposed via REST maybe the schema will become handy then? The fact that if I define a schema and version in my theme.json
they'll be overwritten on export, doesn't seem to count for anything, since we do
$this->theme_json = WP_Theme_JSON_Schema_Gutenberg::migrate( $theme_json );
right in WP_Theme_JSON
's constructor. Also, on export, the schema is taken from the current environment, as if it is a custom
property and
The custom's has higher priority than the theme's, and the theme's higher than defaults's.
Whatever we think of doing in the future, I can't see this addition blocking it.
if ( defined( 'IS_GUTENBERG_PLUGIN' ) ) { | ||
$theme_json_version = 'trunk'; | ||
} | ||
$schema = array( '$schema' => 'https://schemas.wp.org/wp/' . $theme_json_version . '/theme.json' ); |
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.
Considering the convo where we talked about this, I've noticed that this code returns:
https://schemas.wp.org/wp/trunk/theme.json
in the Gutenberg plugin: shouldn't it returnhttps://schemas.wp.org/trunk/theme.json
instead?- It contains the patch version as in:
https://schemas.wp.org/wp/5.9.3/theme.json
when running on WordPress versions with patch versions (all of them but 6.0). Shouldn't it returnhttps://schemas.wp.org/wp/5.9/theme.json
instead? Note that the Gutenberg branches that host the schema don't have the minor: 5.9, 5.8.
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 that. There's a fix here: #40106
What?
Add a $schema key to theme.json if it declares a version.
Why?
If we know the version of the theme.json then we can add a $schema to improve developer experience.
Fixes #39575
How?
We detect the version of the theme.json and then add a $schema attribute if it's present. In the future we'll need to change the schema depending on the version.
We do
array_reverse
on the data so the $schema gets added to the start of the theme.json file, not the end.Testing Instructions
Screenshots or screencast
cc @WordPress/block-themers