PHP 8.2: unexpected deprecation for dynamic property set via magic method #7786
Labels
Comments
|
Honestly, I didn't even know the engine protects you from infinite recursion in this case. The current behavior seems reasonable to me. I'm assuming (but maybe @nikic can correct me on this one) the RFC refers to the fact that triggering |
|
When an inaccessible property is accessed recursively in a magic __get/__set accessor, it will bypass magic __get/__set and access the property directly instead. This may either be an access to a declared property or to a dynamic property. If that ends up creating a dynamic property, a deprecation warning will be thrown. |
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 13, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 13, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 13, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 13, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 21, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Aug 21, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 2, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 2, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 5, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 5, 2022
<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).
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this issue
Sep 7, 2022
This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods. With the deprecation of dynamic properties in PHP 8.2, this needs to be fixed. The new implementation represents a “proper” implementation of the magic methods for a class without non-`public` or typed properties. Notes: * Instead of relying on dynamic properties, the magic methods now store properties in a `private` `$arbitrary_props` array and retrieve them from there as well. * The original `$foo` property, even though declared as `private`, was never `private` in practice due to the way the magic methods were originally implemented. In effect, it was fully publicly retrievable and modifiable without any (type) restrictions. With that in mind, the `foo` property has been moved into the `$arbitrary_props` array to keep the implementation of the magic methods as clean and straightforward as possible. With the adjusted magic methods, access to and modification of `$foo` will (on the surface) continue to work in the same way as before, while under the hood, it is no longer affected by the dynamic properties deprecation. * Take note of the use of `array_key_exists()` instead of `isset()` in the `__get()` method. This is intentional and allows for `null` values to be stored and retrieved. * Also take note of `__set()` method no longer returning. `__set()` is supposed to be a `void` method. In practice, the return value would always be ignored due to how PHP handles magic methods, so in effect, this change will not make any difference and does not constitute a backward compatibility break.[[BR]][[BR]] > The return value of `__set()` is ignored because of the way PHP processes the assignment operator. Alternatives considered: * Instead of fixing the magic methods, they could have been removed instead and the class be made to `extend` `stdClass`. It has been chosen not to do so for two reasons: 1. It’s kind of nice to have at least ''one'' correct implementation of magic methods in WP, which can be used as an example to point to as well. 2. Extending `stdClass` would change the class hierarchy, which ''may'' or ''may not'' affect the tests using this fixture (depending on what’s being done with the class). Extending `stdClass` would also obfuscate what’s going on in the class and would require extensive documentation to prevent the extension being inadvertently removed at a future point in time. * Instead of fixing the magic methods, the test fixture could have been deprecated and/or removed, with the few tests which use the fixture being updated to use `stdClass` for their test fixture instead. It has been chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixture and evaluating whether that is so would be hard, as WP Directory cannot be used, since test code is normally not included in the code published on wp.org. Also note, there is still a (deprecated) `Basic_Subclass` fixture in the test suite, which extends this class. These magic methods and the `Basic_Object` test fixture were originally introduced in [28480] and [28523]. The fixture was deprecated in [42381] and undeprecated again in [45807]. At this time, the test fixture is used in the `WP_Test_REST_Post_Meta_Fields` and the `Tests_REST_API` test classes. References: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.set PHP Manual: Overloading: __set()] * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties] * [php/php-src#7786 php-src: #7786 PHP 8.2: unexpected deprecation for dynamic property set via magic method] Follow-up to [28480], [28493], [28523], [42381], [45807]. Props jrf, costdev. See #56514. Built from https://develop.svn.wordpress.org/trunk@54095 git-svn-id: http://core.svn.wordpress.org/trunk@53654 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot
pushed a commit
to gilzow/wordpress-performance
that referenced
this issue
Sep 7, 2022
This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods. With the deprecation of dynamic properties in PHP 8.2, this needs to be fixed. The new implementation represents a “proper” implementation of the magic methods for a class without non-`public` or typed properties. Notes: * Instead of relying on dynamic properties, the magic methods now store properties in a `private` `$arbitrary_props` array and retrieve them from there as well. * The original `$foo` property, even though declared as `private`, was never `private` in practice due to the way the magic methods were originally implemented. In effect, it was fully publicly retrievable and modifiable without any (type) restrictions. With that in mind, the `foo` property has been moved into the `$arbitrary_props` array to keep the implementation of the magic methods as clean and straightforward as possible. With the adjusted magic methods, access to and modification of `$foo` will (on the surface) continue to work in the same way as before, while under the hood, it is no longer affected by the dynamic properties deprecation. * Take note of the use of `array_key_exists()` instead of `isset()` in the `__get()` method. This is intentional and allows for `null` values to be stored and retrieved. * Also take note of `__set()` method no longer returning. `__set()` is supposed to be a `void` method. In practice, the return value would always be ignored due to how PHP handles magic methods, so in effect, this change will not make any difference and does not constitute a backward compatibility break.[[BR]][[BR]] > The return value of `__set()` is ignored because of the way PHP processes the assignment operator. Alternatives considered: * Instead of fixing the magic methods, they could have been removed instead and the class be made to `extend` `stdClass`. It has been chosen not to do so for two reasons: 1. It’s kind of nice to have at least ''one'' correct implementation of magic methods in WP, which can be used as an example to point to as well. 2. Extending `stdClass` would change the class hierarchy, which ''may'' or ''may not'' affect the tests using this fixture (depending on what’s being done with the class). Extending `stdClass` would also obfuscate what’s going on in the class and would require extensive documentation to prevent the extension being inadvertently removed at a future point in time. * Instead of fixing the magic methods, the test fixture could have been deprecated and/or removed, with the few tests which use the fixture being updated to use `stdClass` for their test fixture instead. It has been chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixture and evaluating whether that is so would be hard, as WP Directory cannot be used, since test code is normally not included in the code published on wp.org. Also note, there is still a (deprecated) `Basic_Subclass` fixture in the test suite, which extends this class. These magic methods and the `Basic_Object` test fixture were originally introduced in [28480] and [28523]. The fixture was deprecated in [42381] and undeprecated again in [45807]. At this time, the test fixture is used in the `WP_Test_REST_Post_Meta_Fields` and the `Tests_REST_API` test classes. References: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.set PHP Manual: Overloading: __set()] * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties] * [php/php-src#7786 php-src: #7786 PHP 8.2: unexpected deprecation for dynamic property set via magic method] Follow-up to [28480], [28493], [28523], [42381], [45807]. Props jrf, costdev. See #56514. Built from https://develop.svn.wordpress.org/trunk@54095 git-svn-id: https://core.svn.wordpress.org/trunk@53654 1a063a9b-81f0-0310-95a4-ce76da25c4cd
pbearne
pushed a commit
to pbearne/wordpress-develop
that referenced
this issue
Sep 9, 2022
This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods. With the deprecation of dynamic properties in PHP 8.2, this needs to be fixed. The new implementation represents a “proper” implementation of the magic methods for a class without non-`public` or typed properties. Notes: * Instead of relying on dynamic properties, the magic methods now store properties in a `private` `$arbitrary_props` array and retrieve them from there as well. * The original `$foo` property, even though declared as `private`, was never `private` in practice due to the way the magic methods were originally implemented. In effect, it was fully publicly retrievable and modifiable without any (type) restrictions. With that in mind, the `foo` property has been moved into the `$arbitrary_props` array to keep the implementation of the magic methods as clean and straightforward as possible. With the adjusted magic methods, access to and modification of `$foo` will (on the surface) continue to work in the same way as before, while under the hood, it is no longer affected by the dynamic properties deprecation. * Take note of the use of `array_key_exists()` instead of `isset()` in the `__get()` method. This is intentional and allows for `null` values to be stored and retrieved. * Also take note of `__set()` method no longer returning. `__set()` is supposed to be a `void` method. In practice, the return value would always be ignored due to how PHP handles magic methods, so in effect, this change will not make any difference and does not constitute a backward compatibility break.[[BR]][[BR]] > The return value of `__set()` is ignored because of the way PHP processes the assignment operator. Alternatives considered: * Instead of fixing the magic methods, they could have been removed instead and the class be made to `extend` `stdClass`. It has been chosen not to do so for two reasons: 1. It’s kind of nice to have at least ''one'' correct implementation of magic methods in WP, which can be used as an example to point to as well. 2. Extending `stdClass` would change the class hierarchy, which ''may'' or ''may not'' affect the tests using this fixture (depending on what’s being done with the class). Extending `stdClass` would also obfuscate what’s going on in the class and would require extensive documentation to prevent the extension being inadvertently removed at a future point in time. * Instead of fixing the magic methods, the test fixture could have been deprecated and/or removed, with the few tests which use the fixture being updated to use `stdClass` for their test fixture instead. It has been chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixture and evaluating whether that is so would be hard, as WP Directory cannot be used, since test code is normally not included in the code published on wp.org. Also note, there is still a (deprecated) `Basic_Subclass` fixture in the test suite, which extends this class. These magic methods and the `Basic_Object` test fixture were originally introduced in [28480] and [28523]. The fixture was deprecated in [42381] and undeprecated again in [45807]. At this time, the test fixture is used in the `WP_Test_REST_Post_Meta_Fields` and the `Tests_REST_API` test classes. References: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.set PHP Manual: Overloading: __set()] * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties] * [php/php-src#7786 php-src: #7786 PHP 8.2: unexpected deprecation for dynamic property set via magic method] Follow-up to [28480], [28493], [28523], [42381], [45807]. Props jrf, costdev. See #56514. git-svn-id: https://develop.svn.wordpress.org/trunk@54095 602fd350-edb4-49c9-b593-d223f7449a82
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 12, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 12, 2022
<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).
whereiscodedude
pushed a commit
to whereiscodedude/wpss
that referenced
this issue
Sep 18, 2022
This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods. With the deprecation of dynamic properties in PHP 8.2, this needs to be fixed. The new implementation represents a “proper” implementation of the magic methods for a class without non-`public` or typed properties. Notes: * Instead of relying on dynamic properties, the magic methods now store properties in a `private` `$arbitrary_props` array and retrieve them from there as well. * The original `$foo` property, even though declared as `private`, was never `private` in practice due to the way the magic methods were originally implemented. In effect, it was fully publicly retrievable and modifiable without any (type) restrictions. With that in mind, the `foo` property has been moved into the `$arbitrary_props` array to keep the implementation of the magic methods as clean and straightforward as possible. With the adjusted magic methods, access to and modification of `$foo` will (on the surface) continue to work in the same way as before, while under the hood, it is no longer affected by the dynamic properties deprecation. * Take note of the use of `array_key_exists()` instead of `isset()` in the `__get()` method. This is intentional and allows for `null` values to be stored and retrieved. * Also take note of `__set()` method no longer returning. `__set()` is supposed to be a `void` method. In practice, the return value would always be ignored due to how PHP handles magic methods, so in effect, this change will not make any difference and does not constitute a backward compatibility break.[[BR]][[BR]] > The return value of `__set()` is ignored because of the way PHP processes the assignment operator. Alternatives considered: * Instead of fixing the magic methods, they could have been removed instead and the class be made to `extend` `stdClass`. It has been chosen not to do so for two reasons: 1. It’s kind of nice to have at least ''one'' correct implementation of magic methods in WP, which can be used as an example to point to as well. 2. Extending `stdClass` would change the class hierarchy, which ''may'' or ''may not'' affect the tests using this fixture (depending on what’s being done with the class). Extending `stdClass` would also obfuscate what’s going on in the class and would require extensive documentation to prevent the extension being inadvertently removed at a future point in time. * Instead of fixing the magic methods, the test fixture could have been deprecated and/or removed, with the few tests which use the fixture being updated to use `stdClass` for their test fixture instead. It has been chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixture and evaluating whether that is so would be hard, as WP Directory cannot be used, since test code is normally not included in the code published on wp.org. Also note, there is still a (deprecated) `Basic_Subclass` fixture in the test suite, which extends this class. These magic methods and the `Basic_Object` test fixture were originally introduced in [28480] and [28523]. The fixture was deprecated in [42381] and undeprecated again in [45807]. At this time, the test fixture is used in the `WP_Test_REST_Post_Meta_Fields` and the `Tests_REST_API` test classes. References: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.set PHP Manual: Overloading: __set()] * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties] * [php/php-src#7786 php-src: #7786 PHP 8.2: unexpected deprecation for dynamic property set via magic method] Follow-up to [28480], [28493], [28523], [42381], [45807]. Props jrf, costdev. See #56514. Built from https://develop.svn.wordpress.org/trunk@54095
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 20, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 20, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 28, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 28, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 28, 2022
<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).
jrfnl
added a commit
to jrfnl/wordpress-develop-official
that referenced
this issue
Sep 28, 2022
<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).
ootwch
pushed a commit
to ootwch/wordpress-develop
that referenced
this issue
Nov 4, 2022
This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods. With the deprecation of dynamic properties in PHP 8.2, this needs to be fixed. The new implementation represents a “proper” implementation of the magic methods for a class without non-`public` or typed properties. Notes: * Instead of relying on dynamic properties, the magic methods now store properties in a `private` `$arbitrary_props` array and retrieve them from there as well. * The original `$foo` property, even though declared as `private`, was never `private` in practice due to the way the magic methods were originally implemented. In effect, it was fully publicly retrievable and modifiable without any (type) restrictions. With that in mind, the `foo` property has been moved into the `$arbitrary_props` array to keep the implementation of the magic methods as clean and straightforward as possible. With the adjusted magic methods, access to and modification of `$foo` will (on the surface) continue to work in the same way as before, while under the hood, it is no longer affected by the dynamic properties deprecation. * Take note of the use of `array_key_exists()` instead of `isset()` in the `__get()` method. This is intentional and allows for `null` values to be stored and retrieved. * Also take note of `__set()` method no longer returning. `__set()` is supposed to be a `void` method. In practice, the return value would always be ignored due to how PHP handles magic methods, so in effect, this change will not make any difference and does not constitute a backward compatibility break.[[BR]][[BR]] > The return value of `__set()` is ignored because of the way PHP processes the assignment operator. Alternatives considered: * Instead of fixing the magic methods, they could have been removed instead and the class be made to `extend` `stdClass`. It has been chosen not to do so for two reasons: 1. It’s kind of nice to have at least ''one'' correct implementation of magic methods in WP, which can be used as an example to point to as well. 2. Extending `stdClass` would change the class hierarchy, which ''may'' or ''may not'' affect the tests using this fixture (depending on what’s being done with the class). Extending `stdClass` would also obfuscate what’s going on in the class and would require extensive documentation to prevent the extension being inadvertently removed at a future point in time. * Instead of fixing the magic methods, the test fixture could have been deprecated and/or removed, with the few tests which use the fixture being updated to use `stdClass` for their test fixture instead. It has been chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixture and evaluating whether that is so would be hard, as WP Directory cannot be used, since test code is normally not included in the code published on wp.org. Also note, there is still a (deprecated) `Basic_Subclass` fixture in the test suite, which extends this class. These magic methods and the `Basic_Object` test fixture were originally introduced in [28480] and [28523]. The fixture was deprecated in [42381] and undeprecated again in [45807]. At this time, the test fixture is used in the `WP_Test_REST_Post_Meta_Fields` and the `Tests_REST_API` test classes. References: * [https://www.php.net/manual/en/language.oop5.overloading.php#object.set PHP Manual: Overloading: __set()] * [https://wiki.php.net/rfc/deprecate_dynamic_properties PHP RFC: Deprecate dynamic properties] * [php/php-src#7786 php-src: #7786 PHP 8.2: unexpected deprecation for dynamic property set via magic method] Follow-up to [28480], [28493], [28523], [42381], [45807]. Props jrf, costdev. See #56514. git-svn-id: https://develop.svn.wordpress.org/trunk@54095 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
The following code:
Resulted in this output:
https://3v4l.org/Ds0gN/rfc#vgit.master
But I expected this output instead:
I realize that this is an "unconventional" interpretation of how magic methods should be implemented, but at the same time, the RFC explicitly excluded dynamic properties being set via magic methods from the deprecation, so I'd really like to invite some discussion on this.
PHP Version
8.2/nightly
Operating System
No response
The text was updated successfully, but these errors were encountered: