Refactor theme.json migrations to be a single class #36182
Merged
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
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
oandregal
added
[Type] Code Quality
oandregal
requested review from
jorgefilipecosta,
youknowriad and
aaronrobertshaw
oandregal
commented
Nov 3, 2021
| * | ||
| * @return array The structure in the last version. | ||
| */ | ||
| private static function migrate( $theme_json ) { |
There was a problem hiding this comment.
This method has been moved to WP_Theme_JSON_Schema::migrate verbatim, see.
oandregal
commented
Nov 3, 2021
| * | ||
| * @return array Data without the custom prefixes. | ||
| */ | ||
| public static function migrate( $old ) { |
There was a problem hiding this comment.
This is now WP_Theme_JSON_Schema::migrate_v1_remove_custom_prefixes.
oandregal
commented
Nov 3, 2021
| * | ||
| * @return array Data without the custom prefixes. | ||
| */ | ||
| public static function migrate( $old ) { |
There was a problem hiding this comment.
This is now WP_Theme_JSON_Schema::migrate_v1_to_v2.
There was a problem hiding this comment.
Thanks for putting this together 👍
Both v1 and v2 schema versions test well however I encountered a few small issues while testing v0 schemas.
There are a couple of references to ALL_BLOCKS_NAME and ROOT_BLOCK_NAME that still need to be updated.
Here's a patch to update those references.
diff --git a/lib/class-wp-theme-json-schema.php b/lib/class-wp-theme-json-schema.php
index a21547401b..e7dc4a96dd 100644
--- a/lib/class-wp-theme-json-schema.php
+++ b/lib/class-wp-theme-json-schema.php
@@ -196,14 +196,14 @@ class WP_Theme_JSON_Schema {
);
// 'defaults' settings become top-level.
- if ( isset( $settings[ self::ALL_BLOCKS_NAME ] ) ) {
- $new = $settings[ self::ALL_BLOCKS_NAME ];
- unset( $settings[ self::ALL_BLOCKS_NAME ] );
+ if ( isset( $settings[ self::V0_ALL_BLOCKS_NAME ] ) ) {
+ $new = $settings[ self::V0_ALL_BLOCKS_NAME ];
+ unset( $settings[ self::V0_ALL_BLOCKS_NAME ] );
}
// 'root' settings override 'defaults'.
- if ( isset( $settings[ self::ROOT_BLOCK_NAME ] ) ) {
- $new = array_replace_recursive( $new, $settings[ self::ROOT_BLOCK_NAME ] );
+ if ( isset( $settings[ self::V0_ROOT_BLOCK_NAME ] ) ) {
+ $new = array_replace_recursive( $new, $settings[ self::V0_ROOT_BLOCK_NAME ] );
// The array_replace_recursive algorithm merges at the leaf level.
// This means that when a leaf value is an array,
@@ -215,7 +215,7 @@ class WP_Theme_JSON_Schema {
foreach ( $paths_to_override as $path ) {
$root_value = _wp_array_get(
$settings,
- array_merge( array( self::ROOT_BLOCK_NAME ), $path ),
+ array_merge( array( self::V0_ROOT_BLOCK_NAME ), $path ),
null
);
if ( null !== $root_value ) {
@@ -223,7 +223,7 @@ class WP_Theme_JSON_Schema {
}
}
- unset( $settings[ self::ROOT_BLOCK_NAME ] );
+ unset( $settings[ self::V0_ROOT_BLOCK_NAME ] );
}
if ( empty( $settings ) ) {
@@ -298,9 +298,9 @@ class WP_Theme_JSON_Schema {
);
// Styles within root become top-level.
- if ( isset( $styles[ self::ROOT_BLOCK_NAME ] ) ) {
- $new = $styles[ self::ROOT_BLOCK_NAME ];
- unset( $styles[ self::ROOT_BLOCK_NAME ] );
+ if ( isset( $styles[ self::V0_ROOT_BLOCK_NAME ] ) ) {
+ $new = $styles[ self::V0_ROOT_BLOCK_NAME ];
+ unset( $styles[ self::V0_ROOT_BLOCK_NAME ] );
// Transform root.styles.color.link into elements.link.color.text.
if ( isset( $new['color']['link'] ) ) {Also, the schema tests fail.
npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-wp-theme-json-schema-test.php
Within class-wp-theme-json-schema-test.php there are a couple of references to the old schema classes e.g. WP_Theme_JSON_Schema_V1_Remove_Custom_Prefixes.
More importantly though, the tests attempt to assert the results from individual migration steps where they are now contained within private functions on the WP_Theme_JSON_Schema class.
As I see it, we could either:
- Make the individual migration functions public so they can be tested in isolation
- Update the tests to assert each version successfully migrates all the way to the latest version
I'll leave which approach is best up to you. In case it saves you some time I'll include a patch below getting the tests to pass while testing the complete migration of each version (this isn't polished).
Patch to update tests.
diff --git a/phpunit/class-wp-theme-json-schema-test.php b/phpunit/class-wp-theme-json-schema-test.php
index f6e0b245fa..e0668ece20 100644
--- a/phpunit/class-wp-theme-json-schema-test.php
+++ b/phpunit/class-wp-theme-json-schema-test.php
@@ -7,6 +7,10 @@
*/
class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
+ /**
+ * The current theme.json schema version.
+ */
+ const LATEST_SCHEMA_VERSION = 2;
function test_migrate_v0_to_v1() {
$theme_json_v0 = array(
@@ -98,7 +102,7 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
$actual = WP_Theme_JSON_Schema::migrate( $theme_json_v0 );
$expected = array(
- 'version' => 1,
+ 'version' => self::LATEST_SCHEMA_VERSION,
'settings' => array(
'color' => array(
'palette' => array(
@@ -117,16 +121,16 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
'link' => true,
),
'border' => array(
- 'customColor' => false,
- 'customRadius' => false,
- 'customStyle' => false,
- 'customWidth' => false,
+ 'color' => false,
+ 'radius' => false,
+ 'style' => false,
+ 'width' => false,
),
'typography' => array(
- 'customFontStyle' => false,
- 'customFontWeight' => false,
- 'customTextDecorations' => false,
- 'customTextTransforms' => false,
+ 'fontStyle' => false,
+ 'fontWeight' => false,
+ 'textDecoration' => false,
+ 'textTransform' => false,
),
'blocks' => array(
'core/paragraph' => array(
@@ -582,10 +586,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
),
);
- $actual = WP_Theme_JSON_Schema_V1_Remove_Custom_Prefixes::migrate( $theme_json_v1 );
+ $actual = WP_Theme_JSON_Schema::migrate( $theme_json_v1 );
$expected = array(
- 'version' => 1,
+ 'version' => self::LATEST_SCHEMA_VERSION,
'settings' => array(
'color' => array(
'palette' => array(
@@ -604,10 +608,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
'link' => true,
),
'border' => array(
- 'color' => false,
- 'customRadius' => false,
- 'style' => false,
- 'width' => false,
+ 'color' => false,
+ 'radius' => false,
+ 'style' => false,
+ 'width' => false,
),
'typography' => array(
'fontStyle' => false,
@@ -619,10 +623,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
'blocks' => array(
'core/group' => array(
'border' => array(
- 'color' => true,
- 'customRadius' => true,
- 'style' => true,
- 'width' => true,
+ 'color' => true,
+ 'radius' => true,
+ 'style' => true,
+ 'width' => true,
),
'typography' => array(
'fontStyle' => true,
@@ -754,10 +758,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
),
);
- $actual = WP_Theme_JSON_Schema_V1_Remove_Custom_Prefixes::migrate( $theme_json_v1 );
+ $actual = WP_Theme_JSON_Schema::migrate( $theme_json_v1 );
$expected = array(
- 'version' => 1,
+ 'version' => self::LATEST_SCHEMA_VERSION,
'settings' => array(
'color' => array(
'palette' => array(
@@ -776,10 +780,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
'link' => true,
),
'border' => array(
- 'color' => false,
- 'customRadius' => false,
- 'style' => false,
- 'width' => false,
+ 'color' => false,
+ 'radius' => false,
+ 'style' => false,
+ 'width' => false,
),
'typography' => array(
'fontStyle' => false,
@@ -791,10 +795,10 @@ class WP_Theme_JSON_Schema_Test extends WP_UnitTestCase {
'blocks' => array(
'core/group' => array(
'border' => array(
- 'color' => true,
- 'customRadius' => true,
- 'style' => true,
- 'width' => true,
+ 'color' => true,
+ 'radius' => true,
+ 'style' => true,
+ 'width' => true,
),
'typography' => array(
'fontStyle' => true,
oandregal
force-pushed
the
refactor/theme-json-migrations
branch
from
0e68b51 to
4057b3f
Compare
|
@aaronrobertshaw thanks for the review and patches! |
aaronrobertshaw
approved these changes
Nov 4, 2021
There was a problem hiding this comment.
LGTM 🚀
✅ Tests for schema migrations are passing
✅ Using a V0 schema works and is shown as correctly migrated in state
✅ Using a V1 schema works and is shown as correctly migrated in state
✅ Using a V2 schema works and is shown as correctly migrated in state
✅ All references to ALL_BLOCKS_NAME and ROOT_BLOCKS_NAME have been updated
✅ Gutenberg suffix has been added to the schema class
Nice work as always @oandregal 👍
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.
Follow-up to #36155
This PR refactors the theme.json migrations to be contained in a single file. Instead of having a class per migration we'll have method per migration.
Test theme.json v1 still works
Using the following theme.json (for example, with the empty theme):
{ "version": 1, "settings": { "spacing": { "customPadding": false, "customMargin": false }, "typography": { "customLineHeight": false }, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "blocks": { "core/button": { "border": { "customRadius": false } } } } }Verify that:
Set the above properties to
trueand verify that the UI controls are enabled.Test theme.json v2
Using the following theme.json (for example, with the empty theme):
{ "version": 2, "settings": { "spacing": { "padding": false, "margin": false }, "typography": { "lineHeight": false }, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "blocks": { "core/button": { "border": { "radius": false } } } } }Verify that:
Set the above properties to
trueand verify that the UI controls are enabled.