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

Merged data should consider origin to return early #45969

Merged
merged 15 commits into from
Nov 25, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 22, 2022

Part of #45171
Related #45971

What?

This makes the WP_Theme_JSON_Resolver::get_merged_data consider the origin the consumers wants to retrieve, preventing it from calculating data consumers don't need.

Why?

This PR is a preparation for #45372 and other public high-level methods. The consumer should be responsible to know which origin needs to be queried, and the resolver should return data based on that.

How?

  • This makes the get_merged_data method use the $origin parameter and bail early.
  • Adds some test to clarify behavior.
  • Brings back a couple of filters that were missing.

Testing Instructions

@codesandbox
Copy link

codesandbox bot commented Nov 22, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@oandregal oandregal self-assigned this Nov 22, 2022
@oandregal oandregal added the [Type] Bug An existing feature does not function as intended label Nov 22, 2022
@oandregal
Copy link
Member Author

@glendaviesnz I wanted to gather your thoughts on whether this change is ok in terms of the spacing presets. What would be a good testing instructions for this change?

@oandregal oandregal added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Nov 22, 2022
*
* @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.

@oandregal oandregal force-pushed the update/get-merged-data-bail-early branch 2 times, most recently from e5ac0a6 to 8f46778 Compare November 22, 2022 16:37
oandregal added a commit that referenced this pull request Nov 22, 2022
@glendaviesnz
Copy link
Contributor

@glendaviesnz I wanted to gather your thoughts on whether this change is ok in terms of the spacing presets. What would be a good testing instructions for this change?

At this point there is no way for a user to add a custom spacing scale, so we only need to test if the spacing scale from the core theme.json is generated as expected, and if spacing scale settings from a themes theme.json override the core ones. I tested the following with this PR and seems to be working as expected:

✅ With no theme settings the default 7 step spacing sizes show in block dimensions tools panel
✅ Theme settings of any of the spacingScale params merge with and override the core theme.json settings
✅ Theme setting of a hard coded spacingSizes array overrides the auto generated spacingScale

I don't particularly like the location of set_spacing_sizes in this method, it seems slightly out of place, but when originally looking at this I couldn't find a better location for this as the spacing sizes data can't be generated until the theme data has been merged ... I guess it sort of fits as the generation of the spacing scale is part of the merge process, but I am open to ideas/suggestions on a better place to locate this.

@oandregal
Copy link
Member Author

At this point there is no way for a user to add a custom spacing scale

Ok, thanks. I need to look at this tomorrow (I'm from mobile now), though it sounds we need to recalculate the spacing at least for core, blocks (they can have any settings as part of their attributes, so I guess it also affect this?) and theme, but could get rid of the user.

I don't particularly like the location of set_spacing_sizes in this method, it seems slightly out of place

We can look at this separately.

A potential idea. If 1) all but the user origin need this computation and 2) we only compute this if the incoming data has new spacing settings, could we perhaps look at absorbing this in the merge method by doing something along these lines (pseudo-code):

function merge( $incoming ) {
  // an some point at the end
  if ( $incoming_has_spacing_settings ) {
    // Do the scale calculation.
  }
}

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Nov 23, 2022

at least for core, blocks (they can have any settings as part of their attributes, so I guess it also affect this?) and theme, but could get rid of the user.

At the moment it is only possible to have spacingScale, or spacingSizes settings at the core and theme level. Unique spacing presets can't be set at the block level, and I don't think there are any plans to support this, so currently everything seems to work as expected if $result->set_spacing_sizes() is only run for the default and theme origins - hopefully I am not overlooking something.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

It would be nice to have some PHP unit tests for this new static method.

