Skip to content

Improve performance in the WP_Theme_JSON class by simplifying calls to array functions #3555

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.

Sign up for GitHub

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

Closed
aristath wants to merge 25 commits into from
Closed

Improve performance in the WP_Theme_JSON class by simplifying calls to array functions #3555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9ca0b27
WIP
jrfnl Nov 2, 2022
57f0dba
WIP step 2
jrfnl Nov 2, 2022
58b0ff1
WIP - step 3
jrfnl Nov 2, 2022
d3bbd20
WIP Step 3b
jrfnl Nov 2, 2022
c17341b
WIP - setp 4
jrfnl Nov 2, 2022
aa13245
WIP - simplify it a little more
jrfnl Nov 2, 2022
188ce0f
WIP - step 5
jrfnl Nov 2, 2022
194368e
WIP - side step 6
jrfnl Nov 2, 2022
8fed0c1
WIP - side step 7
jrfnl Nov 2, 2022
5fa9aae
CS & cleanup
aristath Nov 3, 2022
979bfef
replace array_key_exists with isset
aristath Nov 3, 2022
be50567
replace an array_merge with a foreach loop
aristath Nov 3, 2022
6aa5ede
Add an inline comment to avoid future refactors
aristath Nov 3, 2022
4434b1d
We can use isset() instead of array_key_exists() here (no null values)
aristath Nov 3, 2022
505a4f8
replace array_merge with a foreach loop
aristath Nov 3, 2022
0345837
replace array_merge with foreach loop in the flatten_tree method
aristath Nov 3, 2022
8df441c
replace array_key_exists() with isset()
aristath Nov 3, 2022
8c04874
Replace more array_merge with foreach loops where appropriate
aristath Nov 3, 2022
213e94b
Fix PHP 5.6 errors + added notes for the future
aristath Nov 3, 2022
8923cd8
Merge branch 'trunk' into feature/pp-ari-wp-theme-json
felixarntz Nov 10, 2022
5730ac1
Add comment references pointing to https://core.trac.wordpress.org/ti…
felixarntz Nov 10, 2022
4af0999
Merge branch 'trunk' into feature/pp-ari-wp-theme-json
felixarntz Nov 10, 2022
abf51b6
Merge branch 'trunk' into feature/pp-ari-wp-theme-json
felixarntz Nov 10, 2022
4889396
Merge branch 'trunk' into feature/pp-ari-wp-theme-json
felixarntz Nov 11, 2022
7ecd0a8
Clarify variable name.
felixarntz Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 90 additions & 31 deletions src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,8 @@ class WP_Theme_JSON {
public static function get_element_class_name( $element ) {
$class_name = '';

// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $element, static::__EXPERIMENTAL_ELEMENT_CLASS_NAMES ) ) {
$class_name = static::__EXPERIMENTAL_ELEMENT_CLASS_NAMES[ $element ];
}
Expand Down Expand Up @@ -519,7 +521,10 @@ public function __construct( $theme_json = array(), $origin = 'theme' ) {
$nodes = static::get_setting_nodes( $this->theme_json );
foreach ( $nodes as $node ) {
foreach ( static::PRESETS_METADATA as $preset_metadata ) {
$path = array_merge( $node['path'], $preset_metadata['path'] );
$path = $node['path'];
foreach ( $preset_metadata['path'] as $subpath ) {
$path[] = $subpath;
}
$preset = _wp_array_get( $this->theme_json, $path, null );
if ( null !== $preset ) {
// If the preset is not already keyed by origin.
Expand Down Expand Up @@ -608,6 +613,7 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n
*/
$styles_non_top_level = static::VALID_STYLES;
foreach ( array_keys( $styles_non_top_level ) as $section ) {
// array_key_exists() needs to be used instead of isset() because the value can be null.
if ( array_key_exists( $section, $styles_non_top_level ) && is_array( $styles_non_top_level[ $section ] ) ) {
foreach ( array_keys( $styles_non_top_level[ $section ] ) as $prop ) {
if ( 'top' === $styles_non_top_level[ $section ][ $prop ] ) {
Expand All @@ -631,6 +637,8 @@ protected static function sanitize( $input, $valid_block_names, $valid_element_n
foreach ( $valid_element_names as $element ) {
$schema_styles_elements[ $element ] = $styles_non_top_level;

// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) ) {
foreach ( static::VALID_ELEMENT_PSEUDO_SELECTORS[ $element ] as $pseudo_selector ) {
$schema_styles_elements[ $element ][ $pseudo_selector ] = $styles_non_top_level;
Expand Down Expand Up @@ -1273,8 +1281,12 @@ protected function get_css_variables( $nodes, $origins ) {

$selector = $metadata['selector'];

$node = _wp_array_get( $this->theme_json, $metadata['path'], array() );
$declarations = array_merge( static::compute_preset_vars( $node, $origins ), static::compute_theme_vars( $node ) );
$node = _wp_array_get( $this->theme_json, $metadata['path'], array() );
$declarations = static::compute_preset_vars( $node, $origins );
$theme_vars_declarations = static::compute_theme_vars( $node );
foreach ( $theme_vars_declarations as $theme_vars_declaration ) {
$declarations[] = $theme_vars_declaration;
}

$stylesheet .= static::to_ruleset( $selector, $declarations );
}
Expand Down Expand Up @@ -1601,11 +1613,11 @@ protected static function flatten_tree( $tree, $prefix = '', $token = '--' ) {
);

if ( is_array( $value ) ) {
$new_prefix = $new_key . $token;
$result = array_merge(
$result,
static::flatten_tree( $value, $new_prefix, $token )
);
$new_prefix = $new_key . $token;
$flattened_subtree = static::flatten_tree( $value, $new_prefix, $token );
foreach ( $flattened_subtree as $subtree_key => $subtree_value ) {
$result[ $subtree_key ] = $subtree_value;
}
} else {
$result[ $new_key ] = $value;
}
Expand Down Expand Up @@ -1667,6 +1679,8 @@ protected static function compute_style_properties( $styles, $settings = array()
if ( is_array( $value_path ) ) {
$path_string = implode( '.', $value_path );
if (
// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
array_key_exists( $path_string, static::PROTECTED_PROPERTIES ) &&
_wp_array_get( $settings, static::PROTECTED_PROPERTIES[ $path_string ], null ) === null
) {
Expand Down Expand Up @@ -1742,15 +1756,15 @@ protected static function get_property_value( $styles, $path, $theme_json = null
* where the values is an array with a "ref" key, pointing to a path.
* For example: { "ref": "style.color.background" } => "#fff".
*/
if ( is_array( $value ) && array_key_exists( 'ref', $value ) ) {
if ( is_array( $value ) && isset( $value['ref'] ) ) {
$value_path = explode( '.', $value['ref'] );
$ref_value = _wp_array_get( $theme_json, $value_path );
// Only use the ref value if we find anything.
if ( ! empty( $ref_value ) && is_string( $ref_value ) ) {
$value = $ref_value;
}

if ( is_array( $ref_value ) && array_key_exists( 'ref', $ref_value ) ) {
if ( is_array( $ref_value ) && isset( $ref_value['ref'] ) ) {
$path_string = json_encode( $path );
$ref_value_string = json_encode( $ref_value );
_doing_it_wrong(
Expand Down Expand Up @@ -1886,6 +1900,8 @@ protected static function get_style_nodes( $theme_json, $selectors = array() ) {
);

// Handle any pseudo selectors for the element.
// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) ) {
foreach ( static::VALID_ELEMENT_PSEUDO_SELECTORS[ $element ] as $pseudo_selector ) {

Expand All @@ -1905,7 +1921,10 @@ protected static function get_style_nodes( $theme_json, $selectors = array() ) {
return $nodes;
}

$nodes = array_merge( $nodes, static::get_block_nodes( $theme_json ) );
$block_nodes = static::get_block_nodes( $theme_json );
foreach ( $block_nodes as $block_node ) {
$nodes[] = $block_node;
}

/**
* Filters the list of style nodes with metadata.
Expand Down Expand Up @@ -1982,6 +2001,8 @@ private static function get_block_nodes( $theme_json ) {
);

// Handle any pseudo selectors for the element.
// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) ) {
foreach ( static::VALID_ELEMENT_PSEUDO_SELECTORS[ $element ] as $pseudo_selector ) {
if ( isset( $theme_json['styles']['blocks'][ $name ]['elements'][ $element ][ $pseudo_selector ] ) ) {
Expand Down Expand Up @@ -2035,7 +2056,9 @@ public function get_styles_for_block( $block_metadata ) {
// the feature selector. This may occur when multiple block
// support features use the same custom selector.
if ( isset( $feature_declarations[ $feature_selector ] ) ) {
$feature_declarations[ $feature_selector ] = array_merge( $feature_declarations[ $feature_selector ], $new_feature_declarations );
foreach ( $new_feature_declarations as $new_feature_declaration ) {
$feature_declarations[ $feature_selector ][] = $feature_declaration;
Copy link
Member

@oandregal oandregal Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@aristath @desrosj @felixarntz @spacedmonkey @jrfnl @hellofromtonya

Shouldn't the variable assigned be $new_feature_declaration instead of $feature_declaration? This has been reported as problematic by the Gutenberg lint jobs (see conversation).

I haven't looked at what regression this introduced but it'd be good to create a follow-up PR fixing it and adding a test case.

Copy link
Member

@jrfnl jrfnl Dec 15, 2022

Choose a reason for hiding this comment

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

@oandregal I concur. This is a bug which should be fixed.

Suggested change
$feature_declarations[ $feature_selector ][] = $feature_declaration;
$feature_declarations[ $feature_selector ][] = $new_feature_declaration;

Copy link
Member Author

@aristath aristath Dec 15, 2022

Choose a reason for hiding this comment

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

🤦 yeah, looks like this was a typo.. Good catch 👍

Copy link
Member

@jrfnl jrfnl Dec 15, 2022

Choose a reason for hiding this comment

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

Happens to the best of us ;-)

Copy link
Member

@oandregal oandregal Dec 15, 2022

Choose a reason for hiding this comment

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

I'm not familiar with this code path, so asked folks to provide details for testing at WordPress/gutenberg#46579 (comment) so we can add a new unit test and gauge how severe this is.

Copy link
Member

@jrfnl jrfnl Dec 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICS, this should go in the next 6.1 patch release.

Edit to clarify: I'm basing this assessment on the following:

  • It's very clearly a bug with an obvious fix.
  • It's likely a regression compared to previous behaviour.

}
} else {
$feature_declarations[ $feature_selector ] = $new_feature_declarations;
}
Expand All @@ -2059,6 +2082,8 @@ public function get_styles_for_block( $block_metadata ) {

$element_pseudo_allowed = array();

// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $current_element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) ) {
$element_pseudo_allowed = static::VALID_ELEMENT_PSEUDO_SELECTORS[ $current_element ];
}
Expand All @@ -2084,6 +2109,8 @@ function( $pseudo_selector ) use ( $selector ) {
* Otherwise just compute the styles for the default selector as normal.
*/
if ( $pseudo_selector && isset( $node[ $pseudo_selector ] ) &&
// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
array_key_exists( $current_element, static::VALID_ELEMENT_PSEUDO_SELECTORS )
&& in_array( $pseudo_selector, static::VALID_ELEMENT_PSEUDO_SELECTORS[ $current_element ], true )
) {
Expand Down Expand Up @@ -2283,11 +2310,11 @@ public function merge( $incoming ) {
$nodes = static::get_setting_nodes( $incoming_data );
$slugs_global = static::get_default_slugs( $this->theme_json, array( 'settings' ) );
foreach ( $nodes as $node ) {
$slugs_node = static::get_default_slugs( $this->theme_json, $node['path'] );
$slugs = array_merge_recursive( $slugs_global, $slugs_node );

// Replace the spacing.units.
$path = array_merge( $node['path'], array( 'spacing', 'units' ) );
$path = $node['path'];
$path[] = 'spacing';
$path[] = 'units';

$content = _wp_array_get( $incoming_data, $path, null );
if ( isset( $content ) ) {
_wp_array_set( $this->theme_json, $path, $content );
Expand All @@ -2298,19 +2325,25 @@ public function merge( $incoming ) {
$override_preset = ! static::get_metadata_boolean( $this->theme_json['settings'], $preset['prevent_override'], true );

foreach ( static::VALID_ORIGINS as $origin ) {
$base_path = array_merge( $node['path'], $preset['path'] );
$path = array_merge( $base_path, array( $origin ) );
$content = _wp_array_get( $incoming_data, $path, null );
$base_path = $node['path'];
foreach ( $preset['path'] as $leaf ) {
$base_path[] = $leaf;
}

$path = $base_path;
$path[] = $origin;

$content = _wp_array_get( $incoming_data, $path, null );
if ( ! isset( $content ) ) {
continue;
}

if ( 'theme' === $origin && $preset['use_default_names'] ) {
foreach ( $content as &$item ) {
if ( ! array_key_exists( 'name', $item ) ) {
foreach ( $content as $key => $item ) {
if ( ! isset( $item['name'] ) ) {
$name = static::get_name_from_defaults( $item['slug'], $base_path );
if ( null !== $name ) {
$item['name'] = $name;
$content[ $key ]['name'] = $name;
}
}
}
Expand All @@ -2322,6 +2355,9 @@ public function merge( $incoming ) {
) {
_wp_array_set( $this->theme_json, $path, $content );
} else {
$slugs_node = static::get_default_slugs( $this->theme_json, $node['path'] );
$slugs = array_merge_recursive( $slugs_global, $slugs_node );
Copy link
Member

@spacedmonkey spacedmonkey Nov 8, 2022

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

@aristath aristath Nov 9, 2022

Choose a reason for hiding this comment

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

It wasn't added, it was just moved here. 😉
A few lines above we were calling these 2 on the previous foreach loop (used to be line 2281), so they were always running. Instead of always calling them, we moved them here where they are actually used. This way these rather expensive calls run fewer times, only when necessary.


$slugs_for_preset = _wp_array_get( $slugs, $preset['path'], array() );
$content = static::filter_slugs( $content, $slugs_for_preset );
_wp_array_set( $this->theme_json, $path, $content );
Expand Down Expand Up @@ -2434,7 +2470,12 @@ protected static function get_default_slugs( $data, $node_path ) {
$slugs = array();

foreach ( static::PRESETS_METADATA as $metadata ) {
$path = array_merge( $node_path, $metadata['path'], array( 'default' ) );
$path = $node_path;
foreach ( $metadata['path'] as $leaf ) {
$path[] = $leaf;
}
$path[] = 'default';

$preset = _wp_array_get( $data, $path, null );
if ( ! isset( $preset ) ) {
continue;
Expand Down Expand Up @@ -2463,7 +2504,8 @@ protected static function get_default_slugs( $data, $node_path ) {
* @return string|null
*/
protected function get_name_from_defaults( $slug, $base_path ) {
$path = array_merge( $base_path, array( 'default' ) );
$path = $base_path;
$path[] = 'default';
$default_content = _wp_array_get( $this->theme_json, $path, null );
if ( ! $default_content ) {
return null;
Expand Down Expand Up @@ -2539,6 +2581,8 @@ public static function remove_insecure_properties( $theme_json ) {
* $output is stripped of pseudo selectors. Re-add and process them
* or insecure styles here.
*/
// TODO: Replace array_key_exists() with isset() check once WordPress drops
// support for PHP 5.6. See https://core.trac.wordpress.org/ticket/57067.
if ( array_key_exists( $current_element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) ) {
foreach ( static::VALID_ELEMENT_PSEUDO_SELECTORS[ $current_element ] as $pseudo_selector ) {
if ( isset( $input[ $pseudo_selector ] ) ) {
Expand Down Expand Up @@ -2593,8 +2637,9 @@ protected static function remove_insecure_settings( $input ) {
$output = array();
foreach ( static::PRESETS_METADATA as $preset_metadata ) {
foreach ( static::VALID_ORIGINS as $origin ) {
$path_with_origin = array_merge( $preset_metadata['path'], array( $origin ) );
$presets = _wp_array_get( $input, $path_with_origin, null );
$path_with_origin = $preset_metadata['path'];
$path_with_origin[] = $origin;
$presets = _wp_array_get( $input, $path_with_origin, null );
if ( null === $presets ) {
continue;
}
Expand Down Expand Up @@ -2839,7 +2884,10 @@ public function get_data() {
*/
foreach ( $nodes as $node ) {
foreach ( static::PRESETS_METADATA as $preset_metadata ) {
$path = array_merge( $node['path'], $preset_metadata['path'] );
$path = $node['path'];
foreach ( $preset_metadata['path'] as $preset_metadata_path ) {
$path[] = $preset_metadata_path;
}
$preset = _wp_array_get( $output, $path, null );
if ( null === $preset ) {
continue;
Expand Down Expand Up @@ -2873,7 +2921,10 @@ public function get_data() {
foreach ( $nodes as $node ) {
$all_opt_ins_are_set = true;
foreach ( static::APPEARANCE_TOOLS_OPT_INS as $opt_in_path ) {
$full_path = array_merge( $node['path'], $opt_in_path );
$full_path = $node['path'];
foreach ( $opt_in_path as $opt_in_path_item ) {
$full_path[] = $opt_in_path_item;
}
// Use "unset prop" as a marker instead of "null" because
// "null" can be a valid value for some props (e.g. blockGap).
$opt_in_value = _wp_array_get( $output, $full_path, 'unset prop' );
Expand All @@ -2884,9 +2935,14 @@ public function get_data() {
}

if ( $all_opt_ins_are_set ) {
_wp_array_set( $output, array_merge( $node['path'], array( 'appearanceTools' ) ), true );
$node_path_with_appearance_tools = $node['path'];
$node_path_with_appearance_tools[] = 'appearanceTools';
_wp_array_set( $output, $node_path_with_appearance_tools, true );
foreach ( static::APPEARANCE_TOOLS_OPT_INS as $opt_in_path ) {
$full_path = array_merge( $node['path'], $opt_in_path );
$full_path = $node['path'];
foreach ( $opt_in_path as $opt_in_path_item ) {
$full_path[] = $opt_in_path_item;
}
// Use "unset prop" as a marker instead of "null" because
// "null" can be a valid value for some props (e.g. blockGap).
$opt_in_value = _wp_array_get( $output, $full_path, 'unset prop' );
Expand Down Expand Up @@ -3037,7 +3093,10 @@ public function set_spacing_sizes() {
$slug += 10;
}

$spacing_sizes = array_merge( $below_sizes, $above_sizes );
$spacing_sizes = $below_sizes;
foreach ( $above_sizes as $above_sizes_item ) {
$spacing_sizes[] = $above_sizes_item;
}

// If there are 7 or less steps in the scale revert to numbers for labels instead of t-shirt sizes.
if ( $spacing_scale['steps'] <= 7 ) {
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/tests/theme/wpThemeJson.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
* @since 5.8.0
*
* @group themes
*
* @covers WP_Theme_JSON
*/

class Tests_Theme_wpThemeJson extends WP_UnitTestCase {

/**
Expand Down