Add Theme's base global styles endpoint #35985
Conversation
| * @return WP_REST_Response $data | ||
| */ | ||
| function gutenberg_get_themes_global_styles( WP_REST_Request $request ) { | ||
| if ( wp_get_theme()->get_stylesheet() !== $request['stylesheet'] ) { |
There was a problem hiding this comment.
@oandregal our low level APIs (WP_Theme_JSON_Resolver_Gutenberg) doesn't allow us to easily retrieve the base global styles for any theme (if it's not the active theme). So right now I've limited this endpoint to the active theme only but I don't like this limitation, I think the lower level APIs should be more flexible.
There was a problem hiding this comment.
Nice, I can help adding that if we want it.
The current proposal for the endpoint makes me wonder in which cases we want the data coming from a theme other than the active one. Given there're no other use cases than the active theme yet, would it be reasonable to design the endpoint to not ask for that info to consumers? I'd like to understand if this is a limitation of the REST API (perhaps knowing the active theme requires bootstrapping parts of WordPress that aren't bootstrapped in a REST request)? Or perhaps the design vision for the REST API is targeted towards being session-agnostic?
There was a problem hiding this comment.
Given there're no other use cases than the active theme yet, would it be reasonable to design the endpoint to not ask for that info to consumers?
The same question could be asked about the "themes" endpoint. The design of this endpoint map how it works in the themes endpoint, you can retrieve the theme's data even for inactive themes. It's true that there's no use-cases for non active themes at the moment though.
Or perhaps the design vision for the REST API is targeted towards being session-agnostic?
I think this is the case yes, not a limitation. @TimothyBJacobs might speak about this. Potentially we could add a similar "link" to the theme's base styles (like we do for the user ones)
There was a problem hiding this comment.
Yeah, the REST API should be as stateless as possible. And ideally we'd be able to retrieve details about the non-active theme's styles. However, there are some things about themes, like add_theme_support that aren't added in a declarative manner so that we could return them for non-active themes.
So it is fine for now to just include the details about the non-active theme, but I agree with @youknowriad that long term, our low-level APIs should be flexible enough to be able to query that information. Once that is completed, adding that info to the non-active theme endpoints would be great.
There was a problem hiding this comment.
Got it, thanks for the thoughts.
| ); | ||
| } | ||
|
|
||
| $theme = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( array(), 'theme' ); |
There was a problem hiding this comment.
@oandregal In trunk we pass the $settings object that comes from the block_editor_settings filter here while in this PR I'm passing an empty array. I think this highlights the issues we discussed about the separation between producer and consumer of the global styles. For instance this is the endpoint that retrieves the base global styles for a theme, I don't think the block_editor_settings should apply here. Potentially we could have a lower level filter (base_theme_settings or similar) if we want this to be filterable but for now, I'm removing the filter support.
There was a problem hiding this comment.
Yeah, this is problematic in my view, as we're taking theme supports data directly from the theme and not after having run the filter to which 3rd parties could have hooked into (some hosts actually do). The potentially filterable data is only existing theme supports and less than a dozen of them https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-json/#backward-compatibility-with-add_theme_support
I don't see it as a big issue given that this is only for the site editor so far. It'll be a backward-incompatible change if we wanted to do it for the post editor, though.
There was a problem hiding this comment.
This is also "solved" by introducing dedicated filters for global styles. Do you think the introduction of these endpoints is a good place/moment for adding the filters?
There was a problem hiding this comment.
I don't think it's that problematic because global styles only works for themes with theme.json. Meaning themes that don't use theme supports (ideally).
Introducing a new filter is definitely on the table but it won't solve backward compatibility anyway, I think if there are plugins that are using block_editor_settings to alter theme settings, they're doing it wrong because that clearly says "editor settings" and not some agnostic "theme settings".
There was a problem hiding this comment.
We may need to remove the filtered settings anyway. Found a bug while testing Jorge's PR to add the new color UI component with the 3 origins and the fix is not using the filtered settings for presets #36054
|
Size Change: +140 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
2fdb49b to
5cbf0ab
Compare
There was a problem hiding this comment.
I'm not being able to test the PR because the request being made returns 404:
http://yah.local/wp-json/wp/v2/themes/theme-experiments/tt1-blocks/global-styles?_locale=user
This is the path. Maybe we need a regex updated because the stylesheet is theme-experiments/tt1-blocks?
|
@jorgefilipecosta I've tried to align the stylesheet with how the theme's endpoint work, I wonder if theme endpoint is broken or if it expects some kind of normalize stylesheet? cc @TimothyBJacobs |
|
@jorgefilipecosta in the meantime, you can test with any theme that is not installed on a subfolder |
| ); | ||
| } | ||
|
|
||
| $theme = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( array(), 'theme' ); |
There was a problem hiding this comment.
What this is returning is not the theme styles but the result of the merge of the theme styles with core. Should we have a separate endpoint for core?
The shape returned by this endpoint does not match the shape returned by the user endpoint. Here we return the presets by origin, on the user endpoint we just return the array of presets (the origin is obviously the user). I guess the shape should be the same?
That would mean we would need an endpoint for core, or make a static HTTP request to the core theme.json file.
There was a problem hiding this comment.
What this is returning is not the theme styles but the result of the merge of the theme styles with core. Should we have a separate endpoint for core?
For me that's what represents "theme styles", core ones are just default values. (For presets we can merge using the subkeys if needed, which is probably already done)
There was a problem hiding this comment.
In that case we still have the issue of the shape returned by the themes endpoint being different from the shape returned the the user endpoint: {settings: {color: palette: {core:[...], theme: [] } } } vs {settings: {color: palette: [] } }. If both return theme.json structures should the structured be the same?
There was a problem hiding this comment.
I think the core is not a default core is a source of styles and settings in its own right. I guess the solution would be for the theme endpoint to just return the theme.json settings without merging with the core and request the core settings separately.
There was a problem hiding this comment.
In that case we still have the issue of the shape returned by the themes endpoint being different from the shape returned the the user endpoint: {settings: {color: palette: {core:[...], theme: [] } } } vs {settings: {color: palette: [] } }. If both return theme.json structures should the structured be the same?
I think the core is not a default core is a source of styles and settings in its own right. I guess the solution would be for the theme endpoint to just return the theme.json settings without merging with the core and request the core settings separately.
Both of these can make sense, that said, theme.json are actually really defaults for everything but presets so it makes me think, maybe the first option is better.
There was a problem hiding this comment.
Thinking a bit more here, if we add the "user" subkey to the user's endpoint, it might suggest that you could alter "theme" and "core" palettes as well which is not good. On the other hand, everything in core's theme.json is defaults aside for the presets.
So I think having a /core endpoint somehow to fetch the "core presets" essentially (we don't care about everything else) could be a good solution but I don't fee like removing the defaults from "theme" endpoint is a good thing to do.
Anyway I'm a bit lost here to be honest and uncertain what's best. I'm thinking we should land things as they are (simple) for the alpha at least and see how to improve things before beta.
For the template endpoints, the requests are made as "theme-experiments//tt1-blocks" (using a double "/"). |
|
Yeah it looks like it isn't properly handling themes that are in subfolders. Introduced in https://core.trac.wordpress.org/ticket/50152, cc: @spacedmonkey. |
|
I'd love a solution for the "themes in folders" here if possible before merge 🤔 |
|
@spacedmonkey how do you think we should deal with this issue #35985 (review) Do you think we can just update the regex for the argument somehow? How can we defined the regex for |
I think we should focus on #36076. Playing with my implementation, it feels more core ready. |
|
@spacedmonkey this PR and #36076 are about two different endpoints though :) |
|
@youknowriad I've opened, https://core.trac.wordpress.org/ticket/54349, to fix the issue in the Core theme's controller. |
|
@youknowriad A PR is available that will fix the issue with the themes controller, WordPress/wordpress-develop#1793. You can use the Regex from that PR in this endpoint, however, similar to the templates controller, AFAICT we'd need to make the requests be I think this double slashing is more awkward here than in the templates controller because the So I think we have a couple of options. Listed in order of my preference.
My preference is with the first option. I think it is the most RESTful and gives us the most room to grow. But @youknowriad I'm happy with any of the three options you think make the most sense for the client. |
|
Thanks for the fix and for your thoughts. The first option works for me as well. I'll give a try a bit later here and see how far I can go. |
|
I pushed an update here to implement the first suggestion in #35985 (comment) I think things are working properly. |
The themes endpoint does this. |
43b608c to
5fa90d1
Compare
There was a problem hiding this comment.
LGTM 👍 Everything seems to work well even themes on subfolders.
In the future, we need to consider if it makes sense to have core and theme styles merged instead of just theme ones. And if the difference in shape between user and theme styles is something ok.
Builds on top of #35801
This PR adds a
wp/v2/theme/{mytheme}/global-stylesendpoint to avoid the need for the__experimentalGlobalStylesBaseConfigconfig we pass to bootstrap the site editor.See related discussion in #35801 and #35141