Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 7 weeks ago

#56033 closed task (blessed) (fixed)

PHP 8.2: explicitly declare all known properties

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch php82 has-unit-tests
Focuses: Cc:

Description (last modified by 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

This ticket is intended ONLY for category 1 and 2 (see below), where fixing a typo or explicitly declaring the property fixes things.

The problem

What is a dynamic property ?

<?php
class Foo {
    public $id;

    public function __construct( $id, $field_name ) {
        // This is fine, the $id property exists on the class.
        $this->id         = $id;

        // This is a dynamically created property as the property
        // is not declared on the class.
        $this->field_name = $field_name;
    }
}

Dynamic properties can both be created from inside a class, like in the above example, as well as from outside the class (see the below example), which makes these issues very tricky to find via static analysis.

<?php
$obj = new Foo();
$obj->type = 'something';

When is something not a dynamic property ?

  1. When the property is explicitly declared on the class.
  2. When the property is explicitly declared on the (grand)parent class.

When are dynamic properties not problematic ?

  1. When the class, or one of its parents, has a magic __set() method which assigns the property to a declared property.
    <?php
    class Foo {
        private $fields = [];
    
        public function __construct($field_name ) {
            $this->field_name = $field_name; // Not problematic.
        }
    
        public function __set( $name, $value ) {
            $this->fields[ $name ] = $value;
        }
    }
    
  2. When the class, or its parent, extends the PHP native stdClass, which has highly optimized versions of the __set() magic method available.

Mitigation

The solution needed depends on the specific situation and each instance where a deprecation is thrown needs to be individually evaluated.

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 at some point in the future.

Patches

Reminder: This ticket is intended ONLY for patches which fall in category 1 and 2, where fixing a typo or explicitly declaring the property fixes things.

A full set of patches for all such known situations - based on running the WP test suite against PHP 8.2 - is available and PRs for each will be opened on GitHub with a link to this ticket.

However, as the WP test suite does not cover the full code base, more instances of situations which fall under category 1 and 2 are expected to be discovered over time.

Patches for any additional instances found, which fall under category 1 or 2, can be uploaded to this ticket and this ticket should stay open for these kind of patches during the WordPress 6.1 release cycle.


Related: #56009

Trac ticket #56034 is open for addressing dynamic properties in category 3 and 4.

Change History (113)

#1 @jrf
2 years ago

  • Description modified (diff)

This ticket was mentioned in PR #2843 on WordPress/wordpress-develop by jrfnl.


2 years ago
#2

  • Keywords has-unit-tests added

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test classes contain a set_up() method which sets a group of properties, which are _used_ by the tests, but the value of these properties are never _changed_ by the tests.

In other words, setting these properties in the set_up() is an unnecessary overhead and the properties should be changed to class constants.

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

This ticket was mentioned in PR #2844 on WordPress/wordpress-develop by jrfnl.


2 years ago
#3

### PHP 8.2 | Tests_Media: remove dynamic properties

Dynamic (non-explicitly declared) property usage is expected to be deprecated as of PHP 8.2 and will become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method which sets a group of properties, which are _used_ by the tests, but never _changed_ by the tests.

In other words, setting these properties in the set_up() is an unnecessary overhead and the properties should be changed to class constants.

Notes:

  • As the $img_html property which was previously being set, is not actually used in any of the tests, that property has not been converted to a constant.
  • The values which were previously being set using a heredoc, now use a nowdoc (supported since PHP 5.3), as they don't contain any interpolation.
  • The use of constant scalar expressions (IMG_URL) and constant arrays (IMG_META) in class constants is supported since PHP 5.6.

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

### Tests_Media: simplify some assertions

A number of assertions are checking for the number of times a hard-coded substring existed in a larger $haystack.

These assertions were using preg_match_all() to do so, but without actually using a regex.

In these cases, it is more performant (and simpler) to use the PHP native substr_count() function, which will yield the same result, without the overhead of regex parsing.

Ref: https://www.php.net/manual/en/function.substr-count.php

### Tests_Media: simplify some function calls

The remaining assertions using preg_match_all() do actually pass a regex, but the third parameter $matches is never used.

This third parameter became optional in PHP 5.4, so we may as well remove it.

Ref: https://www.php.net/manual/en/function.preg-match-all.php

Trac ticket: https://core.trac.wordpress.org/ticket/55652 (last two commits)

This ticket was mentioned in PR #2845 on WordPress/wordpress-develop by jrfnl.


2 years ago
#4

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method which sets properties only used by select tests, while the fixture method is being run for all tests.

There were a couple of ways this could be fixed:

  • As the values do not change across tests, use set_up_before_class() to set (static) properties instead.
  • For those values which are set up without a function call or variable interpolation, set the values in class constants.
  • Or set these values as local variables for those tests which actually use them.

I've elected to use a combination of option 2 and 3.

I'd highly recommend for these tests to be revisited though as refactoring to use data providers would be a huge improvement (but is outside the scope of ticket 56033).

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

This ticket was mentioned in PR #2846 on WordPress/wordpress-develop by jrfnl.


2 years ago
#5

Appears that the dynamically created $undefined property isn't actually used.

I wonder what the originally intention was of setting this property and if the property was maybe intended to be set on the WP_Customize_Manager object, but as-is, it is undeclared and unused, so can be safely removed.

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

This ticket was mentioned in PR #2847 on WordPress/wordpress-develop by jrfnl.


2 years ago
#6

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method which sets the $img_meta property, which is _used_ by the tests, but never _changed_ by the tests.

In other words, setting this property in the set_up() is an unnecessary overhead and the property should be changed to a class constant.

Includes fixing up the test class name.

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

This ticket was mentioned in PR #2848 on WordPress/wordpress-develop by jrfnl.


2 years ago
#7

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method which sets the $post_type property, which is _used_ by the tests, but never _changed_ by the tests.

In other words, setting this property in the set_up() is an unnecessary overhead and the property should be changed to a class constant.

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

This ticket was mentioned in PR #2849 on WordPress/wordpress-develop by jrfnl.


2 years ago
#8

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this case, as the $badchars property never changes, declaring this as a class constant is the sensible option.

As for the $dir property (which cannot be turned into a constant due to the function call), this is used in multiple tests, so making this property explicit makes sense.

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

This ticket was mentioned in PR #2850 on WordPress/wordpress-develop by jrfnl.


2 years ago
#9

... to allow for arbitrary properties to be declared on it.

Alternatively, the MockClass could be deprecated altogether in favour of directly using stdClass in those places it is used.

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

This ticket was mentioned in PR #2851 on WordPress/wordpress-develop by jrfnl.


2 years ago
#10

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular group of test files, the test classes contain a set_up() method which sets a few dynamic (not explicitly declared) properties.

For those properties which were set using a function call or variable access, the property has been explicitly declared on the class now.

For those properties which were set using a constant scalar expression and for which the value is not changed by any of the tests, the property setting has been removed in favour of declaring a class constant.

Includes removing one unused dynamic property declaration ($this->queries in Test_Block_Supports_Layout, probably copy/pasta from another test class).

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

#11 @jrf
2 years ago

  • Description modified (diff)

This ticket was mentioned in PR #2861 on WordPress/wordpress-develop by jrfnl.


2 years ago
#12

### WP_Test_REST_Posts_Controller: move tear_down() method

... up to be directly below the set_up() method.

### PHP 8.2 | WP_Test_REST_Posts_Controller: fix dynamic properties

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test_create_update_post_with_featured_media() method creates an attachment and writes the ID of the attachment to a dynamic (undeclared) property to be used as a flag to determine whether attachments need to be cleaned after the test in the tear_down() method.

As the actual _value_ of the property is irrelevant for the cleaning up and the property is realistically being used as a "flag", I'd like to suggest fixing this as follows:

  • Have an actual "flag" property declared with a descriptive name - $attachments_created - to make the code in the tear_down() more easily understandable.
  • As for the actual ID of the attachment, save that to a test method local variable as that's the only place it has any relevance.

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

This ticket was mentioned in PR #2862 on WordPress/wordpress-develop by jrfnl.


2 years ago
#13

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In these particular cases, individual tests set a couple of properties ($author_id, $post_id), which are never used outside of the context of the test in which they are created.

In other words: these should never have been properties, but should be test local variables instead.

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

This ticket was mentioned in PR #2862 on WordPress/wordpress-develop by jrfnl.


2 years ago
#14

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In these particular cases, individual tests set a couple of properties ($author_id, $post_id), which are never used outside of the context of the test in which they are created.

In other words: these should never have been properties, but should be test local variables instead.

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

This ticket was mentioned in PR #2863 on WordPress/wordpress-develop by jrfnl.


2 years ago
#15

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the scandir() method sets a dynamic property, which is subsequently used in the delete_folders() method.
Explicitly declaring the property fixes the PHP 8.2 incompatibility.

---

Having said that.... IMO this is not the right solution, but solving this property could be considered a breaking change as these methods are both public and in the abstract base class for all tests.

Note: within the WP Core test suite, these methods are _only_ used in the Tests_Multisite_GetSpaceUsed test class.

The "proper" solution, IMO, would be as follows:

  1. Change the scandir() method to return an array of the found directories. The delete_folders() method then would just need to use the return value and would not need access to the property either.

I imagine this was previously not done due to the person writing the code being uncomfortable with recursive functions, but it is perfectly possible to do so and would get rid of the unnecessary property.
The method is currently a void method and sets the property.

  1. Move both methods to the Tests_Multisite_GetSpaceUsed test class as clearly they are not needed elsewhere.

Even just implementing step 1 would already be a good improvement and would still leave these methods available to future and/or external integration tests.

I'm happy to convert this PR to the proper solution, but would like a second opinion first. /cc @johnbillion

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

This ticket was mentioned in PR #2864 on WordPress/wordpress-develop by jrfnl.


2 years ago
#16

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this test file, the Tests_Comment_Walker::set_up() method created a dynamic $post_id property, which should have been explicitly declared.
Along the same lines, the Comment_Callback_Test::__construct() method also assigned values to two undeclared properties.

Fixed now by explicitly declaring the properties in both classes.

Includes renaming the Comment_Callback_Test class to Comment_Callback_Test_Helper as this class does not contain any test method and it's only purpose is as a "helper" class for the Tests_Comment_Walker::test_has_children() test.

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

This ticket was mentioned in PR #2865 on WordPress/wordpress-develop by jrfnl.


2 years ago
#17

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In each of the cases included in this PR, one or more individual tests set a property to allow a filter/action access to certain information.

This commit:

  • Explicitly declares these properties and documents in which tests they are being used.
  • Add a reset to the default value of the property to a pre-existing tear_down() method or adds that method specifically for that purpose.

This ensures that tests can not accidentally "taint" each other.

As these properties are being declared on test classes, I'm making them all private.
Even though the original dynamic property was public, this should not be considered a BC break as this only involves test classes.

Includes:

  • In the Tests_Post_Query class, there were two tests assigning a value to an undeclared $post_id property, but subsequently not using the property, so those assignments should have been to a local variable (if they should be assignments at all).
  • In the Test_User_Capabilities class, the property name had a leading _ underscore. This is an outdated PHP 4 practice to indicate a private property. As PHP 4 is no longer supported, I have removed the leading underscore from the property name.
  • In the Tests_User_Capabilities class, an unused $_role_test_wp_roles_role property was declared somewhere in the middle of the class. I have removed this property.

This was probably intended to be the declaration of the $_role_test_wp_roles_init property, but then the name used in the assignments was changed, but the property declaration name was not.

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

This ticket was mentioned in PR #2866 on WordPress/wordpress-develop by jrfnl.


2 years ago
#18

Appears that the dynamically created $has_setup_template property isn't actually used.

It's unclear what the originally intention was, but as-is, it is undeclared and unused, so can be safely removed.

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

This ticket was mentioned in PR #2867 on WordPress/wordpress-develop by jrfnl.


2 years ago
#19

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In each of the cases included in this PR, one or more properties are dynamically created in the set_up() method of the test class.

This commit explicitly declares these properties.

As these properties are being declared on test classes, I'm making them all private.
Even though the original dynamic property was public, this should not be considered a BC break as this only involves test classes.

Notes:

  • As these properties receive assignments during test runs or a one-time assignment, but the assignment uses a function call or variable access, these properties can't be changed to class constants, but they should be declared explicitly as properties on the class.
  • In Tests_Theme_CustomHeader, I've given the $customize_manager property a default value of null, same as it was already being reset to null in the tear_down() method.
  • In Tests_Privacy_wpPrivacyProcessPersonalDataExportPage and the Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile classes, the property name had a leading _ underscore. This is an outdated PHP 4 practice to indicate a private property. As PHP 4 is no longer supported, I have renamed the property to $orig_error_level.
  • Along the same lines, in Tests_Menu_Walker_Nav_Menu, the property name also had a leading _ underscore. I have renamed the property to $orig_wp_nav_menu_max_depth.
  • In the Tests_Shortcode class, three properties were already being (re)set in the set_up() method, while three others were being set for most tests via the shortcode_test_shortcode_tag() method or in the tests themselves.

I've now made sure that all six properties are given their initial null value in the set_up() method and are explicitly declared.

Additionally:

  • In the Tests_User_Session class, the set_up() method is incorrect. No assertions should be executed in fixture methods, but the set_up() method contains two assertions.

I've not addressed this as this is outside the scope of this PR, but this should be addressed at a later point in time.

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

This ticket was mentioned in PR #2868 on WordPress/wordpress-develop by jrfnl.


2 years ago
#20

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

The WP_Test_Stream class is a stream wrapper for use in the tests and must comply with the PHP requirements for such stream wrappers.

In this case, the class did not declare the required public $context property, which led to deprecation notices about the property being dynamically created from the Tests_Image_Editor_Imagick::test_streams() and Tests_Image_Meta::test_stream tests.

Refs:

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

#21 @SergeyBiryukov
2 years ago

In 53557:

Code Modernization: Remove dynamic properties in Tests_*_Slashes.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test classes contain a set_up() method that sets a group of properties, which are used by the tests, but the values of these properties are never changed by the tests.

In other words, setting these properties in the set_up() is an unnecessary overhead and the properties should be changed to class constants.

Follow-up to [1041/tests], [1071/tests].

Props jrf.
See #56033.

This ticket was mentioned in PR #2869 on WordPress/wordpress-develop by jrfnl.


2 years ago
#22

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this case, the $plugin_info and $theme_info are set in the Plugin_Upgrader::bulk_upgrade() and the Theme_Upgrader::bulk_upgrade() specifically.

The `Bulk_Plugin_Upgrader_Skin` class and the `Bulk_Theme_Upgrader_Skin` class both already allow for this, but the `wp_ajax_update_plugin()` method and the `wp_ajax_update_theme()` method also call the *_Upgrader::bulk_upgrade() methods, so the WP_Ajax_Upgrader_Skin also needs to have these properties explicitly declared.

Includes adding proper docblocks to the pre-existing properties in the Bulk_Plugin_Upgrader_Skin and the Bulk_Theme_Upgrader_Skin classes.

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

This ticket was mentioned in PR #2870 on WordPress/wordpress-develop by jrfnl.


2 years ago
#24

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it's an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
  • For unknown _use of_ dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $timeout property falls in the "known property" category.

Note: in contrast to the other properties being set in the constructor, this property is not declared on the parent class, so not inherited.

Refs:

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

#25 @SergeyBiryukov
2 years ago

In 53558:

Code Modernization: Remove dynamic properties in Tests_Media.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method that sets a group of properties, which are used by the tests, but never changed by the tests.

In other words, setting these properties in the set_up() is an unnecessary overhead and the properties should be changed to class constants.

Notes:

  • As the $img_html property, which was previously being set, is not actually used in any of the tests, that property has not been converted to a constant.
  • The values which were previously being set using a heredoc, now use a nowdoc (supported since PHP 5.3), as they don't contain any interpolation.
  • The use of constant scalar expressions (IMG_URL) and constant arrays (IMG_META) in class constants is supported since PHP 5.6.

Follow-up to [711/tests], [1260/tests], [34855], [41724], [53557].

Props jrf.
See #56033.

This ticket was mentioned in PR #2871 on WordPress/wordpress-develop by jrfnl.


2 years ago
#26

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it's an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
  • For unknown _use of_ dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $is_overloaded property and the $_f property fall in the "known property" category.
In both cases, these are being set in the __construct() method of the class they apply to.

The $_post property appears to be a typo however. The $_post property looks to be unused, while there is an integer $_pos ("position") property, which is used throughout the class and used by the child classes, which is not declared.

Refs:

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

This ticket was mentioned in PR #2872 on WordPress/wordpress-develop by jrfnl.


2 years ago
#27

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it's an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
  • For unknown _use of_ dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $_nplurals and the $_gettext_select_plural_form property, both being set in the class constructor, fall in the "known property" category.

Refs:

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

This ticket was mentioned in PR #2873 on WordPress/wordpress-develop by jrfnl.


2 years ago
#28

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it's an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set() et al methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods build in.
  • For unknown _use of_ dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $use property, as set in the class constructor, falls in the "known property" category.

Refs:

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

#29 @jrf
2 years ago

A full set of patches for all such known situations - based on running the WP test suite against PHP 8.2 - is available and PRs for each will be opened on GitHub with a link to this ticket.

Done. All patches for all currently known issues which fall under this ticket have now been uploaded as GitHub PRs.

As mentioned before, this set of patches is based on the WP test suite, so there may well be more issues of the "known property" type out there, which we just haven't found yet.

jrfnl commented on PR #2863:


2 years ago
#30

@johnbillion Agreed, but this _is_ still about the dynamic property as the change I propose in "step 1" removes the need for the (dynamic or otherwise) property altogether.

#31 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

SergeyBiryukov commented on PR #2844:


2 years ago
#33

Thanks for the PR! Merged in r53558, r53790, and r53795.

jrfnl commented on PR #2844:


2 years ago
#34

Thank you @SergeyBiryukov !

SergeyBiryukov commented on PR #2863:


2 years ago
#35

Thanks for the PR!

Note: within the WP Core test suite, these methods are _only_ used in the Tests_Multisite_GetSpaceUsed test class.

Right, they were initially added for wp_upload_dir() tests in r30658, but subsequently removed from there in r36565.

The "proper" solution, IMO, would be as follows:

  1. Change the scandir() method to return an array of the found directories. The delete_folders() method then would just need to use the return value and would not need access to the property either. I imagine this was previously not done due to the person writing the code being uncomfortable with recursive functions, but it is perfectly possible to do so and would get rid of the unnecessary property. The method is currently a void method and sets the property.

I think I would prefer this solution here, if possible. The scandir() method is clearly not intended for being used anywhere other than in delete_folders(), so we don't really need the property if scandir() can just return an array of the found directories.

#36 @SergeyBiryukov
2 years ago

In 53850:

Code Modernization: Remove dynamic properties in Tests_POMO_PO.

In this particular case, the test class contains a set_up() method which sets properties only used by select tests, while the fixture method is being run for all tests.

There were a few ways this could be fixed:

  • As the values do not change across tests, use set_up_before_class() to set (static) properties instead.
  • For those values which are set up without a function call or variable interpolation, set the values in class constants.
  • Or set these values as local variables for those tests which actually use them.

The implemented solution is a combination of options 2 and 3.

Follow-up to [1106/tests], [53557], [53558].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2845:


2 years ago
#37

Thanks for the PR! Merged in r53850.

SergeyBiryukov commented on PR #2846:


2 years ago
#38

Thanks for the PR!

  • It looks like there's one more instance in Tests_WP_Customize_Manager, which is not used either and can be removed too.
  • This appears to be a copy/paste from the Tests_WP_Customize_Setting class, which does use the property in some tests.
  • Introduced for Tests_WP_Customize_Manager in r31329, copied to Tests_WP_Customize_[Panel|Section] in r32658.

jrfnl commented on PR #2845:


2 years ago
#39

@SergeyBiryukov Thanks for getting this committed!

#40 @SergeyBiryukov
2 years ago

In 53851:

Code Modernization: Remove unused dynamic property in Tests_WP_Customize_*.

The dynamically created $undefined property appears to be a copy/paste from the Tests_WP_Customize_Setting class, and is not actually used in other test classes:

  • Tests_WP_Customize_Manager
  • Tests_WP_Customize_Panel
  • Tests_WP_Customize_Section

Follow-up to [31329], [32658].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2846:


2 years ago
#41

Merged in r53851.

#42 @SergeyBiryukov
2 years ago

In 53852:

Code Modernization: Remove dynamic properties in Tests_Media_GetPostGalleries.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method that sets the $img_meta property, which is used by the tests, but never changed by the tests.

In other words, setting this property in the set_up() is an unnecessary overhead and the property should be changed to a class constant.

Includes renaming the test class to match the naming conventions.

Follow-up to [52190], [53557], [53558], [53850], [53851].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2847:


2 years ago
#43

Thanks for the PR! Merged in r53852.

#44 @SergeyBiryukov
2 years ago

In 53853:

Code Modernization: Remove dynamic properties in Tests_Post_Revisions.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test class contains a set_up() method that sets the $post_type property, which is used by the tests, but never changed by the tests.

In other words, setting this property in the set_up() is an unnecessary overhead and the property should be changed to a class constant.

Follow-up to [1212/tests], [52389], [53557], [53558], [53850], [53851], [53852].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2848:


2 years ago
#45

Thanks for the PR! Merged in r53853.

#46 @SergeyBiryukov
2 years ago

In 53854:

Code Modernization: Remove dynamic properties in Tests_File.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this case, as the $badchars property never changes, declaring this as a class constant is the sensible option.

As for the $dir property (which cannot be turned into a constant due to the function call), this is used in multiple tests, so making this property explicit makes sense.

Follow-up to [139/tests], [53557], [53558], [53850], [53851], [53852], [53853].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2849:


2 years ago
#47

Thanks for the PR! Merged in r53854.

jrfnl commented on PR #2846:


2 years ago
#48

  • It looks like there's one more instance in Tests_WP_Customize_Manager, which is not used either and can be removed too.

Nice catch! _Unused_ properties was outside the scope of the task I was working on, so not considered. The property is declared in the Tests_WP_Customize_Manager class, so was not a dynamic property.

I noticed you removed the setting of the $this->undefined property in the Tests_WP_Customize_Manager class now anyway, but that now leaves the declared public $undefined; property as unused.

I'd recommend removing that property from the the Tests_WP_Customize_Manager class.

jrfnl commented on PR #2849:


2 years ago
#49

Making swift progress now! Thank @SergeyBiryukov

jrfnl commented on PR #2861:


2 years ago
#50

Rebased without changes to get passed an imaginary merge conflict.

jrfnl commented on PR #2865:


2 years ago
#51

Rebased without changes to get passed an imaginary merge conflict.

jrfnl commented on PR #2863:


2 years ago
#52

I think I would prefer this solution here, if possible. The scandir() method is clearly not intended for being used anywhere other than in delete_folders(), so we don't really need the property if scandir() can just return an array of the found directories.

I've rebased the original commit (without changes) and added a second commit implementing the additional suggested changes per [1] for your review.

If approved, I'd suggest committing this to Core as one commit, but to allow for reviewing the difference between the solutions I've set this up as separate commits for this PR.

#53 @SergeyBiryukov
2 years ago

In 53855:

Code Modernization: Remove unused $undefined property in Tests_WP_Customize_Manager.

This appears to be a copy/paste from the Tests_WP_Customize_Setting class, and is not actually used in WP_Customize_Manager tests.

Follow-up to [31329], [32658], [53851].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2846:


2 years ago
#54

I'd recommend removing that property from the Tests_WP_Customize_Manager class.

Ah, I missed that it was declared :) Thanks! Removed in r53855.

