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

Improve caching and logic in get_user_data_from_wp_global_styles. #2414

Closed
wants to merge 15 commits into from
Closed

Improve caching and logic in get_user_data_from_wp_global_styles. #2414

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
075d00a
Improve caching of global styles.
spacedmonkey Jun 13, 2022
2da31ee
Merge branch 'trunk' into fix/global-query
spacedmonkey Jun 17, 2022
83f9579
Add some unit tests.
spacedmonkey Jun 17, 2022
82ffd45
Improve caching.
spacedmonkey Jun 17, 2022
95667ce
Handle more edge cases.
spacedmonkey Jun 17, 2022
71bcb6c
Improve tests.
spacedmonkey Jun 17, 2022
478fa51
Merge branch 'trunk' into fix/global-query
spacedmonkey Jul 5, 2022
8bb1cd7
Add expiration back again.
spacedmonkey Jul 5, 2022
32d7306
Fix style.
spacedmonkey Jul 5, 2022
7482d90
Merge branch 'trunk' into fix/global-query
spacedmonkey Sep 9, 2022
12e6322
Fix test.
spacedmonkey Sep 9, 2022
67537a2
Apply suggestions from code review
spacedmonkey Sep 15, 2022
0389421
Merge branch 'trunk' into fix/global-query
spacedmonkey Sep 15, 2022
76fd86c
Formatting of tests.
spacedmonkey Sep 15, 2022
8e567de
Fix styling.
spacedmonkey Sep 15, 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
48 changes: 26 additions & 22 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 @@ -255,54 +255,58 @@ public static function get_user_data_from_wp_global_styles( $theme, $create_post
}
$user_cpt = array();
$post_type_filter = 'wp_global_styles';
$stylesheet = $theme->get_stylesheet();
$args = array(
'numberposts' => 1,
draganescu marked this conversation as resolved.
Show resolved Hide resolved
'orderby' => 'date',
'order' => 'desc',
'post_type' => $post_type_filter,
'post_status' => $post_status_filter,
'tax_query' => array(
'posts_per_page' => 1,
'orderby' => 'post_date',
'order' => 'desc',
'post_type' => $post_type_filter,
'post_status' => $post_status_filter,
'ignore_sticky_posts' => true,
'no_found_rows' => true,
'tax_query' => array(
array(
'taxonomy' => 'wp_theme',
'field' => 'name',
'terms' => $theme->get_stylesheet(),
'terms' => $stylesheet,
),
),
);

$cache_key = sprintf( 'wp_global_styles_%s', md5( serialize( $args ) ) );
$post_id = wp_cache_get( $cache_key );

if ( (int) $post_id > 0 ) {
return get_post( $post_id, ARRAY_A );
}

$post_id = (int) get_transient( $cache_key );
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
// Special case: '-1' is a results not found.
if ( -1 === $post_id && ! $create_post ) {
return $user_cpt;
}

$recent_posts = wp_get_recent_posts( $args );
if ( is_array( $recent_posts ) && ( count( $recent_posts ) === 1 ) ) {
$user_cpt = $recent_posts[0];
if ( $post_id > 0 && in_array( get_post_status( $post_id ), (array) $post_status_filter, true ) ) {
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
return get_post( $post_id, ARRAY_A );
}

$gs_query = new WP_Query();
$recent_posts = $gs_query->query( $args );
draganescu marked this conversation as resolved.
Show resolved Hide resolved
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
if ( count( $recent_posts ) === 1 ) {
$user_cpt = get_post( $recent_posts[0], ARRAY_A );
} elseif ( $create_post ) {
$cpt_post_id = wp_insert_post(
array(
'post_content' => '{"version": ' . WP_Theme_JSON::LATEST_SCHEMA . ', "isGlobalStylesUserThemeJSON": true }',
'post_status' => 'publish',
'post_title' => 'Custom Styles',
'post_title' => __( 'Custom Styles' ),
'post_type' => $post_type_filter,
'post_name' => 'wp-global-styles-' . urlencode( wp_get_theme()->get_stylesheet() ),
'post_name' => sprintf( 'wp-global-styles-%s', urlencode( $stylesheet ) ),
'tax_input' => array(
'wp_theme' => array( wp_get_theme()->get_stylesheet() ),
'wp_theme' => array( $stylesheet ),
),
),
true
);
$user_cpt = get_post( $cpt_post_id, ARRAY_A );
if ( ! is_wp_error( $cpt_post_id ) ) {
$user_cpt = get_post( $cpt_post_id, ARRAY_A );
}
}
$cache_expiration = $user_cpt ? DAY_IN_SECONDS : HOUR_IN_SECONDS;
wp_cache_set( $cache_key, $user_cpt ? $user_cpt['ID'] : -1, '', $cache_expiration );
set_transient( $cache_key, $user_cpt ? $user_cpt['ID'] : - 1 );
Copy link
Contributor

@peterwilsoncc peterwilsoncc Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a little worried about the switch to a non-expiring transient as it could affect sites without a persistent object cache.

Once this is set, it's in the options table and loaded in alloptions for life. If the site owner changes theme then the redundant data will remain in their database unless they manually clear it.

I am wondering if using the object cache without an expiration time is a better approach. It will at least fall out of persistent caches after a period of being unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Valid point, I added back the expiration. But the none persistent object cache, is what I am trying to fix here. We replace 1 call to the option table with 3 calls to database, one for post, post meta and term. As most site run without persistent object cache and theme does change, this is a massive performance win.


return $user_cpt;
}
Expand Down
53 changes: 50 additions & 3 deletions tests/phpunit/tests/theme/wpThemeJsonResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,9 @@ 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() {
add_filter( 'query', array( $this, 'filter_db_query' ) );
$query_count = count( $this->queries );
Expand All @@ -329,7 +332,7 @@ function test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries(
WP_Theme_JSON_Resolver::clean_cached_data();
}
$query_count = count( $this->queries ) - $query_count;
$this->assertEquals( 1, $query_count, 'Only one SQL query should be peformed for multiple invocations of WP_Theme_JSON_Resolver::get_global_styles_from_post()' );
$this->assertEquals( 0, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type' );

$user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme() );
$this->assertEmpty( $user_cpt );
Expand All @@ -338,12 +341,56 @@ function test_get_user_data_from_wp_global_styles_does_not_use_uncached_queries(
$this->assertNotEmpty( $user_cpt );

$query_count = count( $this->queries );
for ( $i = 0; $i < 3; $i++ ) {
WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme() );
for ( $i = 0; $i < 3; $i ++ ) {
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
$new_user_cpt = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme() );
WP_Theme_JSON_Resolver::clean_cached_data();
$this->assertSameSets( $user_cpt, $new_user_cpt );
}
$query_count = count( $this->queries ) - $query_count;
$this->assertEquals( 0, $query_count, 'Unexpected SQL queries detected for the wp_global_style post type' );
remove_filter( 'query', array( $this, 'filter_db_query' ) );
}

/**
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
* @ticket 55392
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
*/
function test_get_user_data_from_wp_global_styles_does_exist() {
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme(), true );
$this->assertIsArray( $post1 );
$this->assertArrayHasKey( 'ID', $post1 );
wp_delete_post( $post1['ID'], true );
$post2 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme(), true );
$this->assertIsArray( $post2 );
$this->assertArrayHasKey( 'ID', $post2 );
}

