Remove additional call to `WP_Theme_JSON_Gutenberg::__construct` #61262

kt-12 · 2024-04-30T21:05:39Z

What?

Introduce WP_Theme_JSON_Data_Gutenberg::get_theme_json() to avoid the cyclic call to WP_Theme_JSON_Gutenberg::__construct.

Trac ticket: Trac 61112

Why?

Doing similar changes in core, we got a consistent ~2.5% improvement by reducing 8 calls to the constructor reference core pr.

How?

WP_Theme_JSON_Data_Gutenberg internally has a theme_json private attribute which is an instance of WP_Theme_JSON_Gutenberg. At the moment we try to form the raw JSON data from the WP_Theme_JSON_Gutenberg object and then again from the Object from the raw JSON data, which leads to regression. In this PR we use the existing WP_Theme_JSON_Gutenberg object available to WP_Theme_JSON_Data_Gutenberg instead of the cyclic way.

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-04-30T21:05:54Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: kt-12 <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: joemcgill <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

oandregal

See related feedback in the original wordpress-develop PR WordPress/wordpress-develop#6271

oandregal · 2024-05-04T09:19:07Z

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

-				$config     = $theme_json->get_data();
-				return new WP_Theme_JSON_Gutenberg( $config, 'custom' );
+
+				return $theme_json->get_theme_json();


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.

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.

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

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?

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;

}

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

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

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

joemcgill

This looks good to me. I think looking at the isGlobalStylesUserThemeJSON question can be resolved in a follow-up PR.

joemcgill · 2024-05-06T17:04:05Z

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

-				$config     = $theme_json->get_data();
-				return new WP_Theme_JSON_Gutenberg( $config, 'custom' );
+
+				return $theme_json->get_theme_json();


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?

joemcgill · 2024-05-21T17:22:50Z

@oandregal I don't have permissions to merge this PR but am planning to commit the related Core PR this week. Can you help us get this one shipped in a GB release as well?

joemcgill · 2024-05-22T15:32:45Z

Thanks @adamsilverstein!

joemcgill · 2024-05-23T16:37:35Z

This change was committed to WP trunk in 58185.

oandregal · 2024-05-27T08:52:35Z

@oandregal I don't have permissions to merge this PR but am planning to commit WordPress/wordpress-develop#6271 this week. Can you help us get this one shipped in a GB release as well?

Sorry I wasn't around to help with this the past week (I was AFK) — happy I wasn't a blocker and you were able to fix it!

…dPress#61262)

remove additional call to Theme_JSON

c520ea5

kt-12 requested a review from spacedmonkey as a code owner April 30, 2024 21:05

kt-12 changed the title ~~remove additional call to~~ Remove additional call to WP_Theme_JSON_Gutenberg::__construct Apr 30, 2024

kt-12 mentioned this pull request Apr 30, 2024

Remove additional call to WP_Theme_JSON::_construct WordPress/wordpress-develop#6271

Closed

joemcgill requested review from oandregal and joemcgill April 30, 2024 21:41

joemcgill added the [Type] Performance Related to performance efforts label Apr 30, 2024

oandregal approved these changes May 4, 2024

View reviewed changes

oandregal reviewed May 4, 2024

View reviewed changes

joemcgill approved these changes May 6, 2024

View reviewed changes

adamsilverstein merged commit 51775a1 into WordPress:trunk May 22, 2024
66 of 69 checks passed

github-actions bot added this to the Gutenberg 18.5 milestone May 22, 2024

joemcgill mentioned this pull request May 22, 2024

PHP backports for WP 6.6 #61226

Closed

carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024

Remove additional call to WP_Theme_JSON_Gutenberg::__construct (Wor…

b2260f7

…dPress#61262)

ajlende mentioned this pull request Jun 4, 2024

Backport theme.json version 3 migrations WordPress/wordpress-develop#6616

Closed

oandregal mentioned this pull request Jun 5, 2024

Migrate theme.json based on origin #62305

Merged

patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024

Remove additional call to WP_Theme_JSON_Gutenberg::__construct (Wor…

b6df809

…dPress#61262)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove additional call to `WP_Theme_JSON_Gutenberg::__construct` #61262

Remove additional call to `WP_Theme_JSON_Gutenberg::__construct` #61262

kt-12 commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

oandregal left a comment

oandregal May 4, 2024

oandregal May 4, 2024

kt-12 May 6, 2024

joemcgill May 6, 2024

ajlende May 22, 2024

ajlende Jun 4, 2024

joemcgill Jun 4, 2024

oandregal Jun 5, 2024

joemcgill left a comment

joemcgill May 6, 2024

joemcgill commented May 21, 2024

joemcgill commented May 22, 2024

joemcgill commented May 23, 2024

oandregal commented May 27, 2024

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

Remove additional call to WP_Theme_JSON_Gutenberg::__construct #61262

Remove additional call to WP_Theme_JSON_Gutenberg::__construct #61262

Conversation

kt-12 commented Apr 30, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Apr 30, 2024 • edited Loading

oandregal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill commented May 21, 2024

joemcgill commented May 22, 2024

joemcgill commented May 23, 2024

oandregal commented May 27, 2024

Remove additional call to `WP_Theme_JSON_Gutenberg::__construct` #61262

Remove additional call to `WP_Theme_JSON_Gutenberg::__construct` #61262

kt-12 commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading