-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Make global styles data filterable #44015
Conversation
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.
YES!!
I've tried adding filters multiple times but it was too soon for those PRs. I think now we're at a point in time where we can do this with confidence.
I only had one note for this PR, and this is the addition of a global_styles_user
filter 👍
$result->merge( static::get_theme_data() ); | ||
$result->merge( apply_filters( 'global_styles_core', static::get_core_data() ) ); | ||
$result->merge( apply_filters( 'global_styles_blocks', static::get_block_data() ) ); | ||
$result->merge( apply_filters( 'global_styles_theme', static::get_theme_data() ) ); | ||
if ( 'custom' === $origin ) { | ||
$result->merge( static::get_user_data() ); | ||
} |
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.
Just in the line below there's set_spacing_sizes
, a configuration for spacing presets that does not follow the normal algorith (core > theme > user), so it wouldn't be filterable. I need to figure out how that works and whether it can work like any other theme.json
setting. See https://github.com/WordPress/gutenberg/pull/41527/files#r966773252
With the current proposal, this is what a theme should do to provide dynamic values: <?php
function filter_global_styles_theme( $theme_json ){
$dynamic_data = array(
'version' => 2,
'settings' => array(
'color' => array(
'text' => false, /* New setting */
'palette' => array( /* New palette */
array(
'slug' => 'foreground',
'color' => 'red',
'name' => 'Foreground',
),
array(
'slug' => 'New foreground',
'color' => 'hotpink',
'name' => 'New foreground',
),
),
),
),
);
$theme_json->merge( new WP_Theme_JSON( $dynamic_data, 'theme' ) );
return $theme_json;
}
add_filter( 'global_styles_theme', 'filter_global_styles_theme' ); A few things here:
|
Pushed a few changes, and now consumers can use the new (edit: pushed 5d3b638 so <?php
function filter_global_styles_theme( $existing ){
$dynamic_data = array(
'version' => 2,
'settings' => array(
'color' => array(
'text' => false, /* New setting */
'palette' => array( /* New palette */
array(
'slug' => 'foreground',
'color' => 'red',
'name' => 'Foreground',
),
array(
'slug' => 'New foreground',
'color' => 'hotpink',
'name' => 'New foreground',
),
),
),
),
);
$filtered = gutenberg_merge_theme_json( $existing, $dynamic_data );
return $filtered;
}
add_filter( 'global_styles_theme', 'filter_global_styles_theme' ); |
<?php | ||
|
||
function gutenberg_merge_theme_json( $existing_data, $incoming_data, $origin = null ) { | ||
if ( $origin === null && doing_filter( 'global_styles_filter_theme' ) ) { |
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.
By filling the $origin
depending on which filter is running, we avoid asking consumers to do it themselves. Though if the consumer does provide it, we use it instead.
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.
I like this. It abstracts away this requirement which might not be easily understood by consumers implementing the filter.
As mentioned in Slack, absolutely in favor of adding this. Low level hooks at all the levels in which we construct global style objects is necessary and should provide flexibility for plugins and sites to do a lot of creative things. |
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.
I think the direction of this is good. I do wonder if there should be some validation of returned filter values. While I appreciate the availability of gutenberg_merge_them_json
to handle merging, it's not something intuitive for consumers to implement on whatever they are merging into existing objects. There's also increased fragility. I wonder if it'd be better to pass through an instance of a WP_Theme_JSON
object and consumers would be expected to use the merge
method and then return an instance of WP_Theme_JSON
(which could be validated).
So for instance the theme filter would be something like this:
<?php
$unfiltered_json_data = new WP_Theme_JSON_Gutenberg( $theme_json_data );
try {
$theme_json_data = apply_filters( 'global_styles_theme', $unfiltered_json_data );
if ( ! $theme_json_data instanceof WP_Theme_JSON ) {
throw new Exception( 'Incorrect usage of filter' );
}
} catch (Exception $e) {
// just handling incorrect usage of filter and resetting to unfiltered data
$theme_json_data = $unfiltered_json_data;
}
static::$theme = $theme_json_data;
And implementation would be:
function filter_global_styles_theme( WP_Theme_JSON $theme_json ){
$dynamic_data = array(
'version' => 2,
'settings' => array(
'color' => array(
'text' => false, /* New setting */
'palette' => array( /* New palette */
array(
'slug' => 'foreground',
'color' => 'red',
'name' => 'Foreground',
),
array(
'slug' => 'New foreground',
'color' => 'hotpink',
'name' => 'New foreground',
),
),
),
),
);
/**
* This would be a new method on WP_Theme_JSON for merging in raw data.
* It internally handles converting the dynamic data to a WP_Theme_JSON object
* and the consumer would not need to provide $origin.
*/
$theme_json->merge_raw( $dynamic_data );
return $theme_json;
}
add_filter( 'global_styles_theme', 'filter_global_styles_theme' );
I think this goes a bit back towards your original solution with the change of implementing a new method on WP_Theme_JSON
for handling merging raw data.
<?php | ||
|
||
function gutenberg_merge_theme_json( $existing_data, $incoming_data, $origin = null ) { | ||
if ( $origin === null && doing_filter( 'global_styles_filter_theme' ) ) { |
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.
I like this. It abstracts away this requirement which might not be easily understood by consumers implementing the filter.
The returned values will be validated in the same way a |
Having tested both approaches, using a simple array is more familiar and ergonomic for consumers: it's a construct people know how to work with in the language, and it mimics the mechanics of At first sight, the two different approaches may look similar: $existing->merge_raw( $new_data ); vs gutenberg_merge_theme_json( $existing, $new_data ); Though the 1st approach forces us to making |
/** | ||
* Class to update with a theme.json structure. | ||
*/ | ||
class WP_Theme_JSON_Data { |
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.
Apologies for commenting just after this PR merged, but should this line have a Gutenberg
suffix, or have a class_exists
check before defining it, so that we don't run into any issues when this is backported to core?
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.
Yes, it'd be best to call class_exists
before requiring this file.
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.
+1
I'm a bit late since this was already merged, but I'll leave my feedback anyway; this implementation makes sense and looks good to me 👍 |
Great job bringing this to the finish line @oandregal. The way you implemented it is even better than what I initially drafted in #44015 (comment), as the new class might be applicable in different contexts, too. It might seem like overhead adding a wrapper class at first glance, but it also gives you space for future optimizations as you have control over the input and output value of the filter. As a follow-up, we need to document all new filters inline in the codebase like: This is going to be necessary when backporting to WordPress core. In addition to that, we could extend documentation in the block editor handbook. It might even be worth adding a new document specific to themes in: https://github.com/WordPress/gutenberg/tree/trunk/docs/reference-guides/filters |
$config = apply_filters( 'global_styles_default', new WP_Theme_JSON_Data( $config, 'default' ) ); | ||
static::$core = new WP_Theme_JSON_Gutenberg( $config->get_data(), 'default' ); |
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.
With WP filters we need to return the same data type as in the input, so that should be $data
instead of config, so maybe the following would be more accurate:
$data = apply_filters( 'global_styles_default', new WP_Theme_JSON_Data( $config, 'default' ) );
static::$core = new WP_Theme_JSON_Gutenberg( $data->get_config(), 'default' );
I see the same pattern in other places. WP_Theme_JSON
lists $theme_json
in the constructor:
Maybe, we could rename that to $theme_config
in WP core and here it would be $data->get_theme_config()
?
@nerrad @andrewserong @gziolo Addressed your feedback at #44109 |
#44111 documents the new filters in the block editor handbook |
I've added the Needs Dev Note label here because I think this should get a dev note for the 6.1 release. CC: @bph |
The devnote content can actually be the same as the documentation. It can work either as a standalone note or as part of a larger one. Would we publish a consolidated devnote for all global styles work in WordPress 6.1? (I've been out a few months in this cycle and don't have a good context of what has changed). |
#44159 proposes we rename the filters from |
This is being backported as part of WordPress/wordpress-develop#3247 |
@oandregal Yes, I think you should. I am working through collected all the PRs but I am uncertain whether I get them all. 😸 |
I like how this PR opens the door for updating the global styles in plugins without introducing new core APIs. That being said, I’m a bit worried, too. Imagine 5 different plugins competing to update similar config values. I can't think of a clean way of solving it, but if this was a declarative API then it could at least run some checks and notify developers about, say, conflicting updates. Perhaps I’m thinking of |
…6.1. This changeset ports the work done in Gutenberg (released in 14.1) to add hooks to filter the `theme.json` data. Specifically, it adds the following filters: `theme_json_default`, `theme_json_blocks`, `theme_json_theme`, and `theme_json_user`. For more details, see the following Gutenberg pull requests: - [WordPress/gutenberg#44015 gutenberg#44015]: Make global styles data filterable - [WordPress/gutenberg#44109 gutenberg#44109]: Prepare `WP_Theme_JSON_Data` class for backporting - [WordPress/gutenberg#44159 gutenberg#44159]: Update hook's names from `global_styles_*` to `theme_json_*` Props oandregal, czapla, gziolo, bernhard-reiter. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54251 602fd350-edb4-49c9-b593-d223f7449a82
…6.1. This changeset ports the work done in Gutenberg (released in 14.1) to add hooks to filter the `theme.json` data. Specifically, it adds the following filters: `theme_json_default`, `theme_json_blocks`, `theme_json_theme`, and `theme_json_user`. For more details, see the following Gutenberg pull requests: - [WordPress/gutenberg#44015 gutenberg#44015]: Make global styles data filterable - [WordPress/gutenberg#44109 gutenberg#44109]: Prepare `WP_Theme_JSON_Data` class for backporting - [WordPress/gutenberg#44159 gutenberg#44159]: Update hook's names from `global_styles_*` to `theme_json_*` Props oandregal, czapla, gziolo, bernhard-reiter. See #56467. Built from https://develop.svn.wordpress.org/trunk@54251 git-svn-id: http://core.svn.wordpress.org/trunk@53810 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…6.1. This changeset ports the work done in Gutenberg (released in 14.1) to add hooks to filter the `theme.json` data. Specifically, it adds the following filters: `theme_json_default`, `theme_json_blocks`, `theme_json_theme`, and `theme_json_user`. For more details, see the following Gutenberg pull requests: - [WordPress/gutenberg#44015 gutenberg#44015]: Make global styles data filterable - [WordPress/gutenberg#44109 gutenberg#44109]: Prepare `WP_Theme_JSON_Data` class for backporting - [WordPress/gutenberg#44159 gutenberg#44159]: Update hook's names from `global_styles_*` to `theme_json_*` Props oandregal, czapla, gziolo, bernhard-reiter. See #56467. Built from https://develop.svn.wordpress.org/trunk@54251 git-svn-id: https://core.svn.wordpress.org/trunk@53810 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…6.1. This changeset ports the work done in Gutenberg (released in 14.1) to add hooks to filter the `theme.json` data. Specifically, it adds the following filters: `theme_json_default`, `theme_json_blocks`, `theme_json_theme`, and `theme_json_user`. For more details, see the following Gutenberg pull requests: - [WordPress/gutenberg#44015 gutenberg#44015]: Make global styles data filterable - [WordPress/gutenberg#44109 gutenberg#44109]: Prepare `WP_Theme_JSON_Data` class for backporting - [WordPress/gutenberg#44159 gutenberg#44159]: Update hook's names from `global_styles_*` to `theme_json_*` Props oandregal, czapla, gziolo, bernhard-reiter. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54251 602fd350-edb4-49c9-b593-d223f7449a82
What?
This PR tries making the different layers of global styles data filterable.
Why?
Some use cases become a lot easier to do:
How?
By creating a filter for each data layer of Global Styles that 3rd parties can use:
global_styles_default
=> to filter data provided by coreglobal_styles_blocks
=> to filter data provided by the blocks (only styles so far)global_styles_theme
=> to filter data provided by the themeglobal_styles_user
=> to filter data provided by the userPrior work
There were previous attempts to add filters to global styles that didn't make the cut for different reasons.
wp_get_global_*
functions) Add filters to shortcircuit the public API of global styles #36909 by @oandregalTesting Instructions
Use the filters to modify some of the data. For example, this is how the theme data can be modified:
TODO / Follow-ups
set_spacing_sizes
filterable as well. See https://github.com/WordPress/gutenberg/pull/41527/files#r966773252The changes for the resolver live inThe changes that affect this PR have been ported.lib/experimental
though some are expected to be ported to 6.1.