Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
668c31e
Backport 58409 class-wp-theme-json.php
ajlende Jun 3, 2024
4c02184
Backport 58409 class-wp-theme-json-schema.php
ajlende May 23, 2024
a4e24a0
Backport 58409 class-wp-theme-json-data.php
ajlende May 23, 2024
4696b35
Backport 58409 class-wp-theme-json-resolver.php
ajlende May 23, 2024
97a24e0
Backport 58409 theme.json
ajlende May 23, 2024
719a4bd
Backport 58409 PHPUnit wpThemeJsonSchema.php
ajlende May 23, 2024
9a0e4b4
Backport 58409 PHPUnit wpThemeJson.php
ajlende May 23, 2024
915f254
Backport 58409 PHPUnit rest-global-styles-controller.php
ajlende May 23, 2024
2f9b431
Backport 58409 PHPUnit data
ajlende May 23, 2024
3769e79
Backport 61842 class-wp-theme-json.php
ajlende May 23, 2024
dc57d38
Backport 61842 class-wp-theme-json-resolver.php
ajlende May 23, 2024
943d9bb
Backport 61842 class-wp-theme-json-schema.php
ajlende May 23, 2024
0a33eb1
Backport 61842 theme.json
ajlende May 23, 2024
35695f2
Backport 61842 PHPUnit wpThemeJsonSchema.php
ajlende May 23, 2024
7599a7f
Backport 61842 PHPUnit wpThemeJson.php
ajlende May 23, 2024
ca161da
Merge 6.6 `@since` tags
ajlende Jun 3, 2024
caaeb72
Update LATEST_THEME_JSON_VERSION_SUPPORTED in font-faces controller
ajlende Jun 3, 2024
6569a11
Add default-font-sizes options for classic themes
ajlende Jun 3, 2024
d073251
Add default-spacing-sizes options for classic themes
ajlende Jun 3, 2024
93e3c82
Add editor-spacing-sizes for consistency
ajlende Jun 3, 2024
ea23bf0
Update LATEST_THEME_JSON_VERSION_SUPPORTED in font-families controller
ajlende Jun 3, 2024
afe235e
Fix phpunit tests
ajlende Jun 3, 2024
1b583a1
Update wp-api-generated
ajlende Jun 3, 2024
1e93db4
Added 6.6.0 doc comments
ajlende Jun 3, 2024
c8cdcc1
register_theme_feature( 'editor-spacing-sizes' )
ajlende Jun 4, 2024
5214e4f
Copy spacingSizes from editor-spacing-sizes
ajlende Jun 4, 2024
6b6d2b6
Fix tests
ajlende Jun 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/wp-includes/block-editor.php
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ function get_block_editor_theme_styles() {
* Returns the classic theme supports settings for block editor.
*
* @since 6.2.0
* @since 6.6.0 Add support for 'editor-spacing-sizes' theme support.
*
* @return array The classic theme supports settings.
*/
Expand Down Expand Up @@ -844,5 +845,10 @@ function get_classic_theme_supports_block_editor_settings() {
$theme_settings['gradients'] = $gradient_presets;
}

$spacing_sizes = current( (array) get_theme_support( 'editor-spacing-sizes' ) );
if ( false !== $spacing_sizes ) {
$theme_settings['spacingSizes'] = $spacing_sizes;
}
ajlende marked this conversation as resolved.
Show resolved Hide resolved

return $theme_settings;
}
2 changes: 1 addition & 1 deletion src/wp-includes/class-wp-theme-json-data.php
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) {
Copy link
Member

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?

Copy link
Member

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?

Copy link
Author

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.

$this->origin = $origin;
$this->theme_json = new WP_Theme_JSON( $data, $this->origin );
}
Expand Down
45 changes: 37 additions & 8 deletions src/wp-includes/class-wp-theme-json-resolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ protected static function has_same_registered_blocks( $origin ) {
* @since 5.8.0
* @since 5.9.0 Theme supports have been inlined and the `$theme_support_data` argument removed.
* @since 6.0.0 Added an `$options` parameter to allow the theme data to be returned without theme supports.
* @since 6.6.0 Add support for 'default-font-sizes' and 'default-spacing-sizes' theme supports.
*
* @param array $deprecated Deprecated. Not used.
* @param array $options {
Expand All @@ -243,7 +244,7 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$theme_json_data = static::read_json_file( $theme_json_file );
$theme_json_data = static::translate( $theme_json_data, $wp_theme->get( 'TextDomain' ) );
} else {
$theme_json_data = array();
$theme_json_data = array( 'version' => WP_Theme_JSON::LATEST_SCHEMA );
}

/**
Expand Down Expand Up @@ -310,6 +311,32 @@ public static function get_theme_data( $deprecated = array(), $options = array()
}
$theme_support_data['settings']['color']['defaultGradients'] = $default_gradients;

if ( ! isset( $theme_support_data['settings']['typography'] ) ) {
$theme_support_data['settings']['typography'] = array();
}
$default_font_sizes = false;
if ( current_theme_supports( 'default-font-sizes' ) ) {
ajlende marked this conversation as resolved.
Show resolved Hide resolved
$default_font_sizes = true;
}
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;
Copy link
Member

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... 😄

Copy link
Author

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

}
$theme_support_data['settings']['typography']['defaultFontSizes'] = $default_font_sizes;

if ( ! isset( $theme_support_data['settings']['spacing'] ) ) {
$theme_support_data['settings']['spacing'] = array();
}
$default_spacing_sizes = false;
if ( current_theme_supports( 'default-spacing-sizes' ) ) {
$default_spacing_sizes = true;
}
if ( ! isset( $theme_support_data['settings']['spacing']['spacingSizes'] ) ) {
// If the theme does not have any spacing sizes, we still want to show the core one.
$default_spacing_sizes = true;
}
$theme_support_data['settings']['spacing']['defaultSpacingSizes'] = $default_spacing_sizes;

if ( ! isset( $theme_support_data['settings']['shadow'] ) ) {
$theme_support_data['settings']['shadow'] = array();
}
Expand Down Expand Up @@ -359,7 +386,7 @@ public static function get_block_data() {
return static::$blocks;
}

$config = array( 'version' => 2 );
$config = array( 'version' => WP_Theme_JSON::LATEST_SCHEMA );
foreach ( $blocks as $block_name => $block_type ) {
if ( isset( $block_type->supports['__experimentalStyle'] ) ) {
$config['styles']['blocks'][ $block_name ] = static::remove_json_comments( $block_type->supports['__experimentalStyle'] );
Expand Down Expand Up @@ -494,6 +521,7 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
* Returns the user's origin config.
*
* @since 5.9.0
* @since 6.6.0 The 'isGlobalStylesUserThemeJSON' flag is left on the user data.
*
* @return WP_Theme_JSON Entity that holds styles for user data.
*/
Expand Down Expand Up @@ -531,14 +559,18 @@ public static function get_user_data() {
isset( $decoded_data['isGlobalStylesUserThemeJSON'] ) &&
$decoded_data['isGlobalStylesUserThemeJSON']
) {
unset( $decoded_data['isGlobalStylesUserThemeJSON'] );
$config = $decoded_data;
}
}

