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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Backport 61842 class-wp-theme-json.php
ajlende authored and ellatrix committed Jun 4, 2024
commit 3769e79c6415462cacf894b738ffbedd5c260451
187 changes: 169 additions & 18 deletions src/wp-includes/class-wp-theme-json.php
Original file line number Diff line number Diff line change
@@ -125,6 +125,7 @@ class WP_Theme_JSON {
* @since 6.3.0 Replaced value_func for duotone with `null`. Custom properties are handled by class-wp-duotone.php.
* @since 6.6.0 Added the `dimensions.aspectRatios` & `dimensions.defaultAspectRatios` preset.
* @since 6.6.0 Updated the 'prevent_override' value for font size presets to use 'typography.defaultFontSizes'.
* @since 6.6.0 Updated the 'prevent_override' value for spacing size presets to use `spacing.defaultSpacingSizes`.
* @var array
*/
const PRESETS_METADATA = array(
@@ -188,7 +189,7 @@ class WP_Theme_JSON {
),
array(
'path' => array( 'spacing', 'spacingSizes' ),
'prevent_override' => false,
'prevent_override' => array( 'spacing', 'defaultSpacingSizes' ),
'use_default_names' => true,
'value_key' => 'size',
'css_vars' => '--wp--preset--spacing--$slug',
@@ -381,6 +382,7 @@ class WP_Theme_JSON {
* `background.backgroundSize` and `dimensions.aspectRatio`.
* @since 6.6.0 Added support for `dimensions.aspectRatios` and `dimensions.defaultAspectRatios`.
* @since 6.6.0 Added support for `typography.defaultFontSizes`.
* @since 6.6.0 Added support for `spacing.defaultSpacingSizes`.
* @var array
*/
const VALID_SETTINGS = array(
@@ -435,13 +437,14 @@ class WP_Theme_JSON {
'sticky' => null,
),
'spacing' => array(
'customSpacingSize' => null,
'spacingSizes' => null,
'spacingScale' => null,
'blockGap' => null,
'margin' => null,
'padding' => null,
'units' => null,
'customSpacingSize' => null,
'defaultSpacingSizes' => null,
'spacingSizes' => null,
'spacingScale' => null,
'blockGap' => null,
'margin' => null,
'padding' => null,
'units' => null,
),
'shadow' => array(
'presets' => null,
@@ -740,6 +743,7 @@ public static function get_element_class_name( $element ) {
* Constructor.
*
* @since 5.8.0
* @since 6.6.0 Pre-generate the spacingSizes from spacingScale.
*
* @param array $theme_json A structure that follows the theme.json schema.
* @param string $origin Optional. What source of data this object represents.
@@ -754,8 +758,8 @@ public function __construct( $theme_json = array( 'version' => WP_Theme_JSON::LA
$valid_block_names = array_keys( static::get_blocks_metadata() );
$valid_element_names = array_keys( static::ELEMENTS );
$valid_variations = static::get_valid_block_style_variations();
$theme_json = static::sanitize( $this->theme_json, $valid_block_names, $valid_element_names, $valid_variations );
$this->theme_json = static::maybe_opt_in_into_settings( $theme_json );
$this->theme_json = static::sanitize( $this->theme_json, $valid_block_names, $valid_element_names, $valid_variations );
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.

I couldn't find anything in the Gutenberg PR about this change.

Any reason to set the class property $this->theme_json here?

If so, maybe it's worth mentioning in the annotation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets set again on the next line anyway so I wonder this is just a style/consistency thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there's no functional change here. If it's not ok, let's follow-up separately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At one point during development I was using the $theme_json argument to check the theme.json version and run code conditionally based on that. Turns out the way that we merge files in get_merged_data (https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR618-R636) means that it isn't a reliable way to check if we're starting with a v2 theme.json.

I didn't switch it back because the code is functionally the same, and I think it reads better this way.

$this->theme_json = static::maybe_opt_in_into_settings( $this->theme_json );

// Internally, presets are keyed by origin.
$nodes = static::get_setting_nodes( $this->theme_json );
@@ -774,6 +778,27 @@ public function __construct( $theme_json = array( 'version' => WP_Theme_JSON::LA
}
}
}

// In addition to presets, spacingScale (which generates presets) is also keyed by origin.
$scale_path = array( 'settings', 'spacing', 'spacingScale' );
$spacing_scale = _wp_array_get( $this->theme_json, $scale_path, null );
if ( null !== $spacing_scale ) {
// If the spacingScale is not already keyed by origin.
if ( empty( array_intersect( array_keys( $spacing_scale ), static::VALID_ORIGINS ) ) ) {
_wp_array_set( $this->theme_json, $scale_path, array( $origin => $spacing_scale ) );
}
}

// Pre-generate the spacingSizes from spacingScale.
$scale_path = array( 'settings', 'spacing', 'spacingScale', $origin );
$spacing_scale = _wp_array_get( $this->theme_json, $scale_path, null );
if ( isset( $spacing_scale ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

There was a recent effort to reduce the use of _wp_array_get

https://core.trac.wordpress.org/ticket/59405

More verbose, but could also be

if ( isset( $this->theme_json['settings']['spacing']['spacingScale'][ $origin ] ) ) {
 ....
$spacing_scale_sizes = static::compute_spacing_sizes( $this->theme_json['settings']['spacing']['spacingScale'][ $origin ] );
}

might save about 0.000002ms 😄

Not a huge deal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using _wp_array_get is perfectly valid, IMO. The before/after comparisons I've seen report profiler numbers which are skewed for smaller functions (they report bigger differences than it really is) due to how profilers work (they need to set instrumentation for each function). We'd need before/after comparison with production setup (no profiler, env variables set to production, etc.) to really gauge the impact.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the awareness of the effort. I learned that we can use the ?? operator now that the minimum PHP version is 7 from the discussion. In many places it looks like the code is more readable and more performant than _wp_array_get, and I'll definitely be using that in the future.

However, I'm skeptical of the performance benefit in this particular case considering the alternative is less readable and more prone to errors. Judging from the discussion on WordPress/gutenberg#51116, They were seeing a ~1-2ms improvement (when taking into account the profiler overhead) from replacing more than 2000 _wp_array_get calls (WordPress/gutenberg#51116 (comment)).

I think it makes sense to use the null coalescing operator when everything is on one line, but the path here is used in multiple different places, so I'll probably leave this one as-is during my refactors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, thanks for the follow up discussion!

$sizes_path = array( 'settings', 'spacing', 'spacingSizes', $origin );
$spacing_sizes = _wp_array_get( $this->theme_json, $sizes_path, array() );
$spacing_scale_sizes = static::compute_spacing_sizes( $spacing_scale );
$merged_spacing_sizes = static::merge_spacing_sizes( $spacing_scale_sizes, $spacing_sizes );
_wp_array_set( $this->theme_json, $sizes_path, $merged_spacing_sizes );
}
}

/**
@@ -2919,6 +2944,40 @@ public function merge( $incoming ) {
$incoming_data = $incoming->get_raw_data();
$this->theme_json = array_replace_recursive( $this->theme_json, $incoming_data );

/*
* Recompute all the spacing sizes based on the new hierarchy of data. In the constructor
* spacingScale and spacingSizes are both keyed by origin and VALID_ORIGINS is ordered, so
* we can allow partial spacingScale data to inherit missing data from earlier layers when
* computing the spacing sizes.
*
* This happens before the presets are merged to ensure that default spacing sizes can be
* removed from the theme origin if $prevent_override is true.
*/
$flattened_spacing_scale = array();
foreach ( static::VALID_ORIGINS as $origin ) {
$scale_path = array( 'settings', 'spacing', 'spacingScale', $origin );

// Apply the base spacing scale to the current layer.
$base_spacing_scale = _wp_array_get( $this->theme_json, $scale_path, array() );
$flattened_spacing_scale = array_replace( $flattened_spacing_scale, $base_spacing_scale );

$spacing_scale = _wp_array_get( $incoming_data, $scale_path, null );
if ( ! isset( $spacing_scale ) ) {
continue;
}

// Allow partial scale settings by merging with lower layers.
$flattened_spacing_scale = array_replace( $flattened_spacing_scale, $spacing_scale );

// Generate and merge the scales for this layer.
$sizes_path = array( 'settings', 'spacing', 'spacingSizes', $origin );
$spacing_sizes = _wp_array_get( $incoming_data, $sizes_path, array() );
$spacing_scale_sizes = static::compute_spacing_sizes( $flattened_spacing_scale );
$merged_spacing_sizes = static::merge_spacing_sizes( $spacing_scale_sizes, $spacing_sizes );

_wp_array_set( $incoming_data, $sizes_path, $merged_spacing_sizes );
}

/*
* The array_replace_recursive algorithm merges at the leaf level,
* but we don't want leaf arrays to be merged, so we overwrite it.
@@ -3709,10 +3768,16 @@ public function get_data() {
* Sets the spacingSizes array based on the spacingScale values from theme.json.
*
* @since 6.1.0
* @deprecated 6.6.0
*
* @param string $origin Optional. What source of data to set the spacing sizes for.
* One of 'default', 'theme', or 'custom'. Default 'default'.
*
* @return null|void
*/
public function set_spacing_sizes() {
_deprecated_function( __METHOD__, '6.6.0' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a $replacement method that users should prefer? It can be in the third argument.

https://developer.wordpress.org/reference/functions/_deprecated_function/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to follow-up with a fix?

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.

There is no replacement. This is being called automatically in the constructor and merge now. That being said, I seem to have missed some comments in the backport that explained that. I'll add them in a follow-up.


$spacing_scale = isset( $this->theme_json['settings']['spacing']['spacingScale'] )
? $this->theme_json['settings']['spacing']['spacingScale']
: array();
@@ -3746,6 +3811,99 @@ public function set_spacing_sizes() {
return null;
}

$spacing_sizes = static::compute_spacing_sizes( $spacing_scale );

// If there are 7 or fewer steps in the scale revert to numbers for labels instead of t-shirt sizes.
if ( $spacing_scale['steps'] <= 7 ) {
for ( $spacing_sizes_count = 0; $spacing_sizes_count < count( $spacing_sizes ); $spacing_sizes_count++ ) {
$spacing_sizes[ $spacing_sizes_count ]['name'] = (string) ( $spacing_sizes_count + 1 );
}
}

_wp_array_set( $this->theme_json, array( 'settings', 'spacing', 'spacingSizes', 'default' ), $spacing_sizes );
}

/**
* Merges two sets of spacing size presets.
*
* @since 6.6.0
*
* @param array $base The base set of spacing sizes.
* @param array $incoming The set of spacing sizes to merge with the base. Duplicate slugs will override the base values.
* @return array The merged set of spacing sizes.
*/
private static function merge_spacing_sizes( $base, $incoming ) {
// Preserve the order if there are no base (spacingScale) values.
if ( empty( $base ) ) {
return $incoming;
}
$merged = array();
foreach ( $base as $item ) {
$merged[ $item['slug'] ] = $item;
}
foreach ( $incoming as $item ) {
$merged[ $item['slug'] ] = $item;
}
ksort( $merged, SORT_NUMERIC );
return array_values( $merged );
}

/**
* Generates a set of spacing sizes by starting with a medium size and
* applying an operator with an increment value to generate the rest of the
* sizes outward from the medium size. The medium slug is '50' with the rest
* of the slugs being 10 apart. The generated names use t-shirt sizing.
*
* Example:
*
* $spacing_scale = array(
* 'steps' => 4,
* 'mediumStep' => 16,
* 'unit' => 'px',
* 'operator' => '+',
* 'increment' => 2,
* );
* $spacing_sizes = static::compute_spacing_sizes( $spacing_scale );
* // -> array(
* // array( 'name' => 'Small', 'slug' => '40', 'size' => '14px' ),
* // array( 'name' => 'Medium', 'slug' => '50', 'size' => '16px' ),
* // array( 'name' => 'Large', 'slug' => '60', 'size' => '18px' ),
* // array( 'name' => 'X-Large', 'slug' => '70', 'size' => '20px' ),
* // )
*
* @since 6.6.0
*
* @param array $spacing_scale {
* The spacing scale values. All are required.
*
* @type int $steps The number of steps in the scale. (up to 10 steps are supported.)
* @type float $mediumStep The middle value that gets the slug '50'. (For even number of steps, this becomes the first middle value.)
* @type string $unit The CSS unit to use for the sizes.
* @type string $operator The mathematical operator to apply to generate the other sizes. Either '+' or '*'.
* @type float $increment The value used with the operator to generate the other sizes.
* }
* @return array The spacing sizes presets or an empty array if some spacing scale values are missing or invalid.
*/
private static function compute_spacing_sizes( $spacing_scale ) {
/*
* This condition is intentionally missing some checks on ranges for the values in order to
* keep backwards compatibility with the previous implementation.
*/
if (
! isset( $spacing_scale['steps'] ) ||
! is_numeric( $spacing_scale['steps'] ) ||
0 === $spacing_scale['steps'] ||
! isset( $spacing_scale['mediumStep'] ) ||
! is_numeric( $spacing_scale['mediumStep'] ) ||
! isset( $spacing_scale['unit'] ) ||
! isset( $spacing_scale['operator'] ) ||
( '+' !== $spacing_scale['operator'] && '*' !== $spacing_scale['operator'] ) ||
! isset( $spacing_scale['increment'] ) ||
! is_numeric( $spacing_scale['increment'] )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My brain melted slightly... 😆

Just checking, this has test coverage? It might help folks understand what's going on and also confirm the intentions here.

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.

/*
 * This condition is intentionally missing some checks on ranges for the values in order to
 * keep backwards compatibility with the previous implementation.
 */

I tried to get rid of it or at least fix the missing checks, but that would have required a much bigger refactor and API design work. This was already landing rather late in the release cycle, so I opted to keep spacingScale working the same and just copy the condition from above https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-e12c3008d747d1bec5be9927c5e7b1ced0a88641abe52c278d495da936714817R3792-R3814 without the trigger_error (which seemed important to keep the public function working the same).

Triggering the error didn't make sense when this happened automatically in the constructor. Partial settings are valid for the theme and custom origins https://github.com/wordpress/wordpress-develop/blob/d088e31c73456179502c9bd5354fc43c6267bd7a/src/wp-includes/class-wp-theme-json-resolver.php#L142. It wasn't a problem before because merging the settings happened before set_spacing_sizes was called.

Just checking, this has test coverage?

The code already had test coverage for each of the checks https://github.com/WordPress/wordpress-develop/pull/6616/files#diff-5b485fe0725d5e02bb5aa976681c29e20b164c67c96250c6144b4b119a067eedR4754, but it didn't have tests for the invalid use cases that didn't trigger an error like "steps": 20 only producing 15 steps. I did add checks in the theme.json JSON schema so it'll at least get flagged there now.

) {
return array();
}

$unit = '%' === $spacing_scale['unit'] ? '%' : sanitize_title( $spacing_scale['unit'] );
$current_step = $spacing_scale['mediumStep'];
$steps_mid_point = round( $spacing_scale['steps'] / 2, 0 );
@@ -3828,14 +3986,7 @@ public function set_spacing_sizes() {
$spacing_sizes[] = $above_sizes_item;
}

// If there are 7 or fewer steps in the scale revert to numbers for labels instead of t-shirt sizes.
if ( $spacing_scale['steps'] <= 7 ) {
for ( $spacing_sizes_count = 0; $spacing_sizes_count < count( $spacing_sizes ); $spacing_sizes_count++ ) {
$spacing_sizes[ $spacing_sizes_count ]['name'] = (string) ( $spacing_sizes_count + 1 );
}
}

_wp_array_set( $this->theme_json, array( 'settings', 'spacing', 'spacingSizes', 'default' ), $spacing_sizes );
return $spacing_sizes;
}

/**