#56033 closed task (blessed) (fixed)
PHP 8.2: explicitly declare all known properties
Reported by: | jrf | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch php82 has-unit-tests |
Focuses: | Cc: |
Description (last modified by )
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 ?
- When the property is explicitly declared on the class.
- When the property is explicitly declared on the (grand)parent class.
When are dynamic properties not problematic ?
- 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; } }
- 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)
This ticket was mentioned in PR #2843 on WordPress/wordpress-develop by jrfnl.
2 years ago
#2
- Keywords has-unit-tests added
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
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 thetear_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:
- Change the
scandir()
method to return an array of the found directories. Thedelete_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 avoid
method and sets the property.
- 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 aprivate
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 ofnull
, same as it was already being reset tonull
in thetear_down()
method. - In
Tests_Privacy_wpPrivacyProcessPersonalDataExportPage
and theTests_Privacy_wpPrivacyGeneratePersonalDataExportFile
classes, the property name had a leading_
underscore. This is an outdated PHP 4 practice to indicate aprivate
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 theset_up()
method, while three others were being set for most tests via theshortcode_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 theset_up()
method and are explicitly declared.
Additionally:
- In the
Tests_User_Session
class, theset_up()
method is incorrect. No assertions should be executed in fixture methods, but theset_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
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
SergeyBiryukov commented on PR #2843:
2 years ago
#23
Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/53557.
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 extendstdClass
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
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 extendstdClass
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 extendstdClass
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 extendstdClass
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
@
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.
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.
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
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:
- Change the
scandir()
method to return an array of the found directories. Thedelete_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 avoid
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.
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 toTests_WP_Customize_[Panel|Section]
in r32658.
SergeyBiryukov commented on PR #2846:
2 years ago
#41
Merged in r53851.
SergeyBiryukov commented on PR #2847:
2 years ago
#43
Thanks for the PR! Merged in r53852.
SergeyBiryukov commented on PR #2848:
2 years ago
#45
Thanks for the PR! Merged in r53853.
SergeyBiryukov commented on PR #2849:
2 years ago
#47
Thanks for the PR! Merged in r53854.
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.
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 indelete_folders()
, so we don't really need the property ifscandir()
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.
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.
SergeyBiryukov commented on PR #2850:
2 years ago
#56
Thanks for the PR! Merged in r53856.
SergeyBiryukov commented on PR #2851:
2 years ago
#59
Thanks for the PR! Merged in r53916.
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:
- Make sure you are on PHP 8.2.
- Instantiate an object of a
Services_JSON()
class:
{{{php
ini_set('error_reporting', E_ALL);
new Services_JSON();
}}}
- ❌ 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.
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 theServices_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.
SergeyBiryukov commented on PR #2861:
2 years ago
#64
Thanks for the PR! Merged in r53935.
SergeyBiryukov commented on PR #2862:
2 years ago
#66
Thanks for the PR! Merged in r53936.
SergeyBiryukov commented on PR #2863:
2 years ago
#70
Thanks for the PR! Merged in r53937.
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:
- Make sure you are on PHP 8.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');
}}}
- ❌ 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:
- Make sure you are on PHP 8.2.
- Instantiate an object of a
POMO_FileReader()
class:
{{{php
ini_set('error_reporting', E_ALL);
$translations = new POMO_FileReader(FILE);
}}}
- ❌ 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:
- Make sure you are on PHP 8.2.
- Instantiate an object of a
WP_SimplePie_File
class:
{{{php
ini_set('error_reporting', E_ALL);
new WP_SimplePie_File('http://test.file');
}}}
- ❌ 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
2 years ago
#77
Note: to see the failing tests (without this patch) on PHP 8.2, run the tests with phpunit --group ajax
.
2 years ago
#78
Note: to see the failing tests (without this patch) on PHP 8.2, run the tests with phpunit --group ajax
.
SergeyBiryukov commented on PR #2865:
2 years ago
#80
Thanks for the PR! Merged in r53942.
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:
- Make sure you are on PHP 8.2.
- Make sure
Imagick
extension is enabled. - Run the
Tests_Image_Meta::test_stream
test method.
{{{bash
php vendor/bin/phpunit -c phpunit.xml.dist --filter test_stream
}}}
- ❌ 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:
- Make sure you are on PHP 8.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
}}}
- ❌ 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.
SergeyBiryukov commented on PR #2866:
2 years ago
#85
Thanks for the PR! Merged in r53945.
SergeyBiryukov commented on PR #2867:
2 years ago
#88
Thanks for the PR! Merged in r53948.
SergeyBiryukov commented on PR #2868:
2 years ago
#91
Thanks for the PR! Merged in r53949.
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.
2 years ago
#95
Thanks @SergeyBiryukov! And yes, using those backticks is a habit which is useful everywhere but here ;-)
SergeyBiryukov commented on PR #2870:
2 years ago
#97
Thanks for the PR! Merged in r53953.
SergeyBiryukov commented on PR #2871:
2 years ago
#99
Thanks for the PR! Merged in r53954.
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
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
@
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
23 months ago
#110
So several people have approved this, what needs to be done for it to be committed?
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.
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