You can check the current state of coding standards compliance using phpcs and coder from the command line:

$ cd /path/to/drupal/root/
$ composer install
$ cd core
$ ../vendor/bin/phpcs -ps

There are a few changes to automate code review for the different parts:

Origin

This is a followup of #1518116: [meta] Make Core pass Coder Review which was the original initiative from 2012 to fix coding standards in core. This new issue was made to replace the old one because the suggested workflow changed from a module-per-module approach to a sniff-per-sniff approach. The old issue has a large number of linked issues which are now obsolete.

Note that no coding standards fixes will be committed until the beta phase ends. Only when the first release candidate is released these issues will be eligible for committing.

Problem/Motivation

Drupal Core fails our automated coding standards tests.

Proposed resolution

It would be impossible to fix all coding standards at once. We will split up the work in manageable chunks. In a previous effort (#1518116: [meta] Make Core pass Coder Review) we split up the work per module, but this proved to be less than ideal since it requires intimate knowledge of ALL coding standards for everyone involved in the issue: developers as well as reviewers.

Instead we will now split up the work per type of coding standard. For example in separate issues we'll deal with trailing whitespace, inline comments, line lengths, etc. This will be much easier to review, since every patch will only contain very similar fixes.

In addition to fixing the coding standards, we'll also add automatic coding standards checking to the DrupalCI test bot. Once a particular coding standard is fixed, all new patches will be automatically checked and will fail if they do not meet this standard. This way we can progressively fix all coding standards, and ensure that what is once fixed will remain fixed.

An XML file (phpcs.xml.dist) was added to core to list all coding standards currently passing and it is used by the automated coding standards check. Whenever a particular standard is fixed we will add that sniff to phpcs.xml.dist or remove the exception from it, so it will be automatically checked from that point onwards.
Eventually phpcs.xml.dist would contain all sniffs defined in Drupal CS along with their configuration from ruleset.xml. All the sniffs defined in other CS and referenced in ruleset.xml should also be present in phpcs.xml.dist.
If rulesest.xml is modified or new sniffs are defined in Drupal CS, phpcs.xml.dist should also be modified.

Approach

  1. We analyzed the available "sniffs" and put them in related groups. The goal is that each group can be easily fixed and reviewed as a whole.
  2. Choose an unclaimed sniff from the Remaining Tasks section below and create a new issue for it. You can also work on one of the active child issues.
  3. If you are creating a new issue:
    • it's best to clone one of the child issues. Pay attention to Title, Status and Parent issue fields. Parent issue field should reference this issue.
    • You may also assign the newly created issue to yourself.
    • Edit this issue summary and remove the sniff from Remaining Tasks section.
    • As always, when editing an issue summary, you should add a comment to the issue saying what you edited, so that anyone following this issue will get an email. In this case, provide a link to the new issue you filed and state which sniff you are claiming.

Remaining tasks

This list contains all sniffs for which we don't have an issue yet.

  1. Drupal.Array.Array.ArrayClosingIndentation
  2. Drupal.Classes.PropertyDeclaration
  3. Drupal.Commenting.ClassComment.Missing
  4. Drupal.Commenting.DocComment.LongNotCapital
  5. Drupal.Commenting.DocComment.MissingShort
  6. Drupal.Commenting.DocComment.ParamNotFirst
  7. Drupal.Commenting.DocComment.ShortNotCapital
  8. Drupal.Commenting.DocComment.ShortSingleLine
  9. Drupal.Commenting.DocComment.TagGroupSpacing
  10. Drupal.Commenting.DocComment.TagsNotGrouped
  11. Drupal.Commenting.DocCommentAlignment
  12. Drupal.Commenting.VariableComment
  13. Drupal.Formatting.MultipleStatementAlignment
  14. Drupal.NamingConventions.ValidClassName
  15. Drupal.NamingConventions.ValidFunctionName
  16. Drupal.NamingConventions.ValidVariableName.LowerCamelName
  17. Drupal.Semantics.FunctionT.NotLiteralString
  18. Drupal.Semantics.FunctionT.ConcatString
  19. Generic.CodeAnalysis.UselessOverridingMethod
  20. PSR2.Classes.PropertyDeclaration.Underscore
  21. Squiz.PHP.NonExecutableCode
CommentFileSizeAuthor
#67 2571965_67.patch64.76 KBvaplas
#66 2571965_66.patch20.63 KBvaplas
#48 core-issues-DrupalStandard.latest.txt.gz405.97 KBanoopjohn
#9 2571965_9.patch34.39 KBMile23
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,724 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pfrenssen created an issue. See original summary.

larowlan’s picture

Please please consider automating this with github.com/grom358/pharborist - the library that powers drupal_module_upgrader

tatarbj’s picture

I'm starting to check which groups we can use for creating child tickets on this topic.
As we see in the Coder there are already groups under the Sniffs folder, but for checking maybe these are a bit too big ones.
The current structure is the following:

  • Array
  • CSS
  • Classes
  • Commenting
  • ControlStructures
  • Files
  • Formatting
  • Functions
  • InfoFiles
  • NamingConventions
  • Semantics
  • Strings
  • WhiteSpaces

Obviously if there is a ticket for fixing all of the 'Strings' it can be very easy to review the patch, but if you see the topic of 'Commenting' where 7 classes are, we should split it for sub-sub topics to make the review easier as we discusses with @pfrenssen and @xjm.
I'm diving to the folders to take some proposals about the groups.

pfrenssen’s picture

Please please consider automating this with github.com/grom358/pharborist - the library that powers drupal_module_upgrader

I've been doing some work with Pharborist lately and it could indeed be used to automatically fix coding standards, but it would require a script to be written for each type of fix.

There is also the Coder module which can be used to fix a large number of the coding standards automatically with PHP CodeSniffer.

For the more tricky cases we can certainly use Pharborist.

pfrenssen’s picture

Issue summary: View changes
tatarbj’s picture

First of all, there are the types of coding standards:
Errors which need to be fix by manual.
Errors which can be fixed by PHP Code Beautifier and Fixer.
Warnings which need to be fix by manual.
Warnings which can be fixed by PHPCBF.
In this case based on what i mentioned above, we should focus on the manually fixable coding standards which cannot be fixed by PHPCBF, i mean those issues what can be fixed automatically are easy to check by a review, so those can be in one task, the other ones are in separated tickets.
Here is the sniffs regarding the arrays and my proposal to split those to 3 tickets.

  1. If the line declaring an array spans longer than 80 characters, each element should be broken into its own line.
  2. Array closing indentation error, expected X spaces but found Y (Fixable Error)
    Array indentation error, expected X spaces but found Y (Fixable Error)
  3. A comma should follow the last multiline array item. (Fixable Warning)

All of the new tasks should contain at least one example regarding the fixable coding standards (obviously if the ticket contains more than 1 coding standard it has to have more examples) then all of the information about fixing of those should be there, like does it fixable by PHPCBF or not, is it an error or warning, stuff like this.

tatarbj’s picture

Array Sniff
Errors:

  • If the line declaring an array spans longer than 80 characters, each element should be broken into its own line.
  • Array closing indentation error, expected X spaces but found Y (Fixable Error)
  • Array indentation error, expected X spaces but found Y (Fixable Error)

Warnings:

  • A comma should follow the last multiline array item. (Fixable Warning)

CSS
Errors:

  • Multiple selectors should each be on a single line (Fixable Error)
  • Selectors must be on a single line (Fixable Error)
  • Blank lines are not allowed between class names (Fixable Error)
  • CSS colours must be defined in lowercase; expected X but found Y (Fixable Error)

Classes
Errors:

  • Calling class constructors must always include parentheses (Fixable Error)
  • Closing X brace must be on a line by itself
  • Opening brace must be the last content on the line (Fixable Error)
  • Opening brace should be on the same line as the declaration (Fixable Error)
  • Expected 1 space before opening brace; found X (Fixable Error)
  • The closing brace for the X must have an empty line before it (Fixable Error)

Warnings:

  • Possible parse error: X missing opening or closing brace
  • Interface names should always have the suffix “Interface”

Commenting
Errors:

  • Missing X doc comment (Fixable Error, but it will generate another coding standard bug which needs to be fixed by manual!)
  • You must use "/**" style comments for a X comment (Fixable error)
  • There must be exactly one newline after the X comment (Fixable error)
  • Doc comment is empty
  • The open comment tag must be the only content on the line (Fixable error)
  • Wrong function doc comment end; expected "*/", found “X”
  • Additional blank lines found at end of doc comment (Fixable error)
  • Missing short description in doc comment
  • Doc comment short description must be on the first line (Fixable error)
  • Function comment short description must start with exactly one space (Fixable error)
  • Doc comment short description must start with a capital letter (Fixable error)
  • Doc comment short description must end with a full stop (Fixable error)
  • Doc comment short description must be on a single line, further text should be a separate paragraph
  • There must be exactly one blank line between descriptions in a doc comment (Fixable error)
  • Doc comment long description must start with a capital letter (Fixable error)
  • Doc comment long description must end with a full stop (Fixable error)
  • There must be exactly one blank line before the tags in a doc comment (Fixable error)
  • Parameter tags must be grouped together in a doc comment
  • Separate the X and Y sections by a blank line. (Fixable error)
  • There must be a single blank line after a tag group (Fixable error)
  • Tag value indented incorrectly; expected 1 space but found X (Fixable error)
  • Parameter tags must be defined first in a doc comment
  • Tags must be grouped together in a doc comment
  • Doc comment star missing (Fixable error)
  • You must use "/**" style comments for a file comment (Fixable error)
  • Missing file doc comment (Fixable error, but it will generate another coding standard bug which needs to be fixed by manual!)
  • The second line in the file doc comment must be “@file" (Fixable error, but it will generate another coding standard bug which needs to be fixed by manual!)
  • There must be exactly one blank line after the file comment
  • Missing function doc comment (Fixable error, but it will generate another coding standard bug which needs to be fixed by manual!)
  • You must use "/**" style comments for a function comment
  • There must be no blank lines after the function comment
  • Content missing for @see tag in function comment
  • Only 1 @return tag is allowed in a function comment
  • Return type missing for @return tag in function comment
  • Function return type "X" is invalid, Expected "X" but found "X" for function return type.

Warnings:
-

ControlStructures
Errors:
Warnings:

Files
Errors:
Warnings:

Formatting
Errors:
Warnings:

Functions
Errors:
Warnings:

InfoFiles
Errors:
Warnings:

NamingConventions
Errors:
Warnings:

Semantics
Errors:
Warnings:

Strings
Errors:
Warnings:

WhiteSpaces
Errors:
Warnings:

andypost’s picture

Category: Task » Plan
Parent issue: » #1518116: [meta] Make Core pass Coder Review
Mile23’s picture

Status: Active » Needs review
FileSize
34.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,724 pass(es). View

We can say:

phpcs --standard=/path/to/coder/coder_sniffer/Drupal -e

This gives us a list of all the available sniffs. Let's check for Drupal.Files.EndFileNewline which tells us whether the file has only one newline at the end. This is easy to apply without requiring a lot of rerolls.

And while we're at it, have phpcs generate a patch for us. (Really it's a diff, so we have to apply it and then have git make a patch, but still...)

$ phpcs --standard=/path/to/coder/coder_sniffer/Drupal --sniffs=Drupal.Files.EndFileNewline --ignore=vendor,assets/vendor --report-diff=file_end.diff -p core/
$ patch -p0 -ui file_end.diff
$ git diff > patch_name.patch

This gets us pretty far, but it does weird stuff like this, which I believe is an artifact of how phpcs makes its diff file:

diff --git a/core/misc/dialog.theme.css b/core/misc/dialog.theme.css
index 837e49f..bc3efe3 100644
--- a/core/misc/dialog.theme.css
+++ b/core/misc/dialog.theme.css
@@ -6,7 +6,7 @@
   position: absolute;
   z-index: 1260;
   overflow: visible;
-  color: #000;
+  color: 1000;
   background: #fff;
   border: solid 1px #ccc;
   padding: 0;
@@ -68,4 +68,3 @@
 .ui-dialog .ajax-progress-throbber .message {
   display: none;
 }
-

The workaround is to review the patch. :-) We might also exclude css from review, though we shouldn't.

So having fixed those things, here's a patch to fix violations of the one-newline-at-the-end-of-the-file rule for the core/ directory. Presented here as a proof of concept for mulling over, or, you know, committing. :-)

Will set back to 'Active' once the test is over.

attiks’s picture

#9 Patch looks clean, but why not use git to create the patch?

Mile23’s picture

Status: Needs review » Active

@attiks: The point here is that I used a tool to automatically generate most of the fixes. And I did use git to generate the patch as a last step.

attiks’s picture

#11 You're right, I was assuming phpcbf had an option to limit the sniffs as well

attiks’s picture

tatarbj’s picture

@Mile23 i think we should focus only the php files instead of checking css and js as well. i'll create those tickets tomorrow with mentioning this as a parent one, but keep this issue to discuss about the topic instead of putting patches here.
@attiks you can use git instead of patch command to generate the patch file, i don't get why we need diff, i mean when a coding standard, a sniff can be fixed by phpcbf it's good, it can be very easy to review it, then no need to wait for years to see it in the CI test bot and obviously in the core :)
The only problem with the sniffs sometimes those has tons of coding standards (like the comments and functions packages) which are not that easy to review it in one step. The origin idea regarding check the modules and bigger part of the core one-by-one was cancelled because for instance there is the block module which still have at least 15 different coding standards, if you've cleaned it up then made the patch file a simple person who is not a big fan of this topic (like we are) wants to review it to help us he will scream a lot just about the different topic, the first one is one case, the second is another, the third is one another, it makes him crazy :) So that's why we should split somehow those sniffs to smaller, more direct, more easier to check parts.

pfrenssen’s picture

Our coding standards also cover css and javascript, so in my opinion it's OK to have them in scope for this issue. I'm totally fine with focusing on PHP first though.

attiks’s picture

There are better tools for CSS and js, so +1 to focus on php only

pfrenssen’s picture

I really like #9 as a proof of concept. I was able to review this patch in 1.87 seconds.

Printing out the sniffs with the phpcs -e trick from #9 yields 54 sniffs. It would be doable to use this as a basis for creating separate issues. Note that many of these sniffs can do a great number of checks so we might have to divide some a bit further to improve reviewability.

  • Drupal.Array.Array
  • Drupal.CSS.ClassDefinitionNameSpacing
  • Drupal.CSS.ColourDefinition
  • Drupal.Classes.ClassCreateInstance
  • Drupal.Classes.ClassDeclaration
  • Drupal.Classes.InterfaceName
  • Drupal.Commenting.ClassComment
  • Drupal.Commenting.DocComment
  • Drupal.Commenting.DocCommentStar
  • Drupal.Commenting.FileComment
  • Drupal.Commenting.FunctionComment
  • Drupal.Commenting.HookComment
  • Drupal.Commenting.InlineComment
  • Drupal.ControlStructures.ControlSignature
  • Drupal.ControlStructures.ElseIf
  • Drupal.ControlStructures.InlineControlStructure
  • Drupal.ControlStructures.TemplateControlStructure
  • Drupal.Files.EndFileNewline
  • Drupal.Files.LineLength
  • Drupal.Files.TxtFileLineLength
  • Drupal.Formatting.MultiLineAssignment
  • Drupal.Formatting.SpaceInlineIf
  • Drupal.Formatting.SpaceUnaryOperator
  • Drupal.Functions.DiscouragedFunctions
  • Drupal.Functions.FunctionDeclaration
  • Drupal.InfoFiles.ClassFiles
  • Drupal.InfoFiles.DuplicateEntry
  • Drupal.InfoFiles.Required
  • Drupal.NamingConventions.KeywordLowerCase
  • Drupal.NamingConventions.ValidClassName
  • Drupal.NamingConventions.ValidFunctionName
  • Drupal.NamingConventions.ValidGlobal
  • Drupal.NamingConventions.ValidVariableName
  • Drupal.Semantics.ConstantName
  • Drupal.Semantics.EmptyInstall
  • Drupal.Semantics.FunctionAlias
  • Drupal.Semantics.FunctionT
  • Drupal.Semantics.FunctionWatchdog
  • Drupal.Semantics.InstallHooks
  • Drupal.Semantics.LStringTranslatable
  • Drupal.Semantics.PregSecurity
  • Drupal.Semantics.RemoteAddress
  • Drupal.Semantics.TInHookMenu
  • Drupal.Semantics.TInHookSchema
  • Drupal.Strings.UnnecessaryStringConcat
  • Drupal.WhiteSpace.CloseBracketSpacing
  • Drupal.WhiteSpace.Comma
  • Drupal.WhiteSpace.EmptyLines
  • Drupal.WhiteSpace.ObjectOperatorIndent
  • Drupal.WhiteSpace.ObjectOperatorSpacing
  • Drupal.WhiteSpace.OpenBracketSpacing
  • Drupal.WhiteSpace.OperatorSpacing
  • Drupal.WhiteSpace.ScopeClosingBrace
  • Drupal.WhiteSpace.ScopeIndent
attiks’s picture

I created a github repo at https://github.com/attiks/Fix-coding-standards-in-core, PR's welcome.

Best to keep the discussion in this issue

attiks’s picture

I'm running into coder issues from time to time, so we probably will need to fix some things upstream.

attiks’s picture

Some patches are huge, we need to find a way to split these in smaller chunks, but before we can do it, we need an automatic way to run this. #2572635: Fix 'Drupal.Commenting.DocComment' coding standard is 500K, we might be able to split it for each module, but this will give us 50+ issues for 1 sniff.

tatarbj’s picture

@attiks there is a chance to add an xml file to drupal core in the future what will contain a black list with all of the sniffs in the beginning, then when one issue is solved and committed to the core we just remove that sniff from the list and ci test bot will know it can check already that one, and so on - but i see the creativity of your github repo, i think it can be a good start for using phpcbf but it won't work with those issue which are not solvable via this way but need manual fixes. That's why i started to collect all of the "topics" based on the sniffs, but as you saw there are couple of it which not that easy to review (look at the reaction under #2572635: Fix 'Drupal.Commenting.DocComment' coding standard and unfortunately there are some fake fixes what shouldn't be part of the core like semicolon then a dot and stuff like this.

attiks’s picture

#21 I saw your comment, I just wanted to find out how much could be automated, I briefly spoke to Klausi and he asked to file issues against https://www.drupal.org/project/issues/coder and cross link them with our issue, so once it gets fixed upstream we can use it.

tatarbj’s picture

Sure, very good idea to create a link between this plan and the Coder! Maybe we should find someone from the Documentation team as well because i'm not sure all of the sniffs what Coder has documented in the Coding standards area, it would be great when someone fix a sniff (or a part of a sniff) in the same step the coding standard docs will be also checked here.

attiks’s picture

All auto fixes are ran,

- 'Drupal.Array.Array' => 2572613, // @see https://www.drupal.org/node/2572587
- 'Drupal.CSS.ClassDefinitionNameSpacing' => 0, // Skipping css
- 'Drupal.CSS.ColourDefinition' => 0, // Skipping css
- 'Drupal.Classes.ClassCreateInstance' => 2572601,
- 'Drupal.Classes.ClassDeclaration' => 2572619,
- 'Drupal.Classes.InterfaceName' => 2572631,
- 'Drupal.Commenting.ClassComment' => 2572633,
- 'Drupal.Commenting.DocComment' => 2572635,
- 'Drupal.Commenting.DocCommentStar' => 2572641,
- 'Drupal.Commenting.FileComment' => 2572643,
- 'Drupal.Commenting.FunctionComment' => 2572645,
- 'Drupal.Commenting.HookComment' => 2572649, // No fixable errors were found
- 'Drupal.Commenting.InlineComment' => 2572659,
- 'Drupal.ControlStructures.ControlSignature' => 2572693,
- 'Drupal.ControlStructures.ElseIf' => 2572695,
- 'Drupal.ControlStructures.InlineControlStructure' => 2572699,
- 'Drupal.ControlStructures.TemplateControlStructure' => 2572701,
- 'Drupal.Files.EndFileNewline' => 2572707,
- 'Drupal.Files.LineLength' => 2572709,
- 'Drupal.Files.TxtFileLineLength' => 0,
- 'Drupal.Formatting.MultiLineAssignment' => 0, // No fixable errors were found
- 'Drupal.Formatting.SpaceInlineIf' => 0, // No fixable errors were found
- 'Drupal.Formatting.SpaceUnaryOperator' => 2572731,
- 'Drupal.Functions.DiscouragedFunctions' => 0, // No fixable errors were found
- 'Drupal.Functions.FunctionDeclaration' => 0, // No fixable errors were found
- 'Drupal.InfoFiles.ClassFiles' => 0, // No fixable errors were found
- 'Drupal.InfoFiles.Required' => 0, // No fixable errors were found
- 'Drupal.NamingConventions.KeywordLowerCase' => 0, // No fixable errors were found
- 'Drupal.NamingConventions.ValidClassName' => 0, // No fixable errors were found
- 'Drupal.NamingConventions.ValidFunctionName' => 0, // No fixable errors were found
- 'Drupal.NamingConventions.ValidGlobal' => 0, // No fixable errors were found
- 'Drupal.NamingConventions.ValidVariableName' => 0, // No fixable errors were found
- 'Drupal.Semantics.ConstantName' => 0, // No fixable errors were found
- 'Drupal.Semantics.EmptyInstall' => 0, // No fixable errors were found
- 'Drupal.Semantics.FunctionAlias' => 0, // No fixable errors were found
- 'Drupal.Semantics.FunctionT' => 0, // No fixable errors were found
- 'Drupal.Semantics.FunctionWatchdog' => 0, // No fixable errors were found
- 'Drupal.Semantics.InstallHooks' => 0, // No fixable errors were found
- 'Drupal.Semantics.InstallT' => 0, // No fixable errors were found
- 'Drupal.Semantics.LStringTranslatable' => 0, // No fixable errors were found
- 'Drupal.Semantics.PregSecurity' => 0, // No fixable errors were found
- 'Drupal.Semantics.RemoteAddress' => 0, // No fixable errors were found
- 'Drupal.Semantics.TInHookMenu' => 0, // No fixable errors were found
- 'Drupal.Semantics.TInHookSchema' => 0, // No fixable errors were found
- 'Drupal.Strings.ConcatenationSpacing' => 2572777,
- 'Drupal.Strings.UnnecessaryStringConcat' => 0, // No fixable errors were found
- 'Drupal.WhiteSpace.CloseBracketSpacing' => 2572787,
- 'Drupal.WhiteSpace.Comma' => 2572787,
- 'Drupal.WhiteSpace.EmptyLines' => 0,
- 'Drupal.WhiteSpace.ObjectOperatorIndent' => 0,
- 'Drupal.WhiteSpace.ObjectOperatorSpacing' => 0,
- 'Drupal.WhiteSpace.OpenBracketSpacing' => 2572791,
- 'Drupal.WhiteSpace.OperatorSpacing' => 2572793,
- 'Drupal.WhiteSpace.ScopeClosingBrace' => 2572795,
- 'Drupal.WhiteSpace.ScopeIndent' => 2572801,
- 'Generic.PHP.UpperCaseConstant' => 2572307,

pfrenssen’s picture

Wow this is really taking off like a rocket :D

attiks’s picture

We need to add our own phpcs.xml file to the root (or only in core?), more info can be found at https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml

Initially we blacklist all rules one by one and commit it asap, see #2573377: Add our own phpcs.xml file

We probably need to update composer.json as well to pull in code sniffer

{
    "require": {
        "drupal/coder": "~8.2",
        "fabpot/php-cs-fixer": "~1.0",
        "squizlabs/php_codesniffer": "*"
    }
}
tatarbj’s picture

@attiks did you used 8.x-2.3 of coder? I'm checking those issues what you created yesterday, but all of them has other issues what your patches don't fix - as you see the released version was updated in June and the latest version of it just couple of days old in the dev version, i always use it to make sure all of our new fixes are inside the process, and as i see under the #2572635: Fix 'Drupal.Commenting.DocComment' coding standard there is a lot of other issues what your patch didn't fix, btw the patches are also failed :(

Mile23’s picture

So are we fixing coding standards, or are we including a fixer tool in core?

pfrenssen’s picture

I wouldn't add PHP CodeSniffer itself to core, since core doesn't need it to function. Having the phpcs.xml file will be enough, we can install PHP CodeSniffer on the testbot.

Mile23’s picture

I'd suggest that the turnaround on changes to the testbot will be a relatively long time.

We can make changes in child issues here per the meta and then review them.

pfrenssen’s picture

YesCT’s picture

Status: Active » Postponed

postponed on testbot being able to test for the rule that each child will try and fix. #1266444: DrupalCI should return failed for poor code style - php

Mixologic’s picture

Status: Postponed » Active

Im un-postponing this. Having the testbots run this will act as a gatekeeper going forward to prevent new patches from going in with coding standards violations, but having the testbots run phpcs isnt a requirement for fixing existing code standards violations. Everybody who has an IDE should be able to configure their environment for code standards checks, and everybody else can run phpcs from the command line. This will be implemented on the testbots eventually, but it might take a while to get this in, as there is still a lot of work to do to add it.

In the meantime, even new patches can be reviewed by folks locally - i.e. theres really nothing special or more official that the testbots are going to do that cannot be done locally.

pfrenssen’s picture

Issue tags: +rc eligible

Good plan. We can keep on top of it easily during RC phase if we just ensure that all coding standards related patches fully pass. We can catch any accidentally committed coding standards violations this way.

xjm’s picture

That makes sense to me. In that case, let's enable each rule in the XML file as core becomes compliant? Should we create a followup for each fix? See #2572695: Fix 'Drupal.ControlStructures.ElseIf' coding standard.

xjm’s picture

Also, child issues can be tagged "rc eligible" so committers can find them when they are RTBC. If a rule is risky or disruptive we may choose to postpone it, but we can handle that as part of the normal patch review process.

YesCT’s picture

the summary talks about a blacklist which we would remove rules from one at a time. which in general is the idea. but if coder added some rules, it might get confusingly auto enabled, since it isn't in the blacklist. might be better to have an explicit whitelist, so only rules we have really thought about are tested. (maybe this is what we mean, and the words are just a bit confusing)

alexpott’s picture

Re #35 I don't get why we need a followup to remove the exclude - surely the patch that fixes so we're compliant should enable the rule. Doing that and running phpcs should be part of the proof that the patch is ready. It also makes it possible to combine running phpcs with a commit workflow and ensure we don't introduce regressions.

andypost’s picture

I think that better to enable the integration to drupalci already to prevent accepting patches that break already fixed at least

alexpott’s picture

@andypost well obviously we want DrupalCI integration but wait for that is the reason why we don't do things. So having a phpcs.xml.dist that reflects what we expect to pass makes sense to me because then I can adapt my commit hooks to scan every changed file.

pfrenssen’s picture

There was some work done at Drupalcon Barcelona to enable this in DrupalCI and we even got an issue to RTBC but it has stalled in the queue: #1266444: DrupalCI should return failed for poor code style - php. Probably it needs to be reworked by now.

elachlan’s picture

There are a few changes to automate code review for the different parts:

Most of them are held up by container changes. Which are in turn held up by #2580007: Add phantomjs2 to the testrunners, which is a child issue of #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary.

We are attempting to make changes easier to make to the containers with #2609560: Base DCI containers off official containers.

We really just need more people working on it all.

alexpott’s picture

Created #2623680: Fix phpcs.xml.dist to work with the latest rules and not have unnecessary lines to update the phpcs.xml.dist to represent the current state of things

YesCT’s picture

Issue summary: View changes

adding information about what is blocking this to the summary.

YesCT’s picture

Issue tags: +coding standards
YesCT’s picture

Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anoopjohn’s picture

I ran the 8.2.x branch of core against the 8.x.2.x branch (head) of the Drupal standards and got the following set of errors. Although this issue is against 8.1.x branch I wanted to see if we need to have a comprehensive list of coder issues that we have to fix.

This was run using

phpcs -s --standard=Drupal --ignore=core/assets/vendor/* --ignore=core/modules/system/tests/fixtures/HtaccessTest --extensions=inc,install,module,php,profile,test,theme core

I have attached the list of errors and also a summary of the list of errors below

7860	Drupal.WhiteSpace.CloseBracketSpacing.ClosingWhitespace
7797	Drupal.WhiteSpace.OpenBracketSpacing.OpeningWhitespace
6585	Generic.WhiteSpace.DisallowTabIndent.NonIndentTabsUsed
3045	Drupal.WhiteSpace.ScopeIndent.Incorrect
1542	Drupal.Commenting.InlineComment.InvalidEndChar
1476	Drupal.Commenting.FunctionComment.Missing
1070	Squiz.Scope.MethodScope.Missing
1046	Squiz.WhiteSpace.FunctionSpacing.After
930	Drupal.Commenting.FunctionComment.MissingParamType
926	Drupal.Commenting.DocComment.MissingShort
855	Drupal.Classes.ClassDeclaration.CloseBraceAfterBody
815	Generic.WhiteSpace.DisallowTabIndent.TabsUsed
448	Drupal.Commenting.DocComment.ShortSingleLine
396	Drupal.Commenting.FunctionComment.MissingReturnComment
373	Drupal.WhiteSpace.OperatorSpacing.NoSpaceBefore
362	Drupal.WhiteSpace.OperatorSpacing.NoSpaceAfter
359	Squiz.CSS.ClassDefinitionClosingBraceSpace.ContentBeforeClose
359	Squiz.CSS.ClassDefinitionOpeningBraceSpace.ContentBefore
336	Squiz.ControlStructures.SwitchDeclaration.SpacingAfterBreak
287	Drupal.Commenting.InlineComment.SpacingAfter
267	Drupal.ControlStructures.ControlSignature.NewlineAfterCloseBrace
254	Drupal.Commenting.DocComment.TagGroupSpacing
235	Drupal.Commenting.FunctionComment.MissingReturnType
228	Squiz.Commenting.DocCommentAlignment.SpaceAfterStar
202	Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps
183	Drupal.Array.Array.CommaLastItem
176	Drupal.WhiteSpace.ScopeClosingBrace.Line
176	Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines
174	Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine
169	Drupal.Commenting.PostStatementComment.Found
167	Drupal.Commenting.DocComment.TagsNotGrouped
161	Drupal.Commenting.ClassComment.Missing
149	Squiz.WhiteSpace.FunctionSpacing.Before
145	Drupal.Commenting.FunctionComment.ParamCommentFullStop
142	Drupal.NamingConventions.ValidVariableName.LowerCamelName
139	Squiz.Strings.ConcatenationSpacing.PaddingFound
125	Drupal.ControlStructures.InlineControlStructure.NotAllowed
120	Drupal.WhiteSpace.OperatorSpacing.SpacingAfter
115	Drupal.Commenting.FunctionComment.MissingParamComment
115	Drupal.Commenting.InlineComment.NotCapital
106	Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar
98	Generic.PHP.UpperCaseConstant.Found
96	Drupal.Commenting.InlineComment.SpacingBefore
90	Drupal.Files.EndFileNewline.TooMany
88	Drupal.NamingConventions.ValidFunctionName.NotCamelCaps
87	Drupal.Commenting.DocComment.SpacingBeforeTags
81	Drupal.Commenting.InlineComment.DocBlock
77	Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing
77	Squiz.CSS.SemicolonSpacing.NotAtEnd
69	Drupal.Array.Array.ArrayClosingIndentation
67	Drupal.Commenting.FunctionComment.TypeHintMissing
61	Drupal.Commenting.InlineComment.NoSpaceBefore
61	Squiz.WhiteSpace.SemicolonSpacing.Incorrect
59	Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace
57	PSR2.Classes.PropertyDeclaration.ScopeMissing
56	Squiz.Arrays.ArrayDeclaration.FirstIndexNoNewline
55	Drupal.WhiteSpace.EmptyLines.EmptyLines
54	Drupal.Semantics.FunctionT.ConcatString
49	Drupal.Commenting.DocComment.ShortNotCapital
45	Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeFirst
44	Drupal.ControlStructures.ControlSignature.SpaceAfterKeyword
43	Drupal.Semantics.FunctionT.NotLiteralString
40	Drupal.Commenting.FunctionComment.ReturnCommentIndentation
39	Drupal.Commenting.FunctionComment.MissingParamName
38	Drupal.Commenting.FunctionComment.ParamNameNoMatch
37	Drupal.WhiteSpace.ScopeIndent.IncorrectExact
36	Drupal.Strings.UnnecessaryStringConcat.Found
36	Squiz.PHP.NonExecutableCode.Unreachable
31	Drupal.Commenting.FunctionComment.IncorrectTypeHint
31	Drupal.CSS.ClassDefinitionNameSpacing.MultipleSelectors
30	Drupal.NamingConventions.ValidFunctionName.InvalidName
30	Generic.Functions.OpeningFunctionBraceKernighanRitchie.BraceOnNewLine
29	Squiz.Arrays.ArrayDeclaration.CommaAfterLast
27	Squiz.Arrays.ArrayDeclaration.SpaceAfterKeyword
26	Drupal.CSS.ColourDefinition.NotLower
26	PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse
25	Drupal.Commenting.DocComment.SpacingAfter
24	Drupal.Commenting.FunctionComment.SeePunctuation
24	Squiz.Arrays.ArrayDeclaration.SpaceInEmptyArray
24	Squiz.WhiteSpace.SuperfluousWhitespace.StartFile
23	Drupal.ControlStructures.ControlSignature.SpaceAfterCloseParenthesis
23	PSR2.Classes.PropertyDeclaration.Underscore
23	Squiz.CSS.DisallowMultipleStyleDefinitions.Found
22	Drupal.Commenting.FunctionComment.InvalidNoReturn
22	Drupal.ControlStructures.ControlSignature.NewlineAfterOpenBrace
21	Drupal.Classes.ClassDeclaration.SpaceBeforeBrace
20	Drupal.WhiteSpace.ScopeClosingBrace.Indent
18	Drupal.Commenting.FileComment.Missing
18	Drupal.CSS.ClassDefinitionNameSpacing.SeletorSingleLine
17	Drupal.Commenting.DocComment.LongNotCapital
16	Drupal.Commenting.DocComment.ShortStartSpace
15	Generic.Functions.FunctionCallArgumentSpacing.SpaceBeforeComma
15	Generic.Functions.OpeningFunctionBraceKernighanRitchie.SpaceAfterBracket
15	Squiz.Arrays.ArrayDeclaration.NoSpaceAfterDoubleArrow
14	Squiz.Arrays.ArrayDeclaration.SpaceAfterDoubleArrow
13	PEAR.Functions.ValidDefaultValue.NotAtEnd
13	Squiz.CSS.ClassDefinitionOpeningBraceSpace.NoneBefore
12	Drupal.Classes.ClassCreateInstance.ParenthesisMissing
12	Drupal.Files.EndFileNewline.NoneFound
12	Squiz.Arrays.ArrayDeclaration.NoSpaceBeforeDoubleArrow
11	PEAR.Functions.FunctionCallSignature.SpaceBeforeOpenBracket
10	Drupal.Commenting.FunctionComment.ParamCommentNewLine
9	Drupal.Commenting.FunctionComment.InvalidReturnNotVoid
9	PSR2.Namespaces.UseDeclaration.UseAfterNamespace
8	Drupal.Commenting.FunctionComment.IncorrectParamVarName
8	Drupal.Commenting.FunctionComment.$InReturnType
8	Drupal.Commenting.InlineComment.TabBefore
8	Drupal.Semantics.FunctionT.BackslashSingleQuote
8	Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeSecond
8	Squiz.WhiteSpace.SuperfluousWhitespace.EndFile
7	Drupal.Commenting.FunctionComment.SpacingAfterParamType
7	Drupal.Commenting.FunctionComment.WrongStyle
7	Drupal.WhiteSpace.OperatorSpacing.NoSpaceAfterAmp
7	Drupal.WhiteSpace.OperatorSpacing.NoSpaceBeforeAmp
7	PEAR.Functions.FunctionCallSignature.SpaceAfterCloseBracket
7	Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma
6	Drupal.CSS.ClassDefinitionNameSpacing.BlankLinesFound
6	Squiz.Arrays.ArrayBracketSpacing.SpaceBeforeBracket
6	Squiz.CSS.EmptyStyleDefinition.Found
5	Drupal.Classes.ClassDeclaration.SpaceAfterName
5	Drupal.Commenting.FunctionComment.ThrowsComment
5	Drupal.ControlStructures.ControlSignature.SpaceBeforeSemicolon
5	Drupal.NamingConventions.ValidVariableName.LowerStart
5	Squiz.Arrays.ArrayDeclaration.SpaceBeforeDoubleArrow
5	Squiz.Commenting.DocCommentAlignment.NoSpaceAfterStar
4	Drupal.Commenting.FileComment.SpacingAfterComment
4	Drupal.Commenting.FunctionComment.ParamCommentNotCapital
4	Drupal.Commenting.FunctionComment.ThrowsNoFullStop
4	Drupal.Formatting.SpaceInlineIf.SpaceInlineElse
3	Drupal.Classes.InterfaceName.InterfaceSuffix
3	Drupal.Commenting.DocComment.SpacingBeforeShort
3	Drupal.ControlStructures.ControlSignature.SpaceAfterCloseBrace
3	Drupal.Files.LineLength.TooLong
3	Drupal.Functions.FunctionDeclaration.SpaceBeforeParenthesis
3	Drupal.Semantics.FunctionAlias.FunctionAlias
3	Squiz.ControlStructures.ForEachLoopDeclaration.SpacingBeforeAs
2	Drupal.Classes.ClassDeclaration.BraceOnNewLine
2	Drupal.Commenting.DocComment.SpacingAfterTagGroup
2	Drupal.Commenting.DocComment.SpacingBetween
2	Drupal.Commenting.FunctionComment.InvalidTypeHint
2	Drupal.Commenting.FunctionComment.SeeAdditionalText
2	Drupal.Formatting.MultiLineAssignment
2	Drupal.Formatting.SpaceUnaryOperator.BooleanNot
2	Drupal.NamingConventions.ValidClassName.StartWithCaptial
2	Drupal.WhiteSpace.ScopeClosingBrace.BreakIdent
2	Generic.Formatting.SpaceAfterCast.NoSpace
2	Generic.NamingConventions.ConstructorName.OldStyle
2	Squiz.Arrays.ArrayBracketSpacing.SpaceAfterBracket
2	Squiz.Arrays.ArrayDeclaration.NoKeySpecified
2	Squiz.ControlStructures.ForEachLoopDeclaration.NoSpaceAfterArrow
2	Squiz.ControlStructures.SwitchDeclaration.SpacingAfterDefault
2	Squiz.CSS.ClassDefinitionOpeningBraceSpace.Before
2	Squiz.CSS.EmptyClassDefinition.Found
2	Squiz.CSS.Indentation.BlankLine
2	Squiz.CSS.SemicolonSpacing.SpaceFound
2	Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceAfterDefault
2	Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeEquals
2	Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose
2	Squiz.PHP.NonExecutableCode.ReturnNotRequired
1	Drupal.Classes.ClassDeclaration.SpaceBeforeExtends
1	Drupal.Classes.ClassDeclaration.SpaceBeforeImplements
1	Drupal.Commenting.DocComment.ParamNotFirst
1	Drupal.Commenting.DocComment.TagValueIndent
1	Drupal.Commenting.FileComment.NamespaceNoFileDoc
1	Drupal.Commenting.FunctionComment.InvalidReturn
1	Drupal.Commenting.FunctionComment.SpacingAfter
1	Drupal.Commenting.FunctionComment.ThrowsNotCapital
1	Drupal.Commenting.FunctionComment.VoidReturn
1	Drupal.Formatting.SpaceUnaryOperator.IncDecLeft
1	Drupal.Functions.FunctionDeclaration.SpaceAfter
1	Drupal.NamingConventions.ValidGlobal.GlobalUnderScore
1	Drupal.Semantics.FunctionT.Concat
1	Drupal.Semantics.RemoteAddress.RemoteAddress
1	Drupal.WhiteSpace.ObjectOperatorSpacing.After
1	Squiz.ControlStructures.ForLoopDeclaration.NoSpaceAfterFirst
1	Squiz.ControlStructures.ForLoopDeclaration.NoSpaceAfterSecond
1	Squiz.ControlStructures.SwitchDeclaration.SpaceBeforeColonCase
1	Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma
1	Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterOpenHint
1	Squiz.WhiteSpace.LanguageConstructSpacing.Incorrect
1	Squiz.WhiteSpace.LanguageConstructSpacing.IncorrectSingle
1	Squiz.WhiteSpace.SuperfluousWhitespace.EndLine

The list of errors alphabetically

Drupal.Array.Array.ArrayClosingIndentation
Drupal.Array.Array.CommaLastItem
Drupal.Classes.ClassCreateInstance.ParenthesisMissing
Drupal.Classes.ClassDeclaration.BraceOnNewLine
Drupal.Classes.ClassDeclaration.CloseBraceAfterBody
Drupal.Classes.ClassDeclaration.SpaceAfterName
Drupal.Classes.ClassDeclaration.SpaceBeforeBrace
Drupal.Classes.ClassDeclaration.SpaceBeforeExtends
Drupal.Classes.ClassDeclaration.SpaceBeforeImplements
Drupal.Classes.FullyQualifiedNamespace.UseStatementMissing
Drupal.Classes.InterfaceName.InterfaceSuffix
Drupal.Commenting.ClassComment.Missing
Drupal.Commenting.DocComment.LongNotCapital
Drupal.Commenting.DocComment.MissingShort
Drupal.Commenting.DocComment.ParamNotFirst
Drupal.Commenting.DocComment.ShortNotCapital
Drupal.Commenting.DocComment.ShortSingleLine
Drupal.Commenting.DocComment.ShortStartSpace
Drupal.Commenting.DocComment.SpacingAfter
Drupal.Commenting.DocComment.SpacingAfterTagGroup
Drupal.Commenting.DocComment.SpacingBeforeShort
Drupal.Commenting.DocComment.SpacingBeforeTags
Drupal.Commenting.DocComment.SpacingBetween
Drupal.Commenting.DocComment.TagGroupSpacing
Drupal.Commenting.DocComment.TagsNotGrouped
Drupal.Commenting.DocComment.TagValueIndent
Drupal.Commenting.FileComment.Missing
Drupal.Commenting.FileComment.NamespaceNoFileDoc
Drupal.Commenting.FileComment.SpacingAfterComment
Drupal.Commenting.FunctionComment.$InReturnType
Drupal.Commenting.FunctionComment.IncorrectParamVarName
Drupal.Commenting.FunctionComment.IncorrectTypeHint
Drupal.Commenting.FunctionComment.InvalidNoReturn
Drupal.Commenting.FunctionComment.InvalidReturn
Drupal.Commenting.FunctionComment.InvalidReturnNotVoid
Drupal.Commenting.FunctionComment.InvalidTypeHint
Drupal.Commenting.FunctionComment.Missing
Drupal.Commenting.FunctionComment.MissingParamComment
Drupal.Commenting.FunctionComment.MissingParamName
Drupal.Commenting.FunctionComment.MissingParamType
Drupal.Commenting.FunctionComment.MissingReturnComment
Drupal.Commenting.FunctionComment.MissingReturnType
Drupal.Commenting.FunctionComment.ParamCommentFullStop
Drupal.Commenting.FunctionComment.ParamCommentNewLine
Drupal.Commenting.FunctionComment.ParamCommentNotCapital
Drupal.Commenting.FunctionComment.ParamNameNoMatch
Drupal.Commenting.FunctionComment.ReturnCommentIndentation
Drupal.Commenting.FunctionComment.SeeAdditionalText
Drupal.Commenting.FunctionComment.SeePunctuation
Drupal.Commenting.FunctionComment.SpacingAfter
Drupal.Commenting.FunctionComment.SpacingAfterParamType
Drupal.Commenting.FunctionComment.ThrowsComment
Drupal.Commenting.FunctionComment.ThrowsNoFullStop
Drupal.Commenting.FunctionComment.ThrowsNotCapital
Drupal.Commenting.FunctionComment.TypeHintMissing
Drupal.Commenting.FunctionComment.VoidReturn
Drupal.Commenting.FunctionComment.WrongStyle
Drupal.Commenting.InlineComment.DocBlock
Drupal.Commenting.InlineComment.InvalidEndChar
Drupal.Commenting.InlineComment.NoSpaceBefore
Drupal.Commenting.InlineComment.NotCapital
Drupal.Commenting.InlineComment.SpacingAfter
Drupal.Commenting.InlineComment.SpacingBefore
Drupal.Commenting.InlineComment.TabBefore
Drupal.Commenting.PostStatementComment.Found
Drupal.ControlStructures.ControlSignature.NewlineAfterCloseBrace
Drupal.ControlStructures.ControlSignature.NewlineAfterOpenBrace
Drupal.ControlStructures.ControlSignature.SpaceAfterCloseBrace
Drupal.ControlStructures.ControlSignature.SpaceAfterCloseParenthesis
Drupal.ControlStructures.ControlSignature.SpaceAfterKeyword
Drupal.ControlStructures.ControlSignature.SpaceBeforeSemicolon
Drupal.ControlStructures.InlineControlStructure.NotAllowed
Drupal.CSS.ClassDefinitionNameSpacing.BlankLinesFound
Drupal.CSS.ClassDefinitionNameSpacing.MultipleSelectors
Drupal.CSS.ClassDefinitionNameSpacing.SeletorSingleLine
Drupal.CSS.ColourDefinition.NotLower
Drupal.Files.EndFileNewline.NoneFound
Drupal.Files.EndFileNewline.TooMany
Drupal.Files.LineLength.TooLong
Drupal.Formatting.MultiLineAssignment
Drupal.Formatting.SpaceInlineIf.SpaceInlineElse
Drupal.Formatting.SpaceUnaryOperator.BooleanNot
Drupal.Formatting.SpaceUnaryOperator.IncDecLeft
Drupal.Functions.FunctionDeclaration.SpaceAfter
Drupal.Functions.FunctionDeclaration.SpaceBeforeParenthesis
Drupal.NamingConventions.ValidClassName.StartWithCaptial
Drupal.NamingConventions.ValidFunctionName.InvalidName
Drupal.NamingConventions.ValidFunctionName.NotCamelCaps
Drupal.NamingConventions.ValidFunctionName.ScopeNotCamelCaps
Drupal.NamingConventions.ValidGlobal.GlobalUnderScore
Drupal.NamingConventions.ValidVariableName.LowerCamelName
Drupal.NamingConventions.ValidVariableName.LowerStart
Drupal.Semantics.FunctionAlias.FunctionAlias
Drupal.Semantics.FunctionT.BackslashSingleQuote
Drupal.Semantics.FunctionT.Concat
Drupal.Semantics.FunctionT.ConcatString
Drupal.Semantics.FunctionT.NotLiteralString
Drupal.Semantics.RemoteAddress.RemoteAddress
Drupal.Strings.UnnecessaryStringConcat.Found
Drupal.WhiteSpace.CloseBracketSpacing.ClosingWhitespace
Drupal.WhiteSpace.EmptyLines.EmptyLines
Drupal.WhiteSpace.ObjectOperatorSpacing.After
Drupal.WhiteSpace.OpenBracketSpacing.OpeningWhitespace
Drupal.WhiteSpace.OperatorSpacing.NoSpaceAfter
Drupal.WhiteSpace.OperatorSpacing.NoSpaceAfterAmp
Drupal.WhiteSpace.OperatorSpacing.NoSpaceBefore
Drupal.WhiteSpace.OperatorSpacing.NoSpaceBeforeAmp
Drupal.WhiteSpace.OperatorSpacing.SpacingAfter
Drupal.WhiteSpace.ScopeClosingBrace.BreakIdent
Drupal.WhiteSpace.ScopeClosingBrace.Indent
Drupal.WhiteSpace.ScopeClosingBrace.Line
Drupal.WhiteSpace.ScopeIndent.Incorrect
Drupal.WhiteSpace.ScopeIndent.IncorrectExact
Generic.Formatting.SpaceAfterCast.NoSpace
Generic.Functions.FunctionCallArgumentSpacing.SpaceBeforeComma
Generic.Functions.OpeningFunctionBraceKernighanRitchie.BraceOnNewLine
Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace
Generic.Functions.OpeningFunctionBraceKernighanRitchie.SpaceAfterBracket
Generic.NamingConventions.ConstructorName.OldStyle
Generic.PHP.UpperCaseConstant.Found
Generic.WhiteSpace.DisallowTabIndent.NonIndentTabsUsed
Generic.WhiteSpace.DisallowTabIndent.TabsUsed
PEAR.Functions.FunctionCallSignature.SpaceAfterCloseBracket
PEAR.Functions.FunctionCallSignature.SpaceBeforeOpenBracket
PEAR.Functions.ValidDefaultValue.NotAtEnd
PSR2.Classes.PropertyDeclaration.ScopeMissing
PSR2.Classes.PropertyDeclaration.Underscore
PSR2.Namespaces.UseDeclaration.SpaceAfterLastUse
PSR2.Namespaces.UseDeclaration.UseAfterNamespace
Squiz.Arrays.ArrayBracketSpacing.SpaceAfterBracket
Squiz.Arrays.ArrayBracketSpacing.SpaceBeforeBracket
Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine
Squiz.Arrays.ArrayDeclaration.CommaAfterLast
Squiz.Arrays.ArrayDeclaration.FirstIndexNoNewline
Squiz.Arrays.ArrayDeclaration.NoKeySpecified
Squiz.Arrays.ArrayDeclaration.NoSpaceAfterDoubleArrow
Squiz.Arrays.ArrayDeclaration.NoSpaceBeforeDoubleArrow
Squiz.Arrays.ArrayDeclaration.SpaceAfterDoubleArrow
Squiz.Arrays.ArrayDeclaration.SpaceAfterKeyword
Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma
Squiz.Arrays.ArrayDeclaration.SpaceBeforeDoubleArrow
Squiz.Arrays.ArrayDeclaration.SpaceInEmptyArray
Squiz.Commenting.DocCommentAlignment.NoSpaceAfterStar
Squiz.Commenting.DocCommentAlignment.SpaceAfterStar
Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Squiz.ControlStructures.ForEachLoopDeclaration.NoSpaceAfterArrow
Squiz.ControlStructures.ForEachLoopDeclaration.SpacingBeforeAs
Squiz.ControlStructures.ForLoopDeclaration.NoSpaceAfterFirst
Squiz.ControlStructures.ForLoopDeclaration.NoSpaceAfterSecond
Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeFirst
Squiz.ControlStructures.ForLoopDeclaration.SpacingBeforeSecond
Squiz.ControlStructures.SwitchDeclaration.SpaceBeforeColonCase
Squiz.ControlStructures.SwitchDeclaration.SpacingAfterBreak
Squiz.ControlStructures.SwitchDeclaration.SpacingAfterDefault
Squiz.CSS.ClassDefinitionClosingBraceSpace.ContentBeforeClose
Squiz.CSS.ClassDefinitionOpeningBraceSpace.Before
Squiz.CSS.ClassDefinitionOpeningBraceSpace.ContentBefore
Squiz.CSS.ClassDefinitionOpeningBraceSpace.NoneBefore
Squiz.CSS.DisallowMultipleStyleDefinitions.Found
Squiz.CSS.EmptyClassDefinition.Found
Squiz.CSS.EmptyStyleDefinition.Found
Squiz.CSS.Indentation.BlankLine
Squiz.CSS.SemicolonSpacing.NotAtEnd
Squiz.CSS.SemicolonSpacing.SpaceFound
Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceAfterDefault
Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma
Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeEquals
Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterOpenHint
Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose
Squiz.PHP.NonExecutableCode.ReturnNotRequired
Squiz.PHP.NonExecutableCode.Unreachable
Squiz.Scope.MethodScope.Missing
Squiz.Strings.ConcatenationSpacing.PaddingFound
Squiz.WhiteSpace.FunctionSpacing.After
Squiz.WhiteSpace.FunctionSpacing.Before
Squiz.WhiteSpace.LanguageConstructSpacing.Incorrect
Squiz.WhiteSpace.LanguageConstructSpacing.IncorrectSingle
Squiz.WhiteSpace.SemicolonSpacing.Incorrect
Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines
Squiz.WhiteSpace.SuperfluousWhitespace.EndFile
Squiz.WhiteSpace.SuperfluousWhitespace.EndLine
Squiz.WhiteSpace.SuperfluousWhitespace.StartFile

I hope the test was done right. If so, which of these are we shooting to fix?

Mile23’s picture

Is it coding standards time since we're close to 8.1.0 release?

I just looked at #2572791: Fix 'Drupal.WhiteSpace.OpenBracketSpacing' coding standard (at random) and the instructions are wrong. I'll update that issue, but I bet the other ones are wrong, as well.

pfrenssen’s picture

@Mile23, I think putting some focus on coding standards now before the release would be great! We had a similar push right before the release of 8.0.x. It's very well possible that the instructions for running the coding standards are outdated. These have been added to the issues a long time ago and things have changed since then.

alexpott’s picture

@anoopjohn you should be able to see links to issues created for each rule in the related issues linked above. Any sniff that does not have an issue we should create one.

anoopjohn’s picture

I have compiled all the coder rules that were listed as failed into a Google spreadsheet and have also mapped the rules to the sniffs and to the respective issues in the issue queue.

https://docs.google.com/spreadsheets/d/1CtpbUdx8F0rGLuVcOOup--pWAhmQz-fL...

I hope the sheet is useful

alexpott’s picture

@anoopjohn are you sure that the list you generated was against core and using the latest released coder ruleset? Some things listed in there were fixed at the time you commented.

alexpott’s picture

alexpott’s picture

anoopjohn’s picture

I ran the 8.2.x branch of core against the 8.x.2.x branch (head) of the coder (commit a5b5f6b5769393a016da0d106a3b33f932c73331). Should I run coder against their latest release or head? Head has moved forward after my last check as well.

Mile23’s picture

@anoopjohn: Drupal has a phpcs.xml.dist file under the core/ directory.

Copy it, rename it to phpcs.xml, add/remove the sniffs you want to, and then run phpcs from there.

And please: It'd be great if you could show off whatever shell script you're using to get counts of errors by sniff... We could use that to do a lot of easily-reviewable issues.

alexpott’s picture

@Mile23 well the point is to run against the Drupal standard provided by coder... our phpcs.xml.dist is a list of the rules that we comply with.

Mile23’s picture

Right, but if you want to make a meaningful report of what needs to happen for CS, you have to use the same exclusions and file extensions as the phpcs.xml.dist file, and also limit the report to core/.

alexpott’s picture

@Mile23 good point... here's a command to run from the core directory... phpcs -p --extensions=inc,install,module,php,profile,test,theme --ignore=./assets/vendor/* --standard=Drupal ./

anoopjohn’s picture

I used the command as listed in comment #48, given below.

phpcs -s --standard=Drupal --ignore=core/assets/vendor/* --ignore=core/modules/system/tests/fixtures/HtaccessTest --extensions=inc,install,module,php,profile,test,theme core

I had used the same exclusions and file extensions as in phpcs.xml. Also it is running only on core.

Here is the command i used to get the report

cat core-issues-DrupalStandard.latest.4.txt | grep " (" | cut -f2 -d'(' | cut -f1 -d')' | sort | uniq -c | sort -nr

core-issues-DrupalStandard.latest.4.txt is the file created with the phpcs command listed earlier. There will be some incorrectly identified sub-sniff names which I have to manually edit and remove but this should approximately be alright.

Mile23’s picture

Here's a little script I made I call sniffreport.php.

It reads a phpcs json report and then outputs each error encountered as a line.

You can then pipe it into sort and uniq to get a report about which error to tackle.

<?php

/**
 * sniffreport.php
 *
 * Reads in a json file generated by phpcs and spits out each error as a line.
 *
 * 1) Modify core/phpcs.xml to sniff for the sniffs you're sniffing for.
 *
 * 2) Run phpcs like this:
 *
 * $ ../vendor/bin/phpcs -p --report-json=sniff.json
 *
 * 3) Now you can run sniffreport like this:
 *
 * $ php ./scripts/sniffreport.php | sort | uniq -c | sort
 *
 * This will show you a sort-of histogram of all the error codes in the json
 * file. You can use this to determine which patches will be small if you fix
 * a specific error.
 *
 * 4) ????
 *
 * 5) PROFIT.
 */

$errors = json_decode(file_get_contents(__DIR__ . '/../sniff.json'), TRUE);

$files = $errors['files'];

foreach ($files as $file) {
  foreach ($file['messages'] as $message) {
    echo $message['source'] . "\n";
  }
}

For instance, I configured core/phpcs.xml to sniff for Drupal.Semantics, and this is the report for that sniffspace:

   1 Drupal.Semantics.FunctionT.Concat
   1 Drupal.Semantics.RemoteAddress.RemoteAddress
   3 Drupal.Semantics.ConstantName.ConstantStart
   6 Drupal.Semantics.FunctionT.WhiteSpace
  17 Drupal.Semantics.FunctionT.BackslashSingleQuote
  39 Drupal.Semantics.FunctionAlias.FunctionAlias
  62 Drupal.Semantics.FunctionT.NotLiteralString
  66 Drupal.Semantics.FunctionT.ConcatString

This a) verifies that all the current Drupal.Semantics sniffs are still valid, an b) gives us a handy list of things we could work on.

In this case, I decided to see what those ConstantName.ConstantStart errors were, and here's the issue with its teeny tiny patch: #2714815: Fix 'Drupal.Semantics.ConstantName.ConstantStart' coding standard

Mile23’s picture

pfrenssen’s picture

For people wanting to check the current coding standard as included with core, using a locally installed PHP CodeSniffer + coder, here's how to do it (starting from the Drupal root folder):

$ composer require drupal/coder squizlabs/php_codesniffer
$ cd core/
$ ../vendor/bin/phpcs --runtime-set installed_paths ../../drupal/coder/coder_sniffer/ -p
vaplas’s picture

FileSize
20.63 KB

Several fix coding standards in core

vaplas’s picture

FileSize
64.76 KB

Oh no, it was by manual and seems i have forgotten about the transfer line at the end, sorry. This is bonus

alexpott’s picture

@vaplas this is issue is a plan issue - each coding standard has its own issue. See the related issues at the top.

vaplas’s picture

Excuse me for this flood, I was drunk. Hopefully these patches will fall into good hands. But be careful, example 8bbdb57..6f73a79 commit clearly wrong

andypost’s picture

somehow current 8.2.x has

    148 Drupal.Commenting.InlineComment.NotCapital

EDIT #2719657: Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard

anoopjohn’s picture

There are a few patches that are posted under the sub-issues under the FunctionComment fix plan issue - #2572645: [plan] Fix 'Drupal.Commenting.FunctionComment' coding standard. Would be great if somebody could review those.

Mile23’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

There is a new release of the Coder module. I created #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anoopjohn’s picture

Version: 8.3.x-dev » 8.4.x-dev

Changing version to 8.4.x-dev

clemens.tolboom’s picture

Using commands from #65 I get

composer require drupal/coder squizlabs/php_codesniffer
Using version ^8.2 for drupal/coder
Using version ^3.0 for squizlabs/php_codesniffer
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

Problem 1
- Installation request for squizlabs/php_codesniffer ^3.0 -> satisfiable by squizlabs/php_codesniffer[3.0.0, 3.0.0RC1, 3.0.0RC2, 3.0.0RC3, 3.0.0RC4, 3.0.0a1, 3.0.1, 3.0.x-dev, 3.x-dev].
- drupal/coder 8.2.12 requires squizlabs/php_codesniffer >=2.8.1 <3.0 -> satisfiable by squizlabs/php_codesniffer[2.9.x-dev].
- drupal/coder 8.2.12 requires squizlabs/php_codesniffer >=2.8.1 <3.0 -> satisfiable by squizlabs/php_codesniffer[2.9.x-dev].
- drupal/coder 8.2.12 requires squizlabs/php_codesniffer >=2.8.1 <3.0 -> satisfiable by squizlabs/php_codesniffer[2.9.x-dev].
- Conclusion: don't install squizlabs/php_codesniffer 2.9.x-dev
- Installation request for drupal/coder 8.2.12 -> satisfiable by drupal/coder[8.2.12].

