Padding component doesn't respond for non-block theme

[JoryHogeveen]

Description

While the margin component responds perfectly, the padding component doesn't change the inner styles of the HTML element. The spacing simply isn't added at all (even though the setting is adjusted and stored in the block).

Step-by-step reproduction instructions

Can be reproduced by using a regular (non-block) theme without a theme.json file which has custom-spacing support enabled. I am using a Genesis framework child theme (Genesis isn't a block theme) and added the following supports:

• align-wide
• align-full
• custom-line-height
• custom-spacing <<<
• appearance-tools
• editor-styles
• block-templates
• html5

Easiest way to check if it works is by using a Group block and adding a paragraph within it. When adjusting the margin you'll see it works perfectly fine but trying to adjust the padding will not affect the actual HTML.

Screenshots, screen recording, code snippet

Environment info

• WordPress 6.5.3
• Both with and without Gutenberg plugin enabled (v18.4.1), same result

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[t-hamano]

Hi @JoryHogeveen,

I tested it with Twenty Twenty-One, which supports custom-spacing and does not have theme.json, but I was unable to reproduce this issue. I also tried adding add_theme_support( 'appearance-tools' ) to the theme, but the result was the same.

Can you reproduce this issue when you enable TT1 in your environment?

[JoryHogeveen]

Hi @t-hamano

I saw that there was a difference in the controls in your screenshot and mine. In your example the theme shows two sliders, vertical and horizontal. In my theme it only shows one.

After hours testing I finally found it and I'm unsure what part would be the cause here. Apparently, if you pass true as the second argument to add_theme_support then the padding will show as a single bar and won't do anything at all.

Do you have any idea why this would be? I thought that passing true would be the default, see: https://github.com/WordPress/WordPress/blob/6.5.3/wp-includes/theme.php#L2691-L2693

Btw. You can reproduce by changing the following line (replacing the existing)

add_theme_support( 'custom-spacing', true );

[t-hamano]

Thanks for the detailed info. I was able to reproduce this issue in TT1d by adding true to this line.

However, I'm still not confident that this is a bug. For example, I think it is necessary to investigate whether other supports such as responsive-embeds and custom-line-height cause unintended behavior when true is passed as the second argument.

If this problem only occurs with custom-spacing, it might be a bug.

[JoryHogeveen]

Hi @t-hamano

I'm must say that I'm unsure as well whether this is a bug or an expected outcome. In any case, it's not clearly documented. Also, this might be more a WordPress core issue rather than a Gutenberg one, I don't know for sure.

I think it is necessary to investigate whether other supports such as responsive-embeds and custom-line-height cause unintended behavior when true is passed as the second argument. If this problem only occurs with custom-spacing, it might be a bug.

Totally agree. In my case I've used a loop to auto-register all my theme supports from an array (ext. config) and this function passed true to any support that didn't have custom arguments. All the others were working just fine with true passed as the default argument so as far as I can tell this only happens with custom-spacing.

[JoryHogeveen]

I think I've found the cause of this, just try the following code in any PHP sandbox:

<?php

function test( $name, ...$args ) {
    if ( ! $args ) {
        $args = true;
    }
    var_dump( $args );
}
test( 'check', true );

You will see that $args will be an array instead of a boolean. The custom-spacing setting explicitly requires a boolean and not anything else. This array gets passed seemingly without validation/parsing to the frontend group the global-styles rest endpoint from the theme (/wp-json/wp/v2/global-styles/themes/NAME).

Response with custom-spacing second param not set:

{
    "settings": {
    ......
        "spacing": {
            "blockGap": true,
            "margin": true,
            "padding": true,
            "customSpacingSize": true,
    .......
}

Response with custom-spacing second param set to true:

{
    "settings": {
    ......
        "spacing": {
            "blockGap": true,
            "margin": true,
            "padding": [
                true
            ],
            "customSpacingSize": true,
    .......
}

Now, as I mentioned earlier, I'm not sure whether to consider this a bug or bad usage. If it's a bug it might be best to just case the array to a boolean. If you do not consider this a bug then this should be clearly documented. The documentation currently states that this is a boolean type so passing a boolean makes perfectly sense.

IMO: This is a WordPress core (minor) bug and the following line should be changed: https://github.com/WordPress/WordPress/blob/6.5.3/wp-includes/theme.php#L2691-L2693

For example changed to:

    if ( ! $args ) {
        $args = true;
    }
    if ( 1 === count( $args ) && is_bool( $args[0] ) ) {
        $args = $args[0];
    }

Alternatively the padding component should accept any var type that is considered "true".

[t-hamano]

Thanks for looking into it! I think the issue with PHP's variable length arguments comes from the fact that even when a single argument is passed, it's always an array.

We might want to submit a core ticket about this issue.

[JoryHogeveen]

Hi @t-hamano Sorry for my late reply, I'm on holiday right now. I'll leave it up to you what to do with this issue/bug, TBH I consider it both a core AND a Gutenberg/Blockeditor issue.

Core: I don't think core will change this since it will most likely break backwards compatibility. In any case I do think this should be documented properly in core's codex: https://developer.wordpress.org/reference/functions/add_theme_support/

Gutenberg: I think it's best to parse the values and accept an array with only a true value as well. Same thing goed when providing false to disable this feature. Even though it's opt-in, users might do this to specifically set it to disabled for some reason.

[carolinan]

Why are you passing true as an argument to add_theme_support? The function sets $args to true already if there are no $args passed.

Also passing false as an argument to add_theme_support does not disable it. https://developer.wordpress.org/reference/functions/add_theme_support/ https://developer.wordpress.org/reference/functions/_remove_theme_support/

[carolinan]

Did you happen to open a support request for Generatepress?

[JoryHogeveen]

@carolinan I'm aware of that, and no I didn't open a support ticket on GeneratePress and honestly don't see why it's relevant to this issue.

My use-case was merely a simple script to manage theme supports in a larger array all at once and allowing sub-themes to extent it. A bit similar to how to Genesis Sample child theme works: https://github.com/studiopress/genesis-sample/blob/develop/config/theme-supports.php So in that context it makes sense to set it as a boolean to properly show whether supports are enabled or disabled.

For me, this issue is already fixed as I just prevented passing booleans as arguments. The only thing that I would imagine is that the docs could state that booleans are not supported as arguments which could prevent other users from having the same issue and not figuring out what is causing it.

Anyhow, if you guys decide that no further action is needed then his issue can be closed.

[t-hamano]

I tried unintended parameters as boolean values ​​for some theme supports.

add_theme_support( 'editor-font-sizes', true );
// Causes a PHP warning error

add_theme_support( 'editor-font-sizes', false );
// Nothing happens

add_theme_support( 'editor-color-palette', true );
// Causes a PHP warning error

add_theme_support( 'editor-color-palette', false );
// Nothing happens

add_theme_support( 'custom-background', true );
// Causes a PHP fatal error

add_theme_support( 'custom-background', false );
// Causes a PHP warning error

add_theme_support( 'border', true );
add_theme_support( 'border', false );
// Border support is properly added

add_theme_support( 'custom-spacing', true );
add_theme_support( 'custom-spacing', true );
// Support is added, but only one slide is displayed, and does not work.

add_theme_support( 'apperance-tools', false );
add_theme_support( 'apperance-tools', false );
// All supports are properly added

What do you think about these results?

In my opinion, the second argument is meant to add some extra options to the support, and was never meant to be used to explicitly enable or disable support.

Therefore, I think it's okay to close this issue as "wontfix" or "not a bug".

[JoryHogeveen]

Hi @t-hamano

I would agree that this probably won't be fixed. For whether it's a bug or not, as you've found, the results are very different for each and personally I would consider inconsistency a bug in general. I leave it up to you what to do with it.

In my opinion, the second argument is meant to add some extra options to the support, and was never meant to be used to explicitly enable or disable support.

This is a fair statement, so I would suggest updating documentation instead of changing the code.

[t-hamano]

I think that most functions/hooks in WordPress, not just this function, can cause unintended behavior when parameters other than the specified ones are passed. It may be possible to process unintended parameters so that they do not cause errors, but then there is a concern that developers may accidentally use those APIs.

Regarding this function, the following is mentioned here:

$args mixed optional Optional extra arguments to pass along with certain features.

In my feeling, this explanation is clear and implicitly indicates that this is not intended to enable or disable support.

I would like to close this issue, but if you feel that updating the documentation makes sense, please feel free to submit a core ticket in Trac.