#49922 closed task (blessed) (fixed)
PHP Compatibility fixes for 5.5
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 5.5 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | General | Keywords: | has-patch has-dev-note |
| Focuses: | coding-standards | Cc: |
Description
Previously in #46152, a job was added to Travis builds to run the PHPCompatibilityWP ruleset to detect potential compatibility issues with supported versions of PHP.
This job was very helpful identifying PHP 7.4 compatibility issues so that they could be fixed prior to support being added in 5.3. However, there are still some warnings and errors being reported.
Tickets are being created for the more controversial items (see #49919), but each of the other issues reported need to be investigated individually to determine if the ruleset is reporting a false positive or a compatibility issue that actually needs to be corrected.
This ticket is for handling these remaining issues in order to get the compatibility job passing so that it can be used to identify newly introduced compatibility issues.
Attachments (4)
Change History (28)
#3
@
4 years ago
Opened a pull request upstream to remove old PHP directives in SimplePie.
#4
@
4 years ago
- Keywords has-patch needs-testing added
49922.diff addresses a large handful of the remaining issues and when excluding errors/warnings flagged in bundled external libraries (explained below) brings the total number of flagged compatibility issues down to around 20.
Some notes:
I have been ignoring external libraries unless they are abandoned and WordPress now maintains its own version. Most of the flagged issues are fixed upstream.
safe_mode: The instances still being flagged for this are in external libraries, all of which have been removed upstream. These will get fixed by other tickets updating the libraries to the latest versions:
There were also instances in the getID3 library that I added exclusions for. The library supports PHP 5.3.0. Because safe_mode was deprecated in 5.3.0 and removed in 5.4.0, it cannot be removed upstream yet.
$php_errormsg: This can be found only in wp-includes/rss.php, which is MagpieRSS, an outdated and deprecated library. This was deprecated as of PHP 7.2.0. I think that the if() statement in question could either be removed, or changed to use error_get_last(), but I also think that it could just receive an inline comment just in case sites with PHP < 7.2.0 are using this.
I'll be opening a new ticket to remove references to the $HTTP_RAW_POST_DATA global variable just to give that a bit more visibility for feedback.
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.
mbstring.func_overload usage also still needs to be addressed.
#9
follow-ups:
↓ 10
↓ 11
@
4 years ago
- Keywords needs-testing removed
After [47735-47737], there are now only 33 warnings/errors remaining. They breakdown as follows:
- 12 occurrences of
$HTTP_RAW_POST_DATA(all in Core code) - see #49810. - 4 occurrences of
func_get_args()- see ##47678. - 2 occurrences of
list(): 1 inwp-includes/capabilities.phpthat needs to be very carefully reviewed, and one in getID3. - 2 occurrences of
php_errormsginwp-includes/rss.php. - 9 issues in PHPMailer. All but two will be fixed by the library update in #41750 (one
idn_to_ascii()call and one occurrence ofmbstring.func_overload). - 2 occurrences of
mbstring.func_overload. - 1 occurrence of
debug_backtrace()inclass-simplepie.phpthat needs to be examined. - 1 occurrence of
dl().
For the issues in the getID3 library, I decided to use inline comments to exclude the lines in question for a few reasons:
- The library still supports PHP 5.3, so there are some snippets that the library must keep because of the conflicting support policies.
- All of the issues flagged have fallbacks for more modern versions of PHP.
- Some of the issues present are
ifstatements informing developers that they shouldn't use something (which is good).
I also though through the upgrade process for that library. Right now, it is wholesale copied over to core unmodified. The next time that this happens, the compatibility check job will be passing and removed from the allowed_failures list. The removal of the inline comments will cause the job to fail if they are not re-added. This is a good thing, in my opinion, as it prompts contributors to look at why. Each time that this happens, we can check the libraries support policy upstream and submit PRs to remove the code in question if appropriate.
In 49922.3.diff:
- Ignore two occurrences of
mbstring.func_overloadin core functionality. Because this was not deprecated until PHP 7.2, these likely needs to remain for a bit. - Ignore an occurrence of
dl(). This was not removed from PHP-FPM until 7.0.0, so is a similar situation tombstring.func_overload.
I also wanted to note a great nice to have after the script is passing: the script should be added as a part of the grunt precommit command to help committers ensure they are not introducing compatibility issues before making commits.
#10
in reply to:
↑ 9
@
4 years ago
Replying to desrosj:
2 occurrences of
list(): 1 inwp-includes/capabilities.phpthat needs to be very carefully reviewed
This code is simpler than it looks. It's plucking the object type from the hardcoded list of string expressions for its switch condition and assigning it to the $object_type variable. The $_ variable is not used.
This list() call can be replaced with $object_type = explode( '_', $cap )[1].
#11
in reply to:
↑ 9
@
4 years ago
Replying to desrosj:
2 occurrences of
php_errormsginwp-includes/rss.php.
The $php_errormsg variable is set within the current scope after an error occurs. Given this check is on the first line of the function, and therefore the first line of the scope, I can't see how this variable will ever exist at this point. It looks safe to remove both instances of the handling of $php_errormsg here.
Edit: Note that the definition of RSSCache::debug() is indented incorrectly which might confuse you for a few moments when following the code flow here.
#12
@
4 years ago
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 L234wp-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.Affectedissue (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
$ignored1and$ignored2would 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/XgKCrwp-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 appropriatefunction_exists()andis_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
usestatements 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.
This ticket was mentioned in PR #307 on WordPress/wordpress-develop by desrosj.
4 years ago
#13
This fixes the remaining PHPCS issues related to PHP compatibility scanning except for the following (which have specific tickets):
- The last remaining warnings in the SimplePie external library
- The usage of
$HTTP_RAW_POST_DATA
Trac ticket: https://core.trac.wordpress.org/ticket/49922
Trac ticket:
#14
@
4 years ago
- Keywords commit added
As always, many thanks @jrf for your fantastic and detailed review! It is always appreciated.
49922.4.diff addresses all remaining issues except the ones being tackled separately in #49810 and #41750. I reviewed all of the ignore statements added so far and none of them would cause a fatal error due to a function being removed in newer versions of PHP currently. It's possible this could change in PHP 8, but we can address that once it's officially in beta/RC.
I did not address your comment about adding a cache directive. I am going to follow up over on #49783 so that is tackled at the same time.
Also, thanks for detailing the rough plan and high level items for 10.0.0! It seems we'll definitely need a new ticket to explore properly supporting once everything is finalized, but it seems it should not be a large undertaking.
#16
@
4 years ago
- Keywords commit removed
Leaving this open for now in case additional compatibility warnings/errors appear, and to move the build job out of the allowed_failures list once the remaining issues are addressed.
#17
@
4 years ago
@desrosj I've reviewed the patch and all looks good (and while I'm typing this I notice you've just committed it ;-) ).
One side-note which is not specific to this ticket: I notice that you generally use trailing comments for the // phpcs:ignore comments.
As this makes for very long lines, from a readability perspective using the "ignore comment on its own line" pattern may be better.
When an ignore comment is on its own line, it will ignore that line and the line after, so functionality-wise they are equivalent.
I.e. both the below have the same effect, but IMO the second one is better from a readability perspective, especially when the line being ignored is long.
<?php eval('1+2'); // phpcs:ignore Squiz.PHP.Eval
<?php // phpcs:ignore Squiz.PHP.Eval eval('1+2');
4 years ago
#18
This was committed in https://core.trac.wordpress.org/changeset/47902.
#22
@
4 years ago
@desrosj 🎉🎉
P.S.: the ignore comment on line 1404 of PHPMailer is unnecessary (the parameter is set).
#24
@
4 years ago
- Keywords has-dev-note added; needs-dev-note removed
Detailed in the following dev note: https://make.wordpress.org/core/2020/07/14/php-related-improvements-changes-wordpress-5-5-edition/.
This is the current result of running
composer run compatlocally ontrunk: