WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 months ago

Last modified 2 months ago

#49922 closed task (blessed) (fixed)

PHP Compatibility fixes for 5.5

Reported by: desrosj Owned by: desrosj
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.

Previously: #46152, #48966.
Related: #49919.

Attachments (4)

49922.diff (7.3 KB) - added by desrosj 5 months ago.
49922.2.diff (8.5 KB) - added by desrosj 5 months ago.
49922.3.diff (1.9 KB) - added by desrosj 5 months ago.
49922.4.diff (8.7 KB) - added by desrosj 4 months ago.

Download all attachments as: .zip

Change History (28)

#1 @desrosj
5 months ago

This is the current result of running composer run compat locally on trunk:

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
src/xmlrpc.php                                        5       0
src/wp-admin/includes/class-ftp.php                   0       1
src/wp-admin/includes/schema.php                      0       1
src/wp-includes/capabilities.php                      1       0
src/wp-includes/class-phpmailer.php                   5       4
src/wp-includes/class-pop3.php                        0       2
src/wp-includes/class-simplepie.php                   0       2
src/wp-includes/class-snoopy.php                      0       14
src/wp-includes/cron.php                              0       1
src/wp-includes/functions.php                         0       1
src/wp-includes/load.php                              0       1
src/wp-includes/plugin.php                            0       3
src/wp-includes/rss.php                               0       2
src/wp-includes/ID3/getid3.lib.php                    1       0
src/wp-includes/ID3/getid3.php                        0       4
src/wp-includes/ID3/module.audio-video.quicktime.php  1       0
src/wp-includes/IXR/class-IXR-server.php              3       0
src/wp-includes/pomo/streams.php                      0       1
src/wp-includes/rest-api/class-wp-rest-server.php     4       0
src/wp-includes/SimplePie/File.php                    0       1
src/wp-includes/SimplePie/Item.php                    0       1
...-includes/sodium_compat/lib/php72compat_const.php  15      0
src/wp-includes/sodium_compat/src/Compat.php          3       0
----------------------------------------------------------------------
A TOTAL OF 38 ERRORS AND 39 WARNINGS WERE FOUND IN 23 FILES
----------------------------------------------------------------------

Time: 372ms; Memory: 10MB


PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
PHPCompatibility.FunctionUse.RemovedFunctions.eachDeprecated     14
PHPCompatibility.Variables.RemovedPredefinedGlobalVariables.htt  12
PHPCompatibility.IniDirectives.RemovedIniDirectives.safe_modeDe  8
PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValu  5
PHPCompatibility.IniDirectives.RemovedIniDirectives.mbstring_fu  5
PHPCompatibility.FunctionUse.RemovedFunctions.set_magic_quotes_  2
PHPCompatibility.IniDirectives.RemovedIniDirectives.magic_quote  2
PHPCompatibility.IniDirectives.RemovedIniDirectives.zend_ze1_co  2
PHPCompatibility.Lists.AssignmentOrder.Affected                  2
PHPCompatibility.Variables.RemovedPredefinedGlobalVariables.php  2
PHPCompatibility.Constants.NewConstants.php_int_minFound         1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kdf_bytes  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kdf_bytes  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kdf_conte  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kdf_keyby  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kx_keypai  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_kx_sessio  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_crypto_pwhash_sc  1
PHPCompatibility.Constants.NewConstants.sodium_library_major_ve  1
PHPCompatibility.Constants.NewConstants.sodium_library_minor_ve  1
PHPCompatibility.Constants.NewConstants.sodium_library_versionF  1
PHPCompatibility.FunctionUse.NewFunctions.sodium_crypto_sign_ke  1
PHPCompatibility.FunctionUse.NewFunctions.sodium_padFound        1
PHPCompatibility.FunctionUse.NewFunctions.sodium_unpadFound      1
PHPCompatibility.FunctionUse.RemovedFunctions.dlDeprecated       1
PHPCompatibility.FunctionUse.RemovedFunctions.get_magic_quotes_  1
PHPCompatibility.IniDirectives.RemovedIniDirectives.register_gl  1
PHPCompatibility.ParameterValues.NewIDNVariantDefault.NotSet     1
----------------------------------------------------------------------
A TOTAL OF 77 SNIFF VIOLATIONS WERE FOUND IN 33 SOURCES
----------------------------------------------------------------------

#2 @desrosj
5 months ago

Related: #49938.

#3 @desrosj
5 months ago

Opened a pull request upstream to remove old PHP directives in SimplePie.

@desrosj
5 months ago

@desrosj
5 months ago

#4 @desrosj
5 months 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.

#5 @desrosj
5 months ago

Looks like there was a ticket for $HTTP_RAW_POST_DATA already: #49810.

#6 @desrosj
5 months ago

In 47735:

General: Fix various issues flagged by the PHPCompatibilityWP PHPCS ruleset.

As part of the continued effort to improve PHP compatibility, the following improvments are being made:

  • Removing deprecated PHP safe_mode checks not found in bundled external libraries.
  • Change the remaining while loops using each() to foreach loops.
  • Prevent false positives from being flagged for the sodium_compat library being caused by loading this in a non-standard way.
  • Add inline comments to not flag deprecated PHP directives in the getID3 library.

Props desrosj, earnjam, dryanpress.
See #49922.

#7 @desrosj
5 months ago

In 47736:

General: Fix typo within phpcs:ignore inline comment.

Follow up of [47735].

See #49922.

#8 @desrosj
5 months ago

In 47737:

General: More PHP compatibility adjustments.

In this round:

  • Removed references to safe_mode in class-php3.php. This was removed in PHP 5.4.
  • Add inline exclude comments for compatibility checks in getID3.

Follow up of [47735-47736].

See #49922.

@desrosj
5 months ago

#9 follow-ups: @desrosj
5 months 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 in wp-includes/capabilities.php that needs to be very carefully reviewed, and one in getID3.
  • 2 occurrences of php_errormsg in wp-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 of mbstring.func_overload).
  • 2 occurrences of mbstring.func_overload.
  • 1 occurrence of debug_backtrace() in class-simplepie.php that 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 if statements 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_overload in 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 to mbstring.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 @johnbillion
4 months ago

Replying to desrosj:

2 occurrences of list(): 1 in wp-includes/capabilities.php that 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 @johnbillion
4 months ago

Replying to desrosj:

2 occurrences of php_errormsg in wp-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.

Last edited 4 months ago by johnbillion (previous) (diff)

#12 @jrf
4 months 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 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.

This ticket was mentioned in PR #307 on WordPress/wordpress-develop by desrosj.


4 months ago

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:

@desrosj
4 months ago

#14 @desrosj
4 months 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.

#15 @desrosj
4 months ago

In 47902:

General: Continuing to work towards a passing PHP Compatibility scan.

This is a final pass to fix PHP compatibiilty issues in the codebase with code changes or adding phpcs:ignore comments.

With this change, all PHP compatibility warnings and errors without specific tickets have been addressed (see #49810 and #41750).

Props desrosj, johnbillion, jrf.
See #49922.

#16 @desrosj
4 months 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 @jrf
4 months 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');

#19 @desrosj
3 months ago

In 47926:

General: Remove or add inline comments to $HTTP_RAW_POST_DATA occurrences.

The $HTTP_RAW_POST_DATA global was deprecated in PHP 5.6 and removed completely in PHP 7.0. In general, php://input should be used instead of $HTTP_RAW_POST_DATA.

Because WordPress Core still supports PHP 5.6, some plugins or sites may still rely on this variable being present and populated with the expected data. For that reason, occurrences of the variable will remain with updated inline documentation until support for PHP 5.6 is officially dropped in WordPress.

Props skoskie, jrf, desrosj, TimothyBlynJacobs.
See #49922.
Fixes #49810.

#20 @desrosj
3 months ago

In 48045:

General: Continuing to work towards a passing PHP Compatibility scan.

  • Add phpcs:ignore statements to compatibility checks in PHPMailer.
  • Remove quotes around the assertion in an assert() call. This will trigger a deprecated notice under certain conditions on PHP 7.2.

Props jrf, desrosj.
See #49922, #48033.

#21 @desrosj
3 months ago

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

In 48046:

Coding Standards: Remove the PHP Compatibility scanning job from the allowed_failures list.

All pre-existing compatibility warnings and errors as flagged by the PHPCompatibilityWP ruleset have been addressed.

Fixes #49922.

#22 @jrf
3 months ago

@desrosj 🎉🎉

P.S.: the ignore comment on line 1404 of PHPMailer is unnecessary (the parameter is set).

#23 @desrosj
3 months ago

  • Keywords needs-dev-note added

#24 @desrosj
2 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.