AcquiaPHP extensions takes precedence over AcquiaDrupalStrict extensions list

[greggmarshall]

Line 11 of the ruleset.xml for the AcquiaPHP standard contains the extensions list of
value="php,inc,test,css,txt,md,yml"

Given the way PHPCS V3 processes the extensions list in configuration, such as squizlabs/PHP_CodeSniffer#2602, the extensions list of AcquiaDrupalStrict is ignored, as is the extensions list in the project phpcs.xml.dist. Both of those have extensions list of
value="php,module,inc,install,test,profile,theme,css,info,txt,md,yml"

If I delete line 11 in the AcquiaPHP ruleset.xml, then the correct extensions are used.

[danepowell]

Can you provide a failing test case for this? We specifically worked around squizlabs/PHP_CodeSniffer#2602 in 3ea1e57

The issue of extensions provided via phpcs.xml.dist and the command line being ignored is a fundamental limitation of PHPCS and not something we can change. The only alternative is that we provide no extensions at all. See #7 and https://www.drupal.org/node/3074176

[greggmarshall]

I am willing to try to provide a test, can you point me to how to write one?

As for what to do, I understand the limitation of PHPCS. I'd agree that either no extensions at all, or at least match the extensions in AcquiaPHP to those used by AcquiaDrupalStrict and BLT.

[danepowell]

Sorry I didn't mean specifically an automated test (although that would be great), but a description of how to reproduce the issue. I understand that you can't override the extensions locally (that's by design) but I don't understand how you are seeing conflicts between the different standards, which we specifically worked around in 3ea1e57

[greggmarshall]

Sorry for the delay, its been a busy week. Let's see if this helps...

The project I'm working on is using BLT and BLT-PHPCS, which automatically generated the phpcs.xml.dist file in the project root. The only change to that file I've made is change the ruleset name and description to the project name, which was handy during debugging to see the ruleset was being used.

I noticed the issue while doing a manual code review of the first custom module and seeing obvious formatting issues. Knowing that we had configured our git hooks to check for Drupal coding standards, I got looking at what was going on.

On the command line with the branch checked out I did a blt validate:phpcs and noticed the .module file that had the code didn't get scanned. So I kept updating the verbosity until I had blt validate:phpcs -vvv and still didn't see what PHPCS was doing. But I did notice the command that BLT was executing, although it only passed a single level of verbosity.

I copied that command and added verbosity to -vvv and PHPCS output a lot of additional information. Knowing about the issue with conflict standards, I noticed the displays were indented as they nested. I also realized the "php,inc,test,css,txt,md,yml" was what was being used as the fileset. So I found where the output showed that being processed and moved up through the same indentation level to find the AcquiaPHP ruleset was being processed.

Given that clue, I modified the AcquiaPHP ruleset to remove line 11 and ran the PHPCS command again, which displayed a lot of formatting errors. Then I tried the blt validate:phpcs and it properly caught the formatting issues.

[greggmarshall]

The simplest solution is, given the context of the AcquiaPHP ruleset being part of the Acquia coding standards, and recognizing the extensions list for BLT and AcquiaDrupalStrict contain all the extensions in the AcquiaPHP plus additional Drupal specific extensions, would be to set the extensions list for AcquiaPHP to be the same as AcquiaDrupalStrict and BLT....

[TravisCarden]

Thank you, @greggmarshall. The extensions list for our PHP and Drupal standards differ intentionally. (There's no reason for a PHP-only project to be scanning .install files, for example.) The "correct" solution is to ensure that the extension list is explicitly set in your own phpcs.xml, as here: https://github.com/acquia/coding-standards-php/pull/7/files. If that's not it, the issue may have more to do with BLT & Company--in which case, I yield to @danepowell's expertise.

[greggmarshall]

@TravisCarden the extension list is explicitly set in our phpcs.xml.dist as indicated in my original report.

That phpcs.xml.dist uses the ruleset AcquiaDrupalStrict. It is AcquiaDrupalStrict that uses AcquiaPHP. So it looks to me that BLT isn't an issue.

[wu-edward]

I'm seeing the same behavior as @greggmarshall on a new BLT 13 build. Using the phpcs.xml.dist with extensions specified (or even copying that to a custom phpcs.xml), since it uses AcquiaDrupalStrict, and AcquiaDrupalStrict uses AcquiaPHP, .module files are being ignored. I assume the what happens with the ruleset config merging is a bit more complicated than whatever included rule is first, but looks like PHPCS won't be addressing until 4.0.

[danepowell]

I can reproduce this now, I'll add steps below. I don't know why I couldn't reproduce this before, maybe I was using a different PHPCS version. My guess is that at some point PHPCS was loading extensions linearly/deterministically, but no longer.

Coder seems to have fixed this by removing the extensions from their rulesets. I see no alternative but to follow their example: https://www.drupal.org/project/coder/issues/3074176

composer create-project acquia/drupal-minimal-project --no-interaction
cd drupal-minimal-project/
composer require --dev acquia/coding-standards
composer config extra.phpcodesniffer-search-depth 4
composer require --dev dealerdirect/phpcodesniffer-composer-installer
echo '<?php $not_compliant' >> docroot/modules/custom/foo.module
echo '<?php $not_compliant' >> docroot/modules/custom/foo.php
./vendor/bin/phpcs --standard=AcquiaDrupalStrict docroot/modules/custom/
# Expected behavior is that both files generate errors, actual error is only for foo.php

[danepowell]

@greggmarshall @wu-edward if you don't mind, please review and leave feedback for #27

[wu-edward]

@danepowell I'll try to review/test sometime this week.

[greggmarshall]

@danepowell I've rolled off the project that I was on when I reported this, but we did something that wasn't quite as flexible by patching the ruleset.xml in the AcquiaPHP to include the Drupal extensions. I like this approach a lot better.