Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking"

@alexpott wrote this on #2851661: Ensure that we're using the right ruleset for coding standards checking:

Some might raise an issue that this makes it more difficult to do coding standards checks against a non standard install where vendor has moved. But this is missing the point, coder and phpcs.xml.dist exist for checking core's coding standards at commit time and can suggest direction for the wider community. They are not there to provide a working system for contrib or custom. They are there to ensure core does not regress against the coding standards it is already complaint with.

I am mentioning that position as a discussion viewpoint.

I just want to point out what the testbot does for phpcs:

Since core specifies a version of drupal/coder, it installs that (via composer). If your project specifies a higher version, that will be installed when your contrib is installed.

In all cases, if phpcs is present, the testbot will run phpcs --config-set installed_paths /path/to/vendor/drupal/coder/coder_sniffer/. This means the Drupal and DrupalPractice standards will be available to your project's phpcs.xml.dist file. (That's the change in #2870645: PHPCS is failing on contrib modules)

If your project does not specify a phpcs.xml(.dist) file, phpcs will run against the Drupal standard, as provided by drupal/coder.

So from the d.o project maintenance point of view: Add a phpcs.xml.dist file and specify the standards/sniffs that you want.

For external projects, you can specify whatever CS you want and add a phpcs.xml.dist file. If you need the Drupal standard, add a require-dev for drupal/coder, and then either add a step to your CI build which does the --config-set part, or emulate this as a composer script: http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Composer/C...

That'd be the workaround, assuming we're not going to revert in this issue.

Can someone please explain what will change if what this issue is requesting is actually done? I.e., what the impact on core and/or contrib module automatic testing will be? Because reading the issue summary on the other issue, I am not clear on what impact it had either. Thanks!

My goal would be: if I have a contrib project without its own phpcs.xml.dist and using the standard vendor locations, the standard Drupal Core coding standards checks should be run whenever automated tests are run. Which is what is happening now that #2870645: PHPCS is failing on contrib modules is in, and what was happening a while back too, before the bug that caused that issue happened (I'm not clear on what made that happen).

Yah I guess I could have been clearer...

before the bug that caused that issue happened (I'm not clear on what made that happen).

#2851661: Ensure that we're using the right ruleset for coding standards checking changed core's phpcs.xml.dist like this:

+++ b/core/phpcs.xml.dist
@@ -12,16 +12,16 @@
-  <rule ref="Drupal.Classes.ClassCreateInstance"/>
...
+  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Classes/ClassCreateInstanceSniff.php"/>

[... and a lot more ...]

It used to use class references, now it uses paths to the files containing the classes. It also took out the composer script that configured phpcs to use the Drupal standard.

When this change was made, it trickled down into contrib because the testbot assumed that core was configuring phpcs to use the Drupal standard. This meant that if you had a phpcs.xml file that used the class reference style, phpcs couldn't find it because it was never configured that way.

#2870645: PHPCS is failing on contrib modules changed that so the testbot configures phpcs to use the Drupal standard in all circumstances.

Basically the super-annoying part here is that phpcs requires you to configure it to use external standards. :-)

My goal would be: if I have a contrib project without its own phpcs.xml.dist and using the standard vendor locations, the standard Drupal Core coding standards checks should be run whenever automated tests are run

If your phpcs.xml file uses classes (and not paths), then it should be OK under any circumstance where you *also* configure phpcs for the Drupal standard. The testbot does this, so it's OK.

If your phpcs.xml file uses file paths (and not classes), then it matters where vendor/ is. If you assume that vendor/ is in DRUPAL_ROOT, the testbot puts it there, so it's OK.

So the testbot is good either way, and if it turns out not, we'll fix it. :-)

I guess my point is that prior to 8.4.x phpcs.xml.dist was a codified version of our coding standards

Well, not really. The coding standards document is the real standard: https://www.drupal.org/docs/develop/standards

drupal/coder has the Drupal standard, which is (or should be) the more canonical tool for the standard.

If you look at core/phpcs.xml.dist, it has a ton of exclusions and things that aren't included in the first place. Core doesn't actually follow the coding standards completely.

What I meant by "standard" was "the Coder rules currently being used by the same version of Drupal Core for testing coding standards".

Religious discussion about what the standard is aside,

But it's not a religious discussion. There's a ton of issues moving core closer to the coding standard, by adding each sniff one at a time: #2571965: [meta] Fix PHP coding standards in core If you use core's phpcs.xml.dist file, it will change as these are added, and you'll also find yourself surprised when your tests start reporting CS errors that were formerly excluded.

This is one of the reasons the testbot doesn't use core/phpcs.xml.dist as default for projects that don't have phpcs.xml files, and instead just uses the Drupal standard provided by coder.

--runtime-set 😍

Here's a re-roll. Multiple Drupal CS sniffs were merged in the last weeks.

Another re-roll.

There's still some more changes again from more stuff. Patch won't apply again.

BTW, having this the way it is right now is causing confusion to at least one contrib author (me). See #2906653: How to add phpcs to contrib modules. Let's get this re-rolled and into the RTBC queue.

Re-roll.

I went through and made sure we didn't loose anything in the conversion. We didn't loose anything. However, we seem to have missed MethodDeclarationSniff.

Updated the testbot docs to tell people to use Drupal.Sniff syntax instead of file paths: https://www.drupal.org/drupalorg/docs/drupal-ci/using-coderphpcs-in-drup...

Here is the updated patch and the interdiff.

And are good now.

core's phpcs.xml.dist file is not the defacto coding standard for Drupal projects.

It is incomplete and will change over time.

The best automated tool for evaluating CS errors is the coding standard is the Drupal standard in Coder.

So if you are a contrib maintainer, use the Drupal standard, rather than core's config file.

For instance, Examples for Developers has its own phpcs.xml.dist file. This allows me to do a sniff like this:

$ cd drupal/root
$ composer install
$ cd modules/examples
$ ../../vendor/bin/phpcs -ps --runtime-set installed_paths /path/to/drupal/vendor/drupal/coder/coder_sniffer

The testbot works under the assumption that if you have a phpcs.xml(.dist) file, you want to use it. See: https://www.drupal.org/drupalorg/docs/drupal-ci/using-coderphpcs-in-drup...

You can also specify a different version of Coder than core uses in your composer.json file, if you know you need a certain sniff or a bugfix or whatever. The testbot knows how to do that. If you don't specify a Coder version, the testbot will composer require --dev drupal/coder @stable and use that. Since module dev dependencies aren't accounted for when you do a core install, you have to do a sub-install to run it locally:

$ cd modules/examples
$ composer install
[ composer installs your desired version of coder into modules/examples/vendor/ ]
$ ./vendor/bin/phcps -ps --runtime-set installed_paths /path/to/drupal/modules/examples/vendor/drupal/coder/coder_sniffer

I'm not going to hold up this issue, but the solution is to not use core's phpcs.xml.dist file, which is probably a best practice anyway. Your code might have better CS compliance than core. :-) You could just have the XML file with one rule: Drupal.

@Mile23: I think you meant phpcs.xml.dist in your first sentence in the message above, no?

Er, yeah. :-) Thanks.

We're going to have to agree to disagree on the defacto point. As a contrib developer I want to be able to track the code checks I'll be expected to pass with patches to core. Additionally I'd have to update the file everytime I do maintenance on the code. Ditto on that point for sites I work on. I just don't want to mentally keep track of multiple standards for everything I'm doing when tracking core (and occasionally a PSR CS project) is hard enough. Therefor for me it is my defacto standard and that's the rational for my initial statement.

I echo this statement, although I do understand where @Mile23 is coming from.

I simply think it's helpful for core to nudge contrib into respecting at least the coding standards that Drupal core respects. And since Drupal core is gradually becoming more consistent, this also allows contrib modules to gradually become more consistent.