#55 @SergeyBiryukov
2 years ago

In 53856:

Code Modernization: Let MockClass extend stdClass.

This allows for arbitrary properties to be declared on it.

Alternatively, the MockClass could be deprecated altogether in favor of directly using stdClass where needed.

Follow-up to [53/tests], [65/tests], [53557], [53558], [53850], [53851], [53852], [53853], [53854].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2850:


2 years ago
#56

Thanks for the PR! Merged in r53856.

jrfnl commented on PR #2865:


2 years ago
#57

Rebased without changes to get passed an imaginary merge conflict.

And again...

#58 @SergeyBiryukov
2 years ago

In 53916:

Code Modernization: Remove dynamic properties in theme tests.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular group of test files, the test classes contain a set_up() method which sets a few dynamic (not explicitly declared) properties.

For those properties which were set using a function call or variable access, the property has been explicitly declared on the class now.

For those properties which were set using a constant scalar expression and for which the value is not changed by any of the tests, the property setting has been removed in favor of declaring a class constant.

Includes removing one unused dynamic property declaration: $this->queries in Test_Block_Supports_Layout, which appears to be a copy/paste from Tests_Theme_wpThemeJsonResolver.

Follow-up to [40/tests], [260/tests], [598/tests], [50960], [52675], [53085], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2851:


