Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elements: check value and whitelist before building style nodes #43622

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Aug 26, 2022

Core patch, just in case:

What?

In WP_Theme_JSON::get_style_nodes()

  • Check for elements values in theme.json before creating a node
  • Check for element name in the whitelist before creating a node

Context: #41160 (comment)

Why?

If an element does not exist in the static::ELEMENTS whitelist, PHP will complain about Notice: Undefined index.

This won't fix the current notice in core, but is a preparatory patch for the 6.1 migration.

Testing Instructions

Check that the tests pass.

To check manually that things are still working, here's some test theme.json

{
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"elements": {
			"button": {
				"color": {
					"background": "green"
				}
			}
		}
	}
}

Using this JSON, your buttons should have a green background.

Check for element name in the whitelist
@ramonjd ramonjd added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 26, 2022
@ramonjd ramonjd self-assigned this Aug 26, 2022
@ramonjd ramonjd requested review from mcsf and scruffian August 26, 2022 05:31
@@ -491,7 +491,7 @@ protected static function get_style_nodes( $theme_json, $selectors = array() ) {

if ( isset( $theme_json['styles']['elements'] ) ) {
foreach ( self::ELEMENTS as $element => $selector ) {
if ( ! isset( $theme_json['styles']['elements'][ $element ] ) ) {
if ( ! isset( $theme_json['styles']['elements'][ $element ] ) || empty( static::ELEMENTS[ $element ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will produce an Undefined index in PHP 5.6 - https://3v4l.org/vqKpS#v5.6.40, and we cannot use isset() since it will result in a fatal error.

So let's use array_key_exists, similar to #42567.

Copy link
Member Author

@ramonjd ramonjd Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Mamaduka

Just checking, do you mean the isset( $theme_json['styles']['elements'][ $element ] ) or empty( static::ELEMENTS[ $element ] )?

Or isn't empty okay with constants either?

I'll update to

if ( ! isset( $theme_json['styles']['elements'][ $element ] ) || ! array_key_exists( $element, static::ELEMENTS ) ) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being clear; it's still early morning for me ☕

I mean - empty( static::ELEMENTS[ $element ] )

The isset and empty don't work as you expect with class constants in PHP 5.6.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah great. I knew about isset, but TIL about empty. Thanks a lot for the nudge.

Updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick follow-up.

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix :)

@ramonjd ramonjd merged commit 83442b0 into trunk Aug 29, 2022
@ramonjd ramonjd deleted the update/theme-json-get-style-nodes-check-elements branch August 29, 2022 01:35
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Aug 29, 2022
@scruffian
Copy link
Contributor

Thanks for taking care of this.

@oandregal
Copy link
Member

Hey, while working on #46579 I realized this PR was not backported to WordPress 6.1. When we land that PR and then backport to WordPress 6.2 everything will be fine, but I wanted to raise in case you think this should be ported for a potential WordPress 6.1.X release.

@ramonjd
Copy link
Member Author

ramonjd commented Dec 18, 2022

Thanks for the heads up @oandregal
This one must have fallen through the net. I'll make note of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants