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

CommentFileSizeAuthor
#2 3074176.patch501 byteseiriksm

Comments

eiriksm created an issue. See original summary.

eiriksm’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new501 bytes

Oops, 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:

} else if (substr($arg, 0, 11) === 'extensions=') {
                if (isset(self::$overriddenDefaults['extensions']) === true) {
                    break;
                }

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 :)

klausi’s picture

Status: Needs review » Needs work

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

klausi’s picture

Another workaround in the meantime is to use something like <exclude-pattern>*\.md$</exclude-pattern> in your phpcs.xml.dist config file.

eiriksm’s picture

That is the workaround we are currently using ;)

Will try to find time a bit later to file a pr

eiriksm’s picture

traviscarden’s picture

FYI: 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.

klausi’s picture

Looked 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!

eiriksm’s picture

Updated the PR :)

  • klausi committed 69b071b on 8.x-3.x authored by eiriksm
    fix(phpcs config): Remove file extensions in rulesets because users...
klausi’s picture

Status: Needs work » Fixed

Committed, thanks!

danepowell’s picture

I 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

jonathan1055’s picture

Coder will just stop scanning a ton of files with no warning

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

jonathan1055’s picture

Title: Not possible to override extensions with phpcs.xml file » Allow extensions to be overridden in contrib phpcs.xml file

Just for the record -
File extensions which are still being checked by default:
php, inc, css
Extensions which were being checked by default but not now:
module, install, test, profile, theme, txt, md, yml
Extensions which were not checked by default but are now:
js

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

danepowell’s picture

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

klausi’s picture

Good 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 --extensions when running phpcs or set the extensions argument in your phpcs.xml.dist config file.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.