Git hooks pre-commit code sniffer should not sniff png/gif/jpg files

shelane commented 6 years ago

My system information:

Operating system type: macOS
Operating system version: 10.13.5
BLT version: 9.1.0-alpha1

Output of blt doctor:

blt doctor output

When I run this command:

git commit ... (of a .png file)

I get the following output:

Executing .git/hooks/pre-commit...
> tests:phpcs:sniff:files
Sniffing directories containing changed files...
[File\Write] Writing to /Volumes/UserDrive/shelane/Sites/docksal/llnl8/tmp/phpcs-fileset.
[ExecStack] '/Volumes/UserDrive/shelane/Sites/docksal/llnl8/vendor/bin/phpcs' --file-list='/Volumes/UserDrive/shelane/Sites/docksal/llnl8/tmp/phpcs-fileset' --bootstrap='/Volumes/UserDrive/shelane/Sites/docksal/llnl8/vendor/acquia/blt/src/Robo/Commands/Validate/phpcs-validate-files-bootstrap.php' -l

Notice: Undefined index: parenthesis_closer in /Volumes/UserDrive/shelane/Sites/docksal/llnl8/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php on line 365

And I expected this to happen: The file should be committed without being checked. There was only a single png file in the commit.

Other commits involving only php and yml files have been successful.

shelane commented 6 years ago

Related/along the same lines https://github.com/acquia/blt/issues/2861

gheydon commented 6 years ago

As a work around you can add the extensions to your phpcs.xml file (copy phpcs.xml.dist if you don't have one) and add .png, .gif, .jpg etc to the ignore line that looks something like `<arg name="ignore" value=".css,.md,.txt"/>`

The file will get passed to phpcs but because it is in the ignore list it will not get processed.

shelane commented 6 years ago

Instead of having to ignore a bunch of extensions, why isn’t the Drupal ruleset being used which specifies which extensions to be tested? <arg name="extensions" value="php,module,inc,install,test,profile,theme,yml"/> https://github.com/pfrenssen/coder/blob/8.x-2.x/coder_sniffer/Drupal/ruleset.xml

I know you had a bit of an explanation on the other ticket regarding Javascript files. I guess I’m not understanding why the process doesn’t use that ruleset. I’ll add the exclude so at least I’m not having to bypass pre-commit hooks.

cosmicdreams commented 6 years ago

I think I ran into this issue a while back. The issue here is that BLT 9 and up attempts to find all the changed files with the git command:

git diff --name-only --cached --diff-filter=ACM

And then feeds that into phpcs. So, if an image has been changed, then it gets explicitly feed into phpcs. This apparently overrides established config for phpcs that would have avoided parsing the images.

Therefore, it might be good to figure out a better git command. If we don't explicitly feed images to phpcs then they won't be parsed.

cosmicdreams commented 6 years ago

https://stackoverflow.com/questions/15438915/git-diff-filtered-by-file-name suggests that we could pipe the output of the git diff to a grep. That would allow us to introduce the proper regular expression to filter out files with image file extensions.

something like:

git diff --name-only --cached --diff-filter=ACM | grep -i (?!(.gif|.jpg|.png))

shelane commented 6 years ago

With this in my phpcs.xml file: <arg name="ignore" value="*.css,*.md,*.txt,*.gif,*.png,*.jpg,*.jpeg,*.ico"/> I am getting these errors: Notice: Undefined index: parenthesis_closer in /Users/french8/Sites/docksal/llnl8/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php on line 161

These are the files I'm trying to commit:

    new file:   docroot/themes/custom/llnl_bootstrap/README.md
    new file:   docroot/themes/custom/llnl_bootstrap/config/install/llnl_bootstrap.settings.yml
    new file:   docroot/themes/custom/llnl_bootstrap/config/schema/llnl_bootstrap.schema.yml
    new file:   docroot/themes/custom/llnl_bootstrap/favicon.ico
    new file:   docroot/themes/custom/llnl_bootstrap/llnl_bootstrap.info.yml
    new file:   docroot/themes/custom/llnl_bootstrap/llnl_bootstrap.libraries.yml
    new file:   docroot/themes/custom/llnl_bootstrap/llnl_bootstrap.theme
    new file:   docroot/themes/custom/llnl_bootstrap/logo.png
    new file:   docroot/themes/custom/llnl_bootstrap/screenshot.png
    new file:   docroot/themes/custom/llnl_bootstrap/templates/README.md

The errors go on until I have to forcibly quit the process.

cosmicdreams commented 6 years ago

@shelane I'm not sure why you're getting that error (especially because your one-liner doesn't have an open or closing parentheses and otherwise looks legit).

But until proven wrong, I'm a strong believer that any configuration based intervention will not be enough given the fact that BLT is attempting to explicitly define what files should be parsed by phpcs in this case. I think that overrides any kind of config based exclusion list. Kind of like explicitly saying you want to commit a file that .gitignore has said it wants to ignore.

pcate-fls commented 6 years ago

I'm being bitten by this as well. The blt validate commands are fine, but when run as part of the git commit process, it seems to be completely ignoring the <arg name="ignore" values. Mine are:

<arg name="ignore" value="*.css,*.md,*.txt,*.png,*.gif,*.jpeg,*.jpg,*.svg,*.zip,*.ico" />

but it's throwing errors for a design asset .zip file I have in a custom theme, along with a .png, .ico, and documentation .md file.

My current workaround is to exclude them with <exclude-pattern>

geerlingguy commented 6 years ago

Same problem here—and to expand on what @pcate-fls mentioned, inside phpcs.xml.dist, after the other <exclude-pattern> entries, add something like:

  <exclude-pattern>*/*.png</exclude-pattern>
  <exclude-pattern>*/*.svg</exclude-pattern>
  <exclude-pattern>*/*.ico</exclude-pattern>

rumyanar commented 5 years ago

Same problem here for image files and compiled .css files, BLT version 9.1.7. The same workaround confirmed. Thanks @geerlingguy for the explanation.

star-szr commented 5 years ago

Unless I'm missing something, users need to add a potentially endless list of extensions as <exclude-pattern> directives in their phpcs configuration to work around this, because any changed file is being passed directly to phpcs via the pre-commit hook.

We're using the following patch against BLT 9.x to make this more manageable. I can make a PR but I'm guessing there would be more to it, maybe this needs to be configurable, etc.

only-scan-valid-phpcs-files.patch.txt

I decided against handling this via grep because trying to do an actual regex in grep and supporting all the different flavours and platforms didn't seem viable. The file extensions I've whitelisted are based on DrupalPractice's ruleset.xml because I have no desire to run css, txt, md through phpcs. Uploading in case it helps someone.

danepowell commented 5 years ago

There are numerous issues that same to be caused by PHPCS not properly filtering filesets when run via the git pre-commit hook, so I'm consolidating them into one: #3448

acquia / blt

Git hooks pre-commit code sniffer should not sniff png/gif/jpg files #2879