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

Add backward compat shim for WP_Theme_JSON_Data::get_theme_json #6626

Closed
wants to merge 4 commits into from
Closed

Add backward compat shim for WP_Theme_JSON_Data::get_theme_json #6626

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89dbb9f
Add backward compat shim for WP_Theme_JSON_Data::get_theme_json
joemcgill May 24, 2024
f314789
Merge branch 'trunk' into fix/61112-wp-theme-json-data-compat
joemcgill Jun 3, 2024
327bb48
Ensure objects returned from filters are validated by `WP_Theme_JSON`
joemcgill Jun 7, 2024
544d762
Prefer instanceof to is_a()
joemcgill Jun 18, 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
69 changes: 60 additions & 9 deletions src/wp-includes/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,18 @@ public static function get_core_data() {
*
* @param WP_Theme_JSON_Data $theme_json Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data( $config, 'default' ) );
static::$core = $theme_json->get_theme_json();
$theme_json = apply_filters( 'wp_theme_json_data_default', new WP_Theme_JSON_Data( $config, 'default' ) );
Copy link
Member

Choose a reason for hiding this comment

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

Hi, is anyone able to reproduce this error?

I cannot repro the issue reported using latest WordPress trunk and Gutenberg 14.8.1 (the last release prior to updating WP_Theme_JSON_Data_Gutenberg). I'd like to understand this better before jumping into implementation.

The only way I can think to reproduce the issue is by not using the provided structure to update the data as a consumer of this filter. This is, instead of doing:

add_filter( 'wp_theme_json_data_*', function( $theme) {
  $new_data = array( /* ... */ );

  return $theme->update_with( $new_data );
});

Your plugin does something like:

add_filter( 'wp_theme_json_data_*', function( $theme) {
  /* your plugin code */
  return  $your_custom_structure;
});

Note that this is not supported or recommended. The update_with method is a requirement to use these filters, as stated in the devnote, because it makes sure everything works correctly (data migration, sanitization, etc.). This is what happened to @ryelle, as she reported.

Is there any other use case?

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I'm not against doing it, but I'm reluctant to given that it only happens if a plugin provides their own structure, which is not supported or recommended.

Copy link
Contributor

@ryelle ryelle Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue can be reproduced using the Blocks Everywhere plugin, which does the following, basically:

add_filter( 'wp_theme_json_data_theme', function( $theme) {
	$theme = new \WP_Theme_JSON_Data_Gutenberg( /* ... */ );
	return $theme;
} );

(full code here)

Which I suppose is incorrect according to the dev note, but it did work before the recent change.

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 believe this was only reproducible prior to WP_Theme_JSON_Data_Gutenberg::get_theme_json being added in version 18.3.0, which shipped after the core change. If we don't need to worry about WP 6.6 compat with any Gutenberg versions prior to 18.3.0, then we can probably close this issue as wontfix.

However, I think this use case is instructive for us as we consider better ways of managing the WP_Theme_JSON_ classes between the GB repo and the WP-dev repo to avoid these types of conflicts.


/*
* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( $theme_json instanceof WP_Theme_JSON_Data ) {
static::$core = $theme_json->get_theme_json();
} else {
$config = $theme_json->get_data();
static::$core = new WP_Theme_JSON( $config, 'default' );
}

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

/*
* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( $theme_json instanceof WP_Theme_JSON_Data ) {
static::$theme = $theme_json->get_theme_json();
} else {
$config = $theme_json->get_data();
static::$theme = new WP_Theme_JSON( $config );
}

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

/*
* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( $theme_json instanceof WP_Theme_JSON_Data ) {
static::$blocks = $theme_json->get_theme_json();
} else {
$config = $theme_json->get_data();
static::$blocks = new WP_Theme_JSON( $config, 'blocks' );
}

return static::$blocks;
}

Expand Down Expand Up @@ -519,7 +550,17 @@ public static function get_user_data() {
* @param WP_Theme_JSON_Data $theme_json Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'wp_theme_json_data_user', new WP_Theme_JSON_Data( $config, 'custom' ) );
return $theme_json->get_theme_json();

/*
* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( $theme_json instanceof WP_Theme_JSON_Data ) {
return $theme_json->get_theme_json();
} else {
$config = $theme_json->get_data();
return new WP_Theme_JSON( $config, 'custom' );
}
}

/*
Expand All @@ -537,8 +578,18 @@ public static function get_user_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' ) );

/*
* Backward compatibility for extenders returning a WP_Theme_JSON_Data
* compatible class that is not a WP_Theme_JSON_Data object.
*/
if ( $theme_json instanceof WP_Theme_JSON_Data ) {
static::$user = $theme_json->get_theme_json();
} else {
$config = $theme_json->get_data();
static::$user = new WP_Theme_JSON( $config, 'custom' );
}

return static::$user;
}
Expand Down
Loading