Improved caching. #3624
Improved caching. #3624
Conversation
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@spacedmonkey I like where this is going, but currently there are 2 concerns where the updated logic would break current behavior.
spacedmonkey
requested review from
adamsilverstein,
felixarntz and
peterwilsoncc
|
The test failures look legit https://github.com/WordPress/wordpress-develop/actions/runs/3470057716/jobs/5797787084#step:17:233 |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@spacedmonkey Production code looks good (just 2 nit-picks below), though as @peterwilsoncc there are some test fixes we need to make.
We should also add at least one new test to cover the new caching layer here to make sure it caches correctly.
|
@peterwilsoncc A simple workaround is to call, clean theme json caches, on add_theme_support and remove_theme_supports. |
|
@felixarntz @peterwilsoncc Spent some time on this PR. Caching block settings is harder than you might think. Block settings contain lots of calls to I don't love this, but I don't see anyway around this. |
spacedmonkey
requested review from
felixarntz and
mukeshpanchal27
and removed request for
mukeshpanchal27
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@spacedmonkey I'm not sure the new code here is the way we should go. I agree this is a problem, but I think we need to come up with a better solution, since IMO now it introduces a new problem, by invalidating cache on every add_theme_support call.
Can we move the logic for where the theme support is used for to happen after cache? Or, at a minimum, at least only invalidate the cache that uses theme support instead of all caches in that class?
| add_action( 'wp_print_styles', 'print_emoji_styles' ); | ||
| add_action( 'plugins_loaded', '_wp_theme_json_webfonts_handler' ); | ||
|
|
||
| foreach ( array( 'switch_theme', 'start_previewing_theme', 'add_theme_support', 'remove_theme_support' ) as $action ) { |
There was a problem hiding this comment.
I think the add_theme_support and remove_theme_support integration should be removed. I understand the concern, and I agree this is a problem we need to address.
However, invalidating the cache on add_theme_support means that it will happen on every request, since that function is typically called in every request.
Addressing this problem is not easy, which is why we need to IMO take some more time to think about the best way to address it. The root problem is that data that is cached preferably shouldn't depend on add_theme_support or any filters FWIW.
|
@felixarntz I agree, I think this PR is a little bit of a none starter for now. I think we may have to refactor this logic so it is easier to invalidate. |
|
Closing as this PR is a none starter. |
4 participants
Trac ticket: https://core.trac.wordpress.org/ticket/57077
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.