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

Merged data should consider origin to return early #45969

Merged
merged 15 commits into from
Nov 25, 2022
Merged

Merged data should consider origin to return early #45969

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f2b0c16
Bail early for given origin
oandregal Nov 22, 2022
4683653
Remove method from experimental, it is stable
oandregal Nov 24, 2022
1eabb78
Only call set spacing presets when necessary
oandregal Nov 24, 2022
845495d
Add test coverage per origin
oandregal Nov 24, 2022
3a5602a
Add blocks origin test and improve them all
oandregal Nov 24, 2022
60ec9b7
Extract utility functions
oandregal Nov 24, 2022
40bb25c
Improve block styles check
oandregal Nov 24, 2022
795319a
Clean cached data
oandregal Nov 24, 2022
6865e70
Fix lint issues
oandregal Nov 24, 2022
1de4692
Add the spacing sizes generation back in before final return
glendaviesnz Nov 24, 2022
3b0e7bf
Update phpunit/class-wp-theme-json-resolver-test.php
oandregal Nov 25, 2022
75c1202
Fix suggestions
oandregal Nov 25, 2022
7228291
Use data provider in test
oandregal Nov 25, 2022
6d06e33
Inline utility functions
oandregal Nov 25, 2022
07903d3
Fix lint issues
oandregal Nov 25, 2022
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
55 changes: 55 additions & 0 deletions lib/compat/wordpress-6.2/class-wp-theme-json-resolver-6-2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,59 @@ public static function _clean_cached_data_upon_upgrading( $upgrader, $options )
}
}

/**
* Returns the data merged from multiple origins.
*
* There are four sources of data (origins) for a site:
*
* - default => WordPress
* - blocks => each one of the blocks provides data for itself
* - theme => the active theme
* - custom => data provided by the user
*
* The custom's has higher priority than the theme's, the theme's higher than blocks',
* and block's higher than default's.
*
* Unlike the getters
* {@link https://developer.wordpress.org/reference/classes/wp_theme_json_resolver/get_core_data/ get_core_data},
* {@link https://developer.wordpress.org/reference/classes/wp_theme_json_resolver/get_theme_data/ get_theme_data},
* and {@link https://developer.wordpress.org/reference/classes/wp_theme_json_resolver/get_user_data/ get_user_data},
* this method returns data after it has been merged with the previous origins.
* This means that if the same piece of data is declared in different origins
* (default, blocks, theme, custom), the last origin overrides the previous.
*
* For example, if the user has set a background color
* for the paragraph block, and the theme has done it as well,
* the user preference wins.
*
* @param string $origin Optional. To what level should we merge data:'default', 'blocks', 'theme' or 'custom'.
* 'custom' is used as default value as well as fallback value if the origin is unknown.
*
* @return WP_Theme_JSON
*/
public static function get_merged_data( $origin = 'custom' ) {
Copy link
Member

Choose a reason for hiding this comment

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

So depending on origin, this function is you more and less information. This is very confusing and even the comment doesnt help explain why this is done at all.

  • default origin, gives you core data,
  • blocks, gives you core data + block data,
  • theme, gives you core data + block data + theme data
  • custom give you core data + block data + theme data + user data.

First of all, this is confusing, as the naming seems a little off. Origin means what in this context? Why would you only want to load parts of this data? Why not load the lot?

Instead of param, why not function names, like

get_theme_data_for_blocks,
get_theme_data_for_user,
get_theme_data_for_theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

First of all, this is confusing, as the naming seems a little off. Origin means what in this context? Why would you only want to load parts of this data? Why not load the lot?

That's how the domain works: depending on some context, we need one piece of data or another. Existing use cases are editor vs front-end, or themes with or without theme.json.

Instead of param, why not function names, like

Note that the way this is set up is prior to this PR. This PR only fixes the existing behavior. Whether this could have been done differently is out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @spacedmonkey that it would be better to have separate functions (or even classes) for each type of origin:

$block_origin = new Gutenberg_Block_Origin();
$theme_data = $block_origin->get_theme_data();

or

$user_origin = new Gutenberg_User_Origin();
$theme_data = $user_origin->get_theme_data();

At the same time, I understand @oandregal 's point that the goal of this PR is mainly refactoring/optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a conversation separate from this PR. I also want to share my perspective on that: as a collective, we have limited number of hours to produce work, so I tend to favor work that has an impact on users or developer experience. Given this class is private and consumers should use the public methods (see), I'd personally refrain from refactoring for refactor's sake. If anything, we should aim to remove/deprecate these class (and its methods) entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, note that there already are separate methods to get the individual origins:

WP_Theme_JSON_Resolver::get_core_data();
WP_Theme_JSON_Resolver::get_theme_data();
WP_Theme_JSON_Resolver::get_blocks_data();
WP_Theme_JSON_Resolver::get_user_data();

The get_merged_data method implements the algorithm that merges the sources together.

if ( is_array( $origin ) ) {
_deprecated_argument( __FUNCTION__, '5.9.0' );
}

$result = static::get_core_data();
if ( 'default' === $origin ) {
$result->set_spacing_sizes();
return $result;
}

$result->merge( static::get_block_data() );
if ( 'blocks' === $origin ) {
return $result;
}
mmtr marked this conversation as resolved.
Show resolved Hide resolved

$result->merge( static::get_theme_data() );
if ( 'theme' === $origin ) {
$result->set_spacing_sizes();
return $result;
}

$result->merge( static::get_user_data() );
return $result;
}
}
2 changes: 2 additions & 0 deletions lib/compat/wordpress-6.2/default-filters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
*/

