Skip to content

Merged data should consider origin to return early #45969

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
oandregal merged 15 commits into from
Nov 25, 2022
Merged

Merged data should consider origin to return early #45969

Changes from 1 commit
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
Next Next commit
Bail early for given origin
  • Loading branch information
@oandregal
oandregal committed Nov 24, 2022
commit f2b0c16b9d84e4e79022d12dfceed66621c1f56d
58 changes: 58 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,62 @@ 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

@spacedmonkey spacedmonkey Nov 22, 2022

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

@oandregal oandregal Nov 22, 2022

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

@anton-vlasenko anton-vlasenko Nov 23, 2022

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

@oandregal oandregal Nov 24, 2022

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

@oandregal oandregal Nov 24, 2022

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 ) {
$result->set_spacing_sizes();
return $result;
}

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

$result->merge( static::get_user_data() );

$result->set_spacing_sizes();
return $result;
}
}