squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

gitstaged filter not working when called through npm husky precommit hook #3412

Open dingo-d opened 3 years ago

dingo-d commented 3 years ago

Describe the bug In my project, I'm using husky to run my pre-commit hooks. In package.json I have

...
"scripts": {
        "__eslintTheme": "eslint src/**/*.js",
        "__stylelintTheme": "stylelint src/**/*.scss",
        "lintStyle": "npm run __stylelintTheme",
        "lintJs": "npm run __eslintTheme",
        "lint": "npm run lintJs && npm run lintStyle && composer standards:check -- --filter=gitstaged",
...
},
"husky": {
    "hooks": {
        "pre-commit": "npm run lint"
    }
}

So it triggers any time I do a commit. In my composer.json I have these scripts

    "scripts": {
        "analyze": "@php ./vendor/bin/phpstan analyze",
        "standards:check": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs",
        "standards:fix": "@php ./vendor/squizlabs/php_codesniffer/bin/phpcbf"
    },

When I run my commit, all the npm hooks run ok, but the phpcs script fails with

 @php ./vendor/squizlabs/php_codesniffer/bin/phpcs '--filter=gitstaged'
PHP Fatal error:  Uncaught Error: Class '\PHP_CodeSniffer\Filters\gitstaged' not found in /home/dzoljom/Sites/Project/project-name/wp-content/themes/project-name/vendor/squizlabs/php_codesniffer/src/Files/FileList.php:83
Stack trace:
#0 /home/dzoljom/Sites/Project/project-name/wp-content/themes/project-name/vendor/squizlabs/php_codesniffer/src/Runner.php(382): PHP_CodeSniffer\Files\FileList->__construct()
#1 /home/dzoljom/Sites/Project/project-name/wp-content/themes/project-name/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run()
#2 /home/dzoljom/Sites/Project/project-name/wp-content/themes/project-name/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS()
#3 {main}
  thrown in /home/dzoljom/Sites/Project/project-name/wp-content/themes/project-name/vendor/squizlabs/php_codesniffer/src/Files/FileList.php on line 83

When I remove the filter, the script runs fine, but over the entire codebase, not just the committed files.

Custom ruleset

<?xml version="1.0"?>
<ruleset name="Eightshift Boilerplate ruleset">
    <description>Ruleset for the Eightshift Boilerplate.</description>

    <rule ref="Eightshift" />

    <exclude-pattern>*/tests/*</exclude-pattern>
    <exclude-pattern>*/vendor/*</exclude-pattern>
    <exclude-pattern>*/public/*</exclude-pattern>
    <exclude-pattern>*/node_modules/*</exclude-pattern>
    <exclude-pattern>*/storybook/*</exclude-pattern>

    <!-- Additional arguments. -->
    <arg value="sp"/>
    <arg name="basepath" value="."/>
    <arg name="parallel" value="8"/>
    <arg name="extensions" value="php"/>

    <file>.</file>

    <!-- Check for PHP cross-version compatibility. -->
    <config name="testVersion" value="7.2-"/>
    <rule ref="PHPCompatibilityWP"/>

    <config name="minimum_supported_wp_version" value="5.3"/>

    <exclude-pattern>/src/CompiledContainer\.php</exclude-pattern>

</ruleset>

Expected behavior PHP_CS should run with the gitstaged filter.

Versions (please complete the following information):

jrfnl commented 3 years ago

@dingo-d While Windows is case-insensitive, Linux is not. I think your issue will be fixed if you use --filter=GitStaged instead 😀

dingo-d commented 3 years ago

Thanks for the tip, I'll give it a go 👍🏼

dingo-d commented 3 years ago

Ok, so setting it to GitStaged fixed throwing of the PHP errors, but I don't get any error reported, and when I run composer standards:check I get the error in the staged file (I introduced it intentionally).

I'll try to investigate what happens when I run one vs the other command with some additional flags.

EDIT:

Ok, so something is odd with the realpath utility. Because when I check the $path variable here https://github.com/squizlabs/PHP_CodeSniffer/blob/b245bb315dae6dc7347681e4f5a55dd7bdfcd045/src/Filters/GitStaged.php#L50

I get the path to be wp-content/themes/my-theme/src/CustomPostType/GuidesAndResourcesPostType.php after it's passed to the Util\Common::realpath helper, it gets converted to boolean false.

Could this be because of WSL? I think my colleagues on mac are getting the same thing, so it could be due to Linux as well (since I am running Ubuntu 20.04 in WSL, and they are using MacOS unix based terminal).

jrfnl commented 3 years ago

@dingo-d I would suspect this is to do with WSL, so would be good if you could check whether your colleagues are getting this issue.

IIRC from previous reports, the PHP native file system functions don't all support WSL well enough.

Also see #2965 and #3388.

dingo-d commented 3 years ago

I'll poke this around since I have a Mac available and a Win PC that I can use, so I'll try to see what is happening, and report back 👍🏼

jrfnl commented 2 years ago

@dingo-d I've created PR #3428 to address a similar issue. That PR may solve your issue as well. Would you be able to test this and leave feedback on the PR ?

dingo-d commented 2 years ago

I've added the changes from the patch, but I'm still not getting an error when using the GitStaged filter

test
jrfnl commented 2 years ago

Hmm... darn... Looking at the code in the GitStaged class, I suspect the use of Util\Common::realpath() may be problematic.

Might be something we could debug together ?

dingo-d commented 2 years ago

Sure, just say when 😄