add_action( 'switch_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
add_action( 'start_previewing_theme', 'wp_theme_has_theme_json_clean_cache' );
add_action( 'start_previewing_theme', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'upgrader_process_complete', '_wp_theme_has_theme_json_clean_cache_upon_upgrading_active_theme', 10, 2 );
add_action( 'save_post_wp_global_styles', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
add_action( 'activated_plugin', array( 'WP_Theme_JSON_Resolver_Gutenberg', 'clean_cached_data' ) );
Expand Down
43 changes: 0 additions & 43 deletions lib/experimental/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,47 +165,4 @@ private static function remove_JSON_comments( $array ) {
return $array;
}

/**
* Returns the data merged from multiple origins.
*
* There are three sources of data (origins) for a site:
* default, theme, and custom. The custom's has higher priority
* than the theme's, and the theme's higher than default's.
*
* Unlike the getters {@link get_core_data},
* {@link get_theme_data}, and {@link get_user_data},
* this method returns data after it has been merged
* with the previous origins. This means that if the same piece of data
* is declared in different origins (user, theme, and core),
* the last origin overrides the previous.
*
* For example, if the user has set a background color
* for the paragraph block, and the theme has done it as well,
* the user preference wins.
*
* @since 5.8.0
* @since 5.9.0 Added user data, removed the `$settings` parameter,
* added the `$origin` parameter.
*
* @param string $origin Optional. To what level should we merge data.
* Valid values are 'theme' or 'custom'. Default 'custom'.
* @return WP_Theme_JSON
*/
public static function get_merged_data( $origin = 'custom' ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why this method was part of lib/experimental. There are a lot of things there that shouldn't. I'm going to prepare a different PR to fix how these classes are organized because there is a lot of confusion around where code should live.

if ( is_array( $origin ) ) {
_deprecated_argument( __FUNCTION__, '5.9' );
}

$result = new WP_Theme_JSON_Gutenberg();
$result->merge( static::get_core_data() );
$result->merge( static::get_block_data() );
$result->merge( static::get_theme_data() );
if ( 'custom' === $origin ) {
$result->merge( static::get_user_data() );
}
// Generate the default spacing sizes presets.
$result->set_spacing_sizes();

return $result;
}
}
160 changes: 160 additions & 0 deletions phpunit/class-wp-theme-json-resolver-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,4 +358,164 @@ public function test_get_user_data_from_wp_global_styles_does_not_use_uncached_q
$user_cpt = WP_Theme_JSON_Resolver_Gutenberg::get_user_data_from_wp_global_styles( $theme );
$this->assertEmpty( $user_cpt, 'User CPT is expected to be empty.' );
}

public function register_block_data( $block_name ) {
register_block_type(
$block_name,
array(
'api_version' => 2,
'attributes' => array(
'borderColor' => array(
'type' => 'string',
),
'style' => array(
'type' => 'object',
),
),
'supports' => array(
'__experimentalStyle' => array(
'typography' => array(
'fontSize' => '42rem',
),
),
),
)
);
}

public function register_user_data() {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
wp_set_current_user( self::$administrator_id );
$user_cpt = WP_Theme_JSON_Resolver_Gutenberg::get_user_data_from_wp_global_styles( wp_get_theme(), true );
$config = json_decode( $user_cpt['post_content'], true );
$config['settings']['color']['palette']['custom'] = array(
array(
'color' => 'hotpink',
'name' => 'My color',
'slug' => 'my-color',
),
);
$user_cpt['post_content'] = wp_json_encode( $config );
wp_update_post( $user_cpt, true, false );
}