2 years ago
#59

Thanks for the PR! Merged in r53916.

jrfnl commented on PR #2851:


2 years ago
#60

Thanks for getting this committed @SergeyBiryukov !

anton-vlasenko commented on PR #2873:


2 years ago
#61

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2873

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache
  • PHP: PHP 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.2.0

### Steps to Reproduce:

  1. Make sure you are on PHP 8.2.
  2. Instantiate an object of a Services_JSON() class:

{{{php
ini_set('error_reporting', E_ALL);
new Services_JSON();
}}}

  1. ❌ You should see this error in the console:
    Deprecated: Creation of dynamic property Services_JSON::$use is deprecated in src/wp-includes/class-json.php on line 151
    

### After applying the patch

  • ✅ There is no error in the console after applying the patch.

jrfnl commented on PR #2873:


2 years ago
#62

I'm wondering if it is possible to declare Services_JSON::$use property private? This is just to enforce encapsulation. This property is only used inside the Services_JSON class. I understand that this would probably break BC. What is your opinion on this?

You're right, this would be a BC-break and therefore not acceptable.
On top of that, we're talking about a deprecated class here, so the less we change, the better.

#63 @SergeyBiryukov
2 years ago

In 53935:

Code Modernization: Remove dynamic properties in WP_Test_REST_Posts_Controller.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the test_create_update_post_with_featured_media() method creates an attachment and writes the ID of the attachment to a dynamic (undeclared) property to be used as a flag to determine whether attachments need to be cleaned up after the test in the tear_down() method.

As the actual value of the property is irrelevant for the cleaning up and the property is realistically being used as a “flag”, this is now fixed as follows:

  • Have an actual “flag” property declared with a descriptive name — $attachments_created — to make the code in the tear_down() more easily understandable.
  • As for the actual ID of the attachment, save that to a test method local variable as that is the only place where it has any relevance.

Includes moving the tear_down() method up to be directly below the set_up() method.

Follow-up to [38832], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2861:


2 years ago
#64

Thanks for the PR! Merged in r53935.

#65 @SergeyBiryukov
2 years ago

In 53936:

Code Modernization: Remove dynamic properties in WP_Test_REST_Users_Controller.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In these particular cases, individual tests set a couple of properties ($author_id, $post_id) that are never used outside of the context of the test in which they are created.

In other words: these should never have been properties, but should be local variables instead.

Follow-up to [38832], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2862:


2 years ago
#66

Thanks for the PR! Merged in r53936.

jrfnl commented on PR #2862:


2 years ago
#67

Thanks @SergeyBiryukov !

jrfnl commented on PR #2861:


2 years ago
#68

Thanks @SergeyBiryukov !

#69 @SergeyBiryukov
2 years ago

In 53937:

Code Modernization: Remove dynamic properties in WP_UnitTestCase_Base.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this particular case, the scandir() method sets a dynamic $matched_dirs property, which is subsequently used in the delete_folders() method.

