PHP 8.2: fix magic method use within the test suite

[jrf]

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0, though this last part is not 100% certain yet.

RFC: ​https://wiki.php.net/rfc/deprecate_dynamic_properties

We can basically categorize dynamic properties into four base situations:

	Situation	Solution
1.	Typo in property name	Fix the typo, either in the property as declared or where the property assignment happens
2.	Known, named, dynamic property	Declare the property on the (parent) class
3.	Known use of unknown dynamic properties	Declare the full set of magic methods on a class (preferred) or let the class extend stdClass (discouraged)
4.	Unknown use of unknown dynamic properties	Use the #[AllowDynamicProperties] attribute on the (parent) class and/or declare the full set of magic methods on a class and/or let the class extend stdClass

Note: the #[AllowDynamicProperties] attribute is expected to be a temporary solution and it is expected that support for the attribute will be removed from PHP at some point in the future.

Current status

Patches have been created for Trac #56033 to explicitly declare all "known" dynamic properties (type 1 + 2).

A proposal to mitigate the deprecation for the "unknown" dynamic properties (type 3 + 4) via traits is open in Trac #56034.

Magic methods

A deep-dive into the magic methods implemented in WP Core has shown that most, if not all will need additional fixes.

For those magic methods in the WP Core src, this will be addressed as part of Trac #56034.

This ticket intends to address fixing the magic methods in the WP Core test suite.

The necessary patches for this are already available via open PRs on GitHub:

• ​https://github.com/WordPress/wordpress-develop/pull/3126
• ​https://github.com/WordPress/wordpress-develop/pull/3130

### PHP 8.2 | WP_UnitTestCase_Base: remove magic methods (without BC break)

⚠️ This PR depends on PR #3127, #3128 and #3129, which should be merged first (as otherwise the tests will start failing). ⚠️

Introduced in [35242] in response to Trac#30017 and Trac#33968.

These magic methods were introduced to prevent a BC-break, but in actual fact:

1. _Caused_ a BC break.

The original $factory property was a static property and this declared property was replaced by the magic methods.
Unfortunately, it wasn't realized at the time, that these magic methods _are not called for static property access_.

Property overloading only works in object context. These magic methods will not be triggered in static context.

And as approaching a static property in a non-static manner is ​not supported in PHP, this effectively created a BC-break instead of preventing it.

2. Were hiding errors in tests...

... as the magic methods would be invoked for non-existent properties and would return null (get)/false (isset).
See PRs #3127, #3128 and #3129 for bug fixes related to this.

3. Are problematic in relation to PHP 8.2...

... as the implementation is incomplete, doesn't protect against dynamic properties and hides PHP notices about undefined properties.

Now, there were several options to mitigate this:

1. Revert the original commit.

This would be problematic, as the _non-static_ version of these properties has now been supported for 7 years, so this would create a new BC-break.

2. Improve the magic methods.

With all the issues we've seen with magic methods (see the discussion in the ​livestream from August 16), this would probably cause more problems than its worth and would make for a much more complex implementation, which is over the top for this relative simple functionality, especially in the context of a test suite.

3. Remove the magic methods without adding the property.

This would again cause a BC-break, though one for which the mitigation solution would be relatively straight-forward. I.e. to replace property access using $this->factory with a function call $this->factory() (or self::factory() as the method is declared as static).
While we can (and have in a subsequent commit) mitigate this for the WP Core test suite, mitigating this for integration tests for plugins/themes is outside of our purview and they would still need to deal with this BC-break.

4. The current solution: removing the magic methods and explicitly declaring the (non-static) property and setting it in the set_up() method.

This does not constitute a BC-break with the functionality as it was over the past 7 years.
Setting the property in set_up() may be "late", but that is the earliest place in which the property can be set as non-static.
If the factory would be needed prior to set_up(), the (static) WP_UnitTestCase_Base::factory() method should be called directly. This is no different from how this functionality behaved over the past 7 years.

Note: I've straight-away marked the property as "deprecated" as the method should be favoured over the use of the property.

Refs:

• ​https://www.php.net/manual/en/language.oop5.overloading.php#object.get

---

While working on the above commit, I have investigated how the factory method and property were used in the WP Core test suite.

These were my findings:

• Direct use of the property in tests $this->factory->...: 614 instances in 53 test files
• Direct use of the method in tests (non-static) $this->factory()->...: 131 instances in 38 test files
• Direct use of the method in tests (static) [self|static|parent]::factory()->...: 3441 instances in 268 test files

The use of static function calls to the factory() method clearly wins out and this is in line with the original intention of the changes which were made in [35242].

With that in mind, I've created the two below commits which fix the non-static usages of the method and usages of the property within the WP Core test suite.

---

### Tests: correctly use the factory method

This replaces all non-static calls to the WP_UnitTestCase_Base::factory() method with static function calls as the method is declared as static.

This is a consistency improvement for the test suite.

Follow up to [35242].

### Tests: use the factory method instead of the property

This replaces all references to the WP_UnitTestCase_Base::$factory property with static function calls to the WP_UnitTestCase_Base::factory() method.

This is a consistency improvement for the test suite.

Follow up to [35242].

Trac ticket: https://core.trac.wordpress.org/ticket/56514 (first commit)
Trac ticket: https://core.trac.wordpress.org/ticket/55652 (second + third commit)

\<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\>

This is a test fixture (dummy class only used in a test context), which incorrectly implements the magic methods.

What 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 straight-forward 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 BC-break.

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.

I've 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 deprecate and/or removed, with the few tests which use the fixture being updated to use stdClass for their test fixture instead.

I've chosen not to do so as there may well be external (plugin/theme) tests relying on this test fixtures and evaluating if this is so, would be hard as WP Directory cannot be used, as 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.

Refs:

• ​https://www.php.net/manual/en/language.oop5.overloading.php#object.set
• ​https://wiki.php.net/rfc/deprecate_dynamic_properties
• php/php-src#7786

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.

Trac ticket: https://core.trac.wordpress.org/ticket/56514

I've created a new Trac ticket for WP 6.1 specifically for this patch (and the one in #3130): https://core.trac.wordpress.org/ticket/56514

I've created a new Trac ticket for WP 6.1 specifically for this patch (and the one in #3126): https://core.trac.wordpress.org/ticket/56514

Rebased without changes now all three of the PRs which needed to be merged first, have been. This should now show a passing build. (unless new issues were introduced in the mean time....)

[SergeyBiryukov]

In 54087:

Build/Test Tools: Remove magic methods from WP_UnitTestCase_Base (without a backward compatibility break).

These magic methods were introduced to prevent a backward compatibility break, but in actual fact:

1. Caused a backward compatibility break. The original $factory property was a static property and this declared property was replaced by the magic methods. Unfortunately, it was not realized at the time that these magic methods are not called for static property access.
   
   

   Property overloading only works in object context. These magic methods will not be triggered in static context.
   

   And as approaching a static property in a non-static manner is ​not supported in PHP, this effectively created a backward compatibility break instead of preventing it.

2. Were hiding errors in tests, as the magic methods would be invoked for non-existent properties and would return null (get) or false (isset). See [54040], [54041], and [54077] for bug fixes related to this.

3. Are problematic in relation to PHP 8.2, as the implementation is incomplete, does not protect against dynamic properties and hides PHP notices about undefined properties.

Now, there were several options to mitigate this:

1. Revert the original commit. This would be problematic, as the non-static version of these properties has now been supported for 7 years, so this would create a new backward compatibility break.

2. Improve the magic methods. With all the issues with magic methods (see the discussion in the ​livestream from August 16, 2022, this would probably cause more problems than it’s worth and would make for a much more complex implementation, which is over the top for this relatively simple functionality, especially in the context of a test suite.

3. Remove the magic methods without adding the property. This would again cause a backward compatibility break, though one for which the mitigation solution would be relatively straightforward, i.e. to replace property access using $this->factory with a function call $this->factory() (or self::factory(), as the method is declared as static). While we can (and have in a subsequent commit) mitigate this for the WP Core test suite, mitigating this for plugin or theme integration tests is outside of our purview and they would still need to deal with this backward compatibility break.

4. The current solution: removing the magic methods, explicitly declaring the (non-static) property and setting it in the set_up() method. This does not constitute a backward compatibility break with the functionality as it was over the past 7 years. Setting the property in set_up() may be “late”, but that is the earliest place in which the property can be set as non-static. If the factory would be needed prior to set_up(), the (static) WP_UnitTestCase_Base::factory() method should be called directly. This is no different from how this functionality behaved over the past 7 years.

Note: The property is straight away marked as “deprecated”, since the method should be favored over the use of the property.

Reference: ​PHP Manual: Property overloading: __get()

Follow-up to [35225], [35242].

Props jrf, costdev.
See #56514.

Thanks for the PR! Merged in r54087, r54088, and r54090.

Thanks @SergeyBiryukov !

[SergeyBiryukov]

In 54095:

Tests: Correct magic methods in Basic_Object.

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.
  
  

  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:

• ​PHP Manual: Overloading: __set()
• ​PHP RFC: Deprecate dynamic properties
• ​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.

Thanks for the PR! Merged in r54095.

[jrf]

Thanks @SergeyBiryukov ! With both fixes now committed, I'll close this ticket as fixed.

Should this be mentioned in a WP 6.1 dev-note ? I.e. reminder for integrators to use self::factory()->... in their tests instead of using $this->factory->... ?

[jrf]

(sorry, clicked too fast)

[SergeyBiryukov]

Replying to jrf:

Should this be mentioned in a WP 6.1 dev-note ? I.e. reminder for integrators to use self::factory()->... in their tests instead of using $this->factory->... ?

Good point, adding the keyword for that.