/**
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
* @ticket 55392
*/
function test_get_user_data_from_wp_global_styles_create_post() {
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ) );
$this->assertIsArray( $post1 );
$this->assertSameSets( array(), $post1 );
$post2 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ) );
$this->assertIsArray( $post2 );
$this->assertSameSets( array(), $post2 );
$post3 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ), true );
Copy link
Member

Choose a reason for hiding this comment

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

Can we set one variable for wp_get_theme( 'testing' ) and use it?

i.e.

Suggested change
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ) );
$this->assertIsArray( $post1 );
$this->assertSameSets( array(), $post1 );
$post2 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ) );
$this->assertIsArray( $post2 );
$this->assertSameSets( array(), $post2 );
$post3 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'testing' ), true );
$theme = wp_get_theme( 'testing' );
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertIsArray( $post1 );
$this->assertSameSets( array(), $post1 );
$post2 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme );
$this->assertIsArray( $post2 );
$this->assertSameSets( array(), $post2 );
$post3 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( $theme, true );

This also applies to the other test method.

$this->assertIsArray( $post3 );
$this->assertArrayHasKey( 'ID', $post3 );
}

/**
* @covers WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles
* @ticket 55392
*/
function test_get_user_data_from_wp_global_styles_filter_state() {
$post1 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'foo' ), true, array( 'publish' ) );
$this->assertIsArray( $post1 );
$this->assertArrayHasKey( 'ID', $post1 );
$post2 = WP_Theme_JSON_Resolver::get_user_data_from_wp_global_styles( wp_get_theme( 'foo' ), false, array( 'draft' ) );
$this->assertIsArray( $post2 );
$this->assertSameSets( array(), $post2 );
}
}