search

Automattic / woocommerce-services

WooCommerce Services is a feature plugin that integrates hosted services into WooCommerce (3.0+), and currently includes automated tax rates and the ability to purchase and print USPS shipping labels.
GNU General Public License v2.0
105 stars 20 forks source link

Chore: phpcbf and phpcs will now check against WP and WC standards #2749

Closed harriswong closed 1 week ago

harriswong commented 2 weeks ago

Description

Running composer phpcbf <file> doesn't fix the file. Same for phpcs. This PR fixes the lint-staged script as well as the phpcbf and phpcs composer scripts. These will now check against the following standards --standard=WooCommerce-Core,WordPress-Core,WordPress-Extra.

Related issue(s)

N/A

Steps to test

  1. Run composer phpcbf woocommerce-services.php
  2. Confirm that you see it auto fixed 4 things and 21 remaining items

PHPCBF RESULT SUMMARY

FILE FIXED REMAINING

..wordpress/wp-content/plugins/woocommerce-services/woocommerce-services.php 4 21

A TOTAL OF 4 ERRORS WERE FIXED IN 1 FILE



### Checklist

<!-- All testable code should have tests. -->
- [ ] unit tests

<!-- An entry in the changelog file is always required -->
- [ ] `changelog.txt` entry added
- [ ] `readme.txt` entry added
kloon commented 2 weeks ago

I am also getting the same results as @kallehauge, I also noticed my version of wpcs was throwing a parser error in the woocommerce-services.php file, then saw our packages are quite outdated so ran the following command to update everything:

composer require -W --dev wp-coding-standards/wpcs woocommerce/woocommerce-sniffs squizlabs/php_codesniffer dealerdirect/phpcodesniffer-composer-installer wp-coding-standards/wpcs woocommerce/qit-cli

After this I did not get any more parser errors and PHPCS was reporting errors as it should, this did not however produce the results as per testing instructions.

Do you think we should consider updating the packages in this PR? I'm not sure what effect it might have on our CI tests, though.

harriswong commented 2 weeks ago

Thank you @kallehauge @kloon!! The details helped!

I can reproduce your result. I did a composer install on mine and I saw

Package operations: 3 installs, 2 updates, 2 removals
  - Removing phpcsstandards/phpcsutils (1.0.9)
  - Removing phpcsstandards/phpcsextra (1.2.1)
  - Installing phpcompatibility/php-compatibility (9.3.5): Extracting archive
  - Installing phpcompatibility/phpcompatibility-paragonie (1.3.2): Extracting archive
  - Downgrading wp-coding-standards/wpcs (3.0.1 => 2.3.0): Extracting archive
  - Installing phpcompatibility/phpcompatibility-wp (2.1.4): Extracting archive
  - Upgrading woocommerce/woocommerce-sniffs (0.0.2 => 0.1.3): Extracting archive

Then, I remember I upgraded my composer packages 😆 But when I switched branches (probably with a reset --hard somewhere), the composer.* was reset but not my vendor/.

The "no fixable errors were found" issue

If we run composer phpcs -- -v (PHP 8.2), we will see something like this:

 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in
   |       | wordpress/wp-content/plugins/woocommerce-services/vendor/phpcompatibility/php-compatibility/PHPCompatibility/Sniff.php on line 131
   |       | The error originated in the .PHPCompatibility. sniff on line 131. (Internal.Exception)

This issue (https://github.com/WordPress/WordPress-Coding-Standards/issues/2203) is fixed in wp-coding-standards 3.0.

Solution

  composer remove woocommerce/woocommerce-sniffs
  composer require --dev wp-coding-standards/wpcs:"^3.0"
  composer require --dev woocommerce/woocommerce-sniffs

I have to first remove woocommerce/woocommerce-sniffs because it is fixed on the existing wpcs version. Then, I bumped up wpcs to ^3.0 and then reinstall woocommerce-sniffs.

I updated the composer.json and composer.lock in this commit https://github.com/Automattic/woocommerce-services/pull/2749/commits/5c007e60d3ebc11bf68a3db444f63191d4abe3fc.

Test

I did 

rm -Rf vendor
git reset --hard
composer install
composer phpcbf -- -v woocommerce-services.php

I can confirm the 4 fixes and 21 remaining. Let me know if this doesn't work and I will look again. Thank you!

kloon commented 2 weeks ago

Thanks @harriswong, should we also update QIT, PHPCS, phpcodesniffer-composer-installer, and woocommerce-sniffs? I also noticed that you updated wpcs to 3.0 where 3.1 is the latest version?

The following command should update everyone to the latest versions fixing the dependency issues as well:

composer require -W --dev wp-coding-standards/wpcs woocommerce/woocommerce-sniffs squizlabs/php_codesniffer dealerdirect/phpcodesniffer-composer-installer wp-coding-standards/wpcs woocommerce/qit-cli
harriswong commented 1 week ago

composer require -W --dev wp-coding-standards/wpcs woocommerce/woocommerce-sniffs squizlabs/php_codesniffer dealerdirect/phpcodesniffer-composer-installer wp-coding-standards/wpcs woocommerce/qit-cli

TIL about the -W flag!

Thanks! I gave this a try and then I ran composer phpcs woocommerce-services.php under PHP 8.2.12 (cli) (built: Oct 26 2023 18:21:35) (NTS), I got the following error:

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: trim(): Passing null to parameter #1 ($string) of type string is deprecated in
   |       | /ordpress/wp-content/plugins/woocommerce-services/vendor/phpcompatibility/php-compatibility/PHPCompatibility/Sniff.php on line 131
   |       | The error originated in the .PHPCompatibility. sniff on line 131. (Internal.Exception)

It looks like it's breaking from vendor/phpcompatibility.

I tried skipping dealerdirect/phpcodesniffer-composer-installer. It seems good with only composer require -W --dev wp-coding-standards/wpcs woocommerce/woocommerce-sniffs squizlabs/php_codesniffer wp-coding-standards/wpcs woocommerce/qit-cli. bde649da

Then after that is done, I ran a separate composer require -W --dev dealerdirect/phpcodesniffer-composer-installer. e64fd2d7

Somehow, this works now!

Test

I did 

rm -Rf vendor
git reset --hard
composer install
composer phpcbf -- -v woocommerce-services.php

composer phpcs woocommerce-services.php should print

FOUND 5 ERRORS AND 20 WARNINGS AFFECTING 18 LINES
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
    1 | ERROR   | [ ] Class file names should be based on the class name with "class-" prepended. Expected class-wc-connect-loader.php, but found woocommerce-services.php. (WordPress.Files.FileName.InvalidClassFileName)
  335 | ERROR   | [x] Expected 1 space after FUNCTION keyword; 0 found (Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterFunction)
  612 | ERROR   | [x] Expected 1 space after FUNCTION keyword; 0 found (Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterFunction)
  684 | WARNING | [ ] The method parameter $zone_id is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
  868 | WARNING | [ ] Processing form data without nonce verification. (WordPress.Security.NonceVerification.Recommended)
 1181 | WARNING | [ ] The method parameter $order_id is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
 1181 | WARNING | [ ] The method parameter $data is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
 1181 | WARNING | [ ] The method parameter $order is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
 1254 | WARNING | [x] Found precision alignment of 1 spaces. (Universal.WhiteSpace.PrecisionAlignment.Found)
 1255 | WARNING | [x] Found precision alignment of 1 spaces. (Universal.WhiteSpace.PrecisionAlignment.Found)
 1434 | WARNING | [ ] Silencing errors is strongly discouraged. Use proper error checking instead. Found: @file_get_contents( $i18n_json ... (WordPress.PHP.NoSilencedErrors.Discouraged)
 1434 | WARNING | [ ] file_get_contents() is discouraged. Use wp_remote_get() for remote URLs instead. (WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents)
 1459 | WARNING | [ ] In footer ($in_footer) is not set explicitly wp_register_script; It is recommended to load scripts in the footer. Please set this value to `true` to load it in the footer, or explicitly `false` if it should be
      |         |     loaded in the header. (WordPress.WP.EnqueuedResourceParameters.NotInFooter)
 1459 | WARNING | [ ] Resource version not set in call to wp_register_script(). This means new versions of the script may not always be loaded due to browser caching. (WordPress.WP.EnqueuedResourceParameters.MissingVersion)
 1461 | WARNING | [ ] In footer ($in_footer) is not set explicitly wp_register_script; It is recommended to load scripts in the footer. Please set this value to `true` to load it in the footer, or explicitly `false` if it should be
      |         |     loaded in the header. (WordPress.WP.EnqueuedResourceParameters.NotInFooter)
 1461 | WARNING | [ ] Resource version not set in call to wp_register_script(). This means new versions of the script may not always be loaded due to browser caching. (WordPress.WP.EnqueuedResourceParameters.MissingVersion)
 1509 | WARNING | [ ] Not using strict comparison for in_array; supply true for $strict argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
 1609 | WARNING | [ ] The method parameter $meta_type is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
 1609 | WARNING | [ ] It is recommended not to use reserved keyword "protected" as function parameter name. Found: $protected (Universal.NamingConventions.NoReservedKeywordParameterNames.protectedFound)
 1735 | ERROR   | [ ] All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found 'wc_esc_json'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 1737 | ERROR   | [ ] A function call to esc_html__() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.
      |         |     (WordPress.WP.I18n.MissingTranslatorsComment)
 1758 | WARNING | [ ] Processing form data without nonce verification. (WordPress.Security.NonceVerification.Recommended)
 1758 | WARNING | [ ] Processing form data without nonce verification. (WordPress.Security.NonceVerification.Recommended)
 1787 | WARNING | [ ] Not using strict comparison for in_array; supply true for $strict argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
 1788 | WARNING | [ ] Not using strict comparison for in_array; supply true for $strict argument. (WordPress.PHP.StrictInArray.MissingTrueStrict)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 251ms; Memory: 24MB

Please take another look!

harriswong commented 1 week ago

Uh oh QIT failed. Checking 👀

harriswong commented 1 week ago 

Running test...
Request failed... Retrying (HTTP Status Code 0) Operation timed out after 15000 milliseconds with 0 bytes received
Request failed... Retrying (HTTP Status Code 0) Operation timed out after 15000 milliseconds with 0 bytes received
Request failed... Retrying (HTTP Status Code 0) Operation timed out after 15000 milliseconds with 0 bytes received
Request failed... Retrying (HTTP Status Code 0) Operation timed out after 15000 milliseconds with 0 bytes received
Timed out while waiting for test run to complete.
The test is still executing in QIT servers, but the timeout for waiting was reached.

I am going to rerun this.

update Passed!

harriswong commented 1 week ago

Thanks for testing this out for me @kallehauge @kloon! I appreciate testing this on multiple different machines due to PHP/composer/etc.

Merging. 🚀