Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Global styles variations PHP changes #47075

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Global styles variations PHP changes #47075

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
da15655
Global styles variations PHP changes
ribaricplusplus Jan 11, 2023
d8f49fa
Merge remote-tracking branch 'origin/trunk' into rpp-php-changes
ribaricplusplus Jan 13, 2023
0e0ae98
Fixes
ribaricplusplus Jan 13, 2023
88147e5
Merge trunk
ribaricplusplus Jan 20, 2023
1fdcb58
WIP: Change post name
ribaricplusplus Jan 21, 2023
44140a8
Merge remote-tracking branch 'origin/trunk' into rpp-php-changes
ribaricplusplus Jan 21, 2023
d9ed682
Change post name
ribaricplusplus Jan 21, 2023
60b1b07
Update
ribaricplusplus Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 138 additions & 12 deletions lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
$args = array(
'posts_per_page' => 1,
'orderby' => 'date',
'order' => 'desc',
'order' => 'asc',
'post_type' => $post_type_filter,
'post_status' => $post_status_filter,
'ignore_sticky_posts' => true,
Expand All @@ -453,19 +453,14 @@ 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,
)
);

if ( ! is_wp_error( $cpt_post_id ) ) {
$user_cpt = get_object_vars( get_post( $cpt_post_id ) );
}
Expand All @@ -474,6 +469,94 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
return $user_cpt;
}

// @codingStandardsIgnoreStart Squiz.Commenting.FunctionComment.ParamCommentFullStop
/**
* Saves a new user variation into the database.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Saves a new user variation into the database.
* Saves a new user style variation into the database.

*
*
* @param array $args Arguments. All are required.
* {
* @type string title Global styles variation name.
* @type string global_styles Global styles settings as a JSON string.
* @type string $stylesheet Slug of the theme associated with these global styles.
* }
*
* @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' ) );
Copy link
Contributor

Choose a reason for hiding this comment

The 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_id = wp_insert_post(
array(
'post_content' => $args['global_styles'],
'post_status' => 'publish',
'post_title' => $args['title'],
'post_type' => 'wp_global_styles',
'post_name' => sprintf( 'wp-global-styles-%s', urlencode( $args['stylesheet'] ) ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

'tax_input' => array(
'wp_theme' => array( $args['stylesheet'] ),
),
),
true
);

return $post_id;
}
// @codingStandardsIgnoreEnd Squiz.Commenting.FunctionComment.ParamCommentFullStop

/**
* Make an association between post $id and post containing current user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Make an association between post $id and post containing current user
* Make an association between $post_id and post containing current user

* global styles
*
* @since 6.2
*
* @param int|null $id ID of the associated post. Null to delete the asociation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a function that deletes the association instead of this null type for the $id argument?

* @return int|false Meta ID on success, false on failure.
*/
public static function associate_user_variation_with_global_styles_post( $id ) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 );
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

@ribaricplusplus ribaricplusplus Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋🏻 what is AS and what is CUGS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood 👍🏻 🙇🏻

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we replace the if and the get post meta call with get_associated_user_variation_id which I see defined later?


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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
*
Expand Down Expand Up @@ -698,4 +781,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() {
Copy link
Member

Choose a reason for hiding this comment

The 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 wp_global_styles CPT: the "user styles" and the "user style variations". Should the "user styles" be listed in the "custom style variations" UI?

Copy link
Member Author

@ribaricplusplus ribaricplusplus Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The 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 $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.

$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;
}
}
Loading