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::_construct #6271

Closed
wants to merge 5 commits into from
Closed

Remove additional call to WP_Theme_JSON::_construct #6271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a184a82
Remove additional call to theme_json
kt-12 Mar 14, 2024
a3523d0
cs fixes
kt-12 Mar 14, 2024
53a5e2e
Merge branch 'trunk' into temp/get_theme_data_performace_improvement
kt-12 Apr 30, 2024
bbcc411
remove cyclic WP_Theme_JSON call
kt-12 Apr 30, 2024
002c2cc
Update src/wp-includes/class-wp-theme-json-data.php
kt-12 May 1, 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
11 changes: 11 additions & 0 deletions src/wp-includes/class-wp-theme-json-data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,15 @@ public function update_with( $new_data ) {
public function get_data() {
return $this->theme_json->get_raw_data();
}

/**
* Returns theme JSON object.
*
* @since 6.6.0
*
* @return WP_Theme_JSON
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
*/
public function get_theme_json() {
return $this->theme_json;
}
}
16 changes: 5 additions & 11 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 @@ -173,8 +173,7 @@ 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' ) );
$config = $theme_json->get_data();
static::$core = new WP_Theme_JSON( $config, 'default' );
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
static::$core = $theme_json->get_theme_json();

return static::$core;
}
Expand Down Expand Up @@ -255,8 +254,7 @@ 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' ) );
$theme_json_data = $theme_json->get_data();
static::$theme = new WP_Theme_JSON( $theme_json_data );
static::$theme = $theme_json->get_theme_json();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some linting issues in the committed code, incorrect spacing before the equals signs here.

The main reason I'm looking at this though is that a new feature was merged in Gutenberg - WordPress/gutenberg#57908. It was testing fine prior to the changes on these lines, but after the change there's an exception being thrown by this change (I bisected to find it).

To repro:

  1. Make sure you have the latest core trunk and gutenberg trunk code checked out and built, you have the plugin active and are using twentytwentyfour as your theme
  2. Add a file with the following content into wp-content/themes/twentytwentyfour/styles/block-style-contrast.json:
{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"title": "Variation A",
	"blockTypes": [ "core/group", "core/columns", "core/media-text" ],
	"styles": {
		"color": {
			"background": "var(--wp--preset--color--contrast)",
			"text": "var(--wp--preset--color--base)"
		}
	}
}
  1. Load the site editor, and you should see an error:
Warning: Undefined array key "styleVariations" in /var/www/html/wp-includes/class-wp-theme-json.php on line 2465
Warning: Trying to access array offset on value of type null in /var/www/html/wp-includes/class-wp-theme-json.php on line 2465

If you then revert the change on this line(s), the error goes away.

I don't have much of an understanding why this causes an issue. It'd be great to understand why it's happening before taking any action. I'll continue looking into it, but any help appreciated, as this'll need to be fixed prior to the 6.6 beta.

Copy link
Contributor

@talldan talldan May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kt-12 @joemcgill @oandregal I think I have an understanding of what's happening, but worth noting I'm not an expert with these theme json classes.

The gutenberg codebase hooks into the wp_theme_json_data_theme filter, but can return a WP_Theme_JSON_Gutenberg from the filter. Before your change, it didn't really matter as core would construct a WP_Theme_JSON from that, but now I think the change can mean core holding on to a WP_Theme_JSON_Gutenberg reference. This might be leading to some incompatibilities in the data structures. At some point the newer WP_Theme_JSON_Gutenberg data is being processed by an older WP_Theme_JSON class, and that's when the error happens. I'm not sure exactly how that happens, still investigating.

