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

Fix for WP_Theme_JSON_Resolver_Gutenberg::get_merged_data #48644

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Mar 1, 2023

Related trac ticket https://core.trac.wordpress.org/ticket/57824
Reverts WordPress/wordpress-develop#3441
Backport PR WordPress/wordpress-develop#4145

What

This PR fixes a bug by which the reset function of the global styles sidebar wouldn't work as expected in the site editor.

How to reproduce

Take the following steps:

  • Using TwentyTwentyTwo and any WordPress version after rev54517, for example, WordPress 6.2 beta 3.
  • Go to the site editor.
  • Open the global styles sidebar and set the padding values to something else (222px, for example).
  • Save the changes and reload the editor.
  • Open the global styles sidebar and try resetting the padding values (for example, by using the "Reset to defaults" option displayed under the three dot menu of the global styles sidebar).
  • The expected result is that the values would be reset. However, they are not.

Note that if this is saved and then site editor reloaded, the proper data is shown.

How this PR fixes the issue

By reverting WordPress/wordpress-develop#3441 which was the one that introduced the issue.

That PR changed the get_merged_data code to (pseudo-code):

function get_merged_data( $origin ) {
    $result = static::get_core_data();

    if ( 'block' === $origin ) { $result->merge( static::get_block_data() ); }
    if ( 'theme' === $origin ) { $result->merge( static::get_theme_data() ); }
    if ( 'custom' === $origin ) { $result->merge( static::get_custom_data() ); }

    return $result;
}

However, by doing so, the base object ($result) is modifying the $core object directly, and $core ends up storing the consolidated values instead of only the ones coming from the theme.json provided by core.

This is problematic because $core data is cached. Take, for example, the following scenario:

$data = get_merged_data( 'custom' );
$data = get_merged_data( 'theme' );

The expected output for $data is that it should not have data coming from the 'custom' origin, however, it does.

The fix is reverting the change and use an empty object as base (pseudo-code):

function get_merged_data( $origin ) {
    $result = new WP_Theme_JSON();

    $result->merge( static::get_core_data() );
    if ( 'block' === $origin ) { $result->merge( static::get_block_data() ); }
    if ( 'theme' === $origin ) { $result->merge( static::get_theme_data() ); }
    if ( 'custom' === $origin ) { $result->merge( static::get_custom_data() ); }

    return $result;
}

Performance

(The performance data is taken from core, not Gutenberg. Left here as a section for visibility).

This is a bug that needs to be fixed. I thought about running some performance analysis anyway, given the intent of the PR this reverts was improving performance. Given the improvements introduced in 6.2, reverting the PR doesn't affect performance.

Using XDebug (only for completeness, we should not use data extracted from a xdebug profiler to make perf decisions, as there's a cost to observability):

Method WordPress 6.2 rev55436 This PR (based off trunk@55436)
WP_Theme_JSON_Resolver::get_merged_data 23ms (6 calls) 24ms (6 calls)
WP_Theme_JSON->merge 3.5ms (24ms) 5.8ms (30 calls)

Using production code (see raw data): the variance is sub-millisecond, so it may be attributed to the measuring conditions.

image

@oandregal oandregal requested a review from spacedmonkey as a code owner March 1, 2023 15:25
@oandregal oandregal requested a review from youknowriad March 1, 2023 15:26
@oandregal oandregal self-assigned this Mar 1, 2023
@oandregal oandregal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Mar 1, 2023
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@oandregal oandregal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 1, 2023
@oandregal oandregal merged commit 588ef6c into trunk Mar 1, 2023
@oandregal oandregal deleted the fix/get-merged-data branch March 1, 2023 16:33
@github-actions github-actions bot added this to the Gutenberg 15.3 milestone Mar 1, 2023
@ntsekouras
Copy link
Contributor

ntsekouras commented Mar 1, 2023

@oandregal you backported this change here: WordPress/wordpress-develop#4145, so I'm removing the label. Unless I'm missing something.

--edit

I guess it doesn't hurt to add it to wp/6.2 as well. I'll do it now

@ntsekouras ntsekouras removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants