Style Engine: Harden add_declaration against being passed illegal values

[andrewserong]

Currently the add_declaration method assumes that it has been passed a string, when it might not have been.

I noticed on a website that someone had malformed block markup that nested margin within the spacing.padding object for a block, instead of having margin be a sibling to padding. These sorts of issues should generally fail silently rather than throwing an error.

It turns out that block spacing is passed on by the layout block support to the Style Engine, where it assumes the properties and values are correct.

We can harden against these sorts of circumstances by ensuring that the Style Engine bails early if it isn't given a string when adding a declaration.

This has already been fixed in Gutenberg, and this ticket proposes including the fix in core. The Gutenberg PR is: ​https://github.com/WordPress/gutenberg/pull/74881

This PR hardens add_declaration in the Style Engine when passed non-string values. The changes are already in Gutenberg over in: ​https://github.com/WordPress/gutenberg/pull/74881

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

[isabel_brison]

In 61516:

Editor: guard against non-string values in style engine.

Checks that the value passed to add_declaration is a string to prevent fatal errors due to malformed block attributes.

Props andrewserong.
Fixes #64545.

[isabel_brison]

@jorbin @wildworks adding this bug to the milestone because it can cause fatal errors on the front end when block attributes are malformed. Thought it would be good to get it in! The patch is committed to trunk already but needs a double check from another committer to go into the release branch, right?

[wildworks]

it can cause fatal errors on the front end when block attributes are malformed.

If I understand correctly, this bug has been there since 6.1 when the WP_Style_Engine_CSS_Declarations class was introduced, right?

​https://github.com/WordPress/wordpress-develop/blob/6.1/src/wp-includes/style-engine/class-wp-style-engine-css-declarations.php#L58-L67

I don't have a strong opinion, but personally I don't think it's necessary to backport it to a minor release right now.

[jorbin]

Where there any changes in 6.9 that would make this easier to encounter? If not, I would agree with @wildworks that this isn't in scope for 6.9.1.

[jorbin]

Also, this is minor but I think the new test_should_reject_non_string_values test should use a dataProvidor and test each of the non string values seperatly so that it's always clear which case breaks if the underlying code is changed.

[wildworks]

Where there any changes in 6.9 that would make this easier to encounter?

AFAIK, there wasn’t any such change in 6.9. Let's change the milestone to 7.0.

Also, this is minor but I think the new test_should_reject_non_string_values test should use a dataProvidor and test each of the non string values seperatly so that it's always clear which case breaks if the underlying code is changed.

I'll leave this ticket open to address the feedback above.