In any case, this is what I've been doing and plan to continue to do for my contrib modules — e.g. for the CDN module I did this two weeks ago: #2910800: Core coding standards fixes: FileCommentSniff + DisallowLongArraySyntaxSniff + ExpectedExceptionSniff.

The point is though that Coder's standards are for everyone and core's standards are for core. #2851661: Ensure that we're using the right ruleset for coding standards checking was done for valid reasons that I think outweigh the reason given for contrib. Contrib has coders standards or can even role their own phpcs.xml.dist and they can keep that inline with core if they want.

1. It is not the community standard. If the community standard provided by coder doesn't work that is not the fault of core.
2. where does it say that .dist is designed to be copied out?
3. It is less bound to global state and funky errors that come from not having the right standards installed.

As alternative we could put some documentation on top of the phpcs.xml.dist file to explain how you can create your own file.

imho: If for whatever reason Drupal CS is not ok for the contrib / custom project, a specific phpcs.xml.dist is the best solution. Something like:

<?xml version="1.0"?>
<ruleset name="my_project_name">
  <rule ref="Drupal" />
  <exclude-pattern>*/somepath/*</exclude-pattern>
  <exclude name="NonImportant.Sniff.For.This.Project"/>
</ruleset>

More info at: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a....

It doesn't depend on what coding standards core is following (which can change often and / or unexpectedly) and it's not bound to specific paths. You also don't depend on how soon this issue will get merged! :) My 2 cents!

If we switch this back, we need to ensure that codesniffing works on the testbots for both core and contrib, or we need instructions on how to make it work. So, we'll need to review drupalci_testbot and determine if anything needs to change in how we kick off codesniffing.

The change was made to the drupalci_testbot phpcs plugin here: #2870645: PHPCS is failing on contrib modules

@neclimdul: http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Composer/C... is setting up PHPCS for drupalci_testbot itself, not the build.

The phpcs configuration code is here: http://cgit.drupalcode.org/drupalci_testbot/tree/src/DrupalCI/Plugin/Bui...

It always sets installed_paths for all projects. If the phpcs.xml file uses literal paths to sniffs, then it should still work.

But nevertheless, here's an issue to make sure: #2916858: Test to ensure that various phpcs configuration types work properly

+++ b/core/phpcs.xml.dist
@@ -63,15 +63,15 @@
-    <!-- Sniff for: DuplicateVar, InlineVariableName -->
...
+    <!-- Sniff for: DuplicateVar -->

This comment shouldn't be changing.

I think we need to add the composer commands first. Then update all the coding standards issues. And then do this. So that we don't have people working on those issues using out-of-date standards.

Fortunately someone just had the same idea and created #2886279: Add PHPCS and PHPCBF as Composer scripts so let's get that done first.

One problem with the composer script though is that it suffers from the same problems - ie. it is making a hard-coded assumption about vendor.

I think this issue is great + #50 still applies which makes it good RTBC candidate, but I think, and as of #51 it is not just me, it should be postponed on #2886279: Add PHPCS and PHPCBF as Composer scripts.

+++ b/core/phpcs.xml.dist
@@ -146,7 +146,7 @@
   <rule ref="Generic.PHP.DisallowShortOpenTag"/>
-  <rule ref="Generic.PHP.LowerCaseKeyword" />
+  <rule ref="Generic.PHP.LowerCaseKeyword"/>
   <rule ref="Generic.PHP.UpperCaseConstant"/>
   <rule ref="Generic.WhiteSpace.DisallowTabIndent"/>
 
@@ -200,7 +200,7 @@

@@ -200,7 +200,7 @@
   <rule ref="PSR2.Classes.PropertyDeclaration">
     <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
   </rule>
-  <rule ref="PSR2.Namespaces.NamespaceDeclaration" />
+  <rule ref="PSR2.Namespaces.NamespaceDeclaration"/>
   <rule ref="PSR2.Namespaces.UseDeclaration">
     <exclude name="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/>
   </rule>
@@ -245,7 +245,7 @@

@@ -245,7 +245,7 @@
   <rule ref="Squiz.Arrays.ArrayDeclaration.ValueNoNewline">
     <severity>0</severity>
   </rule>
-  <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration" />
+  <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration"/>
   <!-- Disable some error messages that we already cover. -->
   <rule ref="Squiz.ControlStructures.ForEachLoopDeclaration.AsNotLower">
     <severity>0</severity>
@@ -264,7 +264,7 @@

@@ -264,7 +264,7 @@
   <rule ref="Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeClose">
     <severity>0</severity>
   </rule>
-  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration" />
+  <rule ref="Squiz.Functions.MultiLineFunctionDeclaration"/>
   <rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine">

🙃: Can we avoid these changes to keep the patch size small?

Here is a suggestion of an updated text for all the CS issues:

Before

Step 2: Install PHP CodeSniffer and the ruleset from the Coder module

Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:

$ composer require drupal/coder squizlabs/php_codesniffer
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer
Once you have installed the phpcs package, you can list all the sniffs available to you like this:

$ ./vendor/bin/phpcs --standard=Drupal -e
This will give you a big list of sniffs, and the Drupal-based ones should be present.

After

Step 2: Run phpcs

Run phpcs using <code>composer run phpcs

.
You can add a progress in the command line by adding -- -ps

This has been in RTBC a few times, or at least close and seems to be in re-roll perdition. Let's get this committed.

I am ok with the patch - it does a very decent work.
I am ok-ish if unrelated CS fixes mentioned in #55 and #56 are fixed along, but I see potential for separate issue which aims to fix all whitespaces across all xml files in core (I counted 8 of them in two files).
Leaving it RTBC for committer to decide.

Committed e9b1184 and pushed to 8.5.x. Thanks!

I used to do

$ phpcs modules/CONTRIB_MODULE --standard="core/phpcs.xml.dist"

and that worked fine.

Since this issue, I'm now getting:

$ phpcs modules/CONTRIB_MODULE --standard="core/phpcs.xml.dist"
PHP Fatal error:  Uncaught PHP_CodeSniffer_Exception: Referenced sniff "Drupal.Array.DisallowLongArraySyntax" does not exist in /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1167
Stack trace:
#0 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(780): PHP_CodeSniffer->_expandRulesetReference(Object(SimpleXMLElement), '/Users/wim.leer...', 0)
#1 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(578): PHP_CodeSniffer->processRuleset('/Users/wim.leer...')
#2 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(956): PHP_CodeSniffer->initStandard(Array, Array, Array)
#3 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(113): PHP_CodeSniffer_CLI->process()
#4 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs(25): PHP_CodeSniffer_CLI->runphpcs()
#5 {main}
  thrown in /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 1167

Fatal error: Uncaught PHP_CodeSniffer_Exception: Referenced sniff "Drupal.Array.DisallowLongArraySyntax" does not exist in /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1167
Stack trace:
#0 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(780): PHP_CodeSniffer->_expandRulesetReference(Object(SimpleXMLElement), '/Users/wim.leer...', 0)
#1 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php(578): PHP_CodeSniffer->processRuleset('/Users/wim.leer...')
#2 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(956): PHP_CodeSniffer->initStandard(Array, Array, Array)
#3 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/CLI.php(113): PHP_CodeSniffer_CLI->process()
#4 /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcs(25): PHP_CodeSniffer_CLI->runphpcs()
#5 {main}
  thrown in /Users/wim.leers/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php on line 1167

It seems we now have to run phpcs like this:

$ composer phpcs modules/CONTRIB_MODULE
> phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- 'modules/CONTRIB_MODULE'

<actual output removed>

Time: 1.87 secs; Memory: 24Mb

Hopefully this helps others who run into this.