WordPress / wordpress-importer

The WordPress Importer
https://wordpress.org/plugins/wordpress-importer/
GNU General Public License v2.0
78 stars 76 forks source link

PHP 8.2 | Explicitly declare all properties #128

Closed jrfnl closed 2 years ago

jrfnl commented 2 years ago

Dynamic (non-explicitly declared) property usage is expected to be 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:

Refs:

WXR_Parser_XML: explicitly declare all properties [1]

In this case, the parse() method explicitly sets each of those properties, so these fall in the "known property" category and should be explicitly declared.

WXR_Parser_XML: explicitly declare all properties [2]

In this case, the tag_close() method explicitly sets each of those properties, so these fall in the "known property" category and should be explicitly declared.

WXR_Parser_Regex: explicitly declare all properties

In this case, the __construct() method explicitly sets this property, so $has_gzip falls in the "known property" category and should be explicitly declared.

jrfnl commented 2 years ago

P.S.: not claiming completeness with this PR, but these were the obvious ones I found straight off. These may be more to find though.

jrfnl commented 2 years ago

N.B.: The build failure against PHP 8.1 is unrelated to this PR.

jrfnl commented 2 years ago

As an initial patch for this, this seems safe to merge.

I do ponder if we should explicitly set default values for readability, even though the code would ignore that though? for example, setting has_gzip to false by default.

I wondered the exact same thing, but as the value would be overwritten anyway, I figured it wasn't worth spending the time to determine what the defaults should be.

jrfnl commented 2 years ago

I don't think adding these makes a huge difference, as they'll all be overridden.. but I'll leave it up to you :)

Well, the current patch does not break BC, while adding defaults potentially could break BC, as a check with isset() for one of these properties will no longer return false.

As the test suite for this plugin is minimal, I would prefer to err on the side of caution and not run the risk of breaking BC.

jrfnl commented 2 years ago

Note; not breaking BC is also the reason why I made these properties all public, even though, purely on their (known) use/merit, each of these properties should be private or at most protected.

dd32 commented 2 years ago

Well, the current patch does not break BC, while adding defaults potentially could break BC

While I doubt there would be any BC issues, it ultimately makes almost no difference here to skip documenting these. Merged as-is.