Backport: add theme json class changes to edit style variations #3941

tellthemachines · 2023-01-30T06:15:10Z

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

Backports changes from WordPress/gutenberg#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 😅

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

costdev

Thanks for the PR @tellthemachines! I've just left a comment in this review regarding documentation standards for this file and future backports.

costdev · 2023-01-30T09:12:59Z

src/wp-includes/class-wp-theme-json.php

+							// Merge new declarations with any that already exist for
+							// the feature selector. This may occur when multiple block
+							// support features use the same custom selector.


Multiline comments should be formatted as such:

/* * Line 1 * Line 2 * ... */

Ref
🔢 Applies elsewhere in the PR.

However, I note that this file contains many multiline comments that don't adhere to the standards.

In lieu of a change in this PR that creates an inconsistency in this file, I strongly suggest a follow up PR to bring this file up to the documentation coding standards, and that all future code being backported meets the standard.

Resolved here 0768aa8.

oandregal

I've tested this PR by doing the following:

Go to TwentyTwentyThree's theme.json and under styles.blocks.core/quote add this: "variations": { "plain": { "color": { "background": "hotpink"} } }.
Then go to the editor and add a post that contains a quote block.
Set the quote to use the "plain" style variation and publish.
The expected result is that the background color is hotpink.

I went ahead and prepared tellthemachines#1 to add some unit test. It'd be great if it could be merged into this PR. Other than that and the comments brought up by @costdev , this is ready 🚢

tellthemachines · 2023-01-31T00:07:32Z

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.

Unit test for Backport/edit style variations

tellthemachines · 2023-01-31T00:27:07Z

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 😕

Edit: they now seem to be failing only on PHP versions 7.2 and below. Not sure why this would happen unless the actual issue is on a different line? That closing bracket it's complaining about seems to be in the right place.

tests/phpunit/tests/theme/wpThemeJson.php

Co-authored-by: André <[email protected]>

Splits the tests into 2 separate tests. Adds a data provider for each. Adds more datasets.

hellofromtonya · 2023-01-31T21:20:53Z

src/wp-includes/class-wp-theme-json.php

-			$schema_styles_blocks[ $block ]             = $styles_non_top_level;
-			$schema_styles_blocks[ $block ]['elements'] = $schema_styles_elements;
+			// Build the schema for each block style variation.
+			$style_variation_names    = isset( $input['styles']['blocks'][ $block ]['variations'] ) ? array_keys( $input['styles']['blocks'][ $block ]['variations'] ) : array();


What if the array structure exists, but the 'variations' are not an array? What would happen?

https://3v4l.org/kHpHT

= PHP 8.0

Fatal error: Uncaught TypeError: array_keys(): Argument #1 ($array) must be of type array, string given

PHP 8.0:

Warning: array_keys() expects parameter 1 to be array, string given

To avoid these, also check if it is an array.

I'd also suggest switching to ! empty() instead of isset(). Why? To avoid doing array_keys() on an empty array.

Suggested change

$style_variation_names = isset( $input['styles']['blocks'][ $block ]['variations'] ) ? array_keys( $input['styles']['blocks'][ $block ]['variations'] ) : array();

$style_variation_names = array();

if (

! empty( $input['styles']['blocks'][ $block ]['variations'] ) &&

is_array( $input['styles']['blocks'][ $block ]['variations'] )

) {

$style_variation_names = array_keys( $input['styles']['blocks'][ $block ]['variations'] );

}

Resolved here 86ac232.

hellofromtonya · 2023-01-31T21:54:54Z

src/wp-includes/class-wp-theme-json.php