This commit removes the need for the property. Effectively, this changes the scandir() method to return an array of the matched directories instead of setting the property by using recursion in the method itself in an optimized manner.

Note the array_merge() not being in the loop itself, but at the very end of the function. This is for performance reasons, see clearPHP: No array_merge() In Loops for a more detailed explanation of this.

It has been verified in detail that the actual results of the previous version of the method and this version match, even when the paths passed are more complex and have deeper nested subdirectories.

Follow-up to [30658], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936].

Props jrf, johnbillion, markjaquith, SergeyBiryukov.
See #56033.

SergeyBiryukov commented on PR #2863:


2 years ago
#70

Thanks for the PR! Merged in r53937.

#71 @SergeyBiryukov
2 years ago

In 53938:

Code Modernization: Remove dynamic properties in Tests_Comment_Walker.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this test file, the Tests_Comment_Walker::set_up() method created a dynamic $post_id property, which should have been explicitly declared.

Along the same lines, the Comment_Callback_Test::__construct() method also assigned values to two undeclared properties.

This is now fixed by explicitly declaring the properties in both classes.

Includes renaming the Comment_Callback_Test class to Comment_Callback_Test_Helper as it does not contain any test methods and its only purpose is as a “helper” class for the Tests_Comment_Walker::test_has_children() test.

