Spacing presets: Add check for 0 spacing steps

[glendaviesnz]

What?

Adds an early return from set_spacing_sizes if spacing.spaceScale.steps === 0.

Why?

Theme authors should be able to set this to 0 to prevent the generation of the core default spacing presets, and currently doing so causing an invalid valid error notice to be thrown.

How?

If pacing.spaceScale.steps === 0 return from method before spaceScale settings are validated

Testing Instructions

• Add the following to your themes theme.json file settings section:

    "spacing": {
      "spacingScale": {
        "steps": 0
      }
    }

• Load the editor or frontend and make double check that there are no --wp--preset--spacing--* CSS properties defined in the page source and that there is no PHP notice at the top of the page.

Screenshots or screencast

Before:

After:

[andrewserong]

Good catch @glendaviesnz, this is working nicely for me!

Before, the following PHP Notice is output:

After, there is no notice and the presets aren't output. Setting to a value greater than 0 correctly still outputs the spacing sizes.

Not at all a blocker, but I was wondering if it's worth changing the check from 0 === $spacing_scale['steps'] to empty( $spacing_scale['steps'] ) so that any falsy value for steps would result in skipping the output without triggering an error? (E.g. 0, false, null, etc)

[glendaviesnz]

but I was wondering if it's worth changing the check from 0 === $spacing_scale['steps'] to empty( $spacing_scale['steps'] ) so that any falsy value for steps would result in skipping the output without triggering an error? (E.g. 0, false, null, etc)

The theory is that if a theme author doesn't want the core scale they explicitly set steps to 0, so it is a valid setting that will be documented .If they have set it to false or null it is invalid, so I was thinking we should throw the error to alert them to the fact that their settings are wrong - what do you think?

[andrewserong]

The theory is that if a theme author doesn't want the core scale they explicitly set steps to 0, so it is a valid setting that will be documented .If they have set it to false or null it is invalid, so I was thinking we should throw the error to alert them to the fact that their settings are wrong - what do you think?

That sounds reasonable to me, particularly because steps is expected to be a number. The only reason I thought some folks might wind up using false or null is because of potentially being used to using those values to switch other features off. From my perspective the warning is most useful when someone attempted to set correct settings but made an accidental mistake. But that's just the thoughts off the top of my head, don't let that stop you from merging!

[andrewserong]

Oh, actually just another thought. This probably doesn't matter in practice because the base theme.json contains the 'steps' key, but if we keep the 0 check, should this line and the > 0 line below occur after the ! isset( $spacing_scale['steps'] ) check further down, to (theoretically) avoid an undefined index notice?

[glendaviesnz]

yeh, makes more sense to check for invalid values first and then check for 0, have switched the order

[andrewserong]

Just retested and this is testing nicely for me, thanks for the update 👍

Again, not at all a blocker (do feel free to merge this PR as-is), but is there a reason we don't also allow opting out at the root feature level?

		"spacing": {
			"spacingScale": false
		},

Whichever way we recommend themes do it, it'd be good to update https://developer.wordpress.org/themes/advanced-topics/theme-json/#enabling-and-disabling-settings with the desired approach prior to 6.1. If / when we document it, we can add _**Note:** Since WordPress 6.1._ to flag the version the feature's targeting. (Since the feature is still in development, I don't think there's any rush to adding the docs, this can happily be a follow-up once more of the presets work has landed, but just thought I'd mention it)

[glendaviesnz]

is there a reason we don't also allow opting out at the root feature level?

no explicit reason @andrewserong , just hasn't been considered to-date - have added a note to explore this to the tracking issue

[andrewserong]

Nice one, thanks @glendaviesnz!