Fix for WP_Theme_JSON_Resolver::get_merged_data

[oandregal]

Trac ticket https://core.trac.wordpress.org/ticket/57824
Reverts #3441

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 #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

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.

[oandregal]

cc @youknowriad as the original reporter

@ntsekouras @Mamaduka as editor tech leads: can this bug be fixed for 6.2 despite it being introduced in the 6.1 cycle?

[oandregal]

The $padding_theme values should be failing without the fix, but they are not. Not sure what's wrong with this test, but I'd consider removing it if it cannot reproduce that the behavior broke.

[ntsekouras]

can this bug be fixed for 6.2 despite it being introduced in the 6.1 cycle?

Yes, but it might be something to be also included in a 6.1 patch release.

[youknowriad]

So this did solve the issue for me without Gutenberg but with Gutenberg it's still broken, I'm assuming this class is overridden in Gutenberg?

[oandregal]

So this did solve the issue for me without Gutenberg but with Gutenberg it's still broken, I'm assuming this class is overridden in Gutenberg?

Yeah, ported it at WordPress/gutenberg#48644

[costdev]

Not a blocker, but let's follow up with the tests after Beta 4 to add a $message parameter to each of the assertions and tidy up the docblock and multiline comments. 🙂

[audrasjb]

Committed in https://core.trac.wordpress.org/changeset/55448

[oandregal]

Not a blocker, but let's follow up with the tests after Beta 4 to add a $message parameter to each of the assertions and tidy up the docblock and multiline comments. slightly_smiling_face

I prepared #4157 to remove the test, as it was still not working as expected (to catch the regression, as mentioned at #4145 (comment)). Unless we find a way in that it works to signal a regression, it should be removed to avoid an illusion of security.