Keep all sniffs in phpcs.xml.dist in sync with the locked version of Coder

The file has the comment
<!-- Only include specific sniffs that pass. This ensures that, if new sniffs are added, HEAD does not fail.-->
but that could have been added years ago, before we had Composer to lock the Coder version. It could have been the case that Coder released a new version, and Core would suddenly fail coding standards. Now we know which Coder version we are using, the format of phpcs.xml.dist could be altered as @longwave suggests, so we only exclude sniffs we know and want to exclude. Then updating to a new Coder release will highlight any new sniffs that Core fails on and the source can either be fixed or the rule added to the ignored list.

I am sure that others will want to discuss this, and there may be good reasons why it has to stay as is. But the list of 'included' sniffs is getting longer and longer, and it would be nice to see the list of excluded sniffs instead, getting shorter and shorter (hopefully :-)

When we added the phpcs.xml.dist file hardly any rules applied so it would have been like use the drupal rule and then exclude everything. That's why it was done this way. It might be time to swap it round however as the #4 makes clear this would then mean that updating Coder has more of a chance to produce a failing phpcs run. However that is always the case. So I'm not sure that should stop us.

There are a couple of things that we need to be very careful about if we make this change:

• We need to ensure any sniffs that are not part of the Drupal standard are still there.
• We need to ensure all sniffs are configured the same way as they currently are.

The attached patch implements this by enabling the full Drupal ruleset, excluding any Drupal rules that core doesn't yet adhere to, and adding a few extra rules/settings that core does follow but that aren't in the Drupal ruleset.xml.

Locally this gave a clean run of phpcs, and the output of phpcs -e before and after the patch is the same implying that the same rulesets are in place.

I think we could swap out Generic.Arrays.DisallowLongArraySyntax for Drupal.Arrays.DisallowLongArraySyntax as they are effectively the same except the Drupal one skips the rule when on Drupal 7, but for now this patch is a 1:1 match with the current ruleset in core.

It's also possible that some of the excluded rules are safe to re-add as we may not violate them in core, but that should probably be done on a rule by rule basis in followups.

I suppose also we could open a followup in Coder to add these to the Drupal ruleset.xml:

  <rule ref="Generic.CodeAnalysis.EmptyPHPStatement" />

  <rule ref="Internal.NoCodeFound">
    <exclude-pattern>*.yml</exclude-pattern>
  </rule>

  <rule ref="PSR2.Classes.PropertyDeclaration">
    <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
  </rule>

As per #5 I'm really not that much a fan of this change. I'd rather opt-in than opt-out. coder doesn;'t really define the drupal coding standard. It has plenty of rules that feel like opinion and not a standard.

That is not the impression I got from #5!

However the current config isn't really pure opt-in either, it is opt-in to a broad group of sniffs and then opt-out of specific sniffs, which to me is more confusing. And what is a standard, other than an agreed opinion - shouldn't Coder be working towards defining the Drupal coding standard in code?

alexpott in #5

It might be time to swap it round ... updating Coder has more of a chance to produce a failing phpcs run. However that is always the case. So I'm not sure that should stop us.

alexpott in #9

As per #5 I'm really not that much a fan of this change.

longwave in #10

That is not the impression I got from #5!

Yes, same here. I thought @alexpott was giving a positive direction to follow.

I've worked on several sniffs, writing new ones, and also fixing bugs in existing ones. Some of these meant that core code had new faults which it had been getting away with. It's not a problem to get these corrected before the updated coder sniff is committed.

I think @longwave is right that the current opt-in to a broad group of sniffs and then opt-out of specific sniffs is confusing. So I'm all for progressing the approach in #7 which reduces phpcs.xml.dist by 275 lines and makes it easier

core/phpcs.xml.dist | 331 +------------
 1 file changed, 28 insertions(+), 303 deletions(-)

I was asked to comment on this and I read the issue. I am a new comer to these issues and still finding my way around. Here is my train of thought.

  <rule ref="Drupal.Commenting.InlineComment">
    <!-- Sniff for: NoSpaceBefore, WrongStyle -->
    <exclude name="Drupal.Commenting.InlineComment.DocBlock"/>
    <exclude name="Drupal.Commenting.InlineComment.InvalidEndChar"/>
    <exclude name="Drupal.Commenting.InlineComment.NotCapital"/>
    <exclude name="Drupal.Commenting.InlineComment.SpacingAfter"/>
    <exclude name="Drupal.Commenting.InlineComment.SpacingBefore"/>
  </rule>

When I see this I think this means that core wants to comply with all the Drupal.Commenting.InlineComment sniffs and there is a clear list of the ones that still need work. Makes a nice to do list.

Then I went back to consider this quote about Coder.

I'd rather opt-in than opt-out. coder doesn;'t really define the drupal coding standard.

I then went and read the project page for Coder. That say "Coder checks your Drupal code against coding standards and other best practices". Are any of these 'other best practices' included in Drupal sniffs and are not an approved sniff? I don't know and finding out seems to require research. And for now, I would rather spend time working on the existing coding standards issues than do that research.

So, sorry, I don't know enough about Coder to have a preference for what is done here.

edit: s/U/I

Hmmm, was also asked to drop my EUR 0.02 by @longwave on Slack.

As @quietone I'm a newbie in Code Sniffing Land.

Personally I like it like it is now, so opt-in, but that's not really based on anything.

One thing to (maybe) consider:

PHPCS is currently ran in the Custom Commands phase.
If a sniff would fail here, there's no feedback in the form of issue-status change or emails sent, see #3189649: Failing Custom Commands script doesn't change issue status from Needs Review to Needs Work.

In my (tiny) mind, this could mean that when updating Coder in Core, a sniff fails on some code and we won't be warned automagically.

Now most probably the person updating Coder will check on the TestBot results, so it's a bit hypothetical.

Nevertheless I would feel more at ease if #3189649: Failing Custom Commands script doesn't change issue status from Needs Review to Needs Work could be fixed before we do the opt-out thingy here.

Found a related issue, #2912811: Sniff definitions approach in phpcs.xml.dist: exclude vs severity

I bet it needs update as https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.6.0 added support for PHP 8

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I found out by accident that core has implemented some sniffs in phpcs.xml.dist that are missing in Coder. Opened #3496958: Adopt PHPCS config from Drupal core that is missing in Coder, Coder 8.3.27 is now in sync again.

The proposed solution in the issue summary here is good, we should do it exactly like that.

And core has a sniff that was removed from Coder, #3503268: Remove Drupal.Commenting.FunctionComment.TypeHintMissing from phpcs.xml.dist.

Recreated #7. I added with the entire Drupal and DrupalPractice sets except the rules we already exclude, removed any duplicates that are already in those sets, and added further exclusions for rules that we don't meet yet.

Given that we now pin to a specific version of Coder and also that our CI jobs are now much better than before, I think this is a better format for the file - it's easier to see which standards we don't adhere to yet, and we will never be surprised as we are in control of upgrading Coder.

Yes, lets do this.

Did some more cleanup on this; I've rearranged the file and many comments to make it easier to follow. It's easier to review the new version of the file instead of trying to read the diff here.

I discovered that Coder disables SlevomatCodingStandard.TypeHints.DeclareStrictTypes.DeclareStrictTypesMissing but we want it enabled for tests, so we explicitly re-enable it here. I've done a number of further checks by breaking coding standards in files and ensuring that things are still caught as expected.

I think the <file> section is wrong as the .sh files don't seem to get checked, but that can wait for a followup.

I don't have any remarks. I think this looks good. It does make it so that coder has to be a bit more careful when adding new rules I guess?

Apologies for the scope creep but SortTest was failing because it required exclude-pattern to be sorted at the top level; it's nicer to be able to group these with comments so I removed that test, but I also added additional tests to ensure include-pattern and exclude-pattern inside rules are correctly sorted, and fixed all cases where this was not met.

Appears to need a rebase

@longwave what's a good way for testing this one?

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

@longwave what's a good way for testing this one?

Not longwave, but I can answer that as well. phpcs -e shows you the list of sniffs being executed, so they need to be compared before/after checking out this branch. They should be the same.