Skip to content

Commit

Permalink
WP_Object_Cache: fix magic methods
Browse files Browse the repository at this point in the history 
<rant>
Magic methods are not supposed to _use_ dynamic properties... they are supposed to _handle_ access _to_ inaccessible (`protected` or `private`) or non-existing properties *sigh*.

What's the point of having the magic methods in place otherwise ?
</rant>

The `WP_Object_Cache` class introduced the magic `__[isset|get|set|unset]()` methods in WP 4.0.0, but those methods were incorrectly implemented.

1. They contain no "allow list" of which `private`/`protected` properties are supposed to still be accessible, which in effect means any newly added `private`/`protected` properties would all still be treated as `public`.
2. They dynamically set and gave access to new (`public`) properties when an undeclared property was encountered.

This basically leaves the class wide open and negates the protection the magic methods (and visibility modifiers) were _intended_ to provide.

Unfortunately, this code has existed in WordPress for too long. Changing this now would constitute a massive BC-break.

However, the magic method implementation as is, is hugely problematic in light of the PHP 8.2 dynamic properties deprecation and the eventual intended removal of dynamic properties.

So this needs fixing _without_ breaking BC, but _with_ protection for potential future `protected`/`private` properties being added.

The fix I'm proposing does exactly that, by:
1. Storing undeclared properties being set on the class in a `private` `$arbitrary_props` array.
2. Adding an "allow list" with the names of those `protected`/`private` properties which should remain accessible via the magic methods.
    The use of this "allow list" makes sure that any new `protected`/`private` properties added at a later date will no longer be accessible via the magic methods.
    This includes the two properties I'm adding in this commit: those will be properly protected against interference from outside now.
3. Fixing the methods themselves to no longer use dynamic property access, but use the `$arbitrary_props` array for undeclared properties.
4. Fixing the methods themselves to respect the allow list.
5. For the new _inaccessible_ properties, an `OutOfBoundsException` will be thrown if any attempt to retrieve their value (`__get()`) or overwrite them (`__set()`) is made.
    This is not a BC-break as this doesn't affect any pre-existing properties. The exception will only be thrown for new properties, i.e. properties which are introduced with this patch or after.
    Calls to `__isset()` will yield `false` and `__unset()` will silently ignore the property, which is in line with the PHP native and expected behaviour.
6. When the value of a property which does not exist and hasn't been (dynamically) declared is requested, an `Undefined property` warning will be thrown.
    PHP would previously natively throw this warning (warning since PHP 8.0, notice in PHP < 8.0). This is now emulated to ensure developer mistakes are not hidden away and developers actually are provided notice of these.
    The only difference is that for PHP < 8.0, the message has been elevated from a "notice" to a "warning".
    The only time that this message will be emitted is in case of developer error, so this message elevation is IMO not a BC-break.
    As for the choice between "notice" and "warning", I've chosen to stay in line with the latest PHP versions, especially as it has already been decided that this warning will be elevated in PHP 9.0 to a fatal error, so preventing those developer errors (or getting them fixed) seems prudent.

Take note of the use of `property_exists()` and `array_key_exists()` in the magic methods instead of using `isset()`. A property may be set to `null` on purpose and the magic methods should handle that situation correctly.
This also means that for _declared_ properties without a value, no error message level elevation is done. The `__get()` method only does a `property_exists()` check on those, no `isset()`, which means the PHP native error handling will kick in and throw a notice in PHP < 8.0 and a warning in PHP 8.0.

Includes fixing the magic `__set()` method to not return as it is supposed to be a `void` method.
While this could be considered a BC-break, the return value would already be ignored due to how PHP handles magic methods, so in reality, this change will not make any difference.

> The return value of __set() is ignored because of the way PHP processes the assignment operator.

Includes updating the tests to expect a warning for the "Undefined property" notice for undeclared properties, independently of the PHP version.

Refs:
* https://www.php.net/manual/en/language.oop5.overloading.php#object.set
* https://www.php.net/manual/en/class.outofboundsexception.php
* https://wiki.php.net/rfc/undefined_property_error_promotion
* php/php-src#7786