Follow-up to [28824], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2864:


2 years ago
#72

Thanks for the PR! Merged in r53938.

anton-vlasenko commented on PR #2872:


2 years ago
#73

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2872

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache 2.4.53
  • PHP: 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.9.0-rc.3

### Steps to reproduce:

  1. Make sure you are on PHP 8.2.
  2. Call the Gettext_Translations::set_header() method:

{{{php
ini_set('error_reporting', E_ALL);
$translations = new Gettext_Translations();
$translations->set_header('Plural-Forms', 'some_value');
}}}

  1. ❌ You should see these errors in the console:
    Deprecated: Creation of dynamic property Gettext_Translations::$_nplurals is deprecated in src/wp-includes/pomo/translations.php on line 291
    Deprecated: Creation of dynamic property Gettext_Translations::$_gettext_select_plural_form is deprecated in src/wp-includes/pomo/translations.php on line 292
    

### After applying the patch:

  • ✅ There are no errors in the console after applying the patch.

✅ Everything works as expected.

anton-vlasenko commented on PR #2871:


2 years ago
#74

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2871

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache
  • PHP: 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.2.0

### Steps to reproduce:

  1. Make sure you are on PHP 8.2.
  2. Instantiate an object of a POMO_FileReader() class:

{{{php
ini_set('error_reporting', E_ALL);
$translations = new POMO_FileReader(FILE);
}}}

  1. ❌ You will see these errors in the console:
    Deprecated: Creation of dynamic property POMO_FileReader::$is_overloaded is deprecated in src/wp-includes/pomo/streams.php on line 27
    Deprecated: Creation of dynamic property POMO_FileReader::$_pos is deprecated in src/wp-includes/pomo/streams.php on line 30
    Deprecated: Creation of dynamic property POMO_FileReader::$_f is deprecated in src/wp-includes/pomo/streams.php on line 163
    

### After applying the patch:

  • ✅ There are no errors in the console after applying the patch.

✅ Everything works as expected.

anton-vlasenko commented on PR #2870:


2 years ago
#75

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2870

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache
  • PHP: 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.9.0-rc3

### Steps to reproduce:

  1. Make sure you are on PHP 8.2.
  2. Instantiate an object of a WP_SimplePie_File class:

{{{php
ini_set('error_reporting', E_ALL);
new WP_SimplePie_File('http://test.file');
}}}

  1. ❌ You will see this error in the console:
    Deprecated: Creation of dynamic property WP_SimplePie_File::$timeout is deprecated in src/wp-includes/class-wp-simplepie-file.php on line 42
    

### After applying the patch:

  • ✅ There is no error in the console after applying the patch.

✅ Everything works as expected.

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

jrfnl commented on PR #2869:


2 years ago
#77

Note: to see the failing tests (without this patch) on PHP 8.2, run the tests with phpunit --group ajax.

jrfnl commented on PR #2869:


2 years ago
#78

Note: to see the failing tests (without this patch) on PHP 8.2, run the tests with phpunit --group ajax.

#79 @SergeyBiryukov
2 years ago

In 53942:

Code Modernization: Explicitly declare all properties in various tests.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In each of the cases included in this commit, one or more individual tests set a property to allow a filter or action access to certain information.

This commit:

  • Explicitly declares these properties and documents in which tests they are being used.
  • Adds a reset to the default value of the property to a pre-existing tear_down() method or adds that method specifically for that purpose. This ensures that tests do not accidentally “taint” each other.

As these properties are being declared on test classes, they are marked as private. Even though the original dynamic property was public, this should not be considered a backward compatibility break as this only involves test classes.

Includes:

  • In the Tests_Post_Query class, there were two tests assigning a value to an undeclared $post_id property, but subsequently not using the property, so those assignments should have been to a local variable (if they should be assignments at all).
  • In the Test_User_Capabilities class, the property name had a leading _ underscore. This is an outdated PHP 4 practice to indicate a private property. As PHP 4 is no longer supported, the leading underscore is removed from the property name.
  • In the Tests_User_Capabilities class, an unused $_role_test_wp_roles_role property was declared somewhere in the middle of the class. That property is now removed in favor of $_role_test_wp_roles_init, which appears to be the intended name, previously misspelled.

Follow-up to [27294], [36277], [36750], [37712], [38571], [39082], [40290], [43049], [44628], [48328], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2865:


2 years ago
#80

Thanks for the PR! Merged in r53942.

jrfnl commented on PR #2865:


2 years ago
#81

Thanks @SergeyBiryukov !

anton-vlasenko commented on PR #2868:


2 years ago
#82

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2868

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache
  • PHP: 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.9.0-rc3

### Steps to reproduce:

  1. Make sure you are on PHP 8.2.
  2. Make sure Imagick extension is enabled.
  3. Run the Tests_Image_Meta::test_stream test method.

{{{bash
php vendor/bin/phpunit -c phpunit.xml.dist --filter test_stream
}}}

  1. ❌ The test method will fail with these error messages:
    There were 4 errors:
    
    1) Tests_Image_Editor_Imagick::test_streams
    Creation of dynamic property WP_Test_Stream::$context is deprecated
    src/wp-includes/class-wp-image-editor-imagick.php:131
    tests/phpunit/tests/image/editorImagick.php:597
    
    2) Tests_Image_Meta::test_stream with data set "Orientation only metadata" ('testimagemeta://wp_read_image...e1.jpg', array('0', '', '', '', '0', '', '0', '0', '0', '', '3', array()))
    Creation of dynamic property WP_Test_Stream::$context is deprecated
    src/wp-admin/includes/image.php:734
    tests/phpunit/tests/image/meta.php:181
    
    3) Tests_Image_Meta::test_stream with data set "Exif from a Nikon D70 with IPTC data added later" ('testimagemeta://wp_read_image...e2.jpg', array('6.3', 'IPTC Creator', 'NIKON D70', 'IPTC Caption', '1090516475', 'IPTC Copyright', '18', '200', '0.04', 'IPTC Headline', '0', array()))
    Creation of dynamic property WP_Test_Stream::$context is deprecated
    src/wp-admin/includes/image.php:734
    tests/phpunit/tests/image/meta.php:181
    
    4) Tests_Image_Meta::test_stream with data set "Exif from a DMC-LX2 camera with keywords" ('testimagemeta://wp_read_image...e3.jpg', array('8', 'Photoshop Author', 'DMC-LX2', 'Photoshop Description', '1306315327', 'Photoshop Copyrright Notice', '6.3', '100', '0.0025', 'Photoshop Document Ttitle', '1', array('beach', 'baywatch', 'LA', 'sunset')))
    Creation of dynamic property WP_Test_Stream::$context is deprecated
    src/wp-admin/includes/image.php:734
    tests/phpunit/tests/image/meta.php:181
    

### After applying the patch:

  • ✅ The test method passes.

✅ Everything works as expected.

anton-vlasenko commented on PR #2866:


2 years ago
#83

## Test Report
This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/2866

### Environment

  • OS: macOS 12.5.1
  • Web Server: Apache
  • PHP: 8.2.0-dev
  • WordPress: 6.1-alpha-53344-src
  • Browser: Safari 15.6
  • Theme: Twenty Twenty-Two
  • Active Plugins:
    • Gutenberg 13.9.0-rc3

### Steps to reproduce:

  1. Make sure you are on PHP 8.2.
  2. Run the WP_Test_REST_Pages_Controller test class.

{{{bash
php vendor/bin/phpunit -c phpunit.xml.dist --filter WP_Test_REST_Pages_Controller
}}}

  1. ❌ The tests will fail; there will be many error messages about the $has_setup_template property in the console, e.g.:
    34) WP_Test_REST_Pages_Controller::test_get_item_schema
    Creation of dynamic property WP_Test_REST_Pages_Controller::$has_setup_template is deprecated
    

### After applying the patch:

  • ✅ The tests pass.

✅ Everything works as expected.

#84 @SergeyBiryukov
2 years ago

In 53945:

Code Modernization: Remove unused dynamic property in WP_Test_REST_Pages_Controller.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this case, it appears that the dynamically created $has_setup_template property is not actually used. It is unclear what the original intention was, but since it is undeclared and unused, it can be safely removed.

Follow-up to [38832], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942].

Props jrf, antonvlasenko.
See #56033.

SergeyBiryukov commented on PR #2866:


2 years ago
#85

Thanks for the PR! Merged in r53945.

jrfnl commented on PR #2866:


2 years ago
#86

Thanks @SergeyBiryukov !

#87 @SergeyBiryukov
2 years ago

In 53948:

Code Modernization: Explicitly declare all properties created in set_up() methods of various test classes.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In each of the cases included in this commit, one or more properties are dynamically created in the set_up() method of the test class. This commit explicitly declares these properties.

As these properties are being declared on test classes, they are marked as private. Even though the original dynamic property was public, this should not be considered a backward compatibility break as this only involves test classes.

Notes:

  • As these properties receive assignments during test runs or a one-time assignment, but the assignment uses a function call or variable access, these properties cannot be changed to class constants, but they should be declared explicitly as properties on the class.
  • In Tests_Theme_CustomHeader, the $customize_manager property is given a default value of null, same as it was already being reset to null in the tear_down() method.
  • In Tests_Privacy_wpPrivacyProcessPersonalDataExportPage and Tests_Privacy_wpPrivacyGeneratePersonalDataExportFile classes, the property name had a leading _ underscore. This is an outdated PHP 4 practice to indicate a private property. As PHP 4 is no longer supported, the property has been renamed to $orig_error_level.
  • Along the same lines, in Tests_Menu_Walker_Nav_Menu, the property name also had a leading _ underscore. The property has been renamed to $orig_wp_nav_menu_max_depth.
  • In the Tests_Shortcode class, three properties were already being (re)set in the set_up() method, while three others were being set for most tests via the shortcode_test_shortcode_tag() method or in the tests themselves. It is ensured now that all six properties are given their initial null value in the set_up() method and are explicitly declared.

Additionally:

  • In the Tests_User_Session class, the set_up() method is incorrect. No assertions should be executed in fixture methods, but the set_up() method contains two assertions. This has not been addressed yet as it is outside the scope of this commit, but should be addressed at a later point in time.

Follow-up to [12/tests], [218/tests], [374/tests], [384/tests], [986/tests], [1106/tests], [1239/tests], [28704], [29221], [29347], [32648], [36519], [37953], [38832], [40142], [40825], [43584], [43768], [44786], [45141], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945].

Props jrf.
See #56033.

SergeyBiryukov commented on PR #2867:


2 years ago
#88

Thanks for the PR! Merged in r53948.

jrfnl commented on PR #2867:


2 years ago
#89

Thanks @SergeyBiryukov !

#90 @SergeyBiryukov
2 years ago

In 53949:

Code Modernization: Explicitly declare all properties in WP_Test_Stream.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

The WP_Test_Stream class is a stream wrapper for use in the tests and must comply with the PHP requirements for such stream wrappers.

In this case, the class did not declare the required public $context property, which led to deprecation notices about the property being dynamically created from the Tests_Image_Editor_Imagick::test_streams() and Tests_Image_Meta::test_stream() tests.

