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

Update which origins are queried for gutenberg_get_global_settings #45971

Merged
merged 6 commits into from
Nov 22, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 22, 2022

Part of #45171
Related #45969

What?

This PR makes sure the settings only query for the data they need.

Why?

We should not do operations that are unnecessary (such as database calls when there is no need).

How?

  • By making the settings method to manage properly the origins.
  • Adding a new condition for themes that do not have theme.json (there is no user data in this case).

Testing Instructions

Make sure automated tests pass.

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

@ramonjd @andrewserong I wanted to double-check if the logic makes sense to you (how base is being used and its relationship with themes with no theme.json).

$path = array_merge( array( 'blocks', $context['block_name'] ), $path );
}

$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.

The only logic modified here is how we determine which $origin to query.

  • adds a new condition for themes without theme.json
  • make sure all is mapped to custom

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@oandregal Approach LGTM, left a few comments with minor improvement suggestions.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the updates!

@oandregal oandregal merged commit 6088f5c into trunk Nov 22, 2022
@oandregal oandregal deleted the update/origins-for-settings branch November 22, 2022 18:39
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 22, 2022
@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Nov 22, 2022
@bph
Copy link
Contributor

bph commented Nov 22, 2022

I attached a 'needs dev note' label, know that this is just an update of an existing feature. I tag it, so it can be surfaced again in two or three months when 6.2 release cycle comes to that point (that's just my personal estimate. I have no insight in any planning around 6.2 release cycle.) I will look through other PRs to get the bundle together.

@oandregal maybe, you have some bandwidth to write make dev notes while everything is fresh in your mind. There is also a label 'has-dev note' you can apply to any of the major PRs for this feature, once you have written a first draft. :-)

@oandregal
Copy link
Member Author

This PR prevents user settings being queried for themes with no theme.json, data they don't have anyway. In terms of API/devexp/user experience this does not change anything, so I guess it does not need a devnote?

@andrewserong
Copy link
Contributor

andrewserong commented Nov 23, 2022

@ramonjd @andrewserong I wanted to double-check if the logic makes sense to you (how base is being used and its relationship with themes with no theme.json).

Thanks for the ping — the logic here makes sense to me, in that a theme without its own theme.json file should not result in a database query for the user styles, as there wouldn't be any. So it would still get the core, block and theme data, including presets, base layout styles, etc as required, but just skips that unnecessary query 👍

@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Nov 23, 2022

@oandregal It seems I'm a bit late, but the testing instruction says:
Make sure automated tests pass.
I haven't found any unit tests for the gutenberg_get_global_settings() method.
I'm wondering how to test this PR, then. 🤔

@oandregal
Copy link
Member Author

@anton-vlasenko this is certainly weird to test. There's no behavioral change for the user or the developer experience. The only change is that there's one less database query when the theme is classic (which, presumably, results in slightly better performance, data I haven't shared in this PR). Agreed, this function needs more tests.

@DaisyOlsen DaisyOlsen added [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts and removed [Type] Enhancement A suggestion for improvement. labels Nov 30, 2022
@bph bph removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 6, 2023
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] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants