Make WordPress Core

Changeset 54087


Ignore:
Timestamp:
09/06/2022 10:00:11 PM (23 months ago)
Author:
SergeyBiryukov
Message:

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.
  1. 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.
  1. 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.
  1. 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.
  1. 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.
  1. 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.

File:
1 edited

Legend:

Unmodified
Added
Removed
  • trunk/tests/phpunit/includes/abstract-testcase.php

    r53937 r54087  
    2424    protected static $ignore_files;
    2525
    26     public function __isset( $name ) {
    27         return 'factory' === $name;
    28     }
    29 
    30     public function __get( $name ) {
    31         if ( 'factory' === $name ) {
    32             return self::factory();
    33         }
    34     }
     26    /**
     27     * Fixture factory.
     28     *
     29     * @deprecated 6.1.0 Use the WP_UnitTestCase_Base::factory() method instead.
     30     *
     31     * @var WP_UnitTest_Factory
     32     */
     33    protected $factory;
    3534
    3635    /**
     
    103102    public function set_up() {
    104103        set_time_limit( 0 );
     104
     105        $this->factory = self::factory();
    105106
    106107        if ( ! self::$ignore_files ) {
Note: See TracChangeset for help on using the changeset viewer.