Add support for editing block style variations in the global styles

[isabel_brison]

Add server side (PHP) support for editing block style variations in the global styles.

Changes will be applied in WP_Theme_JSON.

Reference:

• ​https://github.com/WordPress/gutenberg/pull/46343

Trac ticket: https://core.trac.wordpress.org/ticket/57583

Backports changes from ​https://github.com/WordPress/gutenberg/pull/46343

These changes can't really be tested without the corresponding package updates, but I checked the Site Editor and Global Styles sidebar and at least nothing seems to be breaking 😅

Thanks for reviewing folks!

@costdev it's probably easiest to do a follow-up PR to fix all the comments once this one is in; I'm happy to do it.

@oandregal thanks for the test! I can't get it to pass locally but I'll merge it into this branch so we can see if it passes CI.

I removed the spaces between classnames from the output string as they're not meant to be there, but have no idea why the Parse error: syntax error, unexpected ')' in /var/www/tests/phpunit/tests/theme/wpThemeJson.php on line 3620 - the tests are now passing for me locally 😕

[hellofromTonya]

Self-assigning for review and commit.

@tellthemachines Collectively there are some changes made in the PR for:

• Adding tests
• Multiline comment formatting
• Micro-optimizations
• Guarding to prevent PHP errors

The PR looks ready to me for commit. What do you think? Are the changes okay?

[hellofromTonya]

Marking for commit but noting would like @isabel_brison to take a look at the PR just to make sure. If okay, will commit tomorrow.

Thanks @tellthemachines! Prepping commit now.

[hellofromTonya]

In 55172:

Editor: Add support for editing block style variations in global styles.

To allow editing of block style variations in global styles, this changeset adds the following for server side support:

• building of block style schema into WP_Theme_JSON::sanitize().
• appending of style variation selectors to block metadata in WP_Theme_JSON::get_blocks_metadata().
• building of selectors and variations for nodes in WP_Theme_JSON::get_block_nodes().

Tests for happy and unhappy paths are included.

Reference:

• ​Gutenberg PR 46343

Follow-up to [54118], [50973], [50959].

Props isabel_brison,
Fixes #57583.

Committed via https://core.trac.wordpress.org/changeset/55172.

[SergeyBiryukov]

This appears to have introduced a PHP warning when running the test suite, as seen in the ​recent test runs:

Warning: foreach() argument must be of type array|object, string given in wp-includes/class-wp-theme-json.php on line 835

[isabel_brison]

The line number on that PHP warning doesn't seem right. Could it refer to the foreach on line 860 ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-theme-json.php#L860 as that was introduced in this changeset? I'm not sure if $block_type->styles would ever be a string though.

[hellofromTonya]

The most recent test runs show it at line 860 as @isabel_brison noted. The Warning is on PHP 8+. Looking at the code before going into the foreach, it's checking for a non-empty value, but not where it's also an array:

if ( ! empty( $block_type->styles ) ) {

In my mind, I'm thinking about:

• Could the styles property ever be a non-empty that's not an array?
• If maybe, then an is_array( $block_type->styles ) would skip the foreach for a non-empty string or any other non-empty array values.
• Else, look at the test to see which one is causing this to determine if there's a bug somewhere in the code that's causing the unexpected value and data type.

[isabel_brison]

The styles property of WP_Block_Type should always be an array: ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-block-type.php#L269 but I'm not sure there are any safeguards against registering it as something else. It's always an array in core blocks, at least (I tried printing $block_type->styles just before that line and only arrays are output)

I guess it won't hurt to check if it's an array before going into the foreach though.

Trac ticket: https://core.trac.wordpress.org/ticket/57583

Tries to fix this PHP warning .

[hellofromTonya]

Adjusting the keywords for the follow-up fix.

Currently reviewing the fix.

[hellofromTonya]

Replying to SergeyBiryukov:

This appears to have introduced a PHP warning when running the test suite, as seen in the ​recent test runs:

Warning: foreach() argument must be of type array|object, string given in wp-includes/class-wp-theme-json.php on line 835

The PHP warning is coming from the a block that is registered with a string as its 'styles' ​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/rest-api/rest-block-type-controller.php#L226.

Should the 'styles' setting for a block always be of an array data type? Yes, the data type is documented as:

• register_block_type(): an array[] ​https://developer.wordpress.org/reference/functions/register_block_type/
• WP_Block_Types: an array ​https://developer.wordpress.org/reference/classes/wp_block_type/

register_block_type() uses WP_Block_Type(), which sets the passed $args to its properties via WP_Block_Type::set_props(). But notice, there are no data type checks in set_props(). So the properties could be set any data type and/or value. That's problematic as it could result in unexpected behavior or errors such as seen in the test suites with the PHP warning.

Guarding is one way to go for the code introduced in [55172] to ensure it only processes as array. I'm okay with doing so.

However, the root cause is not in [55172]. Rather it's the lack of validation before setting the block type properties that concerns me. Without such validation, more and more type checks will be required in the code that is processing these properties/settings.

A quick fix for the test suite is to eliminate the 'invalid_styles' value from test that is causing the PHP Warning, as that string value is not being used.

I'll follow-up by opening an issue in Gutenberg to further explore validation before setting properties in WP_Block_Type.

[hellofromTonya]

Replying to hellofromTonya:

I'll follow-up by opening an issue in Gutenberg to further explore validation before setting properties in WP_Block_Type.

​https://github.com/WordPress/gutenberg/issues/48039 opened for discussing and potentially resolving the property validation.

The more I think about this change, the more concerned I am that it is masking a real problem of doing it wrong. The root cause is not this code, but rather, a block type being registered with 'styles' property set as something other than an array. That's doing it wrong.

By introducing a is_array() here, it will mask the problem by skipping over this code. That can make it harder for a developer to debug their block type and styles.

So I think leaving the PHP Warning is a better practice as it'll serve to alert the developer something is wrong. The Warning though won't tell them that they've registered styles of their block type incorrectly. That's where this issue can help ​https://github.com/WordPress/gutenberg/issues/48039.

I think this PR should not be committed. But instead, the test that is incorrectly registering a block type should be fixed. I've opened a new Trac ticket #57706 to fix the test that is throwing the PHP Warning.

[hellofromTonya]

As I noted above, the root cause is not this ticket or commit, but rather an invalid data type being registered in a different test. The PHP Warning being thrown is good as it alerts to an incompatibility / doing it wrong problem.

I've opened a Trac ticket to fix the test that is throwing the PHP Warning #57706.

The more I think about this, adding an is_array() check ​https://github.com/WordPress/wordpress-develop/pull/4014 will mask the problem, making it harder for developers to debug their code.

I'm reclosing this ticket. The test fix will happen in #57706.

The more I think about this change, the more concerned I am that it is masking a real problem of doing it wrong. The root cause is not this code, but rather, a block type being registered with 'styles' property set as something other than an array. That's doing it wrong.

That makes sense! It's more useful and informative to validate the block data and warn if it's the wrong type.