/**
* @covers WP_Theme_JSON_Resolver::get_merged_data
*/
public function test_get_merged_data_returns_origin_default() {
// Make sure there is data from the blocks origin.
self::register_block_data( 'my/block-with-styles' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

// Make sure there is data from the theme origin.
switch_theme( 'block-theme' );

// Make sure there is data from the user origin.
self::register_user_data();
oandregal marked this conversation as resolved.
Show resolved Hide resolved

$theme_json = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( 'default' );
$settings = $theme_json->get_settings();
$styles = $theme_json->get_styles_block_nodes();
$this->assertTrue( isset( $settings['color']['palette']['default'] ), 'core palette is present' );
$block_styles = array_filter(
$styles,
function( $element ) {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
return isset( $element['name'] ) && 'my/block-with-styles' === $element['name'];
}
);
$this->assertTrue( count( $block_styles ) === 0, 'block styles are not present' );
$this->assertFalse( isset( $settings['color']['palette']['theme'] ), 'theme palette is not present' );
$this->assertFalse( isset( $settings['color']['palette']['custom'] ), 'user palette is not present' );

unregister_block_type( 'my/block-with-styles' );
}

/**
* @covers WP_Theme_JSON_Resolver::get_merged_data
*/
public function test_get_merged_data_returns_origin_blocks() {
// Make sure there is data from the blocks origin.
self::register_block_data( 'my/block-with-styles' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

// Make sure there is data from the theme origin.
switch_theme( 'block-theme' );

// Make sure there is data from the user origin.
self::register_user_data();
oandregal marked this conversation as resolved.
Show resolved Hide resolved

$theme_json = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( 'blocks' );
$settings = $theme_json->get_settings();
$styles = $theme_json->get_styles_block_nodes();
$this->assertTrue( isset( $settings['color']['palette']['default'] ), 'core palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$block_styles = array_filter(
$styles,
function( $element ) {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
return isset( $element['name'] ) && 'my/block-with-styles' === $element['name'];
}
);
$this->assertTrue( count( $block_styles ) === 1, 'block styles are present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$this->assertFalse( isset( $settings['color']['palette']['theme'] ), 'theme palette is not present' );
$this->assertFalse( isset( $settings['color']['palette']['custom'] ), 'user palette is not present' );

unregister_block_type( 'my/block-with-styles' );
}

/**
* @covers WP_Theme_JSON_Resolver::get_merged_data
*/
public function test_get_merged_data_returns_origin_theme() {
// Make sure there is data from the blocks origin.
self::register_block_data( 'my/block-with-styles' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

// Make sure there is data from the theme origin.
switch_theme( 'block-theme' );

// Make sure there is data from the user origin.
self::register_user_data();
oandregal marked this conversation as resolved.
Show resolved Hide resolved

$theme_json = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( 'theme' );
$settings = $theme_json->get_settings();
$styles = $theme_json->get_styles_block_nodes();
$this->assertTrue( isset( $settings['color']['palette']['default'] ), 'core palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$block_styles = array_filter(
$styles,
function( $element ) {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
return isset( $element['name'] ) && 'my/block-with-styles' === $element['name'];
}
);
$this->assertTrue( count( $block_styles ) === 1, 'block styles are present' );
$this->assertTrue( isset( $settings['color']['palette']['theme'] ), 'theme palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$this->assertFalse( isset( $settings['color']['palette']['custom'] ), 'user palette is not present' );

unregister_block_type( 'my/block-with-styles' );
}

/**
* @covers WP_Theme_JSON_Resolver::get_merged_data
*/
public function test_get_merged_data_returns_origin_custom() {
// Make sure there is data from the blocks origin.
self::register_block_data( 'my/block-with-styles' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

// Make sure there is data from the theme origin.
switch_theme( 'block-theme' );

// Make sure there is data from the user origin.
self::register_user_data();
oandregal marked this conversation as resolved.
Show resolved Hide resolved

$theme_json = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
$settings = $theme_json->get_settings();
$styles = $theme_json->get_styles_block_nodes();
$this->assertTrue( isset( $settings['color']['palette']['default'] ), 'core palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$block_styles = array_filter(
$styles,
function( $element ) {
oandregal marked this conversation as resolved.
Show resolved Hide resolved
return isset( $element['name'] ) && 'my/block-with-styles' === $element['name'];
}
);
$this->assertTrue( count( $block_styles ) === 1, 'block styles are present' );
$this->assertTrue( isset( $settings['color']['palette']['theme'] ), 'theme palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved
$this->assertTrue( isset( $settings['color']['palette']['custom'] ), 'user palette is present' );
oandregal marked this conversation as resolved.
Show resolved Hide resolved

unregister_block_type( 'my/block-with-styles' );
}

}