theme.json: add util to transfrom from a v0 schema to the latest #30600
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
added
the
Global Styles
|
Size Change: +1.43 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Apr 8, 2021
| * | ||
| * @return array The new theme.json in the v1 format. | ||
| */ | ||
| public static function to_v1( $old ) { |
There was a problem hiding this comment.
Should this method be named something like "toLatest" or "toNext" depending on whether we think the migration will be done on one step by the framework or that it goes through all the previous versions?
There was a problem hiding this comment.
I actually wonder whether it should be more just something like parse and the idea is that all versions including the latest should implement it (from raw config to the parsed one understood by the WP framework).
in the latest version it could be as simple as just returning the array itself.
There was a problem hiding this comment.
I went back and forth between the two options (have all schemas migrate to the latest version or a particular schema only migrates to the next). Here are the trade-offs I considered:
Data. In this migration, we are removing the distinction between defaults and root which had some advantages #28533 (comment) and it's now merged into one. What if v2 wanted to introduce back the idea of "defaults"?
- With schemas that migrate to the last version: we can do it as the v0 schema still have access to defaults.
- With schemas that migrate to the next version: we can't because v1 no longer has the defaults.
Introducing changes. How do we introduce changes from the plugin when this is in core?
- With schemas that migrate to the last version: every schema has to have a hook and the plugin needs to hook into all of them to change how they operate (in addition to the hooks necessary for the operations themselves: merge, get_stylesheet, etc).
- With schemas that migrate to the next version: we only need one hook (in the schema loader) that allows migrating the latest version to the desired changes.
An approach in which schemas migrate to the next version seems simpler. I'm not sure the case I've shared for data is one we'd encounter. That's my rationale to opt for this approach but, definitely, the alternative is viable as well (and if we go that route, yes, what you mentioned at #30600 (comment) and #30600 (review) makes sense to me).
There was a problem hiding this comment.
I have a preference for 1 (last version) personally. I agree it requires us to write a bit more code when adding new versions but we can actually call the next "parse" inside the previous one which mean we have the flexibility of both: chaining or not.
- if for instance v2 need to restore defaults, v0 can just return its values without calling v1
- if v2 is just a new set of transformations, v0 can transform to v1 and then explicitly call the v1 parse function itself.
The other advantage is that we can do optimizations as well and avoid looping multiple times if something is costful.
youknowriad
reviewed
Apr 8, 2021
| * Class that encapsulates the processing of | ||
| * structures that adhere to the theme.json V0. | ||
| */ | ||
| class WP_Theme_JSON_Schema_V0 { |
There was a problem hiding this comment.
I guess ideally, this should implement an "interface" that way new deprecated schemas would know what they have to implement.
youknowriad
reviewed
Apr 8, 2021
There was a problem hiding this comment.
Do you think we should implement the "loader" as part of this PR or should it be separate. The idea I have in mind is that its code could be something like that:
$theme_json_schemas = array(
'0' => 'WP_Theme_JSON_Schema_V0'
);
$theme_json_content = json_decode( file_get_contents() );
$version = isset($theme_json_content['api_version'])?$theme_json_content['api_version'] : '0';
$themeJsonSchemaFactory = $theme_json_schemas[ $version ];
$parsed_settings = $$themeJsonSchemaFactory::parse($theme_json_content);
return $parsed_settings;
This is just pseudo code.
|
Pushed at 78fc146 the changes for this to follow the |
oandregal
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Extracted from #30541 so it can land in pieces and speed up reviews.
This is a utility to support schema migration for theme.json from v0 to v1.
How to test
npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-schema-v0-test.php