Refactor theme config loading #29667
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +3.68 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
youknowriad
added
the
Global Styles
youknowriad
force-pushed
the
refactor/theme-config
branch
from
e3270be
to
78e5d7a
Compare
|
Any reviews on this one. |
|
I'm looking into this today. |
|
I agree with the goal of this PR: parts of the existing API are too low-level. It's been on my mind to do these sort of changes for months but didn't find the time, so thanks for this push. I've created an alternative at #29905 Hopefully, that addresses your concerns. There are upcoming changes to how we work with theme.json #29773 #29891 and we also need to consider hooks and versioning. I'm working on those changes in the coming weeks, so I'd like to take that as an opportunity to keep iterating on the API. |
|
@nosolosw It's unclear to me what are the goals of the alternative PR compared to the API proposed here? What didn't you like here? For me the alternative PR is still too low level. Is still uses Also, the proposed API still leave a lot of things to the consumer instead of absorbing the complexity. Example: Why I need to pass "theme support data" here. It's the role of the resolver to know how to merge things, As a consumer of the config, I don't care where the config comes from. |
|
The things related to the Post Type (registration...) I think are things that should be moved outside the resolver class (they should just be function called directly by the hooks, I don't see any value in adding these to the class but I left this separate. |
|
While I agree with some of the choices this PR makes and other ideas you shared, I wasn't sure I was bought on the overall direction. I thought I'd share a proposal that clarifies what we have (WP_Theme_JSON mimics the format + WP_Theme_JSON_Resolver implements the business logic). I don't think these should be final but I'd like to have a clearer picture before making radical changes (implement the things in the queue, etc). I've also noticed that this PR doesn't take into account the different code paths the API is called from: the |
|
I like the proposed abstraction, it hides all the complexity that is necessary to process On the technical level, I see one failing PHP unit test: https://github.com/WordPress/gutenberg/pull/29667/checks?check_run_id=2110975877#step:8:42
So it would need to be resolved before we move forward with these changes. |
gziolo
reviewed
Mar 17, 2021
| self::$theme = new WP_Theme_JSON( $theme_json_data ); | ||
| } | ||
|
|
||
| private static function get_theme_data( $theme_support_data = array() ) { |
There was a problem hiding this comment.
I use the get_theme_data method in the test I added in #29828 (still waiting for review), but I'm sure it can be changed to use higher-level API.
gziolo
reviewed
Mar 17, 2021
|
|
||
| function test_presets_are_extracted() { | ||
| $actual = WP_Theme_JSON_Resolver::get_presets_to_translate(); | ||
| $actual = WP_Theme_Config_Resolver::get_presets_to_translate(); |
There was a problem hiding this comment.
I think we can eventually remove this test in favor of the test I proposed #29828. It operates on a higher level and ensures that translations are applied. It would let us keep this method private since it is never used outside of the config resolver.
|
while I think this PR is still valuable and a good direction, I don't want to distrupt the current work on theme.json format more. So I'm going to close this PR for now and I'll reopen it once we're done with the format adaptations. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation In this PR #29335 I tried to get the "layout" setting from theme.json and I had to call 4 functions with the right arguments in order to do it.
While working with the Theme config abstraction in the backend, it became very clear to me that the classes and APIs were built initially to map the theme.json file and format, but they were too low level to work with which resulted in a lot of indirection and duplication each time you want to access a config or something in the backend.
This PR makes a step into simplifying the APIs by introducing a
WP_Theme_Configobject that you can access with:Notice that I didn't have to pass any argument or anything, everything is abstracted from the consumer, making it easier to reason about the current config without actually knowing how it's assembled, what kind of theme it is...
Later, I'd like us to probably introduce a function similar to the
useEditorFeaturehook we have in the frontend.Also note that in the PR, I keep the
WP_Theme_JSONclass but in reality this class becomes just a private implementation detail that shouldn't be accessed directly at all. A future refactor might remove it entirely.