These magic methods were originally introduced via changesets [28502](https://core.trac.wordpress.org/changeset/28502), [28521](https://core.trac.wordpress.org/changeset/28521), [28524](https://core.trac.wordpress.org/changeset/28524).
  • Loading branch information
@jrfnl
jrfnl committed Aug 13, 2022
1 parent 3ab7d43 commit 659b89b
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 17 deletions.
120 changes: 111 additions & 9 deletions src/wp-includes/class-wp-object-cache.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,29 @@ class WP_Object_Cache {
*/
private $multisite;

/**
* Storage for arbitrary dynamically set properties.
*
* @since 6.1.0
*
* @var array<string, mixed>
*/
private $arbitrary_props = array();

/**
* Declared private/protected properties which should remain accessible via the magic methods for BC reasons.
*
* @since 6.1.0
*
* @var array<string, true> The value is not relevant.
*/
private $compat_accessible_props = array(
'cache' => true,
'global_groups' => true,
'blog_prefix' => true,
'multisite' => true,
);

/**
* Sets up object properties; PHP 5 style constructor.
*
Expand All @@ -82,51 +105,130 @@ public function __construct() {
}

/**
* Makes private properties readable for backward compatibility.
* Makes private/protected properties readable for backward compatibility.
*
* {@internal As the original implementation of this set of methods contained no safeguard
* against undeclared properties being retrievied/set/etc, explicit support for this
* has been added in a PHP >= 8.2 compatible manner.
* This is NOT a good example of how magic methods _should_ be implemented!}
*
* @since 4.0.0
*
* @param string $name Property to get.
* @return mixed Property.
*
* @throws OutOfBoundsException When an attempt is made to retrieve the value of a truly inaccessible property.
*/
public function __get( $name ) {
return $this->$name;
// Handle private/protected properties declared in WP 6.1 or later. These should really not be accessible.
if ( property_exists( $this, $name ) && isset( $this->compat_accessible_props[ $name ] ) === false ) {
throw new OutOfBoundsException( 'Inaccessible property: ' . self::class . '::$' . $name );
}

// Handle declared private/protected properties.
if ( property_exists( $this, $name ) ) {
return $this->$name;
}

// Handle undeclared properties.
if ( array_key_exists( $name, $this->arbitrary_props ) ) {
return $this->arbitrary_props[ $name ];
}

// Maintain PHP native behaviour for undeclared properties.
trigger_error( 'Undefined property: ' . self::class . '::$' . $name, E_USER_WARNING );
return null;
}

/**
* Makes private properties settable for backward compatibility.
* Makes private/protected properties settable for backward compatibility.
*
* {@internal As the original implementation of this set of methods contained no safeguard
* against undeclared properties being retrievied/set/etc, explicit support for this
* has been added in a PHP >= 8.2 compatible manner.
* This is NOT a good example of how magic methods _should_ be implemented!}
*
* @since 4.0.0
* @since 6.1.0 This method does not return anything anymore.
*
* @param string $name Property to set.
* @param mixed $value Property value.
* @return mixed Newly-set property.
*
* @throws OutOfBoundsException When an attempt is made to set a truly inaccessible property.
*/
public function __set( $name, $value ) {
return $this->$name = $value;
// Handle declared private/protected properties.
if ( property_exists( $this, $name ) ) {
// Handle private/protected properties declared in WP 6.1 or later. These should really not be accessible.
if ( isset( $this->compat_accessible_props[ $name ] ) === false ) {
throw new OutOfBoundsException( 'Inaccessible property ' . self::class . '::$' . $name . ' cannot be set' );
}

// Old property accessible for BC-compat reasons.
$this->$name = $value;
return;
}

// Handle undeclared properties.
$this->arbitrary_props[ $name ] = $value;
}

/**
* Makes private properties checkable for backward compatibility.
* Makes private/protected properties checkable for backward compatibility.
*
* {@internal As the original implementation of this set of methods contained no safeguard
* against undeclared properties being retrievied/set/etc, explicit support for this
* has been added in a PHP >= 8.2 compatible manner.
* This is NOT a good example of how magic methods _should_ be implemented!}
*
* @since 4.0.0
*
* @param string $name Property to check if set.
* @return bool Whether the property is set.
*/
public function __isset( $name ) {
return isset( $this->$name );
// Handle declared private/protected properties.
if ( property_exists( $this, $name ) ) {
// Handle private/protected properties declared in WP 6.1 or later. These should really not be accessible.
if ( isset( $this->compat_accessible_props[ $name ] ) === false ) {
return false;
}

// Old property accessible for BC-compat reasons.
return isset( $this->$name );
}

// Handle undeclared properties.
return isset( $this->arbitrary_props[ $name ] );
}

/**
* Makes private properties un-settable for backward compatibility.
* Makes private/protected properties un-settable for backward compatibility.
*
* {@internal As the original implementation of this set of methods contained no safeguard
* against undeclared properties being retrievied/set/etc, explicit support for this
* has been added in a PHP >= 8.2 compatible manner.
* This is NOT a good example of how magic methods _should_ be implemented!}
*
* @since 4.0.0
*
* @param string $name Property to unset.
*/
public function __unset( $name ) {
unset( $this->$name );
// Handle declared private/protected properties.
if ( property_exists( $this, $name ) ) {
// Silently ignore unsets for inaccessible properties.
if ( isset( $this->compat_accessible_props[ $name ] ) === false ) {
return;
}

// Old property accessible for BC-compat reasons.
unset( $this->$name );
return;
}

// Handle undeclared properties.
unset( $this->arbitrary_props[ $name ] );
}

/**
Expand Down
10 changes: 2 additions & 8 deletions tests/phpunit/tests/Tests_wpObjectCache.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,8 @@ public function test_magic_unset_undeclared_properties() {
$this->assertFalse( isset( $obj->$name ), 'Unsetting the property failed' );

// Make sure that useful PHP native error messages aren't being hidden away by the magic methods.
$expected_msg = 'Undefined property: WP_Object_Cache::$';
if ( PHP_VERSION_ID > 80000 ) {
$this->expectWarning();
$this->expectWarningMessage( $expected_msg );
} else {
$this->expectNotice();
$this->expectNoticeMessage( $expected_msg );
}
$this->expectWarning();
$this->expectWarningMessage( 'Undefined property: WP_Object_Cache::$' );

$unused = $obj->$name;
}
Expand Down

0 comments on commit 659b89b

Please sign in to comment.