Add a Global Styles endpoint and use it in the site editor#35801
Add a Global Styles endpoint and use it in the site editor#35801youknowriad merged 15 commits intotrunkfrom
Conversation
| parsedConfig = contentToParse ? JSON.parse( contentToParse ) : {}; | ||
| // It is very important to verify if the flag isGlobalStylesUserThemeJSON is true. | ||
| // If it is not true the content was not escaped and is not safe. | ||
| if ( ! parsedConfig.isGlobalStylesUserThemeJSON ) { |
There was a problem hiding this comment.
I'm not sure how to handle this thing, I thought it was not something the client should be concerned about but I don't understand it enough to do the right things. cc @jorgefilipecosta
There was a problem hiding this comment.
Yes, I agree that the client should not care about this validation now that we have an endpoint. I think on the endpoint method prepare_item_for_response we should check if the flag isGlobalStylesUserThemeJSON is present if it is we proceed normally if it is not we return empty styles and settings.
There was a problem hiding this comment.
Related to this issue when serializing the user post to the database we should also include isGlobalStylesUserThemeJSON true, so the structure is properly sanitized. Right now we are not including the flag, so the styles load on the site editor, but on the frontend, user styles are ignored.
|
Size Change: +171 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
I've updated the PR and removed the |
|
Hey, took some time to read through the discussion and take stock of the data used by consumers:
They take the settings & styles, either standalone such as Additionally, note that some settings (presets) contain data for the three origins (i.e.: the data in We've also talked about "global styles variations" which I presume is just a list of different |
|
In the mental model I have for this, the endpoint would be something like:
This pattern allows us to grow later, should we need it:
In terms of naming: is there an alternative namespace to |
This one is currently already implemented in this PR as
It is not clear to me whether these should be part of the same endpoint or more something specific to the themes endpoint
This one is already implemented in this PR (kind of, just missing query support) I'd like some input from @TimothyBJacobs to get things as close as possible to the REST API guidelines. |
|
This endpoint should really extend post controller and not be a custom endpoint. We can override anything we need to on that controller. |
I thought about that, it only works if we limit it to the CPT (only server user modified settings and styles) which is still uncertain. #35801 (comment) and #35801 (comment) |
TimothyBJacobs
left a comment
There was a problem hiding this comment.
Overall this looks good to me. I've left some feedback inline.
I think in this case, considering how custom the global styles API is, I think it is fine to extend the base controller instead of using WP_REST_Posts_Controller.
/wp/v2/global-styles/base => core+theme data for the active theme
/wp/v2/global-styles/merged => core+theme+user data for the active theme
It is not clear to me whether these should be part of the same endpoint or more something specific to the themes endpoint
Yeah, my preference would be for this to be linked to by the themes endpoint.
/wp/v2/global-styles/list
This one is already implemented in this PR (kind of, just missing query support) /wp/v2/global-styles
Yeah, the list should just be the collection endpoint.
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
lib/compat/wordpress-5.9/class-gutenberg-rest-global-styles-controller.php
Outdated
Show resolved
Hide resolved
Disagree. Much of this logic can be removed by extending this controller. Not to mention of all the filters and hooks in the post controller that are very useful. |
Since these are going to be a separate endpoint, I'll leave them to their own PR. For the Post Controller vs non Post Controller. I personally don't have a big preference here and it should be something we can change with impact on the API itself. |
I'm not sure this is true. We'd need to be disabling essentially the entirety of how the posts controller works besides the title handling. |
|
What about meta and term handling? What about all the filters and actions in the post type controller? |
|
Hey, I wanted to take a look at the naming of these endpoints all together. Given this and the related PR #35985 it looks like this is what we're aiming for:
I find the URL patterns not very clear: the first one only offers user data but is named as if was the whole merged data (core+theme+user); the second one is named after the theme but it actually returns core+theme data. It's also not clear to me how we're going to extend them should we need it: how are we naming a future endpoint that returns only the theme data for example? An alternative would be a model where we only have core, theme, user endpoints (e.g.: From the alternatives mentioned here, I'd still go with something along the lines of this model. |
|
Hi @youknowriad, Removed the defaults for settings and styles otherwise isset( $request['styles'] ) does not make sense. The request always contains styles. This change allows the users of the endpoints to change just settings or styles independently. We have a complex issue, when unserializing in PHP a json object as an array with I added the required isGlobalStylesUserThemeJSON flag verifications. I added a hardcoded version ( There is a remaining issue. If a global styles post is not already on the database the site editor crashes right away. Previously the call WP_Theme_JSON_Resolver_Gutenberg::get_user_custom_post_type_id(); would create a post if it did not exist. We need a solution to this issue before merging. I guess a possible solution would be something like we had before and if $global_styles_query does not contain a post we would create a post so we can return it. But I guess a better solution would be to create a post only when saving some custom user styles. |
For the hacky solution to retrieve the core theme.json would be to make an HTTP request directly to the static path where the core default theme.json is located. |
The issue I see with this model: That said I imagine a something like |
Yes, let's start with this solution first and iterate on it later if needed. |
jorgefilipecosta
left a comment
There was a problem hiding this comment.
The issues I identified are fixed and I think this PR can be merged and when needed we can iterate on it 👍
efd7219 to
b91a650
Compare
| } else { | ||
|
|
||
| $wp_query_args = array( | ||
| 'post_status' => array( 'publish' ), |
There was a problem hiding this comment.
This should be
'post_status' => 'publish',
| ), | ||
| ); | ||
| $global_styles_query = new WP_Query( $wp_query_args ); | ||
| $id = count( $global_styles_query->posts ) ? $global_styles_query->posts[0]->ID : null; |
There was a problem hiding this comment.
We could have just request 'fields' => 'ids' in the WP_Query and no need for this $global_styles_query->posts[0]->ID, you could just do this.
$id = !empty( $global_styles_query->posts ) ? array_shift( $global_styles_query->posts ) : null;
| * | ||
| * @var string | ||
| */ | ||
| protected $post_type; |
There was a problem hiding this comment.
What is this property used for?
|
I am confused. The global style post type is registered to the REST API. gutenberg/lib/class-wp-theme-json-resolver-gutenberg.php Lines 444 to 445 in 9953bfc Is this REST API used for anything? If not this should be disabled no? CC @TimothyBJacobs ? |
|
Great suggestions @spacedmonkey I opened a follow-up PR here to address these #36071 |
This PR explores the global styles endpoint explored in #35141
At the moment, it only solves the "user global styles" use-cases, so there's no big difference with what we have on trunk aside of the fact that json parsing and serializing is moved to the server.
After that I'd like to explore:
getting rid ofDone in 369183e__experimentalGlobalStylesUserEntityId__experimentalGlobalStylesBaseConfigThe latter here might a bit complex because it will make the source for the base config is not a CPT but the theme file directly, so it's unclear to me at the moment whether it should be dedicated endpoints
global-styles/theme/{some_theme}or whether we should try to merge things using theidlike we did for templates (which feels too heavy for global styles)