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

[Style Engine] PHP Notice: Undefined index: 'individual' #43121

Closed
hellofromtonya opened this issue Aug 10, 2022 · 1 comment · Fixed by #43122
Closed

[Style Engine] PHP Notice: Undefined index: 'individual' #43121

hellofromtonya opened this issue Aug 10, 2022 · 1 comment · Fixed by #43122
Assignees
Labels
[Package] Style Engine /packages/style-engine [Type] Bug An existing feature does not function as intended

Comments

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Aug 10, 2022

Description

A PHP notice "Undefined index: individual" is being thrown in WP_Style_Engine::get_css_declarations() https://github.com/WordPress/gutenberg/blob/trunk/packages/style-engine/class-wp-style-engine.php#L418.

What is happening?

WP_Style_Engine::parse_block_styles() invokes WP_Style_Engine::get_css_declarations() passing the style definition of the group style from static::BLOCK_STYLE_DEFINITIONS_METADATA. But not all 'property_keys' contain an 'individual' parameter, such as 'color'.

Notice how WP_Style_Engine::get_individual_property_css_declarations() provides guarding to ensure the key exists in the style definition before accessing it

$style_definition = _wp_array_get( static::BLOCK_STYLE_DEFINITIONS_METADATA, $style_definition_path, null );
if ( $style_definition && isset( $style_definition['property_keys']['individual'] ) ) {
.

Questions:

  • Should WP_Style_Engine::parse_block_styles() be passing all of the static::BLOCK_STYLE_DEFINITIONS_METADATA to WP_Style_Engine::get_css_declarations() for processing?
  • If yes, what should happen in WP_Style_Engine::get_css_declarations() when a style definition's property_keys' do not define the 'individual' parameter? Should it skip that $style_value? (I assume yes.)

Step-by-step reproduction instructions

  1. Set define( 'WP_DEBUG', true ); in your local environment's wp-config.php file OR in the PHPUnit's bootstrap.php file.
  2. When running locally, check the error logs. When running the PHP tests, notice the test error.

Screenshots, screen recording, code snippet

First found when turning on WP_DEBUG for its PHPUnit tests https://github.com/WordPress/gutenberg/runs/7756878353?check_suite_focus=true

.............................E......                            477 / 477 (100%)

Time: 3.85 seconds, Memory: [46](https://github.com/WordPress/gutenberg/runs/7756878353?check_suite_focus=true#step:7:47).50MB

There was 1 error:

1) WP_Style_Engine_Test::test_generate_get_styles with data set "invalid_classnames_options" (array(array(array('friends'), array('tasty'))), array(), array())
Undefined index: individual

/var/www/html/wp-content/plugins/gutenberg/packages/style-engine/class-wp-style-engine.php:422
/var/www/html/wp-content/plugins/gutenberg/packages/style-engine/class-wp-style-engine.php:338
/var/www/html/wp-content/plugins/gutenberg/packages/style-engine/class-wp-style-engine.php:562
/var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php:37
phpvfscomposer:///var/www/html/wp-content/plugins/gutenberg/vendor/phpunit/phpunit/phpunit:[52](https://github.com/WordPress/gutenberg/runs/7756878353?check_suite_focus=true#step:7:53)

Environment info

  • WordPress trunk
  • Gutenberg trunk
  • localhost: wp-env
  • Plugins: only Gutenberg is activated
  • Theme: TT2

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@hellofromtonya hellofromtonya added [Type] Bug An existing feature does not function as intended [Package] Style Engine /packages/style-engine labels Aug 10, 2022
@hellofromtonya
Copy link
Contributor Author

hellofromtonya commented Aug 10, 2022

I first thought this kind of defensive guarding:

// Default rule builder.
// If the input contains an array, assume box model-like properties
// for styles such as margins and padding.
if ( is_array( $style_value ) ) {
	foreach ( $style_value as $key => $value ) {
		if ( is_string( $value ) && strpos( $value, 'var:' ) !== false && ! $should_skip_css_vars && ! empty( $style_definition['css_vars'] ) ) {
			$value = static::get_css_var_value( $value, $style_definition['css_vars'] );
		}

		// Skip if this style does not define the 'individual' property.
		if ( ! isset( $style_property_keys['individual'] ) ) {
			continue;
		}

		$individual_property = sprintf( $style_property_keys['individual'], _wp_to_kebab_case( $key ) );

		if ( $individual_property && static::is_valid_style_value( $value ) ) {
			$css_declarations[ $individual_property ] = $value;
		}
	}
} else {
	$css_declarations[ $style_property_keys['default'] ] = $style_value;
}

But this causes unnecessarily processing time as none of the loop iterations will process. So instead, I'm thinking of bailing out early like this:

// Default rule builder.
// If the input contains an array, assume box model-like properties
// for styles such as margins and padding.
if ( is_array( $style_value ) ) {
	// Bail out early if the `'individual'` property is not defined.
	if ( ! isset( $style_property_keys['individual'] ) ) {
		return $css_declarations;
	}

	foreach ( $style_value as $key => $value ) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine [Type] Bug An existing feature does not function as intended
Projects
None yet
2 participants