WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#20043 closed enhancement (fixed)

WP_User missing magic __unset() method

Reported by: johnjamesjacoby Owned by: chriscct7
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch
Focuses: Cc:

Description (last modified by johnjamesjacoby)

When the magic methods were put into WP_User in 3.3, __unset() was skipped. This has the result of unset( $user_data->$foo ); not actually doing anything.

Patched against r19926.

Attachments (4)

20043.patch (689 bytes) - added by johnjamesjacoby 8 years ago.
20043-unittests.diff (1.3 KB) - added by MikeHansenMe 6 years ago.
see #30284
20043.diff (2.0 KB) - added by wonderboymusic 5 years ago.
20043.2.patch (517 bytes) - added by tyxla 5 years ago.
WP_User::unset(): blacklisting ID so it can't be unset; setting it to 0 instead.

Download all attachments as: .zip

Change History (21)

#1 @johnjamesjacoby
8 years ago

  • Description modified (diff)

#2 @DJPaul
8 years ago

  • Cc djpaul@… added

#3 @scribu
8 years ago

  • Milestone changed from Awaiting Review to 3.4

Well, since we have __set(), I guess it's fair to have __unset() too.

#4 @ryan
8 years ago

What, no unit tests? ;-)

#6 @kurtpayne
8 years ago

  • Cc kpayne@… added

Ask and ye shall receive [UT618]

#7 @ryan
8 years ago

  • Milestone changed from 3.4 to Future Release

#8 @johnjamesjacoby
8 years ago

  • Milestone changed from Future Release to 3.6

Patch and unit test. Moving into 3.6 for consideration.

#9 @nacin
8 years ago

I'm fine with this, but what's the use case? Other than a change in behavior from a year ago. I'm curious because I'm not sure we should easily allow the unsetting of core properties if we can avoid it. (Doesn't matter, just playing devil's advocate.)

#10 @ryan
7 years ago

  • Milestone changed from 3.6 to Future Release

#11 @chriscct7
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.4
  • Owner set to chriscct7
  • Status changed from new to assigned
  • Type changed from defect (bug) to enhancement

@wonderboymusic
5 years ago

#12 @wonderboymusic
5 years ago

  • Keywords needs-refresh removed

refreshed/combined - not entirely sure how necessary this is

#13 @wonderboymusic
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34380:

Users: add __unset to WP_User.

Adds unit tests.

Props johnjamesjacoby, MikeHansenMe, wonderboymusic.
Fixes #20043.

#14 @DrewAPicture
5 years ago

  • Keywords needs-docs added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Needs an access-modifier
  • Needs complete docs

#15 @DrewAPicture
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 34390:

Docs: Add a missing access modifier to the declaration for the WP_User::__unset() magic method, introduced in [34380].

Also adds missing documentation to the DocBlock.

Fixes #20043.

#16 @boonebgorges
5 years ago

  • Keywords needs-docs removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[34380] is causing errors in the test_user_unset_uppercase_id() test introduced in the same changeset. What's happening is this:

  • __unset() unsets $user->ID
  • The test then attempts to access $user->ID. Since the property no longer exists (! isset( $user->ID )), the magic __get() kicks in.
  • __get() tries to get_user_meta( $this->ID, 'ID', true ). But $this->ID is an undefined property.

Unsetting an object property like ID is going to cause similar problems through core and elsewhere. Either we should have a blacklist of properties that cannot be unset (so unset( $user->ID ) would return false), or unset( $user->ID ) should just empty the property ($user->ID = 0).

@tyxla
5 years ago

WP_User::unset(): blacklisting ID so it can't be unset; setting it to 0 instead.

#17 @wonderboymusic
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 34466:

Users: in WP_User::__unset(), don't reset the deprecated prop id to ID. Still throw the deprecated notice.

Update unit test.

Fixes #20043.

Note: See TracTickets for help on using tickets.