[wp-trac] [WordPress Trac] #46152: Add PHPCompatibility checks to test suite

[WordPress Trac]

#46152: Add PHPCompatibility checks to test suite
------------------------------+-------------------------------
 Reporter:  desrosj           |       Owner:  (none)
     Type:  feature request   |      Status:  new
 Priority:  normal            |   Milestone:  Future Release
Component:  Build/Test Tools  |     Version:
 Severity:  normal            |  Resolution:
 Keywords:  has-patch         |     Focuses:  coding-standards
------------------------------+-------------------------------

Comment (by 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 [https://github.com/WordPress-Coding-Standards/WordPress-Coding-
 Standards/pull/1488 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
 [https://github.com/PHPCompatibility/PHPCompatibility/issues/531 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 ;-)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/46152#comment:3>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform