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

Pre-commit hook: Only use local copies of phpcs and phpcbf #44628

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Aug 3, 2020

Changes proposed in this Pull Request

In theory, we run the PHPCBF auto-formatter in our pre-commit hook. In practice, this has been failing, and I suspect has been the source of a number of PRs being submitted with PHP lint errors that PHPCBF could auto-fix -- see e.g. #43817.

The reason for this is that our pre-commit hook will preferably use a system-wide installed copy of those tools; however, there's no guarantee that dependencies, such as the WordPress Coding Standards, are also available globally. They will however be installed locally through a simply compose install, as will PHPCS and PHPCBF, and our pre-commit hook already has messaging to instruct the user to do so.

The fix is thus to never use the globally installed versions of PHPCS and PHPCBF, but always the locally installed ones, and if they can't be found, instruct the user to run compose install.

Testing instructions

Prerequisites:
  • Make sure you don't have WordPress Coding Standards globally installed. You can verify by running phpcs -i.
To reproduce the issue:
  • Remove your vendor/ subfolder from your local Calypso directory (this holds files installed by Composer).
  • Make a change to a PHP file in the WordPress.com Editing Toolkit plugin (formerly known as FSE) that includes something that PHPCS will fix (e.g. some whitespace change). E.g.
diff --git a/apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php b/apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php
index a67ca71017..4ee456fb24 100644
--- a/apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php
+++ b/apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php
@@ -1,6 +1,6 @@
 <?php
 /**
- * Call to Action pattern.
+ * Call to Action pattern. Blahblah
  *
  * @package A8C\FSE
  */
@@ -37,8 +37,8 @@ $markup = '
 ';
 
 return array(
-       'title'         => esc_html__( 'Call to Action', 'full-site-editing' ),
-       'categories'    => array( 'call-to-action' ),
-       'content'       => $markup,
-       'viewportWidth' => 1280,
+       'title'           => esc_html__( 'Call to Action', 'full-site-editing' ),
+       'categories'      => array( 'call-to-action' ),
+       'content'         => $markup,
+       'viewportWidth'   => 1280,
 );

Make sure your editor isn't configured to format on save!

  • Commit these changes.
  • Note that the commit will fail with the following error:
PHPCBF formatting staged file: apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php
ERROR: Referenced sniff "WordPress" does not exist

Which isn't very helpful 🙁

To reproduce the fix:
  • Now run the same instructions on this branch.
  • This time, committing will fail with:
PHPCBF formatting staged file: apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/call-to-action-02.php
COMMIT ABORTED: Working with PHP files in this repository requires the PHP Code Sniffer and its dependencies to be installed. Make sure you have composer installed on your machine and from the root of this repository, run `composer install`.
  • Follow these instructions (run composer install).
  • Try committing again. This time, it should pass.
  • Inspect the commit. The whitespace changes should be formatted according to standard.

@ockham ockham added the Build label Aug 3, 2020
@ockham ockham requested review from akirk, yansern and a team August 3, 2020 19:35
@ockham ockham self-assigned this Aug 3, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 3, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@ockham ockham force-pushed the update/pre-commit-hook-command-locations branch from 5af57d0 to 6efad20 Compare August 3, 2020 19:36
@ockham ockham force-pushed the update/pre-commit-hook-command-locations branch from 6efad20 to 3ffc504 Compare August 3, 2020 19:41
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D47506-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

This matches a similar thing I experienced a week ago, and the same thing fixed it. Thanks for the standalone fix :)

btw, do we know if this hook runs if the changes are only JS-related? We probably want to avoid forcing everyone to use composer install just for changes in normal calypso

@noahtallen noahtallen merged commit b4b0326 into master Aug 5, 2020
@noahtallen noahtallen deleted the update/pre-commit-hook-command-locations branch August 5, 2020 22:17
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants