WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#19500 closed defect (bug) (fixed)

User deletion doesn't clear user_meta cache

Reported by: duck_ Owned by: 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 2 years ago.
19500.2.diff (475 bytes) - added by johnjamesjacoby 2 years ago.
19500-unit-tests.diff (1.0 KB) - added by ryan 2 years ago.
19500.3.diff (1.7 KB) - added by ryan 2 years ago.
Return array() for all the cases.
19500.4.diff (2.1 KB) - added by duck_ 2 years ago.
19500.5.diff (2.6 KB) - added by ryan 2 years ago.
meta delete loop for wpmu_delete_user()

Download all attachments as: .zip

Change History (20)

comment:1 ryan2 years ago

  • Milestone changed from Future Release to 3.4

nacin2 years ago

comment:2 nacin2 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.

comment:3 ryan2 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 of 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.

Version 0, edited 2 years ago by ryan (next)

comment:4 garyc402 years ago

  • Keywords has-patch added

johnjamesjacoby2 years ago

comment:5 follow-up: johnjamesjacoby2 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.

comment:6 in reply to: ↑ 5 duck_2 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 2 years ago by duck_ (previous) (diff)

comment:7 ryan2 years ago

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

comment:8 ryan2 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.

comment:9 duck_2 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.

ryan2 years ago

comment:10 follow-up: ryan2 years ago

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

ryan2 years ago

Return array() for all the cases.

duck_2 years ago

comment:11 in reply to: ↑ 10 duck_2 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.

comment:12 duck_2 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().

ryan2 years ago

meta delete loop for wpmu_delete_user()

comment:13 ryan2 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.