Skip to content

Commit

Permalink
wpdb: 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 `wpdb` class introduced the magic `__[isset|get|set|unset]()` methods in WP 3.5.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`.
    For this class, this means that the `protected` `$reconnect_retries` and `$incompatible_modes` (WP 3.9.0), the `private` `$use_mysqli` and `$has_connected` (WP 3.9.0) and the `private` `$checking_collation` (WP 4.2.0) were all world-accessible and changable, even though they were introduced **after** WP 3.5.0 and have `private`/`protected` visibility modifiers....
    This also applies to the `private` `$allow_unsafe_unquoted_parameters` property as introduced in WP 6.1.0, though as that one has not been in a tagged release yet, we can still change it without creating a BC-break.
    For the `protected` `$col_meta`, `$table_charset` and `$check_current_query` properties which were introduced in WP 4.2.0 partial protection was put in place, protecting these against being _set_, but not against being _unset_ or read.
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.
    Also note that the `$allow_unsafe_unquoted_parameters` property which is also being introduced in WP 6.1.0 has _not_ been added to this list.
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, declared, `private`/`protected` 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 updating the tests to expect a warning for the "Undefined property" notice for undeclared properties, independently of the PHP version and to expect an "Undefined property" warning for unset `private` properties (this is due to the tests using a mock of `wpdb` and not `wpdb` itself).

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 [21472](https://core.trac.wordpress.org/changeset/21472), [21521](https://core.trac.wordpress.org/changeset/21521) and limited protection for select properties was added in [30345](https://core.trac.wordpress.org/changeset/30345).
  • Loading branch information
@jrfnl
jrfnl committed Sep 2, 2022
1 parent 17d48b8 commit 8b0b26c
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 32 deletions.
131 changes: 116 additions & 15 deletions src/wp-includes/class-wpdb.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,15 @@ class wpdb {
*/
public $error = null;

/**
* 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.
*
Expand All @@ -727,8 +736,20 @@ class wpdb {
*/
private $compat_accessible_props = array(
'check_current_query' => false,
'checking_collation' => true,
'col_info' => true,
'col_meta' => false,
'dbh' => true,
'dbhost' => true,
'dbname' => true,
'dbpassword' => true,
'dbuser' => true,
'has_connected' => true,
'incompatible_modes' => true,
'reconnect_retries' => true,
'result' => true,
'table_charset' => false,
'use_mysqli' => true,
);

/**
Expand Down Expand Up @@ -774,60 +795,140 @@ public function __construct( $dbuser, $dbpassword, $dbname, $dbhost ) {
}

/**
* 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 3.5.0
*
* @param string $name The private member to get, and optionally process.
* @return mixed The private member.
* @param string $name The private/protected member to get, and optionally process.
* @return mixed The private/protected member.
*
* @throws OutOfBoundsException When an attempt is made to retrieve the value of a truly inaccessible property.
*/
public function __get( $name ) {
if ( 'col_info' === $name ) {
$this->load_col_info();
}

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 ] ) ) {
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 3.5.0
*
* @param string $name The private member to set.
* @param string $name The private/protected member to set.
* @param mixed $value The value to set.
*
* @throws OutOfBoundsException When an attempt is made to retrieve the value of a truly inaccessible property.
*/
public function __set( $name, $value ) {
if ( isset( $this->compat_accessible_props[ $name ] ) && false === $this->compat_accessible_props[ $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 ] ) ) {
throw new OutOfBoundsException( 'Inaccessible property ' . self::class . '::$' . $name . ' cannot be set' );
}

// Silently ignore set requests for select private/protected properties.
if ( isset( $this->compat_accessible_props[ $name ] ) && false === $this->compat_accessible_props[ $name ] ) {
return;
}

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

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

/**
* Makes private properties check-able for backward compatibility.
* Makes private/protected properties check-able 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 3.5.0
*
* @param string $name The private member to check.
* @param string $name The private/protected member to check.
* @return bool If the member is set or not.
*/
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 ] ) ) {
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 3.5.0
*
* @param string $name The private member to unset
* @param string $name The private/protected member to unset
*/
public function __unset( $name ) {
if ( isset( $this->compat_accessible_props[ $name ] ) && false === $this->compat_accessible_props[ $name ] ) {
// Handle declared private/protected properties.
if ( property_exists( $this, $name ) ) {
// Silently ignore unsets for inaccessible properties and select accessible properties.
if ( ! isset( $this->compat_accessible_props[ $name ] )
|| false === $this->compat_accessible_props[ $name ]
) {
return;
}

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

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

/**
Expand Down
33 changes: 16 additions & 17 deletions tests/phpunit/tests/db/magicmethods.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,11 @@ public function test_magic_methods_declared_properties( $name, $default = null )
*
* @dataProvider data_magic_methods_declared_settable_properties
*
* @param string $name Property name.
* @param string $name Property name.
* @param mixed $default Unused. Default value for the property.
* @param string $visibility The visibility of the property.
*/
public function test_magic_unset_declared_properties( $name ) {
public function test_magic_unset_declared_properties( $name, $default = null, $visibility = 'protected' ) {
$obj = $this->get_clean_wpdb();

// Make sure the properties all have an initial value.
Expand All @@ -186,7 +188,7 @@ public function test_magic_unset_declared_properties( $name ) {

// Make sure that useful PHP native error messages aren't being hidden away by the magic methods.
$expected_msg = 'Undefined property: ';
if ( PHP_VERSION_ID > 80000 ) {
if ( PHP_VERSION_ID > 80000 || 'private' === $visibility ) {
$this->expectWarning();
$this->expectWarningMessage( $expected_msg );
} else {
Expand Down Expand Up @@ -230,16 +232,19 @@ public function data_magic_methods_declared_settable_properties() {
'name' => 'result',
),
'Declared private property: $checking_collation' => array(
'name' => 'checking_collation',
'default' => false,
'name' => 'checking_collation',
'default' => false,
'visibility' => 'private',
),
'Declared private property: $has_connected' => array(
'name' => 'has_connected',
'default' => false,
'name' => 'has_connected',
'default' => false,
'visibility' => 'private',
),
'Declared private property: $use_mysqli' => array(
'name' => 'use_mysqli',
'default' => false,
'name' => 'use_mysqli',
'default' => false,
'visibility' => 'private',
),
);
}
Expand Down Expand Up @@ -397,14 +402,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: ';
if ( PHP_VERSION_ID > 80000 ) {
$this->expectWarning();
$this->expectWarningMessage( $expected_msg );
} else {
$this->expectNotice();
$this->expectNoticeMessage( $expected_msg );
}
$this->expectWarning();
$this->expectWarningMessage( 'Undefined property: ' );

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

0 comments on commit 8b0b26c

Please sign in to comment.