Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#19500 closed defect (bug) (fixed)

User deletion doesn't clear user_meta cache

Reported by: duck_'s profile duck_ Owned by: ryan's profile ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.2.1
Component: Cache API Keywords: has-patch
Focuses: Cc:

Description

wp_delete_user() and wpmu_delete_user() leave user_meta in the cache. Example:

get_user_meta( $user_id );
wp_delete_user( $user_id );
var_dump( is_user_member_of_blog( $user_id ) ); // bool(true) instead of bool(false)

Possible solutions:

  1. Extend clean_user_cache() to delete from user_meta too
  2. delete_user_meta() in wp_delete_user() instead of direct query

Ryan wasn't keen on 1 as "Most of the time clean_user_cache() doesn't need to touch meta cache".

  1. will introduce the delete(d)_user_meta actions when using wp_delete_user().

wp_cache_delete( $id, 'user_meta' ) in wp_delete_user() was also floated as a stop-gap measure, but since this will be fixed in a future release we can ignore the suggestion.

See: ticket:19160:6, #11761

Attachments (6)

19500.diff (1.4 KB) - added by nacin 13 years ago.
19500.2.diff (475 bytes) - added by johnjamesjacoby 13 years ago.
19500-unit-tests.diff (1.0 KB) - added by ryan 13 years ago.
19500.3.diff (1.7 KB) - added by ryan 13 years ago.
Return array() for all the cases.
19500.4.diff (2.1 KB) - added by duck_ 13 years ago.
19500.5.diff (2.6 KB) - added by ryan 13 years ago.
meta delete loop for wpmu_delete_user()

Download all attachments as: .zip

Change History (20)

#1 @ryan
13 years ago

  • Milestone changed from Future Release to 3.4

@nacin
13 years ago

#2 @nacin
13 years ago

With the help of duck_, 19500.diff passes the skipped unit tests. It required a change to get_blogs_of_user(), as in single-site, you end up with false, while in multisite, you'll get array() (in the case of no blogs). I think that array() might be the more semantic response, however.

#3 @ryan
13 years ago

Looking through core and a few plugins, most places use empty() or is_array() or (array) when dealing with the return value so they are safe with either array or false. One place does this though:

array_key_exists($blog_id, get_blogs_of_user($user_id)) ) 

Perhaps returning an empty array for both single and multi would be safest, as well as more semantic.

Last edited 13 years ago by ryan (previous) (diff)

#4 @garyc40
13 years ago

  • Keywords has-patch added

#5 follow-up: @johnjamesjacoby
13 years ago

19500.2.diff moves clean_user_cache( $id ) ahead of the queries that delete the user in wpmu_delete_user(). Without this, calls to wpmu_delete_user() do not successfully clear the user cache because the user no longer exists. This causes...

$user = WP_User::get_data_by( 'id', $id );

...to fail in clear_user_cache(), and the $user variable caches are never cleared.

#6 in reply to: ↑ 5 @duck_
13 years ago

Replying to johnjamesjacoby:

19500.2.diff moves clean_user_cache( $id ) ahead of the queries that delete the user in wpmu_delete_user(). Without this, calls to wpmu_delete_user() do not successfully clear the user cache because the user no longer exists. This causes...

$user = WP_User::get_data_by( 'id', $id );

...to fail in clear_user_cache(), and the $user variable caches are never cleared.

#20460.

Also Ryan on #19690: "We usually clean [cache] after the delete" because of possibility of actions firing in between. Doesn't look like an issue in this case, but something to think about.

Last edited 13 years ago by duck_ (previous) (diff)

#7 @ryan
13 years ago

Perhaps we start passing objects instead of ids so we can clean the cache after delete.

#8 @ryan
13 years ago

In IRC we decided to start passing objects. Back-compat support for IDs should be retained. The situation where an ID is passed and the object cannot be fetched should be handled without warnings.

#9 @duck_
13 years ago

In [20522]:

Pass full user objects to clean_user_cache(). See #19500, fixes #20460.

Prevents notices when clean_user_cache() is called for a user that has been removed from the database.

#10 follow-up: @ryan
13 years ago

All TestWPUser pass for single site. test_is_user_member_of_blog() fails for multisite. It fails with and without the patch.

@ryan
13 years ago

Return array() for all the cases.

@duck_
13 years ago

#11 in reply to: ↑ 10 @duck_
13 years ago

Replying to ryan:

All TestWPUser pass for single site. test_is_user_member_of_blog() fails for multisite. It fails with and without the patch.

There was a typo in the patch. remove_user_from_blog() was using the undefined variable $user_id and not $id. Updated patch also removes check for is_array() in is_user_member_of_blog() and the falsey check in get_blogs_of_user() as $blogs is initialised as an empty array.

#12 @duck_
13 years ago

Patches still need to deal with wpmu_delete_user(). Should be able to copy the delete_metadata_by_mid() loop from wp_delete_user().

@ryan
13 years ago

meta delete loop for wpmu_delete_user()

#13 @ryan
13 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [20581]:

  • Return empty arrays instead of false for all conditions in get_blogs_of_user().
  • When deleting a user, use a delete_metadata_by_mid() loop over the meta so that the meta cache is cleared.
  • Use remove_user_from_blog() for DRYness.

Props nacin, duck_
Fixes #19500

Note: See TracTickets for help on using tickets.