Global styles variations PHP changes #47075
Conversation
ribaricplusplus
added
[Type] Enhancement
|
Flaky tests detected in d9ed682. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3975033935
|
| * @param WP_Theme $theme Theme data. | ||
| * @return array Links for the given block type. | ||
| */ | ||
| protected function prepare_links( $theme ) { |
There was a problem hiding this comment.
This method is equal to the upstream at https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-themes-controller.php#L358 but using the gutenberg classes.
There was a problem hiding this comment.
Yep, because the regular WP_Theme_JSON_Resolver fetches wp_global_styles posts in descending order. The new functionality assumes that the "current user global styles" are always the first wp_global_styles post that is made (per theme), so it has to be in ascending order.
There was a problem hiding this comment.
So. I've been educating myself about how this currently works and the changes here. Also checked the issue and original PR for context. Let me know if I should check any other things.
Would this be a fair representation of how this works?
- The way this PR adds support for user style variations is by storing them as new
wp_global_stylesCPTs. - Before this PR, there was only a single
wp_global_stylesCPT per theme. We took those user styles to merge them with the others and generate the global stylesheet. - Now, there can be multiple
wp_global_stylesCPT: as many as user style variations + user styles if no style variation is selected.
Once we store everything under the same CPT, how do tell apart the "user styles" from the "user style variations"?
This PR proposes the "user styles" to be the older CPT. Can this be problematic? For example, if the post date changes in uncontrolled situations (exporting/importing the data to a different site?). I am not certain which specific example would break this, though I'd be more comfortable if we can tell apart "user styles" CPT by something explicit, that cannot change based on external factors.
Are there alternatives to using post_date? For example, what would you think of using the post_status field? Perhaps the "user styles" can have post_status=publish while the "user style variations" use the post_status=draft.
There was a problem hiding this comment.
I'll be using the terminology established in the main comment of this issue (I made some edits). CUGS is different from custom styles by being the oldest and by having an associated_user_variation meta key. I think being the oldest is sufficient for all use cases. Even if export / import happens, the post date should be maintained. And if the export / import is done in a way that post_date is not maintained, who is to say that post_status will be?
I'm in favor of keeping complexity lower if possible. If there is ever a scenario discovered in some weird edge case that this approach fails, we could easily do something like add an option cugs_id_{$stylesheet} to wp_options that specifies which post exactly is CUGS. This would make it 100% resilient.
There was a problem hiding this comment.
I slept on the use of post_date as an identifier for the user styles, and I'm still unconvinced it's safe. By depending on an external factor (the clock in a computer) the code is not deterministic: if the clock of any computer that creates global styles goes out of sync, this code will certainly introduce errors. At the scale of WordPress, that's bound to happen at some point, somewhere. This is a "fun" bug that happened because of clocks! That's what I want to avoid.
I'm not sure about what's the best alternative, though, so happy to explore any. This is something we need to figure it out before deploying this. It's not something we can fix after. Once there's user data in the database, it's a lot harder to fix (if at all possible).
There was a problem hiding this comment.
Alrighty. How about we go for the approach to add cugs_id_{$stylesheet} to wp_options?
There was a problem hiding this comment.
re: terminology. I'd appreciate if we can stick to the existing nomenclature:
- user styles for the data that is merged with core, blocks, and theme styles to generate the global stylesheet
- user style variations: for the different set of styles already preconfigured
The WordPress styles sub-system is already a quite loaded domain and a there has been a few people working on it for a while. Changing/Updating established terminology is bound to be confusing.
There was a problem hiding this comment.
I updated the main PR comment with these terminology definitions so things are clearer, I will use that terminology going forward but I won't edit all of my other comments.
In that case, the options key would be called user_styles_id_{$stylesheet} in the database. Should we do that?
There was a problem hiding this comment.
Alrighty. How about we go for the approach to add cugs_id_{$stylesheet} to wp_options?
Do we have alternatives that involve extracting that information from the data we already have instead of querying/linking options? I mean, I don't think it'd be a big deal, though it'd be nice/simpler if we don't have to.
In light of this other conversation, do you think it'd be enough to use the post_name?
- The user styles CPT would be stored as
wp-global-styles-${stylesheet}. - The style variations CPTs would be stored as
wp-global-styles-${stylesheet}-variation-${variation-identifier}.
There was a problem hiding this comment.
@oandregal I made this change. Now the user styles CPT is stored as wp-global-styles-${stylesheet} whereas custom style variations get a different name. Also, when user styles CPT is queried, it checks the post name, so now it no longer depends only on the post date but also on the post name.
|
Hey @ribaricplusplus Thanks for splitting your work in smaller steps :) I'm taking a look at this and educating myself about the changes here. Would you be able to provide some description of the changes done here as well as testing steps? |
Thank you for taking a look into this! It's a little hard to provide testing instructions for these PHP changes. I added a few PHPUnit tests that cover some of the functionality, but there is not really a way to test the PHP changes unless you do curl requests to the REST API or something. I guess I could write how to do that, and I will definitely write a description of the changes and how things work. I will ping you when I write those. Testing can only really be done manually in a feasible way when both PHP and JS changes are done. Then you can test the functionality by performing the actions that I showed in the screencasts of the main PR #46952. I will try to think about how to test this PHP stuff though |
| @@ -16,7 +16,288 @@ class Gutenberg_REST_Global_Styles_Controller_6_2 extends WP_REST_Global_Styles_ | |||
| * @return void | |||
| */ | |||
| public function register_routes() { | |||
| parent::register_routes(); | |||
| register_rest_route( | |||
There was a problem hiding this comment.
The changes in the routes:
| Route | Before | After |
|---|---|---|
global-styles/themes/<stylesheet>/variations/ |
READABLE. Lists the variations of the active theme. | Same. |
global-styles/themes/<stylesheet>/ |
READABLE. Lists the theme.json of the active theme. | Same. |
global-styles/<cpt_id>/ |
READABLE+EDITABLE. Lists and updates the user styles. | READABLE+EDITABLE+DELETABLE. List, updates, and deletes user styles. |
global-styles/ |
- | READABLE+CREATABLE. Lists and creates user style variations. |
| return false; | ||
| } | ||
|
|
||
| $prev_id = get_post_meta( $current_gs_id, 'associated_user_variation', true ); |
There was a problem hiding this comment.
Why do we need the "user styles" to be linked to a "style variation" this way?
There was a problem hiding this comment.
It's used in JS later to keep the changes that are made to custom styles in sync with the saved and associated custom style. Then when the user clicks the "Save" button in Gutenberg, they have the option to untick the checkbox that also pushes the changes to the associated style variation. If you look at the main issue comment, you will see that there are plans to add an indication that the saved style has some changes (with the little blue dot), so clearly the intent is to keep the CUGS in sync with AS, with the possibility to discard changes to AS and keep them only in CUGS.
There was a problem hiding this comment.
👋🏻 what is AS and what is CUGS?
There was a problem hiding this comment.
AS = Associated User Style Variation
CUGS = Current User Global Styles
I've explained this terminology in the initial PR "comment", it's bad, we've decided to switch terminology but I don't want to edit all comments now.
There was a problem hiding this comment.
but I don't want to edit all comments now.
why not? if you're convinced that it's bad one commit will stop these questions repeating 😁
There was a problem hiding this comment.
I'm referring to my comments here in GitHub, if there are problems in code comments or code itself that's a different thing but I don't think that's the case, I didn't refer to CUGS / AS in code comments.
There was a problem hiding this comment.
Should we replace the if and the get post meta call with get_associated_user_variation_id which I see defined later?
| * | ||
| * @return array | ||
| */ | ||
| public static function get_user_style_variations() { |
There was a problem hiding this comment.
This method is the one used by the REST endpoint, and it returns the user style variations to be shown in the UI. I see it returns all wp_global_styles CPT: the "user styles" and the "user style variations". Should the "user styles" be listed in the "custom style variations" UI?
There was a problem hiding this comment.
That's the only place where this method is used currently, so in that sense, no, we shouldn't return CUGS from this method. It actually causes more JS work later (which I've done) because CUGS needs to be filtered out. To be clear, because I filter it out in JS, the CUGS is not shown in the Custom Style Variations UI.
But when I was writing this method, I was thinking of it as something that might be used in general to get all user global styles, without worrying about the arguments that need to be passed to WP_Query. Perhaps it will be used by plugins or other WP core places.
I'm not sure what the right answer is here. When I first wrote this method I didn't include CUGS, later I reverted because I was like... Well, the REST endpoint to get all global styles should actually get all global styles. That's how it works for posts and other objects. So, I'm not sure. If you think I should exclude CUGS, let me know and I'll do it. But then there is an inconsistency where you can use the REST API endpoint to fetch CUGS, but if you query the endpoint to get all global styles CUGS is not included.
Maybe the method should have an $include_cugs argument, which is set to true in the REST API? But then we're adding functionality that is not currently used and we're not sure if it will ever be used.
|
Hey @ribaricplusplus this is not a small change and has some ramifications, thanks for working on it! The overall direction sounds good to me. I've left some questions in comments, mainly for me to learn how this area works and for grabbing your thoughts on potential use cases that could be problematic. I also want to share that I'm going to be out of work for 10 days or so, so I don't know about my availability to do reviews during that time. @youknowriad and @jorgefilipecosta should be able to provide thoughts on this topic, as they implemented this if my memory doesn't fail me. I'll resume my review when I'm back if nobody else has picked this up. |
| 'post_status' => 'publish', | ||
| 'post_title' => $args['title'], | ||
| 'post_type' => 'wp_global_styles', | ||
| 'post_name' => sprintf( 'wp-global-styles-%s', urlencode( $args['stylesheet'] ) ), |
There was a problem hiding this comment.
Can posts of the same type (wp_global_styles) share the same post_name? Thinking we could use something along the lines of wp-global-styles-stylesheet-variation-urlencode(title). I presume two variations of the same theme shouldn't have the same title. We should check for that (also in the client).
ribaricplusplus
requested review from
noahtallen,
ellatrix,
fluiddot,
gziolo,
adamsilverstein,
talldan and
swissspidy
as code owners
ribaricplusplus
requested review from
ajitbohra,
kevin940726,
ntwb,
nerrad,
ryanwelcher,
draganescu,
tellthemachines,
adamziel,
getdave,
noisysocks,
dmsnell,
atimmer,
geriux,
mmtr,
ajlende,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: -19.6 kB (-1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
| 'post_type' => $post_type_filter, | ||
| 'post_status' => $post_status_filter, | ||
| 'post_name' => $post_name, |
There was a problem hiding this comment.
Why is this needed? There should only ever be one global style post with the taxonomy term of the stylesheet slug.
There was a problem hiding this comment.
See this comment: #47075 (comment)
There should only ever be one global style post with the taxonomy term of the stylesheet slug.
Not true, the point of this PR is to make it possible to make more global styles posts (custom user variations). This PR is a split of the main PR that makes this possible #46952
| public static function generate_user_style_variation_post_name( $variation_id ) { | ||
| $stylesheet = get_stylesheet(); |
There was a problem hiding this comment.
Change $stylesheet to be param to the method and pass it in.
|
@oandregal Could you let me know if this feature is something that has been entirely abandoned or is it just on hold for now? |
| * present here. | ||
| */ | ||
| if ( $theme->get_stylesheet() === $args['stylesheet'] && ! wp_theme_has_theme_json() ) { | ||
| return new WP_Error( __( 'Theme does not have theme.json', 'gutenberg' ) ); |
There was a problem hiding this comment.
Is there any way to make the erorr more helpful? Maybe "Theme does not support theme.json. User style variations only work with themes that support theme.json."
| @@ -474,6 +472,120 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post | |||
| return $user_cpt; | |||
| } | |||
|
|
|||
| /** | |||
| * Saves a new user variation into the database. | |||
There was a problem hiding this comment.
| * Saves a new user variation into the database. | |
| * Saves a new user style variation into the database. |
| * @return string Post name. | ||
| */ | ||
| public static function generate_user_style_variation_post_name( $variation_id, $stylesheet ) { | ||
| return "wp-global-styles-{$stylesheet}-variation-{$variation_id}"; |
There was a problem hiding this comment.
When is generate_user_style_variation_post_name called with $variation_id as null?
| * | ||
| * @since 6.2 | ||
| * | ||
| * @param int|null $id ID of the associated post. Null to delete the asociation. |
There was a problem hiding this comment.
Can we have a function that deletes the association instead of this null type for the $id argument?
| } | ||
|
|
||
| /** | ||
| * Make an association between post $id and post containing current user |
There was a problem hiding this comment.
| * Make an association between post $id and post containing current user | |
| * Make an association between $post_id and post containing current user |
| $changes->ID = $request['id']; | ||
| $post = get_post( $request['id'] ); | ||
| $changes = new stdClass(); | ||
| if ( ! empty( $request['id'] ) ) { |
There was a problem hiding this comment.
What was wrong here before?
| } | ||
|
|
||
| /** | ||
| * Retrieves the global styles type' schema, conforming to JSON Schema. |
There was a problem hiding this comment.
| * Retrieves the global styles type' schema, conforming to JSON Schema. | |
| * Retrieves the global styles type schema, conforming to JSON Schema. |
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'id' => array( | ||
| 'description' => __( 'ID of global styles config.', 'gutenberg' ), |
There was a problem hiding this comment.
| 'description' => __( 'ID of global styles config.', 'gutenberg' ), | |
| 'description' => __( 'ID of global style.', 'gutenberg' ), |
| 'context' => array( 'view', 'edit', 'embed' ), | ||
| ), | ||
| 'rendered' => array( | ||
| 'description' => __( 'HTML title for the post, transformed for display.', 'gutenberg' ), |
There was a problem hiding this comment.
| 'description' => __( 'HTML title for the post, transformed for display.', 'gutenberg' ), | |
| 'description' => __( 'Title for the global styles variation, transformed for display.', 'gutenberg' ), |
| <Button | ||
| variant="primary" | ||
| onClick={ handleSubmit } | ||
| className="xblock-editor-link-control__search-submit" |
There was a problem hiding this comment.
xblock-editor-link is the x a typo?
This PR contains the PHP changes needed to add save and delete functionality for Global Styles. It's one part of the main PR: #46952
Description of changes
Terminology (thanks @oandregal)
I will try to write about the changes here as the review is ongoing.
This PR adds endpoints for global styles so that they can be used with the entities functionality from
@wordpress/core-data.It is assumed that the first
wp_global_stylesposts that is created for a theme represents user styles (I refer to them as CUGS in this PR). Then all of the other posts represent variations that were saved later.CUGS has an
associated_user_variationmeta key. Otherwp_global_stylesposts don't. This key represents which saved global style is associated with the CUGS (if any, let's call it AS) so that changes can be kept in sync in JavaScript code.AS does not necessarily need to be the same as CUGS, but when changes are made in JavaScript the user will have the option to sync them up.
Changes in REST API endpoints for the global styles controller (credits @oandregal comment below)
global-styles/themes/<stylesheet>/variations/global-styles/themes/<stylesheet>/global-styles/<cpt_id>/global-styles/