[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:  task (blessed)           |      Status:  new
 Priority:  normal                   |   Milestone:  5.3
Component:  Build/Test Tools         |     Version:  trunk
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-testing  |     Focuses:  coding-standards
-------------------------------------+-------------------------------

Comment (by 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 (!!).
 🎉

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