-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Elements: check value and whitelist before building style nodes #43622
Conversation
Check for element name in the whitelist
@@ -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 ] ) ) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ) ) {
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 follow-up.
There was a problem hiding this 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 :)
Thanks for taking care of this. |
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. |
Thanks for the heads up @oandregal |
Core patch, just in case:
What?
In
WP_Theme_JSON::get_style_nodes()
Context: #41160 (comment)
Why?
If an element does not exist in the
static::ELEMENTS
whitelist, PHP will complain aboutNotice: 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
Using this JSON, your buttons should have a green background.