/** This filter is documented in wp-includes/class-wp-theme-json-resolver.php */
$theme_json = apply_filters( 'wp_theme_json_data_user', new WP_Theme_JSON_Data( $config, 'custom' ) );
static::$user = $theme_json->get_theme_json();
$theme_json = apply_filters( 'wp_theme_json_data_user', new WP_Theme_JSON_Data( $config, 'custom' ) );
$config = $theme_json->get_data();

// Needs to be set for schema migrations of user data.
$config['isGlobalStylesUserThemeJSON'] = true;
Copy link
Member

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?

Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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).

Copy link
Member

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!

Copy link
Member

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


static::$user = new WP_Theme_JSON( $config, 'custom' );

return static::$user;
}
Expand Down Expand Up @@ -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();
Copy link
Member

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 :)

return $result;
}

Expand All @@ -597,12 +628,10 @@ public static function get_merged_data( $origin = 'custom' ) {

$result->merge( static::get_theme_data() );
if ( 'theme' === $origin ) {
$result->set_spacing_sizes();
return $result;
}

$result->merge( static::get_user_data() );
$result->set_spacing_sizes();

return $result;
}
Expand Down
93 changes: 91 additions & 2 deletions src/wp-includes/class-wp-theme-json-schema.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ class WP_Theme_JSON_Schema {
* Function that migrates a given theme.json structure to the last version.
*
* @since 5.9.0
* @since 6.6.0 Migrate up to v3.
*
* @param array $theme_json The structure to migrate.
*
Expand All @@ -47,8 +48,14 @@ public static function migrate( $theme_json ) {
);
}

