Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#57824 closed defect (bug) (fixed)

Global styles values are not reset in site editor under certain scenarios

Reported by: oandregal's profile oandregal Owned by: oandregal's profile oandregal
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.1
Component: Editor Keywords: has-patch has-unit-tests gutenberg-merge
Focuses: Cc:

Description

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.

Change History (12)

This ticket was mentioned in PR #4145 on WordPress/wordpress-develop by @oandregal.


16 months ago
#1

  • Keywords has-patch has-unit-tests added

Trac ticket https://core.trac.wordpress.org/ticket/57824
Reverts https://github.com/WordPress/wordpress-develop/pull/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

By reverting https://github.com/WordPress/wordpress-develop/pull/3441 which was the one that introduced the issue.

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

{{{php
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:

{{{php
$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):

{{{php
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;

}
}}}

@oandregal commented on PR #4145:


16 months ago
#2

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?

@ntsekouras commented on PR #4145:


16 months ago
#3

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 commented on PR #4145:


16 months ago
#4

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 commented on PR #4145:


16 months ago
#5

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 https://github.com/WordPress/gutenberg/pull/48644

#6 @hellofromTonya
16 months ago

  • Keywords gutenberg-merge added
  • Milestone changed from Awaiting Review to 6.2
  • Version changed from trunk to 6.1

Revert of [54517]. See #56467 from 6.1.0 cycle.

Marking as a gutenberg-merge for tracking.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


16 months ago

#8 @audrasjb
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55448:

Editor: Ensure Global styles values are reset in the site editor.

This changeset fixes a bug by which the reset function of the global styles sidebar would not work as expected in the site editor. It reverts [54517] and adds related unit tests.

Props oandregal, ntsekouras, youknowriad, hellofromTonya.
Fixes #57824
See #56467

This ticket was mentioned in PR #4157 on WordPress/wordpress-develop by @oandregal.


16 months ago
#10

Trac ticket https://core.trac.wordpress.org/ticket/57824

The test introduced in https://github.com/WordPress/wordpress-develop/pull/4145 wasn't actually reporting the bug. Unless we find a way in that it works to signal a regression, it should be removed to avoid an illusion of security.

@oandregal commented on PR #4145:


16 months ago
#11

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 https://github.com/WordPress/wordpress-develop/pull/4157 to remove the test, as it was still not working as expected (to catch the regression, as mentioned at https://github.com/WordPress/wordpress-develop/pull/4145#discussion_r1120471830). Unless we find a way in that it works to signal a regression, it should be removed to avoid an illusion of security.

#12 @oandregal
16 months ago

Since this bug has been introduced in the 6.1 cycle, I understand we'll want to cherry-pick the fix to that branch as well.

I see this ticket is marked as 6.1 version, is that enough? Will someone pick this up when time comes? Or should I go ahead and cherry-pick the fix for that branch? I haven't done this before, so I may be missing very obvious things, happy to learn!

Note: See TracTickets for help on using tickets.