Theme Export: Remove default theme.json properties on export

[scruffian]

What?

Three settings in theme.json are enabled by default:

settings.color.custom
settings.color.customGradient
settings.typography.customFontSize

This PR removes these properties on export if they are set to true.

Why?

We don't want to make these settings explicit if they are the default.

How?

We use unset on the theme.json for each property.

Testing Instructions

• Switch to a block theme

• Export the theme from the site editor

• Confirm that the properties above are missing.

• Now try setting these properties to false in the theme.json file for the current theme

• Confirm that the properties are maintained on export.

cc @WordPress/block-themers

[mikachan]

This works as described for me. I can see that all three properties are missing if they are set to true, and they are maintained if they are set to false. I can also see they're maintained regardless without this PR. Looks good!

[carolinan]

It works well in my test, can be merged once the coding standard issue is fixed.

-Should this be extended to include all defaults? It is unclear to me why these three defaults were selected.

[scruffian]

-Should this be extended to include all defaults? It is unclear to me why these three defaults were selected.

In my testing these were the only ones that were exported. We can add more if we notice others that are exported.

[carolinan]

{
	"version": 1
}

Exports as:

{
    "version": 2,
    "settings": {
        "color": {
            "custom": true,
            "customGradient": true
        },
        "typography": {
            "customFontSize": true,
            "lineHeight": false
        },
        "spacing": {
            "units": false,
            "padding": false
        }
    }
}

[scruffian]

Ah yes, I missed the false ones! Added a commit for them...

[carolinan]

🤔 With the latest update to the PR,

{
	"version": 1
}

exports as

{
	"version": 2,
	"settings": {
		"color": [],
		"typography": [],
		"spacing": []
	}
}

[scruffian]

Thanks for testing! Try now... :)

[oandregal]

Hey, I'd like to understand this better before we commit to this path. Why are we removing these properties? Why are they set to true?

This PR removes these properties on export if they are set to true.

I'm worried about this bit: the WP_Theme_JSON::get_data method should be totally agnostic about the consumers that use it.

[pbking]

The benefit I see is that the JSON that comes OUT of the FSE matches the JSON that I put in.

Meaning that if I wasn't explicit about setting a property on purpose then I don't want that property being explicitly set for me when it's exported. I think @carolinan 's examples above best demonstrates how current behavior doesn't mirror my expectations.

The only concerns I have is that if I HAVE explicitly set something to the default value in the source theme.json then (with this change) I lose that explicitly set value when exported. I'm LESS concerned about that then I am properties being explicitly added.

[oandregal]

The benefit I see is that the JSON that comes OUT of the FSE matches the JSON that I put in.

Absolutely, we need this. Can we first understand why this happens and then discussing the fix? I've pushed 5e67ff8 to use as baseline.

[scruffian]

I believe the reason it happen is that there are a bunch of deprecated theme supports that get opted in to here:

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

Line 1860 in 39333d4

public static function get_from_editor_settings( $settings ) {

I suppose since these are the defaults then it makes sense that they are set when the theme.json is created.

[oandregal]

Yeah, there's something in WP_Theme_JSON_Resolver_Gutenberg::get_theme_data that needs to be looked at: get_default_block_editor_settings returns them set and then those are processed at get_from_editor_settings.

Themes that don't have any theme support shouldn't have the need for these, which are already managed via the core theme.json anyway. We need to look at this area and prepare a fix that doesn't modify behavior for themes with and without theme.json.

[scruffian]

I added a commit to separate the theme supports out of get_theme_json so we can not add them on the export. What do you think?

[draganescu]

Since WordPress provides a default theme.json maybe $tree = WP_Theme_JSON_Resolver_Gutenberg::get_theme_data(); should have an "this is for export" flag that does not add defaults to it?

Later edit: this is what get_theme_data_without_supports is actually 🤦🏻 My question remains if exporting theme JSON should be a public thing. The mechanics should not be something exposed via a public static method.

[oandregal]

Since WordPress provides a default theme.json maybe $tree = WP_Theme_JSON_Resolver_Gutenberg::get_theme_data(); should have an "this is for export" flag that does not add defaults to it?

Ben, what do you think of doing this instead of creating a new method? The rationale would be to avoid increasing the API surface of the resolver. *

Andrei and I talked a bit about the current WP_Theme_JSON_Resolver and the proliferation of "public static" methods. The resolver class leans towards the "toolbox, a collection of utilities" model rather than a "container of the object cycle" model. I'd say this has been on purpose, to avoid polluting WordPress global namespace with utility functions that are only used in one place (we also didn't have the alternative solutions that we have now: namespaces).

---

* Ironically, in the initial version of this class (see), the supports were a parameter.

[oandregal]

Clearing my "request for changes" review, as I feel this is in the good direction.

Second attempt to clear my "request for changes".

[scruffian]

Ben, what do you think of doing this instead of creating a new method? The rationale would be to avoid increasing the API surface of the resolver. *

I don't think this will reduce the number of public methods, since get_theme_data_without_supports is a private method. Or is your suggestion to remove the new public export function? I'm not a huge fan of that idea as I think this logic makes a lot more sense being in this class rather than in block-template-utils.php.

[oandregal]

I don't think this will reduce the number of public methods, since get_theme_data_without_supports is a private method

We can't have it as a private method, it'd need to be protected. Otherwise, we can't overwrite it in a lib/compat/6.1 should we need it.

About the export method: I don't feel super strongly, but it's true that's too featured-specific (a method created for the UI and not related to the models: theme.json, origins, variations, etc). I'd personally leave that out.

[scruffian]

Thanks for the feedback, I've updated the approach.

[draganescu]

a method created for the UI and not related to the models: theme.json, origins, variations, etc

I don't think this is true. Exporting is not a UI thing, it should be a function of our JSON system itself. I can see exporting via command line as well for instance.

I think having export functionality part of the public API is good, but the mechanics of the export should be private/protected. It's unfortunate that we are bound to using protected because of how WordPress Core development via Gutenberg is happening. We need to be as cler as possible via comments and docs that we don't support extensions of the JSON system right now.

---

Edit: I don't think this is a blocker for this PR, the way it loos if we decide to add an export public API to the JSON Resolver then it can be added without breaking anything later on. Having the _without_supports method protected is "good enough".