WordPress.org

Make WordPress Core

#20950 closed defect (bug) (fixed)

wpmu_delete_blog leaves zombie cache objects

Reported by: mohanjith Owned by: wonderboymusic
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch
Focuses: Cc:

Description

For example cached objects with keys like current_blog_{$domain}, current_blog_{$domain}, get_id_from_blogname_{$name} will be left to haunt the network even after the blog is deleted.

Symptoms will be not being allowed to create blogs with the same name after it's deleted, blog settings of old blogs creeping into newly created blogs.

Attachments (5)

missing-cache-delete.diff (813 bytes) - added by wonderboymusic 19 months ago.
missing-cache-delete.2.diff (991 bytes) - added by wonderboymusic 19 months ago.
20950.diff (2.2 KB) - added by ryan 19 months ago.
Rename $name to $slug for some clarity.
20950-ut.diff (1.9 KB) - added by ryan 19 months ago.
Unit tests and blog factory fix
20950-ut.2.diff (2.7 KB) - added by ryan 19 months ago.
More cache tests

Download all attachments as: .zip

Change History (20)

comment:1 wonderboymusic19 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.5

I feel dirty after debugging this. Here's the flow:

1) call wpmu_delete_blog()

2) It calls update_blog_status( $blog_id, 'deleted', 1 );

3) ...which calls refresh_blog_details($blog_id);

4) Which does:

wp_cache_delete( $blog_id , 'blog-details' );
wp_cache_delete( $blog_id . 'short' , 'blog-details' );
wp_cache_delete( md5( $details->domain . $details->path )  , 'blog-lookup' );
wp_cache_delete( 'current_blog_' . $details->domain, 'site-options' );
wp_cache_delete( 'current_blog_' . $details->domain . $details->path, 'site-options' );

5) ...but was missing:

wp_cache_delete( 'get_id_from_blogname_' . $details->blogname, 'blog-details' );

One line patch.

comment:2 nacin19 months ago

Noticing the escaping as well. Are the values escaped on storage, hence the bug report suggesting that all three cache entries are not cleared?

comment:3 nacin19 months ago

  • Owner set to wonderboymusic
  • Status changed from new to assigned

comment:4 wonderboymusic19 months ago

Refreshed

wp_cache_delete( md5( $details->domain . $details->path )  , 'blog-lookup' );

Actually needs to be:

wp_cache_delete( md5( $wpdb->escape( $details->domain ) . $wpdb->escape( $details->path ) )  , 'blog-lookup' );

comment:5 wonderboymusic19 months ago

Only the md5 is not read directly from the object

comment:6 ryan19 months ago

I think get_blog_details( $blog_id, false ) needs to become get_blog_details( $blog_id, false ) to get the blogname.

I think get_id_from_blogname() actually expects the path, not the blogname. Go fig.

Version 0, edited 19 months ago by ryan (next)

comment:7 ryan19 months ago

get_id_from_blogname( )actually expects a path, but you can't pass get_blog_details()->path directly to it because of trailing slash problems.

ryan19 months ago

Rename $name to $slug for some clarity.

comment:8 ryan19 months ago

get_id_from_blogname() actually takes an unslashed path rather than a blogname so I updated the variables and phpdoc to use the term slug instead of name.

  • Rename $name to $slug for some clarity.
  • Strip leading and trailing slashes from the slug in get_id_from_blogname() so get_blog_details()->path could be directly passed
  • Delete the get_id_from_blogname cache in refresh_blog_details()
Last edited 19 months ago by ryan (previous) (diff)

ryan19 months ago

Unit tests and blog factory fix

comment:9 ryan19 months ago

Unit tests and a fix to WP_UnitTest_Factory_For_Blog that prepends global $base to the path when creating blogs with generated defaults. get_id_from_blogname() expects "/testpath1/" to be in the DB but the factory was creating "testpath1/".

Last edited 19 months ago by ryan (previous) (diff)

comment:10 ryan19 months ago

Still looking at the blog-lookup cache. In the unit tests sometimes the domain + path passed to get_blog_details() is "example.org" and sometimes "example.org/". I haven't yet looked at what mischief core does during blog creation.

comment:11 ryan19 months ago

The domain and path used for the details blog-lookup key are consistent so far when testing blog create, delete, edit, and load. They're not slashed for the db AFAICT. I haven't tested through signup yet.

comment:12 ryan19 months ago

The current_blog_* cache also appears consistent and not slashed for the DB when testing blog create, delete, edit, and load. This is while testing path based multisite. I haven't tested subdomain yet.

ryan19 months ago

More cache tests

comment:13 ryan19 months ago

In [21979]:

  • Invalidate the get_id_from_blogname_* cache in refresh_blog_details()
  • Change $name to $slug in get_id_from_blogname() for some semblance of clarity.
  • Strip leading and trailing slashes from the slug in get_id_from_blogname() so get_blog_details()->path can be passed directly.

Props wonderboymusic
see #20950

comment:15 ryan19 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.