*
* @return WP_Theme_JSON
*/
public static function get_merged_data( $origin = 'custom' ) {
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.

@oandregal oandregal force-pushed the update/get-merged-data-bail-early branch from 8f46778 to baeebd2 Compare November 24, 2022 12:11
@oandregal
Copy link
Member Author

It would be nice to have some PHP unit tests for this new static method.

This is not a new method, it is part of WordPress since 5.9 and before that in the plugin. I've added a bunch of tests to cover this behavior.

@oandregal oandregal force-pushed the update/get-merged-data-bail-early branch from 00c27c9 to 795319a Compare November 24, 2022 16:36
@glendaviesnz
Copy link
Contributor

so currently everything seems to work as expected if $result->set_spacing_sizes() is only run for the default and theme origins - hopefully I am not overlooking something.

@oandregal, I must have been overlooking something as when I retested today the spacing sizes were not being generated in themes that did not have their own settings in theme.json unless set_spacing_sizes is run before the final return. I am sure I tested this scenario the other day 🤷‍♂️. I pushed the change that fixes this.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests, @oandregal.
I've added my notes.
Also, it should be possible to merge these 4 test methods into a single test method and use the data provider.
The only parameter that get changed across these tests is the argument of the WP_Theme_JSON_Resolver_Gutenberg::get_merged_data() method.

phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
phpunit/class-wp-theme-json-resolver-test.php Outdated Show resolved Hide resolved
@oandregal
Copy link
Member Author

Also, it should be possible to merge these 4 test methods into a single test method and use the data provider.

@anton-vlasenko How do you run a single test using the data provider? Happy to use it if there's an easy way to run individual tests, otherwise, that setup requires a lot of manual intervention to do something that simple.

oandregal and others added 2 commits November 25, 2022 08:47
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
@oandregal oandregal force-pushed the update/get-merged-data-bail-early branch from 782563f to 75c1202 Compare November 25, 2022 07:57
@oandregal
Copy link
Member Author

@oandregal, I must have been overlooking something as when I retested today the spacing sizes were not being generated in themes that did not have their own settings in theme.json unless set_spacing_sizes is run before the final return. I am sure I tested this scenario the other day man_shrugging. I pushed the change that fixes this.

Thanks, Glen!

@oandregal
Copy link
Member Author

@anton-vlasenko taugh me how to run a single dataset: you append @name_of_dataset to the test. For example:

npm run test:unit:php -- --filter test_get_merged_data_returns_origin@origin_default

Runs the test_get_merged_data_returns_origin with the dataset provided by origin_default.

In the past, I had tried with # and other combinations as per some docs, but they didn't work. Happy there is a way! I've adapted the test to use this mechanism.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@oandregal oandregal merged commit cf3cbd8 into trunk Nov 25, 2022
@oandregal oandregal deleted the update/get-merged-data-bail-early branch November 25, 2022 10:17
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 25, 2022
oandregal added a commit that referenced this pull request Dec 22, 2022
oandregal added a commit that referenced this pull request Dec 22, 2022
… inheriting per WordPress version (#46750)

* Backport WP_Theme_JSON_Resolver from core as it is

* Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg

* Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg

* Rename WP_Theme_JSON_Resolver to WP_Theme_JSON_Resolver_Base

The goal is to use it as base to inherit from, until we are able
to remove all the children.

* Make WP_Theme_JSON_Resolver_6_1 inherit from WP_Theme_JSON_Resolver_Base

* 6.1: remove field already defined in base class

* 6.1: remove translate, it is equal to base method

* 6.1: remove get_core_data, it does not have base changes

* 6.1: remove get_user_data

Missed core changes and does not do anything differently from core.

* 6.1: remove get_user_data_from_wp_global_styles

It misses core changes and does not do anything differently.

* 6.2: inherit from WP_Theme_JSON_Resolver_Base instead of 6.1

This makes the WP_Theme_JSON_Resover_6_1 class unused.

* 6.1: remove class no longer in use

* 6.2: deprecate theme_has_support

#45380

* 6.2: substitute WP_Theme_JSON_Resolver::theme_has_support

by wp_theme_has_theme_json()

#45168

* 6.2: port changes to get_user_data_from_wp_global_styles

#46043

* 6.2: update get_merged_data

#45969

* 6.2: remove get_user_data

Same code as core. There's a check for detecting whether the class
is an instance of WP_Theme_JSON_Gutenberg that was not ported.
This check was introduced to make sure the cache of the core class
didn't interfere with the cache of the Gutenberg class,
so it's no longer necessary.

See #42756

* experimental: make it inherit from WP_Theme_JSON_Resolver_Base

* 6.2: remove class no longer in use

* experimental: delete remove_json_comments

It's already part of the base class.

* experimental: remove get_block_data

It's already part of the base class and it didn't have
the changes from core.

* experimental: appearanceTools

Port changes to the base class that were meant to be part of 6.1.
See #43337

* experimental: port webfonts (experimental API)

see #37140

* experimental: use gutenberg_get_legacy_theme_supports_for_theme_json

#46112

* experimental: get_theme_data, all code is in the base class

* experimental: remove empty class

* Rename WP_Theme_JSON_Resolver_Base to WP_Theme_JSON_Resolver_Gutenberg

* Fix lint issue: rename class-wp-theme-json-resolver.php to class-wp-theme-json-resolver-gutenberg.php

* Move theme.json to unversioned lib/

* Move theme-i18n.json to unversioned lib/
noahtallen pushed a commit that referenced this pull request Dec 28, 2022
… inheriting per WordPress version (#46750)

* Backport WP_Theme_JSON_Resolver from core as it is

* Substitute WP_Theme_JSON by WP_Theme_JSON_Gutenberg

* Substitute WP_Theme_JSON_Data by WP_Theme_JSON_Data_Gutenberg

* Rename WP_Theme_JSON_Resolver to WP_Theme_JSON_Resolver_Base

The goal is to use it as base to inherit from, until we are able
to remove all the children.

* Make WP_Theme_JSON_Resolver_6_1 inherit from WP_Theme_JSON_Resolver_Base

* 6.1: remove field already defined in base class

* 6.1: remove translate, it is equal to base method

* 6.1: remove get_core_data, it does not have base changes

* 6.1: remove get_user_data

Missed core changes and does not do anything differently from core.

* 6.1: remove get_user_data_from_wp_global_styles

It misses core changes and does not do anything differently.

* 6.2: inherit from WP_Theme_JSON_Resolver_Base instead of 6.1

This makes the WP_Theme_JSON_Resover_6_1 class unused.

* 6.1: remove class no longer in use

* 6.2: deprecate theme_has_support

#45380

* 6.2: substitute WP_Theme_JSON_Resolver::theme_has_support

by wp_theme_has_theme_json()

#45168

* 6.2: port changes to get_user_data_from_wp_global_styles

#46043

* 6.2: update get_merged_data

#45969

* 6.2: remove get_user_data

Same code as core. There's a check for detecting whether the class
is an instance of WP_Theme_JSON_Gutenberg that was not ported.
This check was introduced to make sure the cache of the core class
didn't interfere with the cache of the Gutenberg class,
so it's no longer necessary.

See #42756

* experimental: make it inherit from WP_Theme_JSON_Resolver_Base

* 6.2: remove class no longer in use

* experimental: delete remove_json_comments

It's already part of the base class.

* experimental: remove get_block_data

It's already part of the base class and it didn't have
the changes from core.

* experimental: appearanceTools

Port changes to the base class that were meant to be part of 6.1.
See #43337

* experimental: port webfonts (experimental API)

see #37140

* experimental: use gutenberg_get_legacy_theme_supports_for_theme_json

#46112

* experimental: get_theme_data, all code is in the base class

* experimental: remove empty class

* Rename WP_Theme_JSON_Resolver_Base to WP_Theme_JSON_Resolver_Gutenberg

* Fix lint issue: rename class-wp-theme-json-resolver.php to class-wp-theme-json-resolver-gutenberg.php

* Move theme.json to unversioned lib/

* Move theme-i18n.json to unversioned lib/
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants