Add PHPCompatibility checks to test suite

[desrosj]

Opening this ticket to explore adding the ​PHPCompatibility/PHPCompatibilityWP ruleset to the test suite when PHPCS is run.

From the repository's page:

A ruleset for PHP_CodeSniffer to check for PHP cross-version compatibility issues in projects based on the WordPress CMS.
This WordPress specific ruleset prevents false positives from the PHPCompatibility standard by excluding back-fills and poly-fills which are provided by WordPress.

This ruleset will help ensure backward and forward compatibility is maintained as new code is committed.

[desrosj]

First pass.

[desrosj]

46152.diff​ does the following:

• Adds the packages through Composer.
• Adds a compatibility script to Composer to run only PHP compatibility checks.
• Adds a php-compatibility.xml.dist file to configure the command as needed.

To start, I have copied the file exclusion/inclusion rules from core's main PHPCS configuration file. These checks are a bit different than coding standards, though. Coding standards shouldn't necessarily be followed within included libraries because it makes keeping them up to date more difficult. But the included libraries should always adhere to the project's PHP requirements.

@jrf I would love your feedback on this. I am new to this ruleset, so I am sure that there are some great ways to make the implementation simpler.

Currently, there are a large handful of warnings and errors. We'll need to look into each and determine if a change to the codebase is needed, or if the check should be disabled in the ruleset. Below is what composer run compatibility currently outputs:

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
src/_index.php                                        1       0
src/wp-mail.php                                       1       0
src/wp-settings.php                                   2       0
src/xmlrpc.php                                        5       0
src/wp-admin/ms-delete-site.php                       1       0
...-admin/includes/class-automatic-upgrader-skin.php  0       1
src/wp-admin/includes/class-bulk-upgrader-skin.php    1       0
...wp-admin/includes/class-wp-ajax-upgrader-skin.php  0       3
...n/includes/class-wp-plugin-install-list-table.php  1       0
src/wp-admin/includes/class-wp-upgrader-skin.php      1       0
src/wp-admin/includes/media.php                       0       1
src/wp-admin/includes/schema.php                      0       1
src/wp-admin/includes/upgrade.php                     0       1
src/wp-admin/user/about.php                           1       0
...ontent/themes/twentyfifteen/inc/template-tags.php  1       0
...twentynineteen/template-parts/content/content.php  1       0
src/wp-content/themes/twentyten/author.php            1       0
src/wp-content/themes/twentytwelve/index.php          1       0
src/wp-includes/author-template.php                   0       1
src/wp-includes/capabilities.php                      1       0
src/wp-includes/class-wp-network-query.php            1       0
src/wp-includes/cron.php                              0       1
src/wp-includes/functions.php                         0       7
src/wp-includes/general-template.php                  0       1
src/wp-includes/load.php                              0       1
src/wp-includes/plugin.php                            0       6
src/wp-includes/spl-autoload-compat.php               0       1
src/wp-includes/wp-db.php                             26      1
...s/customize/class-wp-customize-themes-section.php  1       0
src/wp-includes/pomo/streams.php                      0       1
src/wp-includes/rest-api/class-wp-rest-server.php     4       0
...points/class-wp-rest-post-statuses-controller.php  1       0
tests/phpunit/includes/plural-form-function.php       0       1
tests/phpunit/tests/admin/includesFile.php            0       1
tests/phpunit/tests/ajax/Compression.php              1       0
tests/phpunit/tests/comment/wpListComments.php        1       0
tests/phpunit/tests/db/charset.php                    1       0
tests/phpunit/tests/formatting/SanitizeMimeType.php   1       0
tests/phpunit/tests/formatting/WPTexturize.php        2       0
tests/phpunit/tests/http/curl.php                     1       0
tests/phpunit/tests/multisite/getSite.php             1       0
tests/phpunit/tests/option/option.php                 0       1
tests/phpunit/tests/option/siteOption.php             0       1
tests/phpunit/tests/option/updateOption.php           0       1
tests/phpunit/tests/pomo/mo.php                       0       1
tests/phpunit/tests/post/output.php                   1       0
tests/phpunit/tests/rewrite/oldDateRedirect.php       1       0
tests/phpunit/tests/user/wpSendUserRequest.php        1       0
----------------------------------------------------------------------
A TOTAL OF 63 ERRORS AND 33 WARNINGS WERE FOUND IN 48 FILES
----------------------------------------------------------------------

Time: 23.78 secs; Memory: 6MB


PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
----------------------------------------------------------------------
SOURCE                                                           COUNT
----------------------------------------------------------------------
PHPCompatibility.Extensions.RemovedExtensions.mysql_DeprecatedR  27
Internal.Exception                                               20
PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValu  13
PHPCompatibility.FunctionNameRestrictions.ReservedFunctionNames  9
PHPCompatibility.Variables.RemovedPredefinedGlobalVariables.htt  9
PHPCompatibility.FunctionNameRestrictions.ReservedFunctionNames  4
PHPCompatibility.IniDirectives.RemovedIniDirectives.mbstring_fu  3
PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValu  2
PHPCompatibility.Miscellaneous.ValidIntegers.HexNumericStringFo  2
PHPCompatibility.FunctionNameRestrictions.RemovedMagicAutoload.  1
PHPCompatibility.FunctionUse.RemovedFunctions.create_functionDe  1
PHPCompatibility.IniDirectives.RemovedIniDirectives.magic_quote  1
PHPCompatibility.IniDirectives.RemovedIniDirectives.magic_quote  1
PHPCompatibility.IniDirectives.RemovedIniDirectives.register_gl  1
PHPCompatibility.IniDirectives.RemovedIniDirectives.safe_modeDe  1
PHPCompatibility.Lists.AssignmentOrder.Affected                  1
----------------------------------------------------------------------
A TOTAL OF 96 SNIFF VIOLATIONS WERE FOUND IN 16 SOURCES
----------------------------------------------------------------------

[desrosj]

Also, we should be cognizant of the fact that there is a plan to ​raise the minimum version of PHP when deciding which violations are worth the time fixing vs. excluding in the ruleset.

[jrf]

@desrosj Thanks for taking the initiative for this and pinging me about it.

I've had a quick look at the patch and the current results.

composer.json

• Only phpcompatibility/phpcompatibility-wp should be added, not phpcompatibility/php-compatibility. PHPCompatibility is one of the dependencies for PHPCompatibilityWP, so there is no reason to list it here and it could cause problems in the future if the versions would get out of sync.
• As the composer.lock file is committed, and by extension, updates are managed, I see no reason to use the ~-type version constraint and would recommend using a ^2.0.0 version constraint instead. (I've given the same feedback previously about WPCS and the dealerdirect plugin)
• Script: as PHPCS 3.0+ will be used (as the minimum WPCS PHPCS requirement is PHPCS 3.3.1 for WPCS 2.0.0+), you can simplify the command slightly: --report-summary --report-source can be written as --report=summary,source. The same can be applied to the other commands too.

As a side-note for the scripts: please consider using @php ... to force the scripts to run on the same PHP version as Composer is using, instead of a system default PHP version (which may not be the same).
See ​WPCS PR 1488 for a more detailed explanation.

php-compatibility.xml.dist

• For simplicity, you may want to consider renaming the file to phpcompat.xml.dist.
• The <config name="testVersion" value="5.2.4-"/> directive is invalid. At this moment, PHPCompatibility only accepts minors, not patch versions. There is an ​issue open to change this, but this issue is not currently being worked on. The correct directive would be <config name="testVersion" value="5.2-"/>. This will probably change the output you received as, if I remember correctly, the prior invalid testVersion will have been disregarded.

To start, I have copied the file exclusion/inclusion rules from core's main PHPCS configuration file. These checks are a bit different than coding standards, though. Coding standards shouldn't necessarily be followed within included libraries because it makes keeping them up to date more difficult. But the included libraries should always adhere to the project's PHP requirements.

You correctly surmise that the exclusion rules from the regular PHPCS configuration should not necessarily be copied over.

I would suggest starting off with a much smaller set of file in-/exclusions by scanning only the code which goes into production, though I would like to hear the opinion of others about this as well.

<!-- Only scan code which will be distributed in the download.
<file>/src/</file>

<!-- Code which doesn't go into production may have different requirements. -->
<exclude-pattern>/node_modules/*</exclude-pattern>

Some unit tests may be aimed at different PHP versions, similarly, tools may have been build with a different minimum PHP version in mind. As these are not distributed in a normal WP download, this is not something we should worry about for a first implementation.

At a later stage, these could be added for scanning anyway and additional commands could be added to the script to use different testVersion settings for subsets of files, if needed.

The next step would be to run the scan again with the corrected testVersion and to review the results. I've scanned WP before and quite a few of these involve cross-compat files or code.

For the files, a selective exclusion should be added to the ruleset, i.e.:

<rule ref="PHPCompatibility.Category.SniffName.ErrorCode">
    <exclude-pattern>/path/to/file\.php$</exclude-pattern>
</rule>

Some others could be whitelisted inline with the PHPCS selective ignore annotations:

// phpcs:ignore PHPCompatibility.Category.SniffName.ErrorCode
codetobeignored();

Regarding the errors listed above: I've often used the WP codebase as a test case when writing these sniffs to minimize and weed out false positives from the sniffs.

Note: false positives from a WordPress perspective does not necessarily mean something is a false positive from a PHPCompatibility perspective.

For instance, for the PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue issues, these are "false positives" from a WP perspective, at least for the ones which existed when I wrote the sniff.
However, the code is quite convoluted and the mental overhead to analyse this is (too) high, be it for a human, or a sniff, which is why these are flagged.

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.

Anyways, I hope this helps ;-)

[desrosj]

I'm moving this to 5.3 because I think there is value having this in Core today for a few reasons.

1. It helps with the effort to add PHP 7.4 support.
2. It makes it easier for contributors to test their code for compatibility issues.

Using @jrf's suggestions, I am uploading a new patch. I went through the list of warnings and errors and accounted for a large number of them. I purposefully did not address or add exclusion rules for some issues that I know are being addressed elsewhere as a part of the PHP7.4 support effort (such as func_get_arg warnings being addressed in #47678). This includes the ones in external libraries such as Requests and SimplePie.

• The patch adds a new job to the Travis build that is allowed to fail (for now).
• I left out the @php ... suggestion because this change should also be made to the other phpcs/phpcbf scripts. They can all be addressed at once.
• I chose to supply an upper PHP limit of 7.4. This ensures the build will not start failing randomly as new versions of PHP are accounted for in the tests. This actually may not be a concern depending on how the PHPCompatibility/PHPCompatibilityWP package is versioned. But, having the upper limit be an intentional version bump that is applied clearly shows what Core's version support goal is and could prevent the build from randomly starting to fail when the job is removed from the allow_failures list.
• I opted for inline ignore statements (with a few exceptions) instead of exclusion rules in most cases to match the approach used for the coding standards ruleset.

I have a test branch set up on my fork that ​shows the new Travis build. More specifically, ​this is the added job.

[jrf]

@desrosj Thanks for working on this!

I left out the @php ... suggestion because this change should also be made to the other phpcs/phpcbf scripts. They can all be addressed at once.

Is there a ticket open for this yet ?

I chose to supply an upper PHP limit of 7.4. This ensures the build will not start failing randomly as new versions of PHP are accounted for in the tests. This actually may not be a concern depending on how the PHPCompatibility/PHPCompatibilityWP package is versioned. But, having the upper limit be an intentional version bump that is applied clearly shows what Core's version support goal is and could prevent the build from randomly starting to fail when the job is removed from the allow_failures list.

This is unnecessary and an unfounded concern, for the same reason as I previously mentioned about fixing the versions in the composer.json.

The composer.lock file is committed. This means that, by extension, updates are managed.
In other words: you will never see the "random failures due to new tests being added". You will only see those if and when a (selective) composer update has been done which included PHPCompatibilityWP and this update then gets committed.

In other words, this is always a deliberate action.

So, having said that, I see no reason to use a strict limit on the supported PHP versions in the testVersion config and would recommend using a testVersion of 5.6- instead.

This will also more easily prevent people from "missing" issues as it is easy to forget to update something like testVersion when a new PHP comes out/is being prepared.

I opted for inline ignore statements (with a few exceptions) instead of exclusion rules in most cases to match the approach used for the coding standards ruleset.

👍

---

Other than that:

1. composer.json - script --cache -d memory_limit=256M

These command-line directives could/should both be moved to the phpcompat.xml.dist file.

2. phpcompat.xml.dist - <exclude-pattern>/vendor/*</exclude-pattern>

We need to be very careful with this one.

At this time, WP doesn't have any non-dev dependencies managed via Composer, but as soon as it does, the PHPCompatibility scan should also be run over the vendor directory (or at least over the parts thereof which will be shipped with WP).

I think it would be a good idea to document this in the ruleset.

3. phpcompat.xml.dist - exclusion of random_compat.

The PHPCompatibility ruleset related to this - PHPCompatibilityParagonieRandomCompat - already prevents false positives from the polyfill itself, however, these aren't working because WP has copied the files in using a non-standard path.

It may be better to copy the relevant selective exclusions as used in the PHPCompatibilityParagonieRandomCompat ruleset than to exclude the directories completely:

    <rule ref="PHPCompatibility.IniDirectives.RemovedIniDirectives.mbstring_func_overloadDeprecated">
        <exclude-pattern>/random_compat/byte_safe_strings\.php$</exclude-pattern>
    </rule>
    <rule ref="PHPCompatibility.Constants.RemovedConstants.mcrypt_dev_urandomDeprecatedRemoved">
        <exclude-pattern>/random_compat/random_bytes_mcrypt\.php$</exclude-pattern>
    </rule>
    <rule ref="PHPCompatibility.Extensions.RemovedExtensions.mcryptDeprecatedRemoved">
        <exclude-pattern>/random_compat/random_bytes_mcrypt\.php$</exclude-pattern>
    </rule>
    <rule ref="PHPCompatibility.FunctionUse.RemovedFunctions.mcrypt_create_ivDeprecatedRemoved">
        <exclude-pattern>/random_compat/random_bytes_mcrypt\.php$</exclude-pattern>
    </rule>

See: ​https://github.com/PHPCompatibility/PHPCompatibilityParagonie/blob/b1bb79a7cab1fb856b56f1b5cf110b6e52d8e936/PHPCompatibilityParagonieRandomCompat/ruleset.xml#L20-L38

4. phpcompat.xml.dist - exclusion of snoopy.

I don't think the issue in this library should be excluded (use of deprecated each()). The code is still shipped with WP and, while discouraged via a deprecation notice, plugins and themes _may_ still call it.
These look easy enough to solve anyway by just changing the while(list($key,$val) = each($formvars)) to
using a foreach.

Also see: ​https://wiki.php.net/rfc/deprecations_php_7_2#each and ​https://3v4l.org/t8mZI

5. Ignore comments general

Looking good. While the ones currently used are obvious, I just wanted to mention that where it would be helpful, you can add a comment explaining why something can be safely ignored. This can be done like so: // phpcs:ignore Stnd.Cat.Sniff.Code -- Reason. (everything behind the -- is ignored by PHPCS).

6. Double underscore prefixed functions

While I understand and agree they should be ignored for now, I do think we should probably open an issue about deprecating these functions/methods in favour of the same functions, but then with better names.

I know this will be a pain, especially for methods like __return_true etc, but we kind of need to take that pain at some point, so why not now (or at least in the near foreseeable future) ?

7. Remaining issues

Not much left after the work you've put in here 🎉

Most look easy enough to solve and/or obvious ignores, but I haven't dug into the individual issues.

For fixes: what process do you want to follow ? Should separate tickets be created for each type of issue ? Or do you want to add these to this ticket too ?

---

In related news - I did a scan last night and compared the results with a previous scan from a month ago and between all the patches committed related to PHP 7.4, the patches for #47678 being committed and the updates done for various external libraries, the total issue count which a plain scan (without any exclusions) yields, has gone down from 272 to 110 (!!). 🎉

[desrosj]

@jrf Thanks for all of the detailed feedback!

46152.3.diff​ is refresh with this round of feedback applied. Here are some notes:

• I changed the commands to contain the @php prefix. #47853 was already open with this suggestion and the change for other commands landed in [46187].
• class-snoopy.php: I have removed the exclusion. I looked back at when PHP 7.2 support was tackled, and it appears the decision was made to skip this file because the file is deprecated. If you feel the level of effort is low for this, I have no strong feelings against fixing this. Let's open a new ticket to tackle that.
• I added a compat:detailed script to easily display a full report.
• I moved the command-line directives into the configuration file. I will have a follow up patch shortly for the other commands to commit separately.
• For the double underscore functions, let's open a separate ticket for discussion and feedback.

Moving forward, I think individual tickets for like compatibility updates may be best to ensure each gets properly tested.

[desrosj]

46152-move-command-line-directives-to-ruleset.diff​ moves command line directives for the other Composer scripts to the phpcs.xml.dist configuration file.

I also moved the arguments to the top of the configuration to make that a bit easier to see. These are currently buried between a bunch of rule configurations and exclusion rules.

[desrosj]

46152.4.diff​ is a refresh after [46200].

[jrf]

@desrosj Thanks for updating the patch.

I added a compat:detailed script to easily display a full report.
I moved the command-line directives into the configuration file. I will have a follow up patch shortly for the other commands to commit separately.

Thanks for updating this, but you've taken it a bit too far now.

In my opinion, the --report=summary,source should never be added to a custom project ruleset.
I didn't list it above for a reason 😉

Most people don't know anything but the default full report from PHPCS and wouldn't know how to change it.

While running a Composer script, it's fine for a custom report setting to be used, but when people run phpcs from the command-line, they should - by default - get the full report.

Principle of least astonishment et al.

So, please move the --report setting back to the Composer scripts and out of the ruleset (for both patches).

I changed the commands to contain the @php prefix. #47853 was already open with this suggestion and the change for other commands landed in [46187].

👍🏻

class-snoopy.php: I have removed the exclusion. I looked back at when PHP 7.2 support was tackled, and it appears the decision was made to skip this file because the file is deprecated. If you feel the level of effort is low for this, I have no strong feelings against fixing this. Let's open a new ticket to tackle that.

👍🏻

For the double underscore functions, let's open a separate ticket for discussion and feedback.
Moving forward, I think individual tickets for like compatibility updates may be best to ensure each gets properly tested.

I agree 👍🏻

General note: I think it would be good if either of us would open the above mentioned tickets and list them here for reference before this ticket is closed.

[desrosj]

Thanks for the continued feedback, @jrf! I think the last two patches are good to go after a final review!

[jrf]

@desrosj Thanks for (yet again) updating the patches.

The only remark I have left is that it may be good to add <!-- --> documentation to the newly added settings in the ruleset file, similar to what is already in place for the other settings in the file, but that is not a blocker for merging this.

Something along the lines of (rough notes):

<!-- Set the memory limit to 256M.
     For most standard PHP configurations, this means the memory limit will temporarily be raised.
     Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#specifying-phpini-settings -->
<ini name="memory_limit" value="256M"/>

<!-- Only scan PHP files. -->
<arg name="extensions" value="php"/>

<!-- Whenever possible, cache the scan results and re-use those for
     unchanged files on the next scan. -->
<arg name="cache"/>

<!-- Strip the filepaths down to the relevant bit. -->
<arg name="basepath" value="./"/>

Other than that, all looks good & shiny to me 👍🏻

Note: I haven't tested the config locally, but I know you have and the Travis run looks good.

[desrosj]

In 46290:

Build/Test Tools: Introduce automated PHP compatibility checking.

This change introduces a new Composer script, compat that will scan the codebase for (detectable) potential PHP compatibility issues using the PHP_CodeSniffer and a custom ruleset based off of the PHPCompayibilityWP ruleset (phpcompat.xml.dist).

The command will be run as a separate job within each Travis build. While many compatibility issues and false positives have already been corrected in this commit and other Trac tickets, there are still some remaining. For that reason, the job is allowed to fail while the remainder of the potential compatibility issues are investigated and addressed. After those are resolved, the job should be set as required to pass to help prevent new compatibility issues from being introduced.

Props desrosj, jrf, all PHPCompatibilityWP and PHPCompatibility contributors.
Fixes #46152.

[desrosj]

In 46291:

Build/Test Tools: Move Composer script command line directives to the PHPCS ruleset.

Also, move all arguments up to the top of the custom ruleset to make it easier to understand the conditions the ruleset is run under.

Props desrosj, jrf.
See #46152.

[desrosj]

Alright, everything has been committed that was discussed here. The remaining tasks for separate tickets discussed in this one are:

• Open a ticket to address the PHP 7.2 incompatibilities in class-snoopy.php (see 40109 for history).
• Open a ticket to discuss renaming double underscore functions, such as __return_true and __return_empty_array to prevent ​potential compatibility issues in the future.

I have no preference for tackling other incompatibility issues. I think new tickets for groups of like incompatibilities would be fine.

[jrf]

🎉

[desrosj]

Mentioned in the Miscellaneous Developer Focused Changes dev note for 5.3: ​https://make.wordpress.org/core/2019/10/15/miscellaneous-developer-focused-changes-in-5-3/