[wp-trac] [WordPress Trac] #49922: PHP Compatibility fixes for 5.5
WordPress Trac
noreply at wordpress.org
Tue Jun 2 05:07:59 UTC 2020
#49922: PHP Compatibility fixes for 5.5
----------------------------+-------------------------------
Reporter: desrosj | Owner: desrosj
Type: task (blessed) | Status: assigned
Priority: normal | Milestone: 5.5
Component: General | Version:
Severity: normal | Resolution:
Keywords: has-patch | Focuses: coding-standards
----------------------------+-------------------------------
Comment (by jrf):
At the request of @desrosj I've had a detailed look at this ticket and the
remaining issues.
I've reviewed the three patches already committed. All good 👍
== PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue
> There are flagged 4 occurrences of `func_get_args()`. #47678 moved to
utilizing the spread operator for these instead, but these appear to have
been missed or intentionally excluded. @jrf are you able to clarify? If
they were intentionally skipped, we should just add inline comments to
whitelist these.
Anything not addressed by #47678 had a reason which will be outlined
somewhere in that ticket, though I realize it's a long read.
I suspect that the only remaining ones are "warnings" where the code is
too complex for static analysis to be sure and needs inspection by a dev.
Either way, I've taken a quick look at the currently remaining issues now:
* `wp-includes/class-simplepie.php:L3168`/`debug_backtrace()`: this
warning can be safely ignored, passed parameters have been ''used'', not
''changed''.
* `wp-includes/cron.php:L405`/`func_get_args()`: this warning can be
safely ignored, passed parameters have not been changed. Wasn't changed in
#47678 as it is a BC layer for a deprecated way of passing the arguments,
so nothing to address.
* `wp-includes/plugin.php:L234`/`func_get_args()`: this warning can be
safely ignored, passed parameters have not been changed. Wasn't changed in
#47678 because this code allows for calling the function for different
uses and therefore can't be changed.
* `wp-includes/plugin.php:L456`/`func_get_args()`: dito to L234
* `wp-includes/plugin.php:L529`/`func_get_args()`: dito to L234
== PHPCompatibility.Lists.AssignmentOrder.Affected
`wp-includes/capabilities.php:L293`:
> This list() call can be replaced with $object_type = explode( '_', $cap
)[1].
Agreed or alternatively, the solution I posted previously in
https://core.trac.wordpress.org/ticket/46152#comment:3 can be used:
> As another example, the
`PHPCompatibility.Lists.AssignmentOrder.Affected` issue (if it's the same
issue as when I wrote the sniff) is not a false positive, however the
assigned variables are not used in the function, so this isn't an issue
either.
>
> From a self-documenting code perspective, just renaming the duplicate
variables to `$ignored1` and `$ignored2` would solve the flag and make it
more obvious for the next person looking at the code to get what's going
on.
Same goes for the same issue using duplicate `$dummy` variables in `wp-
includes/ID3/module.audio-video.quicktime.php:L117`, though as that is an
external dependency still maintained, whitelisting the issue in that file
is also perfectly fine.
==
PHPCompatibility.Variables.RemovedPredefinedGlobalVariables.php_errormsgDeprecated
MagPie dependency: I'm not sure if this is a dependency maintained
externally or abandoned and now maintained by WP.
* `wp-includes/rss.php:L389`: agreed with @johnbillion - this is nonsense
code as the variable won't be defined in that scope.
* `wp-includes/rss.php:L825`: dito, also nonsense code.
From the PHP manual:
> This variable will only be available within the scope in which the error
occurred, and only if the track_errors configuration option is turned on
(it defaults to off).
Ref: https://www.php.net/manual/en/reserved.variables.phperrormsg.php
==
PHPCompatibility.Variables.RemovedPredefinedGlobalVariables.http_raw_post_dataDeprecatedRemoved
See: https://core.trac.wordpress.org/ticket/49810#comment:10 and
https://github.com/WordPress/wordpress-develop/pull/305
==
PHPCompatibility.IniDirectives.RemovedIniDirectives.mbstring_func_overloadDeprecated
* `wp-includes/functions.php:L6630`: this can be safely
ignored/whitelisted. Proof: https://3v4l.org/XgKCr
* `wp-includes/pomo/streams.php:L21`: same thing
`ini_get()` will return `false` if an ini directive hasn't been set or
doesn't exist and with it being `false`, the condition will not be met, so
we're ok.
Ref: https://www.php.net/manual/en/function.ini-get.php
== PHPCompatibility.FunctionUse.RemovedFunctions.dlDeprecated
* `wp-admin/includes/class-ftp.php:L902` - this can be safely ignored. The
function call is wrapped in appropriate `function_exists()` and
`is_callable()` checks.
== PHPCompatibility.Variables.NewUniformVariableSyntax
The current PHPCompatibility bleeding edge identified two new issues
regarding cross-version compatibility for a change introduced in PHP 7.0.
The issues are both in the `Requests` dependency. I have pulled a fix
there. Unfortunately, that repo is largely abandoned and hasn't seen any
new releases for years. The fix pulled there, however, can be applied one-
on-one in WP Core.
See: https://github.com/rmccue/Requests/pull/386
== PHPCompatibility.ParameterValues.RemovedAssertStringAssertion
Another newly detected issue from PHPCompatibility bleeding edge.
{{{
FILE: src/wp-includes/Text/Diff/Engine/shell.php
----------------------------------------------------------------------------------------------------------------------------
86 | ERROR | Using a string as the assertion passed to assert() is
deprecated since PHP 7.2 and removed since PHP 8.0.
| | Found: '$match[1] - $from_line_no == $match[4] -
$to_line_no'
----------------------------------------------------------------------------------------------------------------------------
}}}
I'm not sure if this is a dependency maintained externally or abandoned
and now maintained by WP, but this does need to be changed.
The change needed is simple: just remove the string quotes around the
code.
{{{
-assert('$match[1] - $from_line_no == $match[4] - $to_line_no');
+assert($match[1] - $from_line_no == $match[4] - $to_line_no);
}}}
Proof of that this fixes it and still maintains the current functionality:
https://3v4l.org/XmX5H
== Caveat / PHPMailer
I've not looked at PHPMailer as I fully trust that an upgrade of the
library will address any issues in that library and if not, that the
maintainer will be responsive.
== PHPCS ignore comments
Regarding ignore statement for some deprecated functions: please be very
wary if those aren't also accompanied by a `function_exists()` or similar
check, as a lot of deprecated functionality will be (has been) removed in
PHP 8.0, so these may cause problems again soon enough.
In a lot of cases, the error code used by PHPCompatibility will change is
something is removed vs previously deprecated, so that should invalidate
the ignore comments automatically, but I can't guarantee this for every
sniff.
== Ruleset [1]
You may want to change the `cache` directive to point to a fixed file name
and allow the file to be cached by Travis.
See #49783 for the details. Props @johnbillion for taking the initiative
for this.
== Ruleset [2]
{{{
<rule ref="PHPCompatibility.Extensions.RemovedExtensions">
<exclude-pattern>/src/wp-includes/wp-db\.php</exclude-
pattern>
</rule>
}}}
Please make this exclusion specific to the MySQL extension for which it is
intended as any other code related to removed extensions should still be
flagged.
The change needed is this:
{{{
<rule
ref="PHPCompatibility.Extensions.RemovedExtensions.mysql_DeprecatedRemoved">
<exclude-pattern>/src/wp-includes/wp-db\.php</exclude-
pattern>
</rule>
}}}
== Heads-up for the future:
I'm currently hard at work on getting PHPCompatibility 10.0.0 ready, which
will contain some big improvements in accuracy, but also some breaking
changes.
PHPCompatibility 10.0.0 is expected to be released (fingers crossed)
within the next two months and will be accompanied by a new release of
PHPCompatibilityWP (3.0.0), which will account for anything in that
ruleset that needs to change for PHPCompatibility 10.0.0.
Some highlights regarding PHPCompatibility 10.0.0:
* Much more accurate checking for anything coming from new/removed PHP
extensions.
* Some initial checks for PHP 8.0 compatibility.
* Improved handling of modern PHP code, including `use` statements
resolution and such, either in this version or in one of the soon to
follow minors.
* And of course plenty of other improvements to the checks and bugfixes.
If you like, have a look at the milestone to get some idea of what's being
worked on:
https://github.com/PHPCompatibility/PHPCompatibility/milestone/26
Some of the breaking changes will affect WordPress for the excludes used
in the ignore comments as well as in the ruleset. Most notably, the
`RemovedExtensions` sniff will be removed and replaced by checks for all
functionality the extensions offered with individual error codes.
See https://github.com/PHPCompatibility/PHPCompatibility/issues/1022 for
full details.
This will affect the ignoring of the issues around the use of the `MySQL`
extension and would currently mean having to ignore approx 20 different
error codes and/or whitelisting 29 issues inline using the new codes.
The changes in PHPCompatibility for that issue haven't been finalized yet
though, so I'll have a think about allowing a way to ignore all issues
coming from a certain extension for a certain sniff for select files, but
am not making any promises.
== Bleeding edge scanning:
As we're currently in the middle of pulling a lot of changes, it is
difficult for me to do a proper scan with PHPCompatibility 10.0.0 at the
moment without doing tons of rebasing of WIP branches.
Hopefully we'll be through this round of pulls somewhere in the next two
weeks. Once we've reached that point, I'd be happy to do a proper rescan
with bleeding edge to give you some insight on what new issues
PHPCompatibility 10.0.0 will start throwing up.
As things stand at the moment, it would report 65 violations from 32
sources in 18 files, including the issues discussed above.
--
Ticket URL: <https://core.trac.wordpress.org/ticket/49922#comment:12>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list