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

Jump to bottom

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

wants to merge 15 commits into from

Conversation

spacedmonkey
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/55392

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@spacedmonkey spacedmonkey self-assigned this Mar 15, 2022
peterwilsoncc
peterwilsoncc reviewed Jun 14, 2022
View reviewed changes
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Query change looks good but I have a question inlinie.

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved
@spacedmonkey spacedmonkey requested a review from draganescu June 17, 2022 12:37
@spacedmonkey
Copy link
Member Author

I have added some improved unit tests and tweaked this function so it now caches even more.

CC @oandregal @Mamaduka for a review, as they have worked on this function in the past.

spacedmonkey
spacedmonkey commented Jun 17, 2022
View reviewed changes
src/wp-includes/class-wp-theme-json-resolver.php Outdated
),
),
true
);
$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
Member Author

Choose a reason for hiding this comment

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

CC @xknown for context here.

draganescu
draganescu reviewed Jun 28, 2022
View reviewed changes
Copy link

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I think this is great, but I wonder if this wouldn't be better as a Gutenberg PR instead of a core patch. i fear that if a gutenberg specific resolver class is added in compat merging will be hell.

src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved
@spacedmonkey
Copy link
Member Author

I think this is great, but I wonder if this wouldn't be better as a Gutenberg PR instead of a core patch. i fear that if a gutenberg specific resolver class is added in compat merging will be hell.

@draganescu Now that this code lives in core, should PR not be done here? Would this not mean that have to port it over, adding more steps. This process, is not documented and is confusing me to.

@draganescu
Copy link

This process, is not documented and is confusing me to.

This is true! But the code is still being heavily changed so I think to make Gutenberg plugin merges simpler we should not patch core with anything that is related to Gutenberg and can be created as a Gutenberg patch /PR.

peterwilsoncc
peterwilsoncc reviewed Jun 30, 2022
View reviewed changes
src/wp-includes/class-wp-theme-json-resolver.php Show resolved Hide resolved
src/wp-includes/class-wp-theme-json-resolver.php Outdated
}
$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.

@spacedmonkey spacedmonkey mentioned this pull request Jul 5, 2022
Closed
mukeshpanchal27
mukeshpanchal27 reviewed Sep 15, 2022
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

@spacedmonkey Look solid to me. Left some review for document standard and unit test change

src/wp-includes/class-wp-theme-json-resolver.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJsonResolver.php Outdated Show resolved Hide resolved
tests/phpunit/tests/theme/wpThemeJsonResolver.php Outdated
Comment on lines 401 to 407
$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.

tests/phpunit/tests/theme/wpThemeJsonResolver.php Show resolved Hide resolved
oandregal
oandregal approved these changes Sep 15, 2022
View reviewed changes
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

The changes to the get_user_data_from_wp_global_styles logic are good, thank you. Haven't tested this manually.

When this lands, https://github.com/WordPress/gutenberg/blob/trunk/lib/compat/wordpress-6.0/class-wp-theme-json-resolver-6-0.php#L165 needs to be updated with the same changes. @spacedmonkey if you can't do it, please, let me know and I will.

@spacedmonkey
Copy link
Member Author

Committed into core.

@draganescu draganescu mentioned this pull request Sep 20, 2022
Closed
@andrewserong andrewserong mentioned this pull request Oct 18, 2022
Closed
@oandregal oandregal mentioned this pull request Oct 25, 2022
Closed
@MarketingIQ MarketingIQ mentioned this pull request Dec 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 left review comments

@peterwilsoncc peterwilsoncc peterwilsoncc left review comments

@draganescu draganescu draganescu left review comments

@oandregal oandregal oandregal approved these changes

@adamziel adamziel Awaiting requested review from adamziel

@noisysocks noisysocks Awaiting requested review from noisysocks

@hellofromtonya hellofromtonya Awaiting requested review from hellofromtonya

@adamsilverstein adamsilverstein Awaiting requested review from adamsilverstein

@felixarntz felixarntz Awaiting requested review from felixarntz

Assignees

@spacedmonkey spacedmonkey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spacedmonkey @draganescu @peterwilsoncc @oandregal @mukeshpanchal27