WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 5 months ago

#29247 new defect (bug)

Crucial caches are not cleared when deleting site

Reported by: rmccue Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Cache API Keywords: needs-patch 2nd-opinion
Focuses: multisite Cc:
PR Number:

Description

clean_blog_cache clears a bunch of caches, but does not clear any of the options caches. The worst of these is the alloptions cache, which allows some behaviour to continue working on deleted sites.

For example, switch_to_blog will continue to work without error, whereas if the cache is cleared, a Table 'wordpress.{$prefix}_options' doesn't exist error will be generated (helping in debugging).

Another one that's problematic is the is_blog_installed cache.

On that note, is there a reason we aren't checking site existence in switch_to_blog?

(Still investigating this one.)

Change History (9)

#1 follow-up: @stephdau
5 years ago

I'm also curious about why we don't check the blog's existence before switching to it.
Performance (must go to DB directly, no functions or could hit mistakenly uncleared cache too)?!?

I also wanted to not that if we move to checking for the blog's existence before switching to it, we should also likely do the same in restore_current_blog(), for consistency and safety (although I'd hate to see what missing-previous-blog case would do to running code IRL :p).

Good catch, btw.

Version 0, edited 5 years ago by stephdau (next)

#2 in reply to: ↑ 1 @rmccue
5 years ago

Replying to stephdau:

I also wanted to note that if we move to checking for the blog's existence before switching to it, we should also likely do the same in restore_current_blog(), for consistency and safety (although I'd hate to see what missing-previous-blog case would do to running code IRL :p).

Theoretically, you should be able to switch_to_blog then delete the old blog without breaking any code. Nothing should be popping from the stack (restoring) without having pushed (switching) beforehand; that is, you should only be touching the part of the stack you're responsible for.

#3 @stephdau
5 years ago

Indeed. If switch_to_blog() does what its name imply, there should be no hard ties to the "previous" blog.
I meant what it'd do in the codebase as it stands now. If we don't check if a blog exists before switching to it, I can't imagine what else we're not doing in that sphere (yet). :)

#4 @wonderboymusic
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.1

#5 @wonderboymusic
5 years ago

  • Milestone changed from 4.1 to Future Release

As soon as someone steps forward to write a patch, can put back in 4.1

This ticket was mentioned in Slack in #core by jipmoors. View the logs.


5 years ago

#7 @jipmoors
5 years ago

As far as I can see, there are no checks to see if the blog exists. Not in ms-blogs nor wp-db. I guess that will solve the problem in it's general sense. The code suggests that alloptions are not cached on multisite, so that should not be in there at all.

Not sure if this belongs in the Cache API Component anymore.

Would be good to verify that all cache has been removed after deleting, but that does not seem the main problem.

#8 @johnjamesjacoby
4 years ago

  • Keywords 2nd-opinion added

The is_blog_installed() function is a very random shot in the dark at deciding if a site needs to be repaired, and I wouldn't trust it to be the reason you couldn't switch between sites.

What if someone wanted to switch_to_blog() to run the repair tool on that site? I don't see very much wrong with someone switching to a site that's missing some data; I see more wrong with a core API blocking behavior with only a vague idea of why.


Addressing the OP, how deep should cache invalidation go? Posts & taxonomies? Users that were previously members of that site? Maybe these are passively cleared when their data objects & meta are deleted, but they aren't explicitly so.

The way I see it, this function only exists as a wrapper for the various bespoke site & network cache groups that have been half-haphazardly introduced through the course of multisite's half-life. This function reads more as a yard of graves that require exhuming than it does a reliable ledger of what's buried beneath it.

Maybe there is some overlap with the WP_Site object (being proposed for 4.5) that can help guide us into cleaning up the cache groups mayhem.

Last edited 4 years ago by johnjamesjacoby (previous) (diff)

#9 @jeremyfelt
4 years ago

My best read on the intent of clean_blog_cache() is its use in refresh_blog_details() as a way to clear the network level cache for a site. Not as a way to clear site level data.

As John mentions, things get deep quick with options, posts, terms, etc... I don't think we need to worry about that in clean_blog_cache(), though it could be something to consider more as part of the site deletion process itself.

For the aside: It's always seemed to me that verifying the existence of a site during switch_to_blog() would add too much overhead. We blindly step into that site's place and run things in a completely different context (plugins, theme) than that site is normally part of. So many variables make that switch unreliable unless you're verifying it the way you need it to be verified first. Of course, there could be a creative way to solve that.

Note: See TracTickets for help on using tickets.