+				// If the block has feature selectors, generate the declarations for them within the current style variation.
+				if ( ! empty( $block_metadata['features'] ) ) {
+					foreach ( $block_metadata['features'] as $feature_name => $feature_selector ) {
+						if ( ! empty( $style_variation_node[ $feature_name ] ) ) {


Unwrap the inner code by inverting this check. Why? Shifts the code to the left to make it more readable, as more levels of indentation can reduce readability.

Resolved here 994c202.

hellofromtonya · 2023-01-31T21:58:50Z

src/wp-includes/class-wp-theme-json.php

+								function( $split_feature_selector ) use ( $style_variation_selector ) {
+									return trim( $style_variation_selector ) . trim( $split_feature_selector );


A few things here for micro-optimizations:

Use static function. That way the callback is created once rather than on each iteration.

Move the trim( $style_variation_selector ) outside of the foreach. It does not change without the loop. By moving it outside of the loop, trimming happens once instead of on each iteration.

Resolved here 994c202.

hellofromtonya

This PR LGTM 👍 It has test coverage, some micro-optimizations, and meets Core's coding standards.

Will wait for @tellthemachines to confirm the changes are okay before marking for commit.

hellofromtonya · 2023-01-31T22:16:55Z

@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?

tellthemachines

Thanks for the improvements and detailed explanations @hellofromtonya ! The code looks good to me and tests are passing ✅

hellofromtonya · 2023-02-01T13:04:25Z

Thanks @tellthemachines! Prepping commit now.

hellofromtonya · 2023-02-01T16:15:47Z

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

Add theme json class changes to edit style variations

53ce333

tellthemachines requested a review from ntsekouras January 30, 2023 06:15

costdev reviewed Jan 30, 2023

View reviewed changes

oandregal added 2 commits January 30, 2023 14:01

Add test for style variations

c99dace

fixup

2c58b47

oandregal approved these changes Jan 30, 2023

View reviewed changes

ntsekouras requested a review from hellofromtonya January 30, 2023 16:10

tellthemachines and others added 2 commits January 31, 2023 11:09

Merge pull request #1 from oandregal/backport/edit-style-variations

923feca

Unit test for Backport/edit style variations

Fix test output styles.

fe1353c

tellthemachines added 2 commits January 31, 2023 11:50

Fix lint errors.

80ad395

Fix spacing in test file.

1f232da

tellthemachines self-assigned this Jan 31, 2023

oandregal reviewed Jan 31, 2023

View reviewed changes

tests/phpunit/tests/theme/wpThemeJson.php Show resolved Hide resolved

hellofromtonya and others added 2 commits January 31, 2023 13:28

Remove trailing comma to resolve parsing error in < PHP 7.3.

159a3e3

Co-authored-by: André <[email protected]>

Tests: split and use data provider

fc4a2a2

Splits the tests into 2 separate tests. Adds a data provider for each. Adds more datasets.

hellofromtonya reviewed Jan 31, 2023

View reviewed changes

Guard for non-array variations

86ac232

hellofromtonya reviewed Jan 31, 2023

View reviewed changes

hellofromtonya added 2 commits January 31, 2023 15:59

Micro-optimizations in ::get_styles_for_block()

994c202

Formatting for multiline comments

0768aa8

hellofromtonya approved these changes Jan 31, 2023

View reviewed changes

tellthemachines commented Feb 1, 2023

View reviewed changes

hellofromtonya closed this Feb 1, 2023

-			$style_variation_names    = isset( $input['styles']['blocks'][ $block ]['variations'] ) ? array_keys( $input['styles']['blocks'][ $block ]['variations'] ) : array();
+			$style_variation_names = array();
+			if (
+				! empty( $input['styles']['blocks'][ $block ]['variations'] ) &&
+				is_array( $input['styles']['blocks'][ $block ]['variations'] )
+			) {
+				$style_variation_names = array_keys( $input['styles']['blocks'][ $block ]['variations'] );
+			}

		function( $split_feature_selector ) use ( $style_variation_selector ) {
		return trim( $style_variation_selector ) . trim( $split_feature_selector );

Backport: add theme json class changes to edit style variations #3941

Backport: add theme json class changes to edit style variations #3941

Uh oh!

Conversation

tellthemachines commented Jan 30, 2023

Uh oh!

costdev left a comment

Choose a reason for hiding this comment

Uh oh!

costdev Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

oandregal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tellthemachines commented Jan 31, 2023

Uh oh!

tellthemachines commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

hellofromtonya left a comment

Choose a reason for hiding this comment

Uh oh!

hellofromtonya commented Jan 31, 2023

Uh oh!

tellthemachines left a comment

Choose a reason for hiding this comment

Uh oh!

hellofromtonya commented Feb 1, 2023

Uh oh!

hellofromtonya commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

costdev Jan 30, 2023 •

edited

Loading

oandregal left a comment •

edited

Loading

tellthemachines commented Jan 31, 2023 •

edited

Loading