Installation failed, reverting ./composer.json to its original content.

mfernea’s picture

From my pov you only need to run: "composer require drupal/coder". squizlabs/php_codesniffer is listed as a dependency for drupal/coder as ">=2.8.1 <3.0" so no need to require squizlabs/php_codesniffer directly.

Mile23’s picture

Issue summary: View changes

phpcs and coder should already be depdendencies of both 8.3.x and 8.4.x.

Updated IS with instructions for how to perform the sniffs, repeated here:

$ cd /path/to/drupal/root/
$ composer install
$ cd core
$ phpcs -ps
clemens.tolboom’s picture

Issue summary: View changes

Thanks for the update. I changed the path.

I can now work on #2886719: views: follow code style for complex hooks question to extend Drupal_Sniffs_Commenting_HookCommentSniff for it. Thanks.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue summary: View changes

Hm there's some incorrect info in the summary here; this is not blocked. Also removing the beta eval since that's no longer relevant.

mfernea’s picture

Issue summary: View changes

I added comments in the sheet in #52 added by @anoopjohn to update the current status of sniffs. We should try to keep that up to date as much as possible, otherwise it's a bit hard to know what to work on.

To get the latest report I use this command:

cd core
../vendor/bin/phpcs -ps --standard=Drupal ./ --ignore='core/assets/vendor/*' --ignore=core/modules/system/tests/fixtures/HtaccessTest  --ignore='*.css,*info,*.txt,*.md,*.yml' --report=source --report-width=120 --report-file=../phpcs-results.txt

I didn't succeed in running this with --extensions. Drupal CS ruleset.xml lists the targeted extensions, so we have to somehow exclude those not yet in phpcs.xml.dist.

Here is the list of sniffs not passing in the latest 8.5.x and not present in the sheet above:

mfernea’s picture

Issue summary: View changes

I modified the issue summary:

  • adjusted the paragraph that states how new sniffs are added in phpcs.xml.dist.
  • adjusted to Approach section to be more accurate.
  • added Remaining tasks section.
mfernea’s picture

Issue summary: View changes

I added info about what phpcs.xml.dist should contain eventually and how we should use Drupal CS ruleset.xml.

mfernea’s picture

Issue summary: View changes

Drupal.NamingConventions.ValidVariableName.LowerStart should be fixed in #2903331: Fix 'Drupal.NamingConventions.ValidVariableName.LowerStart' coding standard.

Mile23’s picture

mfernea’s picture

Issue summary: View changes

I removed from Remaining tasks all the sniffs related to Drupal.Commenting.FunctionComment. They are handled in #2572645: [plan] Fix 'Drupal.Commenting.FunctionComment' coding standard.