-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Global styles variations PHP changes #47075
base: trunk
Are you sure you want to change the base?
Changes from 7 commits
da15655
d8f49fa
0e0ae98
88147e5
1fdcb58
44140a8
d9ed682
60b1b07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -429,12 +429,14 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post | |||||
$user_cpt = array(); | ||||||
$post_type_filter = 'wp_global_styles'; | ||||||
$stylesheet = $theme->get_stylesheet(); | ||||||
$post_name = "wp-global-styles-{urlencode($stylesheet)}"; | ||||||
$args = array( | ||||||
'posts_per_page' => 1, | ||||||
'orderby' => 'date', | ||||||
'order' => 'desc', | ||||||
'order' => 'asc', | ||||||
'post_type' => $post_type_filter, | ||||||
'post_status' => $post_status_filter, | ||||||
'post_name' => $post_name, | ||||||
'ignore_sticky_posts' => true, | ||||||
'no_found_rows' => true, | ||||||
'update_post_meta_cache' => false, | ||||||
|
@@ -453,19 +455,15 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post | |||||
if ( count( $recent_posts ) === 1 ) { | ||||||
$user_cpt = get_object_vars( $recent_posts[0] ); | ||||||
} elseif ( $create_post ) { | ||||||
$cpt_post_id = wp_insert_post( | ||||||
$cpt_post_id = self::add_user_global_styles_variation( | ||||||
array( | ||||||
'post_content' => '{"version": ' . WP_Theme_JSON_Gutenberg::LATEST_SCHEMA . ', "isGlobalStylesUserThemeJSON": true }', | ||||||
'post_status' => 'publish', | ||||||
'post_title' => 'Custom Styles', // Do not make string translatable, see https://core.trac.wordpress.org/ticket/54518. | ||||||
'post_type' => $post_type_filter, | ||||||
'post_name' => sprintf( 'wp-global-styles-%s', urlencode( $stylesheet ) ), | ||||||
'tax_input' => array( | ||||||
'wp_theme' => array( $stylesheet ), | ||||||
), | ||||||
), | ||||||
true | ||||||
'title' => 'Custom Styles', // Do not make string translatable, see https://core.trac.wordpress.org/ticket/54518. | ||||||
'global_styles' => '{"version": ' . WP_Theme_JSON_Gutenberg::LATEST_SCHEMA . ', "isGlobalStylesUserThemeJSON": true }', | ||||||
'stylesheet' => $stylesheet, | ||||||
'post_name' => $post_name, | ||||||
) | ||||||
); | ||||||
|
||||||
if ( ! is_wp_error( $cpt_post_id ) ) { | ||||||
$user_cpt = get_object_vars( get_post( $cpt_post_id ) ); | ||||||
} | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* | ||||||
* @param array $args { | ||||||
* Arguments to add a style variation. | ||||||
* | ||||||
* @type string title Required. Global styles variation name. | ||||||
* @type string global_styles Required. Global styles settings as a JSON string. | ||||||
* @type string $stylesheet Required. Slug of the theme associated with these global styles. | ||||||
* @type string $post_name Optional. Post name. | ||||||
* } | ||||||
* | ||||||
* @return int|WP_Error Post ID of the new variation or error if insertion failed. | ||||||
*/ | ||||||
public static function add_user_global_styles_variation( $args ) { | ||||||
$theme = wp_get_theme(); | ||||||
|
||||||
/* | ||||||
* Bail early if the theme does not support a theme.json. | ||||||
* | ||||||
* Since wp_theme_has_theme_json only supports the active | ||||||
* theme, the extra condition for whether $theme is the active theme is | ||||||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." |
||||||
} | ||||||
|
||||||
$post_args = array( | ||||||
'post_content' => $args['global_styles'], | ||||||
'post_status' => 'publish', | ||||||
'post_title' => $args['title'], | ||||||
'post_type' => 'wp_global_styles', | ||||||
'tax_input' => array( | ||||||
'wp_theme' => array( $args['stylesheet'] ), | ||||||
), | ||||||
); | ||||||
|
||||||
if ( ! empty( $args['post_name'] ) ) { | ||||||
$post_args['post_name'] = $args['post_name']; | ||||||
} | ||||||
|
||||||
$post_id = wp_insert_post( | ||||||
$post_args, | ||||||
true | ||||||
); | ||||||
|
||||||
if ( empty( $args['post_name'] ) ) { | ||||||
$post_id = wp_update_post( | ||||||
array( | ||||||
'ID' => $post_id, | ||||||
'post_name' => self::generate_user_style_variation_post_name( $post_id ), | ||||||
) | ||||||
); | ||||||
} | ||||||
|
||||||
return $post_id; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Generate a post name for a variation with the given ID. | ||||||
* | ||||||
* @since 6.2 | ||||||
* | ||||||
* @param int|null $variation_id Variation ID. | ||||||
* @return string Post name. | ||||||
*/ | ||||||
public static function generate_user_style_variation_post_name( $variation_id ) { | ||||||
$stylesheet = get_stylesheet(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
return "wp-global-styles-{$stylesheet}-variation-{$variation_id}"; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is |
||||||
} | ||||||
|
||||||
/** | ||||||
* Make an association between post $id and post containing current user | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
* global styles | ||||||
* | ||||||
* @since 6.2 | ||||||
* | ||||||
* @param int|null $id ID of the associated post. Null to delete the asociation. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a function that deletes the association instead of this |
||||||
* @return int|false Meta ID on success, false on failure. | ||||||
*/ | ||||||
public static function associate_user_variation_with_global_styles_post( $id ) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this argument be better named $post_id since it is a post id? |
||||||
$current_gs_id = static::get_user_global_styles_post_id(); | ||||||
|
||||||
if ( ( (int) $id ) === $current_gs_id ) { | ||||||
return false; | ||||||
} | ||||||
|
||||||
$prev_id = get_post_meta( $current_gs_id, 'associated_user_variation', true ); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the "user styles" to be linked to a "style variation" this way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👋🏻 what is AS and what is CUGS? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AS = Associated User Style Variation 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
why not? if you're convinced that it's bad one commit will stop these questions repeating 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I misunderstood 👍🏻 🙇🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we replace the if and the get post meta call with |
||||||
|
||||||
if ( empty( $prev_id ) && ! empty( $id ) ) { | ||||||
return add_post_meta( $current_gs_id, 'associated_user_variation', $id, true ); | ||||||
} | ||||||
|
||||||
if ( empty( $id ) ) { | ||||||
return delete_post_meta( $current_gs_id, 'associated_user_variation' ); | ||||||
} | ||||||
|
||||||
return update_post_meta( $current_gs_id, 'associated_user_variation', $id ); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Get associated variation ID. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should explain in the comments what an associated variation is. |
||||||
* | ||||||
* @since 6.2 | ||||||
* | ||||||
* @return int|null|false Meta ID or null on success, false on failure. | ||||||
*/ | ||||||
public static function get_associated_user_variation_id() { | ||||||
$current_gs_id = static::get_user_global_styles_post_id(); | ||||||
|
||||||
return get_post_meta( $current_gs_id, 'associated_user_variation', true ); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Returns the user's origin config. | ||||||
* | ||||||
|
@@ -724,4 +836,47 @@ public static function get_style_variations() { | |||||
} | ||||||
return $variations; | ||||||
} | ||||||
/** | ||||||
* Returns all style variations. | ||||||
* | ||||||
* @since 6.2.0 | ||||||
* | ||||||
* @return array | ||||||
*/ | ||||||
public static function get_user_style_variations() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
$stylesheet = get_stylesheet(); | ||||||
|
||||||
$args = array( | ||||||
'posts_per_page' => -1, | ||||||
'orderby' => 'date', | ||||||
'order' => 'desc', | ||||||
'post_type' => 'wp_global_styles', | ||||||
'post_status' => 'publish', | ||||||
'ignore_sticky_posts' => true, | ||||||
'no_found_rows' => true, | ||||||
'update_post_meta_cache' => false, | ||||||
'update_post_term_cache' => false, | ||||||
'tax_query' => array( | ||||||
array( | ||||||
'taxonomy' => 'wp_theme', | ||||||
'field' => 'name', | ||||||
'terms' => $stylesheet, | ||||||
), | ||||||
), | ||||||
); | ||||||
|
||||||
$global_style_query = new WP_Query(); | ||||||
$variation_posts = $global_style_query->query( $args ); | ||||||
$variations = array(); | ||||||
|
||||||
foreach ( $variation_posts as $variation_post ) { | ||||||
$decoded = json_decode( $variation_post->post_content, true ); | ||||||
$variation = ( new WP_Theme_JSON_Gutenberg( $decoded ) )->get_raw_data(); | ||||||
$variation['title'] = $variation_post->post_title; | ||||||
$variation['id'] = $variation_post->ID; | ||||||
$variations[] = $variation; | ||||||
} | ||||||
|
||||||
return $variations; | ||||||
} | ||||||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this comment: #47075 (comment)
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