Theme Export: Add missing properties to theme.json #39590

scruffian · 2022-03-18T19:01:05Z

What?

This changes our approach to getting and merging theme.json data when a user exports a theme.

Why?

The export should be a copy of the theme, including the user changes. We also should include everything that was in the original theme.json from the theme.

How?

This abstracts the merge function to a static function, so that it can be used by any two arrays. This gives us the freedom to call it from other contexts and reuse the same logic.

Testing Instructions

Switch to TT2
Export a theme from the Site Editor
Confirm that the $schema and settings > appearanceTools attributes are in the theme.json file.
Make some user edits (e.g. change the site background color, and try changing the settings for a particular block)
Do another export
Confirm that the original settings from TT2 are still present, but combined with the user settings made above.

Notes

This exports theme.json with spaces not tabs. We should convert this to tabs.

pbking · 2022-03-18T20:03:01Z

I'm a bit concerned not just that the abstraction 'appearanceTools' was missing from theme.json but that all that it represented was also added. This breaks the abstraction since now things like settings.color.link are now represented by two properties in theme.json.

If the export tool is explicitly aware of the 'appearanceTools' attribute then it (or get_data() from whence it gets the details) may need to keep out of the configuration the things that it represents.

However, it's better to have it transition over than not... this fixes MOST of the problem (where it's understood that the author's original intent was to support all 'appearance tools' both present and future).

scruffian · 2022-03-18T20:34:21Z

Ah yeah that's a good point, maybe we should remove the added properties? I might have another idea how we can do this...

lib/compat/wordpress-6.0/block-template-utils.php

Co-authored-by: Andrei Draganescu <[email protected]>

scruffian · 2022-03-22T12:33:31Z

lib/compat/wordpress-5.9/class-wp-theme-json-5-9.php

@@ -14,7 +14,7 @@
 *
 * @access private
 */
-class WP_Theme_JSON_5_9 {
+class WP_Theme_JSON_5_9 extends WP_Theme_JSON {


This is necessary as the merge function inherits some functions from the original base class.

scruffian · 2022-03-22T12:34:17Z

lib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.php

+	 * @param array $replacements Data to add onto $base
+	 * @return array The result of the merge
+	 */
+	public static function functional_merge( $base, $replacements ) {


The idea of this function is that it will work with any 2 arrays and do the same thing, so we can use the same logic without being in context of an object.

scruffian · 2022-03-22T12:43:05Z

@pbking I updated the approach here... thoughts?

pbking · 2022-03-22T14:11:02Z

lib/compat/wordpress-6.0/block-template-utils.php

+			$merged_data = WP_Theme_JSON_Gutenberg::functional_merge( $theme_json_data, $user_data->get_raw_data() );
+		}
+	} else {
+		$merged_data = $user_data;


It would be super if the complexity of merging this data together into a snapshot JSON string were something abstracted in WP_Theme_JSON_Gutenberg so that getting that could be a function call instead of repeating this logic.

pbking · 2022-03-22T14:12:33Z

I'm afraid the latest blows up for me when I switch to the branch.

Not sure why, unable to evaluate fully.

I definitely better like the way the bits are merged more abstractly rather than peeking and adding specific entries. I haven't been able to 100% follow everything here but even so definitely more comfortable with this direction.

scruffian · 2022-03-23T11:44:18Z

I've fixed the issue above, and also created a helper function to get the merged data, so the logic lives in the class. Ready for another review...

pbking · 2022-03-23T18:49:13Z

Tested again and while the value appearanceTools isn't being removed (and nothing new added) I'm getting this when I change colors with the FSE (and it is working properly on trunk):

    "settings": {
        "appearanceTools": true,
        "color": {
            "palette": {
                "0": {
                    "slug": "primary",
                    "color": "#776c4e",
                    "name": "Primary"
                },
                "1": {
                    "slug": "secondary",
                    "color": "#a19982",
                    "name": "Secondary"
                },
                "2": {
                    "slug": "foreground",
                    "color": "#000000",
                    "name": "Foreground"
                },
                "3": {
                    "slug": "background",
                    "color": "#ffffff",
                    "name": "Background"
                },
                "4": {
                    "slug": "tertiary",
                    "color": "#f1ede6",
                    "name": "Tertiary"
                },
                "theme": [
                    {
                        "slug": "primary",
                        "color": "#0e00ff",
                        "name": "Primary"
                    },
                    {
                        "slug": "secondary",
                        "color": "#a19982",
                        "name": "Secondary"
                    },
                    {
                        "slug": "foreground",
                        "color": "#000000",
                        "name": "Foreground"
                    },
                    {
                        "slug": "background",
                        "color": "#ffffff",
                        "name": "Background"
                    },
                    {
                        "slug": "tertiary",
                        "color": "#f1ede6",
                        "name": "Tertiary"
                    }
                ]
            },
...

oandregal · 2022-03-25T08:41:05Z

lib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.php

+	 */
+	public static function get_merged_theme_json() {
+		// Get the user data
+		$user_data = WP_Theme_JSON_Resolver_Gutenberg::get_user_data();


We've been trying to make the WP_Theme_JSON class agnostic from where the data comes from and from WordPress APIs (get_stylesheet, etc), while the WP_Theme_JSON_Resolver responsibilitiy is to mangle data together. I think we need to keep concerns separate.

oandregal · 2022-03-25T08:50:53Z

The export should be a copy of the theme, including the user changes. We also should include everything that was in the original theme.json from the theme.

I understand you want appearanceTools as part of the export. That's a valid point and it's worth investigating. I disagree that "everything" in the theme.json of the theme should be part of the export: this ignores important processes such as sanitization, migration to a newer theme.json version, etc. We should keep those in place.

The core issue seems to be that appearanceTools is considered an "alias": it has no function other than "pre-configure" some other flags, and then is unset. What if we tried this?

do not unset appearanceTools in do_opt_in_into_settings
modify the existing get_data method to consider the value of appearanceTools and "undo" what it did in do_opt_in_into_settings

scruffian · 2022-03-25T10:03:40Z

@oandregal sounds like a good idea. What about $schema?

oandregal · 2022-03-25T10:17:39Z

What about $schema?

That's a tricky one. If we tap into the WP_Theme_JSON code, anything that's invalid at runtime, we can't export.

Options I see:

Do not rely on WP_Theme_JSON and do a different thing, so we can get $schema or any other (invalid at runtime, potentially insecure) info the theme provides.
Add $schema to the allowed list of settings. I'm reluctant to bend the system in this way, as the valid data is carried through the data flow (it's sent to the client and stored there).
Ignore $schema from the export.

For $schema I'd go with 3. This is my thinking: this property is helpful to get type info in the editors. Though, if the dev is editing the theme.json can't they as well add that manually?

pbking · 2022-03-25T12:57:47Z

I understand you want appearanceTools as part of the export. That's a valid point and it's worth investigating. I disagree that "everything" in the theme.json of the theme should be part of the export: this ignores important processes such as sanitization, migration to a newer theme.json version, etc. We should keep those in place.

That concept makes me a lot more comfortable.

Add $schema to the allowed list of settings. I'm reluctant to bend the system in this way, as the valid data is carried through the data flow (it's sent to the client and stored there).

In what way does it bend the rules? Could there be a standard schema location that's exported unless overridden? Would be especially helpful if the theme.json version going in was v2 and what was exported to v3.

oandregal · 2022-03-25T13:43:03Z

In what way does it bend the rules? Could there be a standard schema location that's exported unless overridden?

Allowing something we don't use or validate, just for the sake of making it part of the export.

Would be especially helpful if the theme.json version going in was v2 and what was exported to v3.

This is interesting. Looking at the data ("$schema": "https://json.schemastore.org/theme-v1.json"), we could generate this based on the version. Worst-case scenario is that we generate a schema not published in schemastore so it'd be ignored by the editor.

scruffian · 2022-03-25T20:48:15Z

This is interesting. Looking at the data ("$schema": "https://json.schemastore.org/theme-v1.json"), we could generate this based on the version. Worst-case scenario is that we generate a schema not published in schemastore so it'd be ignored by the editor.

I made a PR for this: #39775

scruffian · 2022-03-29T08:34:12Z

I also made this for appearanceTools: #39840

Closing this one...

scruffian added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels Mar 18, 2022

scruffian self-assigned this Mar 18, 2022

scruffian requested review from TimothyBJacobs and spacedmonkey as code owners March 18, 2022 19:01

scruffian requested review from MaggieCabrera, pbking and jffng March 18, 2022 19:01

This was referenced Mar 18, 2022

Theme Export: Add missing schema attributes #39575

Closed

Exported JSON loses 'appearanceTools' abstraction #39464

Closed

scruffian requested review from getdave, adamziel and draganescu March 18, 2022 19:04

scruffian added 2 commits March 21, 2022 14:10

Theme Export: Add missing properties to theme.json

e5361c5

only add properties if they exist in the theme.json

ce5fe20

scruffian force-pushed the update/theme-json-missing-properties branch from 74d03dc to ce5fe20 Compare March 21, 2022 14:10

draganescu reviewed Mar 21, 2022

View reviewed changes

lib/compat/wordpress-6.0/block-template-utils.php Outdated Show resolved Hide resolved

scruffian and others added 2 commits March 21, 2022 23:18

Update lib/compat/wordpress-6.0/block-template-utils.php

5466375

Co-authored-by: Andrei Draganescu <[email protected]>

abstract the new merge function so it can be used functionally

e328874

scruffian commented Mar 22, 2022

View reviewed changes

scruffian requested review from oandregal and jorgefilipecosta March 22, 2022 12:42

pbking reviewed Mar 22, 2022

View reviewed changes

scruffian added 2 commits March 23, 2022 11:13

make the get_name_from_defaults function static

f3cc9d0

fix notices

3c89c0e

create a function to merge theme and user global styles

af7f303

oandregal reviewed Mar 25, 2022

View reviewed changes

scruffian mentioned this pull request Mar 28, 2022

Exporting Block Themes and Styles #27941

Closed

scruffian closed this Mar 29, 2022

scruffian deleted the update/theme-json-missing-properties branch March 29, 2022 08:34

scruffian mentioned this pull request Apr 5, 2022

Theme Export: If the theme declares a version number then add a schema #39775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme Export: Add missing properties to theme.json #39590

Theme Export: Add missing properties to theme.json #39590

scruffian commented Mar 18, 2022 •

edited

Loading

pbking commented Mar 18, 2022

scruffian commented Mar 18, 2022

scruffian Mar 22, 2022

scruffian Mar 22, 2022

scruffian commented Mar 22, 2022

pbking Mar 22, 2022

pbking commented Mar 22, 2022

scruffian commented Mar 23, 2022

pbking commented Mar 23, 2022

oandregal Mar 25, 2022 •

edited

Loading

oandregal commented Mar 25, 2022

scruffian commented Mar 25, 2022

oandregal commented Mar 25, 2022

pbking commented Mar 25, 2022 •

edited

Loading

oandregal commented Mar 25, 2022 •

edited

Loading

scruffian commented Mar 25, 2022

scruffian commented Mar 29, 2022

Theme Export: Add missing properties to theme.json #39590

Theme Export: Add missing properties to theme.json #39590

Conversation

scruffian commented Mar 18, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Notes

pbking commented Mar 18, 2022

scruffian commented Mar 18, 2022

scruffian Mar 22, 2022

Choose a reason for hiding this comment

scruffian Mar 22, 2022

Choose a reason for hiding this comment

scruffian commented Mar 22, 2022

pbking Mar 22, 2022

Choose a reason for hiding this comment

pbking commented Mar 22, 2022

scruffian commented Mar 23, 2022

pbking commented Mar 23, 2022

oandregal Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

oandregal commented Mar 25, 2022

scruffian commented Mar 25, 2022

oandregal commented Mar 25, 2022

pbking commented Mar 25, 2022 • edited Loading

oandregal commented Mar 25, 2022 • edited Loading

scruffian commented Mar 25, 2022

scruffian commented Mar 29, 2022

scruffian commented Mar 18, 2022 •

edited

Loading

oandregal Mar 25, 2022 •

edited

Loading

pbking commented Mar 25, 2022 •

edited

Loading

oandregal commented Mar 25, 2022 •

edited

Loading