The core backport for the feature (#6662) does resolve the issue, as it updates WP_Theme_JSON to be compatible.

But I think there are question marks over what happens when the gutenberg plugin tries to implement new features in the future that extend the 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.

Thanks @talldan. We had this reported to the related Trac ticket and have created this follow-up PR to address the issue. If you could test it and confirm that it fixes the issue, I can get it committed.

But I think there are question marks over what happens when the gutenberg plugin tries to implement new features in the future that extend the theme json.

I think this is a very good question. I've had some discussion with @tellthemachines and @oandregal about making the WP_Theme_JSON_ related classes read only in the WP-dev repo and sync them from the GB repo the way we sync other packages, which is one way to solve these incompatibilities, but we've not confirmed that approach. I need to open a new issue for us to consider those changes.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't have time to look at this today but will do next week. This smells to a bug we need to fix, for which we have time in the beta period. I'm available and happy to help uncover the issue and prepare a fix. Please, ping me in any related tickets/PR (subscribed to the PRs & trac tickets mentioned).

Copy link
Member

Choose a reason for hiding this comment

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

I finally got some time to look at this today. Sharing what I know.

The flow:

Steps Core Gutenberg plugin
1. WP_Theme_JSON triggers the wp_theme_json_data_theme filter.
2. gutenberg_resolve_style_variations_* runs as a filter callback
3. Gutenberg builds its own WP_Theme_JSON_Data_Gutenberg which uses to validate the data (update_with) and then returns.
4. WP_Theme_JSON receives the validated data.

At 4, core receives some data and it expects it to be validated by the consumer of the filter (this is, the consumer uses update_with). While Gutenberg validates the data properly, it uses different metadata/schema than core does.

In the particular case that Dan raised, Gutenberg was able to get more style variations than core had:

Variations detected by Gutenberg Variations detected by core
For block core/columns: variation-a
For block core/group: variation-a
For block core/media-text: variation-a
For block core/button: outline For block core/button: outline
For block core/image: rounded For block core/image: rounded
For block core/quote: plain For block core/quote: plain

At the point of processing the variations, core is unable to find the first three variations, hence the PHP Warning: Undefined array key "styleVariations" in /var/www/src/wp-includes/class-wp-theme-json.php on line 2445 raised here. Before this PR, core did an extra validation step, getting rid of the extraneous data before processing it, so that never happened.

There's a few layers to this issue, and it only happens because Gutenberg is re-triggering the same filters than core (wp_theme_json_data_theme). The consumers of the filters run twice (one for core and the second time for the plugin). It's the first time that it fails because the metadata/schemas are different.

Unlike this other issue, this is very Gutenberg-specific because it is the result of modifying the metadata/schema of the theme.json. I don't have any more time today to look into solutions, but wanted to share what I know about as soon as possible. I'll do some more testing in the next days to assess the severity and what can we do.

Copy link
Member

Choose a reason for hiding this comment

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

For a different issue, I ended up proposing a change to how "block style variations" defined by the theme are registered at #6756 By doing that, the error reported by Dan no longer happens.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking into this @oandregal. It still seems like this is very similar to https://core.trac.wordpress.org/ticket/61112, but seems like the problem is that because core is not converting the WP_Theme_JSON_Data_Gutenberg object back into a WP_Theme_JSON_Data object, the incompatibilities are possible.

It seems wrong that Gutenberg would return an object from these filters that are not compatible with WP_Theme_JSON_Data objects, but we may be able to simply update the fix proposed in #6626 to check for whether the returned object is a WP_Theme_JSON_Data object, and reprocess if not, rather than just if it a get_theme_json method exists on the class.

As a matter of process, I think following up on this whole issue should be done in as a part of https://core.trac.wordpress.org/ticket/61112 so this doesn't get lost, so I'm going to summarize this issue on that ticket and update the PR with the new approach I just proposed.

Copy link
Member

Choose a reason for hiding this comment

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

Updated the approach in #6626 that should address the exception @talldan describes, though better ways of extending these objects in the Gutenberg repo are still worth investigating further.


if ( $wp_theme->parent() ) {
// Get parent theme.json.
Expand Down Expand Up @@ -387,9 +385,7 @@ 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' ) );
$config = $theme_json->get_data();

static::$blocks = new WP_Theme_JSON( $config, 'blocks' );
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
static::$blocks = $theme_json->get_theme_json();
return static::$blocks;
}

Expand Down Expand Up @@ -523,8 +519,7 @@ 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' ) );
$config = $theme_json->get_data();
return new WP_Theme_JSON( $config, 'custom' );
return $theme_json->get_theme_json();
}

/*
Expand All @@ -543,8 +538,7 @@ 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' ) );
$config = $theme_json->get_data();
static::$user = new WP_Theme_JSON( $config, 'custom' );
static::$user = $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.

One thing I've noticed after reviewing the Gutenberg PR WordPress/gutenberg#61262 is that this is different in Gutenberg: there, we do $config['isGlobalStylesUserThemeJSON'] = true; before the call to WP_Theme_JSON. We need to consolidate that logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like #6616 will undo this 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.

@talldan w.r.t to keeping both the repos same, that change should be fine. But w.r.t performance we might need to find a better way to do that later on.


return static::$user;
}
Expand Down
Loading