-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 mechanism that resolves Global Styles data #27237
Conversation
…gs into WP_Theme_JSON_Resolver::extract_legacy_settings
…t_settings into WP_Theme_JSON_Resolver::extract_legacy_settings" This reverts commit b41a904.
@jorgefilipecosta this is one of those PRs that would benefit from quick landing if the direction makes sense (tweaks can be done in subsequent PRs) to avoid getting caught in tons of rebases! |
Size Change: +28 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this but the code looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things seem to work well on my tests 👍
* Class that abstracts the processing | ||
* of the different data sources. | ||
*/ | ||
class WP_Theme_JSON_Resolver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add some tests to this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that but discarded it because I'm not sure if there's a lot of value for this one. This is my rationale: if you look at the methods, all are straightforward calls from WordPress that mostly pull data from sources (files the active theme and core, user data from the CPT, etc). The important logic is in WP_Theme_JSON
, for which I already wrote extensive testing.
However, I'm always happy to reconsider it, adding tests is always fun. What would you think needs testing and is not covered yet?
* | ||
* @var WP_Theme_JSON | ||
*/ | ||
private $theme = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$core and $user are static fields, while $theme is not static. Similarly get_core_origin and get_user_origin and static functions while get_theme_origin. What is the reason to have the difference between core and user and the theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that core & user won't change during a page load while theme can ― hence we can share core&user among all instances while the theme data is per instance. The reason why the theme data can change is that it uses existing theme support data, which can be filtered by 3rd parties ― hence it needs to be recalculated upon every call. Does this make sense?
This PR continues the work started at #26803 to stabilize the "Global Styles" mechanism in the server, so it can be landed to WordPress core as soon the pieces are ready.
It extracts a new piece that takes care of pulling & merging the data from the different origins that are processed: core, theme, and user. It doesn't change existing behavior.
How to test