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.

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

Fix wp_head performance regression for classic themes #3536

Closed
wants to merge 15 commits into from
Closed

Fix wp_head performance regression for classic themes #3536

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
787b2c2
Avoid parsing core's theme.json and theme-i18n.json when using a clas…
felixarntz Oct 27, 2022
aff6347
Avoid DB query for global styles when using a classic theme without s…
felixarntz Oct 27, 2022
196c215
Merge branch 'trunk' into fix/classic-themes-wp_head-regression
felixarntz Nov 2, 2022
0c768d3
Further optimize WP_Theme_JSON_Resolver logic for classic themes, use…
felixarntz Nov 2, 2022
b9ff774
Add demo version of theme-json.php from WP core theme.json (via var_e…
felixarntz Nov 2, 2022
f34fe53
Merge branch 'trunk' into fix/classic-themes-wp_head-regression
felixarntz Nov 8, 2022
6a835a4
Remove temporary theme.json in favor of https://github.com/WordPress/…
felixarntz Nov 8, 2022
1ab511c
Include parsing parent theme.json logic in cached logic block.
felixarntz Nov 8, 2022
bf4e711
Merge branch 'trunk' into fix/classic-themes-wp_head-regression
felixarntz Nov 9, 2022
4966109
Add test coverage and fix relevant tests to require a block theme.
felixarntz Nov 9, 2022
3a24f75
Merge branch 'trunk' into fix/classic-themes-wp_head-regression
felixarntz Nov 9, 2022
34c1957
Address code review feedback.
felixarntz Nov 9, 2022
392391a
Limit comment to 80 characters per line.
felixarntz Nov 10, 2022
782c49d
Merge branch 'trunk' into fix/classic-themes-wp_head-regression
felixarntz Nov 10, 2022
3ab53bf
Merge branch 'fix/classic-themes-wp_head-regression' of github.com:fe…
felixarntz Nov 10, 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
49 changes: 32 additions & 17 deletions src/wp-includes/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,13 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$options = wp_parse_args( $options, array( 'with_supports' => true ) );

if ( null === static::$theme || ! static::has_same_registered_blocks( 'theme' ) ) {
$theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json' ) );
$theme_json_data = static::translate( $theme_json_data, wp_get_theme()->get( 'TextDomain' ) );
$theme_json_file = static::get_file_path_from_theme( 'theme.json' );
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
if ( '' !== $theme_json_file ) {
$theme_json_data = static::read_json_file( $theme_json_file );
$theme_json_data = static::translate( $theme_json_data, wp_get_theme()->get( 'TextDomain' ) );
} else {
$theme_json_data = array();
}

/**
* Filters the data provided by the theme for global styles and settings.
Expand All @@ -259,20 +264,23 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$theme_json = apply_filters( 'wp_theme_json_data_theme', new WP_Theme_JSON_Data( $theme_json_data, 'theme' ) );
$theme_json_data = $theme_json->get_data();
static::$theme = new WP_Theme_JSON( $theme_json_data );
}
felixarntz marked this conversation as resolved.
Show resolved Hide resolved

if ( wp_get_theme()->parent() ) {
// Get parent theme.json.
$parent_theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json', true ) );
$parent_theme_json_data = static::translate( $parent_theme_json_data, wp_get_theme()->parent()->get( 'TextDomain' ) );
$parent_theme = new WP_Theme_JSON( $parent_theme_json_data );

/*
* Merge the child theme.json into the parent theme.json.
* The child theme takes precedence over the parent.
*/
$parent_theme->merge( static::$theme );
static::$theme = $parent_theme;
if ( wp_get_theme()->parent() ) {
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
// Get parent theme.json.
$parent_theme_json_file = static::get_file_path_from_theme( 'theme.json', true );
if ( '' !== $parent_theme_json_file ) {
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
$parent_theme_json_data = static::read_json_file( $parent_theme_json_file );
$parent_theme_json_data = static::translate( $parent_theme_json_data, wp_get_theme()->parent()->get( 'TextDomain' ) );
$parent_theme = new WP_Theme_JSON( $parent_theme_json_data );

/*
* Merge the child theme.json into the parent theme.json.
* The child theme takes precedence over the parent.
*/
$parent_theme->merge( static::$theme );
static::$theme = $parent_theme;
}
}
}

if ( ! $options['with_supports'] ) {
Expand Down Expand Up @@ -403,6 +411,13 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
if ( ! $theme instanceof WP_Theme ) {
$theme = wp_get_theme();
}

// Bail early if the theme does not support a theme.json. Since WP_Theme_JSON_Resolver::theme_has_support()
// only supports the active theme, the extra condition for whether $theme is the active theme is present here.
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
if ( $theme->get_stylesheet() === get_stylesheet() && ! static::theme_has_support() ) {
felixarntz marked this conversation as resolved.
Show resolved Hide resolved
return array();
}

$user_cpt = array();
$post_type_filter = 'wp_global_styles';
$stylesheet = $theme->get_stylesheet();
Expand Down Expand Up @@ -582,8 +597,8 @@ public static function get_user_global_styles_post_id() {
public static function theme_has_support() {
if ( ! isset( static::$theme_has_support ) ) {
static::$theme_has_support = (
is_readable( static::get_file_path_from_theme( 'theme.json' ) ) ||
is_readable( static::get_file_path_from_theme( 'theme.json', true ) )
static::get_file_path_from_theme( 'theme.json' ) !== '' ||
Copy link
Member

Choose a reason for hiding this comment

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

This is nice, get_file_path_from_theme already calls is_readable.

static::get_file_path_from_theme( 'theme.json', true ) !== ''
Comment on lines +605 to +606
Copy link
Contributor

Choose a reason for hiding this comment

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

Yoda

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is needed here. Yoda conditions are only required around variable to value comparisons, not with functions (since setting a function to another value would result in an error anyway).

);
}

Expand Down
3 changes: 2 additions & 1 deletion 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 @@ -2939,7 +2939,8 @@ public function get_data() {
public function set_spacing_sizes() {
$spacing_scale = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'spacingScale' ), array() );

if ( ! is_numeric( $spacing_scale['steps'] )
if ( ! isset( $spacing_scale['steps'] )
|| ! is_numeric( $spacing_scale['steps'] )
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
|| ! isset( $spacing_scale['mediumStep'] )
|| ! isset( $spacing_scale['unit'] )
|| ! isset( $spacing_scale['operator'] )
Expand Down
98 changes: 73 additions & 25 deletions tests/phpunit/tests/theme/wpThemeJsonResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,6 @@ class Tests_Theme_wpThemeJsonResolver extends WP_UnitTestCase {
*/
private $orig_theme_dir;

/**
* Queries.
*
* @var array
*/
private $queries = array();

/**
* WP_Theme_JSON_Resolver::$blocks_cache property.
*
Expand Down Expand Up @@ -105,7 +98,7 @@ public function set_up() {
add_filter( 'theme_root', array( $this, 'filter_set_theme_root' ) );
add_filter( 'stylesheet_root', array( $this, 'filter_set_theme_root' ) );
add_filter( 'template_root', array( $this, 'filter_set_theme_root' ) );
$this->queries = array();

// Clear caches.
wp_clean_themes_cache();
unset( $GLOBALS['wp_themes'] );
Expand All @@ -129,13 +122,6 @@ public function filter_set_locale_to_polish() {
return 'pl_PL';
}

function filter_db_query( $query ) {
if ( preg_match( '#post_type = \'wp_global_styles\'#', $query ) ) {
$this->queries[] = $query;
}
return $query;
}

/**
* @ticket 52991
* @ticket 54336
Expand Down Expand Up @@ -634,58 +620,91 @@ function test_merges_child_theme_json_into_parent_theme_json() {
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries() {
// Switch to a theme that does have support.
switch_theme( 'block-theme' );
peterwilsoncc marked this conversation as resolved.
Show resolved Hide resolved
wp_set_current_user( self::$administrator_id );
$theme = wp_get_theme();
WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
add_filter( 'query', array( $this, 'filter_db_query' ) );
$query_count = count( $this->queries );
$global_styles_query_count = 0;
add_filter(
'query',
function( $query ) use ( &$global_styles_query_count ) {
if ( preg_match( '#post_type = \'wp_global_styles\'#', $query ) ) {
$global_styles_query_count++;
}
return $query;
}
);
for ( $i = 0; $i < 3; $i++ ) {
WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
WP_Theme_JSON_Resolver::clean_cached_data();
}
$query_count = count( $this->queries ) - $query_count;
$this->assertSame( 0, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type prior to creation.' );
$this->assertSame( 0, $global_styles_query_count, 'Unexpected SQL queries detected for the wp_global_style post type prior to creation.' );

$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertEmpty( $user_cpt, 'User CPT is expected to be empty.' );

$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme, true );
$this->assertNotEmpty( $user_cpt, 'User CPT is expected not to be empty.' );

$query_count = count( $this->queries );
$global_styles_query_count = 0;
for ( $i = 0; $i < 3; $i ++ ) {
$new_user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
WP_Theme_JSON_Resolver::clean_cached_data();
$this->assertSameSets( $user_cpt, $new_user_cpt, "User CPTs do not match on run {$i}." );
}
$query_count = count( $this->queries ) - $query_count;
$this->assertSame( 1, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type after creation.' );
$this->assertSame( 1, $global_styles_query_count, 'Unexpected SQL queries detected for the wp_global_style post type after creation.' );
}

/**
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries_for_logged_out_users() {
// Switch to a theme that does have support.
switch_theme( 'block-theme' );
$theme = wp_get_theme();
WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
add_filter( 'query', array( $this, 'filter_db_query' ) );
$query_count = count( $this->queries );
$query_count = get_num_queries();
for ( $i = 0; $i < 3; $i++ ) {
WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
WP_Theme_JSON_Resolver::clean_cached_data();
}
$query_count = count( $this->queries ) - $query_count;
$query_count = get_num_queries() - $query_count;
$this->assertSame( 0, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type prior to creation.' );

$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertEmpty( $user_cpt, 'User CPT is expected to be empty.' );
}

/**
* @ticket 56945
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_does_not_run_for_theme_without_support() {
// The 'default' theme does not support theme.json.
switch_theme( 'default' );
wp_set_current_user( self::$administrator_id );
$theme = wp_get_theme();

$start_queries = get_num_queries();

// When theme.json is not supported, the method should not run a query and always return an empty result.
$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertEmpty( $user_cpt, 'User CPT is expected to be empty.' );
$this->assertSame( 0, get_num_queries() - $start_queries, 'Unexpected SQL query detected for theme without theme.json support.' );

$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme, true );
$this->assertEmpty( $user_cpt, 'User CPT is expected to be empty.' );
$this->assertSame( 0, get_num_queries() - $start_queries, 'Unexpected SQL query detected for theme without theme.json support.' );
}

/**
* @ticket 55392
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_does_exist() {
// Switch to a theme that does have support.
switch_theme( 'block-theme' );
$theme = wp_get_theme();
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme, true );
$this->assertIsArray( $post1 );
Expand All @@ -701,6 +720,8 @@ function test_get_user_data_from_wp_global_styles_does_exist() {
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_create_post() {
// Switch to a theme that does have support.
switch_theme( 'block-theme' );
$theme = wp_get_theme( 'testing' );
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertIsArray( $post1 );
Expand All @@ -718,6 +739,8 @@ function test_get_user_data_from_wp_global_styles_create_post() {
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
*/
function test_get_user_data_from_wp_global_styles_filter_state() {
// Switch to a theme that does have support.
switch_theme( 'block-theme' );
$theme = wp_get_theme( 'foo' );
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme, true, array( 'publish' ) );
$this->assertIsArray( $post1 );
Expand Down Expand Up @@ -749,4 +772,29 @@ function test_get_theme_data_theme_supports_overrides_theme_json() {
$line_height = $current_settings['typography']['lineHeight'];
$this->assertTrue( $line_height, 'lineHeight setting after add_theme_support() should be true.' );
}

/**
* @ticket 56945
* @covers WP_Theme_JSON_Resolver::get_theme_data
*/
function test_get_theme_data_does_not_parse_theme_json_if_not_present() {
// The 'default' theme does not support theme.json.
switch_theme( 'default' );

$theme_json_resolver = new WP_Theme_JSON_Resolver();

// Force-unset $i18n_schema property to "unload" translation schema.
$property = new ReflectionProperty( $theme_json_resolver, 'i18n_schema' );
$property->setAccessible( true );
$property->setValue( null );

// A completely empty theme.json data set still has the 'version' key when parsed.
$empty_theme_json = array( 'version' => WP_Theme_JSON::LATEST_SCHEMA );

// Call using 'with_supports' set to false, so that the method only considers theme.json.
$theme_data = $theme_json_resolver->get_theme_data( array(), array( 'with_supports' => false ) );
$this->assertInstanceOf( 'WP_Theme_JSON', $theme_data, 'Theme data should be an instance of WP_Theme_JSON.' );
$this->assertSame( $empty_theme_json, $theme_data->get_raw_data(), 'Theme data should be empty without theme support.' );
$this->assertNull( $property->getValue(), 'Theme i18n schema should not have been loaded without theme support.' );
}
}