if ( 1 === $theme_json['version'] ) {
$theme_json = self::migrate_v1_to_v2( $theme_json );
// 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 );
// no break
Copy link
Member

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.

Copy link
Contributor

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 😞

Copy link
Member

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. 😄

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?

Copy link
Contributor

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).

Copy link
Member

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?

Copy link
Author

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.

Copy link
Author

@ajlende ajlende Jun 4, 2024

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.

Copy link
Author

@ajlende ajlende Jun 4, 2024

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 // no break exists. The second doesn't seem to need it. I can add some additional description if that's helpful.

		// 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 );
		}

case 2:
$theme_json = self::migrate_v2_to_v3( $theme_json );
// no break
}

return $theme_json;
Expand Down Expand Up @@ -84,6 +91,88 @@ private static function migrate_v1_to_v2( $old ) {
return $new;
}

/**
* Migrates from v2 to v3.
*
* - Sets settings.typography.defaultFontSizes to false.
*
* @since 6.6.0
*
* @param array $old Data to migrate.
*
* @return array Data with defaultFontSizes set to false.
*/
private static function migrate_v2_to_v3( $old ) {
// Copy everything.
$new = $old;

// Set the new version.
$new['version'] = 3;

/*
* Remaining changes do not need to be applied to the custom origin,
* as they should take on the value of the theme origin.
*/
if (
isset( $new['isGlobalStylesUserThemeJSON'] ) &&
true === $new['isGlobalStylesUserThemeJSON']
) {
return $new;
}

/*
* Even though defaultFontSizes and defaultSpacingSizes are new
* settings, we need to migrate them as they each control
* PRESETS_METADATA prevent_override values which were previously
* hardcoded to false. This only needs to happen when the theme provides
* fontSizes or spacingSizes as they could match the default ones and
* affect the generated CSS.
*/
if ( isset( $old['settings']['typography']['fontSizes'] ) ) {
if ( ! isset( $new['settings'] ) ) {
$new['settings'] = array();
}
if ( ! isset( $new['settings']['typography'] ) ) {
$new['settings']['typography'] = array();
}
Comment on lines +132 to +137
Copy link
Contributor

@talldan talldan Jun 4, 2024

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)

$new['settings']['typography']['defaultFontSizes'] = false;
}

/*
* Similarly to defaultFontSizes, we need to migrate defaultSpacingSizes
* as it controls the PRESETS_METADATA prevent_override which was
* previously hardcoded to false. This only needs to happen when the
* theme provided spacing sizes via spacingSizes or spacingScale.
*/
if (
isset( $old['settings']['spacing']['spacingSizes'] ) ||
isset( $old['settings']['spacing']['spacingScale'] )
) {
if ( ! isset( $new['settings'] ) ) {
$new['settings'] = array();
}
if ( ! isset( $new['settings']['spacing'] ) ) {
$new['settings']['spacing'] = array();
}
$new['settings']['spacing']['defaultSpacingSizes'] = false;
Copy link
Member

@ramonjd ramonjd Jun 4, 2024

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)
    }
  }
}*/

Copy link
Member

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.

}

/*
* In v3 spacingSizes is merged with the generated spacingScale sizes
* instead of completely replacing them. The v3 behavior is what was
* documented for the v2 schema, but the code never actually did work
* that way. Instead of surprising users with a behavior change two
* years after the fact at the same time as a v3 update is introduced,
* we'll continue using the "bugged" behavior for v2 themes. And treat
* the "bug fix" as a breaking change for v3.
*/
if ( isset( $old['settings']['spacing']['spacingSizes'] ) ) {
unset( $new['settings']['spacing']['spacingScale'] );
}

return $new;
}

/**
* Processes the settings subtree.
*
Expand Down
Loading
Loading