Fixing broken layout styles for themes using theme.json settings on blocks that require layout styles

[matiasbenedetto]

What?

The function get_property_value from WP_Theme_JSON_6_1 class returns an empty string instead of NULL when a property is not set. So when we call this function to get the styles, of a block that needs layout settings, the block gap value is an empty string instead of null and that prevents that the value is fallback to the correct value.

Let's see a example:
When Gutenberg calls get_property_value to get the layout styles of the core/post-content block, with $block_gap_value = static::get_property_value( $node, array( 'spacing', 'blockGap' ) ), $block_gap_value value is an empty string instead of NULL.This is causing that, for example, the default layout block gap value is not properly set for core/post-content block, ending in invalid CSS being rendered.

Why?

Because returning an empty string is causing problems while rendering layout styles resulting in the rendering of invalid CSS. This problem is described in #43924.

How?

This PR makes NULL the default result of get_property_value().

If this looks like an acceptable solution for the problem probably I should add some PHP unit tests.

Testing Instructions

1. Activate any theme using settings for core/post-content in theme.json as TT3.
2. Add a few elements to a post, for example, a few paragraphs.
3. Save the post and navigate the frontend
4. Check that there is no invalid CSS rendered and that the block gap between post elements is there.

Screenshots or screencast

Before	After

Fixes: #43924

[andrewserong]

Oh, thanks for looking into a fix for this! (And apologies, I believe the issue was likely introduced in #43757 when I attempted to get the layout output to handle 0 string values).

As a potential alternative, we could always update the check in get_layout_styles to be a bit more specific about the value it's looking for. At the moment the check is a not null check on this line: https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L1348

Would it work to use a check like '0' === $block_gap_value || ! empty( $block_gap_value ) like we have in the style engine in this function: https://github.com/WordPress/gutenberg/blob/trunk/packages/style-engine/class-wp-style-engine.php#L263?

[tellthemachines]

Thanks for fixing this! Tested with a few blocks with custom theme.json settings and everything seems to be working fine. Might be good to add some tests to make sure the function still behaves as expected though.

[draganescu]

It feels like the system should be less easy to break, getting an empty string instead of null in PHP is something to guard against - so this fix should fix the fact that invalid styles are output when the value to generate from is empty string.

We should strive to return consistent types as well, but if get_property_value always returns empty string and it did so before the block gap problem was introduced, then it should not be fixed.

[scruffian]

As a potential alternative, we could always update the check in get_layout_styles to be a bit more specific about the value it's looking for.

I think it makes sense to do both things.

[matiasbenedetto]

Hi thanks for your feedback!

Would it work to use a check like '0' === $block_gap_value || ! empty( $block_gap_value )

This check doesn't cover the case when $block_gap_value is 0, so if I set this in my theme.json, the value is ignored and no gap styles are rendered for the block.

"core/post-content": {
        "spacing": {
	        "blockGap": 0
        }
}

There are some themes using 0 as value:

On the other hand this check null !== $block_gap_value && false !== $block_gap_value && '' !== $block_gap_value seems to be covering correctly all the cases:

Using 0, '0' in 'theme.json' render the block gap styles.
Using false, null, or "" doesn't render the block gap styles.

I added 4 tests:

• Without this PR the first and the fourth would fail this way:
  

• The second and third ones are to avoid future errors that could ignore the '0' or 0.

[andrewserong]

Thanks for the follow-ups @matiasbenedetto, this is testing nicely for me! Also, adjusting the return type to null looks reasonably safe to me, since the function is protected and is only used in the layout output and in compute_style_properties where the null case is handled 👍

I tested that it resolves the issue with post content, and also tested with a variety of different values for simple and split (axial) block gap, and all appears to be working well for me.

Thanks for catching the 0 non-string value issue, too — since the UI will always save the 0 value as a string, I hadn't thought of hand-written themes using an integer value, so good to ensure that we don't accidentally cause issues for themes that explicitly set 0.

I just left a totally optional comment about potentially grouping together the test cases into one, but feel free to ignore it, it's not at all a blocker. (Also, it looks like you've already fixed up that final test while I was typing out this comment 😀)