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.

Sign up for GitHub

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

Jump to bottom

Remove additional call to WP_Theme_JSON_Gutenberg::__construct #61262

Merged
merged 1 commit into from
May 22, 2024
Merged

Remove additional call to WP_Theme_JSON_Gutenberg::__construct #61262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions lib/class-wp-theme-json-data-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,15 @@ public function update_with( $new_data ) {
public function get_data() {
return $this->theme_json->get_raw_data();
}

/**
* Return theme JSON object.
*
* @since 18.3.0
*
* @return WP_Theme_JSON
*/
public function get_theme_json() {
return $this->theme_json;
}
}
17 changes: 7 additions & 10 deletions lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,7 @@ public static function get_core_data() {
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data_Gutenberg( $config, 'default' ) );
$config = $theme_json->get_data();
static::$core = new WP_Theme_JSON_Gutenberg( $config, 'default' );
static::$core = $theme_json->get_theme_json();

return static::$core;
}
Expand Down Expand Up @@ -254,9 +253,8 @@ public static function get_theme_data( $deprecated = array(), $options = array()
*
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_theme', new WP_Theme_JSON_Data_Gutenberg( $theme_json_data, 'theme' ) );
$theme_json_data = $theme_json->get_data();
static::$theme = new WP_Theme_JSON_Gutenberg( $theme_json_data );
$theme_json = apply_filters( 'wp_theme_json_data_theme', new WP_Theme_JSON_Data_Gutenberg( $theme_json_data, 'theme' ) );
static::$theme = $theme_json->get_theme_json();

if ( $wp_theme->parent() ) {
// Get parent theme.json.
Expand Down Expand Up @@ -398,10 +396,9 @@ public static function get_block_data() {
*
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_blocks', new WP_Theme_JSON_Data_Gutenberg( $config, 'blocks' ) );
$config = $theme_json->get_data();
$theme_json = apply_filters( 'wp_theme_json_data_blocks', new WP_Theme_JSON_Data_Gutenberg( $config, 'blocks' ) );
static::$blocks = $theme_json->get_theme_json();

static::$blocks = new WP_Theme_JSON_Gutenberg( $config, 'blocks' );
return static::$blocks;
}

Expand Down Expand Up @@ -533,8 +530,8 @@ public static function get_user_data() {
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_user', new WP_Theme_JSON_Data_Gutenberg( $config, 'custom' ) );
$config = $theme_json->get_data();
return new WP_Theme_JSON_Gutenberg( $config, 'custom' );

return $theme_json->get_theme_json();
Copy link
Member

Choose a reason for hiding this comment

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

A bit below, there's another WP_Theme_JSON_Data_Gutenberg call but we cannot update it as we do on this one. That code is different than the core code and we need to consolidate both. That's a follow-up PR that shouldn't prevent this one from landing.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can absorb $config['isGlobalStylesUserThemeJSON'] = true; as part of WP_Theme_JSON_Data_Gutenberg. We may want to still check that's true before returning to be extra-safe (line 553) but we wouldn't need a second transformation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@oandregal I observed it but presumed that GB and Core might be doing things differently here. I'll look at how to incorporate that in a different PR.

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 the $config['isGlobalStylesUserThemeJSON'] modification was made as part of 2012a36 (#58409).

@ajlende is there a reason why this would still be necessary if we use the WP_Theme_JSON_Gutenberg object already present in the WP_Theme_JSON_Data_Gutenberg object?

Copy link
Contributor

Choose a reason for hiding this comment

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

The $config['isGlobalStylesUserThemeJSON'] is required for migrations that don't need to run for the custom origin in WP_Theme_JSON_Schema::migrate (called on new WP_Theme_JSON()). If the origin could be passed to migrate in some other way, then it would be fine.

gutenberg/lib/class-wp-theme-json-schema-gutenberg.php

Lines 115 to 124 in cc2fcbe

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a draft PR to explain what I'm talking about #62305

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @ajlende! I'll try to review this later this week.

Copy link
Member

Choose a reason for hiding this comment

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

Approved #62305 which improves the API and should bring back this change. Thanks for the follow-up, Alex!

}

// Very important to verify that the flag isGlobalStylesUserThemeJSON is true.
Expand Down
Loading