Steps to reproduce:
- Clone coder.
- Create a folder called test_fails_folder
- Create a file called README.md with some errors in it. In my case it contains the following:
this is not a php file
```php
<?php
class Test {};
```
- Run phpcs with the following command:
$ ./vendor/bin/phpcs --standard=./coder_sniffer/Drupal --extensions=php,md test_fails_folder/
FILE: /xxx/coder/test_fails_folder/README.md
----------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------
6 | ERROR | [ ] Class name doesn't match filename; expected "class README"
6 | ERROR | [x] Missing class doc comment
6 | ERROR | [ ] Closing class brace must be on a line by itself
----------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------
Time: 79ms; Memory: 8MB
This gives you errors.
Now try to remove the md extension:
./vendor/bin/phpcs --standard=./coder_sniffer/Drupal --extensions=php test_fails_folder/
As expected, you now get no errors.
Now, try to specify this in a config file:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="myproject">
<file>test_fails_folder</file>
<arg name="extensions" value="php,md"/>
<rule ref="./coder_sniffer/Drupal"></rule>
</ruleset>
And run phpcs with no arguments:
$ ./vendor/bin/phpcs
FILE: /xxx/coder/test_fails_folder/README.md
----------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------
6 | ERROR | [ ] Class name doesn't match filename; expected "class README"
6 | ERROR | [x] Missing class doc comment
6 | ERROR | [ ] Closing class brace must be on a line by itself
----------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------
Time: 82ms; Memory: 8MB
This now gives me an error. So let's try to remove md in the config:
<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="myproject">
<file>test_fails_folder</file>
<arg name="extensions" value="php"/>
<rule ref="./coder_sniffer/Drupal"></rule>
</ruleset>
$ ./vendor/bin/phpcs
FILE: /xxx/coder/test_fails_folder/README.md
----------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------------
6 | ERROR | [ ] Class name doesn't match filename; expected "class README"
6 | ERROR | [x] Missing class doc comment
6 | ERROR | [ ] Closing class brace must be on a line by itself
----------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------
Time: 82ms; Memory: 8MB
I would expect this to not give me any errors, but it does, since Drupal ruleset overrides my config. With the patch attached, it does not give me any errors, as I expect
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3074176.patch | 501 bytes | eiriksm |
Comments
Comment #2
eiriksmOops, sorry, did not mean to post with no comment about it. Also updated the issue summary :)
In the docs it says you can specify a phpcs.xml file to specify your config. This is true. In the example it says you can override the config for "extensions" in this file. This is not true.
The reason is that phpcs loads the ruleset.xml file from the rules you want, and if this rule is setting this value, phpcs will treat the value as overridden, and not let you set it via config:
The way I see it, this seems like a feature in phpcs. And the Drupal ruleset should not set this in the ruleset.xml. So attached is a patch that unsets this config. Just to start the conversation at least :)
Comment #3
klausiThanks for reporting, nice find!
Can you file this as pull request against https://github.com/pfrenssen/coder/pulls so that we see the test cases run?
Can you also update the DrupalPractice standard? I think we have the same problem there.
Comment #4
klausiAnother workaround in the meantime is to use something like
<exclude-pattern>*\.md$</exclude-pattern>in your phpcs.xml.dist config file.Comment #5
eiriksmThat is the workaround we are currently using ;)
Will try to find time a bit later to file a pr
Comment #6
eiriksmhttps://github.com/pfrenssen/coder/pull/55
Comment #7
traviscarden commentedFYI: I reported the same issue upstream in "extensions" argument from first included rule clobbers all others · Issue #2602 · squizlabs/PHP_CodeSniffer. In short, whatever "extensions" value your first included rule declares takes precedence of every other value specified anywehere. The underlying bug is being tracked in Change ruleset processing to be top-to-bottom instead of processing tags in groups · Issue #2197 · squizlabs/PHP_CodeSniffer. The end-user workaround is to include a rule with a comprehensive "extensions" value first or to create and include your own if one doesn't exist.
Comment #8
klausiLooked at the pull request from eiriksm again. I think we can remove the extensions from our ruleset files as proposed, but we should also update our README.md usage instructions to then include the file extensions https://github.com/pfrenssen/coder#usage
The usage examples on https://www.drupal.org/node/1587138 already include the file extensions, so we should be good.
Can you update the PR and change our README.md?
Thanks!
Comment #9
eiriksmUpdated the PR :)
Comment #11
klausiCommitted, thanks!
Comment #12
danepowell commentedI think this was a good change to make, but I wish it hadn't been done as part of a patch release, because I think it's a BC break. Many users were relying on Coder to maintain the list of Drupal-specific extensions to scan, and now that they've gone away, code sniffing is going to fail in subtle and dangerous ways (Coder will just stop scanning a ton of files with no warning).
At the very least, this deserves a callout in the release notes for 8.3.7.
Two downstream issues caused by this: https://github.com/acquia/coding-standards-php/issues/4, https://github.com/acquia/blt/issues/3957
Comment #13
jonathan1055 commentedThe opposite can also happen. For example, the 'js' extension was not previously being checked by default but now is. I have new coding standards messages for .js files which were not being checked before.
Comment #14
jonathan1055 commentedJust for the record -
File extensions which are still being checked by default:
php, inc, cssExtensions which were being checked by default but not now:
module, install, test, profile, theme, txt, md, ymlExtensions which were not checked by default but are now:
jsThis is accurate from my observations, so I hope I am speaking for the general case.
Renamed the issue to show what has actually been achieved, instead of naming the problem.
Comment #15
danepowell commentedThanks a lot jonathan, your observations match my own, and I think that's exactly what should be added to the release notes, at a bare minimum.
Comment #16
klausiGood point about the release notes, added this:
Breaking changes: The hard-coded list of file extensions was removed in #3074176: Allow extensions to be overridden in contrib phpcs.xml file to allow users to specify their own file extensions they want to run Coder/PHPCS on. Please always specify
--extensionswhen running phpcs or set the extensions argument in your phpcs.xml.dist config file.