Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

harriswong
Copy link
Contributor

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

  • unit tests
  • changelog.txt entry added
  • readme.txt entry added

@harriswong harriswong requested review from a team and kloon and removed request for a team June 17, 2024 20:41
@harriswong harriswong changed the title phpcbf and phpcs will now check against WP and WC standards Chore: phpcbf and phpcs will now check against WP and WC standards Jun 17, 2024
Copy link
Contributor

@kallehauge kallehauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried running composer phpcbf woocommerce-services.php but I got "no fixable errors were found":

Screenshot 2024-06-18 at 01 41 23

I shouldn't make a difference, but I tried calling the shell script directly as well and got the same results (./bin/wc-phpcbf.sh woocommerce-services.php).

AFAIK then nothing has been merged into trunk that should have fixed any errors.
If you're still seeing them @harriswong, can you share a few examples of what it complains about? I'm curious if I can force something on my end 😄

@kloon
Copy link
Contributor

kloon commented Jun 18, 2024

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
Copy link
Contributor Author

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 (WordPress/WordPress-Coding-Standards#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 5c007e6.

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
Copy link
Contributor

kloon commented Jun 19, 2024

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

…/php_codesniffer wp-coding-standards/wpcs woocommerce/qit-cli dependencies
composer require -W --dev dealerdirect/phpcodesniffer-composer-installer
@harriswong
Copy link
Contributor Author

harriswong commented Jun 19, 2024

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. bde649d

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

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
Copy link
Contributor Author

Uh oh QIT failed. Checking 👀

@harriswong
Copy link
Contributor Author

harriswong commented Jun 19, 2024


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!

Copy link
Contributor

@kallehauge kallehauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Work as described! ❤️

composer phpcs woocommerce-services.php returns:

-------------------------------------------------------------------------------------------------------------------------------------
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)
...
-------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------------------------

composer phpcbf woocommerce-services.php returns:

-------------------------------------------------------------------------------------------------------------------
FILE                                                                                               FIXED  REMAINING
-------------------------------------------------------------------------------------------------------------------
/Users/kallehauge/sites/escargot-dev-environment/woocommerce-services/woocommerce-services.php     4      21
-------------------------------------------------------------------------------------------------------------------
A TOTAL OF 4 ERRORS WERE FIXED IN 1 FILE
-------------------------------------------------------------------------------------------------------------------

All of the above matches the summary and latest comment from @harriswong 🎉

Copy link
Contributor

@kloon kloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes gives the results expected, thanks @harriswong!

@harriswong
Copy link
Contributor Author

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

Merging. 🚀

@harriswong harriswong merged commit c776ec5 into trunk Jun 20, 2024
9 checks passed
@harriswong harriswong deleted the fix/phpcs-lint-staged branch June 20, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants