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.

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

Cache is cleared multiple times when switching theme #6276

Closed
wordpressfan opened this issue Nov 20, 2023 · 8 comments · Fixed by #6402
Closed

Cache is cleared multiple times when switching theme #6276

wordpressfan opened this issue Nov 20, 2023 · 8 comments · Fixed by #6402
Assignees
Labels
module: cache priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@wordpressfan
Copy link
Contributor

wordpressfan commented Nov 20, 2023

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version Yes
  • Used the search feature to ensure that the bug hasn’t been reported before Yes

Describe the bug
The discussion was started here: #6263 (comment)

Now when switching the theme we are clearing the cache three times but we need to clear it once, this is because we add the callback rocket_clean_domain with different hooks as mentioned in the link above, will state them here too:

First location:

add_action( 'update_option_theme_mods_' . get_option( 'stylesheet' ), 'rocket_clean_domain' ); // When location of a menu is updated.

Second location:

add_action( 'update_option_sidebars_widgets', 'rocket_clean_domain' ); // When you change the order of widgets.

Third and main location:

add_action( 'switch_theme', 'rocket_clean_domain' ); // When user change theme.

So we opened this issue to investigate the first two locations ad why they are firing with switch theme and in all cases we need the cache clear happens only once.

To Reproduce
Steps to reproduce the behavior:

  1. Cache some pages
  2. Add a log inside here:
    $urls = ( ! $lang || is_object( $lang ) || is_array( $lang ) || is_int( $lang ) )
  3. Switch theme from backend
  4. Check the log file, you will find the log message repeated three times.

Expected behavior
Cache clear should happen only once with switching theme.

Acceptance Criteria (for WP Media team use only)

  1. Cache should be cleared only once when switching the theme
  2. It should work without regression when activating theme via WP CLI
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: cache priority: medium Issues which are important, but no one will go out of business. labels Dec 3, 2023
@mostafa-hisham
Copy link
Contributor

@wp-media/php-team
I am not sure what should we do here
based on the WP code here
https://github.com/WordPress/wordpress-develop/blob/1029dde1bc8b8c94dd502c294de51c41c79f741a/src/wp-includes/theme.php#L784
the actions

add_action( 'update_option_sidebars_widgets', 'rocket_clean_domain' );  // When you change the order of widgets.
add_action( 'update_option_category_base', 'rocket_clean_domain' );  // When category permalink is updated.

will fire at the beginning of the function before the switch_theme is fired
and there is no before_switch_theme action and I couldn't find any workaround for it.

Could you please have a look at it?
(moving it back to grooming)

@jeawhanlee
Copy link
Contributor

rocket_clean_domain is fired 2x on my end for the following hooks:
update_option_theme_mods_{$name}
switch_theme
and the first is executed in switch_theme function here:
https://github.com/WordPress/wordpress-develop/blob/1029dde1bc8b8c94dd502c294de51c41c79f741a/src/wp-includes/theme.php#L803 or here: https://github.com/WordPress/wordpress-develop/blob/1029dde1bc8b8c94dd502c294de51c41c79f741a/src/wp-includes/theme.php#L864C1-L864C1
which will fire at the beginning before switch_theme is fired as @mostafa-hisham stated above.
I tried the possibility of using transients with timestamps after the first time rocket_clean_domain is fired but we might be clearing too early and bail out before switch_theme is fired.

Moving back to needs grooming as well...

@vmanthos
Copy link
Contributor

vmanthos commented Dec 4, 2023

How about checking if 1 !== did_action( 'after_rocket_clean_domain' ) and only then run the other two actions?

@remyperona
Copy link
Contributor

The did_action() check could be something we add in each function, so that we avoid cleaning multiple times.

Each callback makes sense individually, but there is some context where they will all fire one after the other.

@Miraeld
Copy link
Contributor

Miraeld commented Jan 8, 2024

I've tried to take a look at it about the did_action() check, but I couldn't make it work on my side, I still had multiple.
I couldn't find another fix that would work.

It's possible that I did something wrong with the did_action()...

@Miraeld
Copy link
Contributor

Miraeld commented Jan 22, 2024

If we add a bail-out within this function

function rocket_clean_domain( $lang = '', $filesystem = null ) {

such as

	if (1 === did_action( 'after_rocket_clean_domain' )) {
		return;
	}

I can get down to 2 calls instead of 3. Not there yet, and I can't figure out where the 2 are from ..

@hanna-meda
Copy link

@piotrbak , tested this and it is as @Miraeld described:

I can get down to 2 calls instead of 3. Not there yet, and I can't figure out where the 2 are from ..

Can you confirm if this is acceptable for now, with 2 entries?

@piotrbak
Copy link
Contributor

piotrbak commented Feb 9, 2024

Yes, that's already an improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: cache priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants