cloudinary / cloudinary_wordpress

Cloudinary's WordPress plugin
GNU General Public License v2.0
39 stars 28 forks source link

PHPCompatibiltyWP missing from PHPCS config file #791

Closed GaryJones closed 2 years ago

GaryJones commented 2 years ago

Bug Description

A testVersion configuration value in the PHPCS config file was uncommented by @pereirinha, but it still doesn't have any effect until the PHPCompatibility or PHPCompatibilityWP rule is added. The line was previously commented out because it wasn't having any effect then.

Expected Behaviour

The PHPCS config file should contain something like:

<!-- Rules: Check PHP version compatibility - see
        https://github.com/PHPCompatibility/PHPCompatibilityWP -->
<rule ref="PHPCompatibilityWP"/>

Steps to reproduce

PHPCompatibilityWP has been added as a dev dependency (good), but it isn't in the PHPCS config where it would be expected to be found along with the other ruleset references.

See https://github.com/Parsely/wp-parsely/blob/d0384362e1815aa557a5447b738d8bac58dbcb80/.phpcs.xml.dist#L30-L35 as an example.

GaryJones commented 2 years ago

Separately, I'll also note that you have the * constraint for the PHPCompatibilityWP package. This will also pull in PHPCompatibility v9, but that doesn't have the sniffs related to PHP 8.

Please follow the steps at https://docs.wpvip.com/how-tos/code-scanning-for-php-upgrade/#Upcoming-releases-of-PHPCompatibility to use PHPCompatibilityWP with PHPCompatibility develop branch which does contain the PHP 8-related sniffs.

PixelCook commented 2 years ago

Gary I've submitted your notes to our production team. Thanks so much! I'll update with any updates I have.

pereirinha commented 2 years ago

Thanks, @GaryJones, for raising this.

I did open 2 PR. The first will take care of your suggestions in the configuration files. The second takes care of some code improvements.

It'd be great if you could chime in the first PR.

Luckily, I don't think the code had any pitfall, but we'll be let know from now on.

pereirinha commented 2 years ago

This was merged and will be part of the next release. Closing.