Reference: PHP Manual: streamWrapper: Properties.

Follow-up to [49230], [50771], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948].

Props jrf, antonvlasenko.
See #56033.

SergeyBiryukov commented on PR #2868:


2 years ago
#91

Thanks for the PR! Merged in r53949.

#92 @SergeyBiryukov
2 years ago

In 53952:

Code Modernization: Explicitly declare all properties in WP_Ajax_Upgrader_Skin.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

In this case, the $plugin_info and $theme_info properties are set in Plugin_Upgrader::bulk_upgrade() and Theme_Upgrader::bulk_upgrade() specifically.

The Bulk_Plugin_Upgrader_Skin class and the Bulk_Theme_Upgrader_Skin class both already allow for this, but the wp_ajax_update_plugin() and wp_ajax_update_theme() functions also call the *_Upgrader::bulk_upgrade() methods, so the WP_Ajax_Upgrader_Skin class also needs to have these properties explicitly declared.

Includes adding proper DocBlocks for the pre-existing properties in the Bulk_Plugin_Upgrader_Skin and the Bulk_Theme_Upgrader_Skin classes.

Follow-up to [13686], [37714], [38199], [42677], [42873], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948], [53949].

Props jrf, costdev.
See #56033.

SergeyBiryukov commented on PR #2869:


2 years ago
#93

Thanks for the PR! Merged in r53952.

SergeyBiryukov commented on PR #2869:


2 years ago
#94

Note: I have removed the code tags (backticks) from function and method names in the DocBlocks, so that they can be automatically linked on the developer.wordpress.org reference, see r53876.

jrfnl commented on PR #2869:


2 years ago
#95

Thanks @SergeyBiryukov! And yes, using those backticks is a habit which is useful everywhere but here ;-)

#96 @SergeyBiryukov
2 years ago

In 53953:

Code Modernization: Explicitly declare WP-specific property in WP_SimplePie_File.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it is an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set(), et al. methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods built in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $timeout property falls in the “known property” category.

Note: In contrast to the other properties being set in the constructor, this property is not declared on the parent class, so not inherited.

Reference: PHP RFC: Deprecate dynamic properties.

Follow-up to [10687], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948], [53949], [53952].

Props jrf, antonvlasenko, costdev.
See #56033.

SergeyBiryukov commented on PR #2870:


2 years ago
#97

Thanks for the PR! Merged in r53953.

#98 @SergeyBiryukov
2 years ago

In 53954:

Code Modernization: Explicitly declare all properties in POMO_Reader et al.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it is an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set(), et al. methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods built in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $is_overloaded property and the $_f property fall in the “known property” category. In both cases, these are being set in the __construct() method of the class they apply to.

The $_post property appears to be a typo however. The $_post property looks to be unused, while there is an undeclared integer $_pos (“position”) property, which is used throughout the class and used by the child classes.

Reference: PHP RFC: Deprecate dynamic properties.

Follow-up to [11626], [12174], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948], [53949], [53952], [53953].

Props jrf, antonvlasenko, costdev.
See #56033.

SergeyBiryukov commented on PR #2871:


2 years ago
#99

Thanks for the PR! Merged in r53954.

#100 @SergeyBiryukov
2 years ago

In 53957:

Code Modernization: Explicitly declare all properties in Gettext_Translations.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it is an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set(), et al. methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods built in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $_nplurals and $_gettext_select_plural_form properties, both being set in the class constructor, fall in the “known property” category.

Reference: PHP RFC: Deprecate dynamic properties.

Follow-up to [10584], [12079], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948], [53949], [53952], [53953], [53954].

Props jrf, antonvlasenko, costdev.
See #56033.

SergeyBiryukov commented on PR #2872:


2 years ago
#101

Thanks for the PR! Merged in r53957.

This ticket was mentioned in Slack in #core-committers by sergey. View the logs.


2 years ago

#103 @SergeyBiryukov
2 years ago

In 54037:

Code Modernization: Explicitly declare all properties in Services_JSON.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it is an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set(), et al. methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods built in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the $use property, as set in the class constructor, falls in the “known property” category.

Reference: PHP RFC: Deprecate dynamic properties.

Follow-up to [11875], [53557], [53558], [53850], [53851], [53852], [53853], [53854], [53856], [53916], [53935], [53936], [53937], [53938], [53942], [53945], [53948], [53949], [53952], [53953], [53954], [53957].

Props jrf, antonvlasenko, costdev.
See #56033.

SergeyBiryukov commented on PR #2873:


2 years ago
#104

Thanks for the PR! Merged in r54037.

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#107 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

I believe this is done for 6.1. With 6.1 RC1 today, closing this ticket.

Thank you everyone for your contributions!

This ticket was mentioned in PR #3777 on WordPress/wordpress-develop by @bjorsch.


2 years ago
#108

The IXR_Message class declares a property _customTag, which is never assigned or used. It does assign to customTag, which outside of that one assignment is never used either. A search on wpdirectory didn't turn up anything using either property (but lots of results for other classes having a property of the same name).

Since there are various other underscore-prefixed properties declared on the class, including one named _currentTagContents which is used in several places, it seems likely the declared property is correct and the assignment is a typo.

Trac ticket: https://core.trac.wordpress.org/ticket/56033
Trac ticket: https://core.trac.wordpress.org/ticket/56009

@bjorsch commented on PR #3777:


2 years ago
#109

Added a test, as requested.

@bjorsch commented on PR #3777:


23 months ago
#110

So several people have approved this, what needs to be done for it to be committed?

@bjorsch commented on PR #3777:


23 months ago
#111

So several people have approved this, what needs to be done for it to be committed?

@SergeyBiryukov commented on PR #3777:


23 months ago
#112

Thanks for the PR! Merged in r55105.

This ticket was mentioned in Slack in #core by drew. View the logs.


7 weeks ago

Note: See TracTickets for help on using tickets.