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
$ ./vendor/bin/phpcs -p -s --standard=./vendor/drupal/coder/coder_sniffer/Drupal core/
Meta or plans
- #1299710: [meta] Automate the coding-standards part of patch review
- #3188974: [Meta] Consistent use of leading \ for class names
- #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard
- #2909364: [meta] Fix 'Drupal.Commenting.VariableComment' coding standard
- #3116859: [meta] Fix Drupal.Array.Array.LongLineDeclaration coding standard
- #3346468: [meta] Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName
- Drupal Practice - #3310127: [META] Fix DrupalPractice best practice in Core
- #2002650: [meta, no patch] improve maintainability by removing unused local variables Contains at least one child issue that is working with a Drupal Practice sniff
Fixes by sniff
- #2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard
- #2572635: Fix 'Drupal.Commenting.DocComment.LongFullStop' coding standard
- 901/692 #3123062: Fix 'Drupal.Commenting.DocComment.MissingShort' coding standard
- 84/60 #2937558: Fix 'Drupal.Commenting.DocComment.ParamNotFirst' coding standard
- #2903027: Fix 'Drupal.Commenting.DocComment.ShortNotCapital' coding standard
- #2842949: Fix Drupal.Commenting.DocComment.SpacingBeforeTags coding standard
- #2719663: Fix 'Drupal.Commenting.InlineComment.InvalidEndChar' coding standard
- 287/196 #2719657: Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard
- 287/196 #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard
- #3180696: Fix 'Drupal.Commenting.TodoComment' coding standard
- #2572709: Fix 'Drupal.Files.LineLength' coding standard
- #3123066: Fix 'Drupal.NamingConventions.ValidFunctionName' coding standard
- #2714815: Fix 'Drupal.Semantics.ConstantName.ConstantStart' coding standard
- 76/53 #3123067: Fix 'Drupal.Semantics.FunctionT.NotLiteralString' coding standard
- #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard This issue is the #coding-standards target issue
- #3123069: Fix 'Generic.CodeAnalysis.UselessOverridingMethod' coding standard
Other
- #3051548: Fix spelling of "email"
- #2912811: Sniff definitions approach in phpcs.xml.dist: exclude vs severity
Numbers are the errors or warnings found and the number of files. phpcs was run on 10.0 on 2022-09-29 to get the numbers.
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
- 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.
- 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.
- 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
Work is done in the child issues on a per-sniff basis. We can conserve resources by not running all the tests on these patches, until it is ready for RTBC. This is done by modifying the drupalci.yml file. There is an example in #3048495#comment-19. That file also removes all the other sniffs from the phpcs.xml file and only adds the sniff being tested there.
This list contains all sniffs for which we don't have an issue yet:
- None as at 28 March 2020
Comment | File | Size | Author |
---|
Comments
Comment #2
larowlanPlease please consider automating this with github.com/grom358/pharborist - the library that powers drupal_module_upgrader
Comment #3
tatarbjI'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:
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.
Comment #4
pfrenssenI'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.
Comment #5
pfrenssenComment #6
tatarbjFirst 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.
Array indentation error, expected X spaces but found Y (Fixable Error)
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.
Comment #7
tatarbjArray Sniff
Errors:
Warnings:
CSS
Errors:
Classes
Errors:
Warnings:
Commenting
Errors:
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:
Comment #8
andypostComment #9
Mile23We can say:
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...)
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:
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.
Comment #10
attiks CreditAttribution: attiks commented#9 Patch looks clean, but why not use git to create the patch?
Comment #11
Mile23@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.
Comment #12
attiks CreditAttribution: attiks commented#11 You're right, I was assuming phpcbf had an option to limit the sniffs as well
Comment #13
attiks CreditAttribution: attiks commented#11 I tried doing a sniff, see #2572307: Fix 'Generic.PHP.UpperCaseConstant' coding standard
Comment #14
tatarbj@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.
Comment #15
pfrenssenOur 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.
Comment #16
attiks CreditAttribution: attiks commentedThere are better tools for CSS and js, so +1 to focus on php only
Comment #17
pfrenssenI 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.Comment #18
attiks CreditAttribution: attiks commentedI 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
Comment #19
attiks CreditAttribution: attiks commentedI'm running into coder issues from time to time, so we probably will need to fix some things upstream.
Comment #20
attiks CreditAttribution: attiks commentedSome 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.LongFullStop' coding standard is 500K, we might be able to split it for each module, but this will give us 50+ issues for 1 sniff.
Comment #21
tatarbj@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.LongFullStop' 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.
Comment #22
attiks CreditAttribution: attiks commented#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.
Comment #23
tatarbjSure, 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.
Comment #24
attiks CreditAttribution: attiks commentedAll 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,
Comment #25
pfrenssenWow this is really taking off like a rocket :D
Comment #26
attiks CreditAttribution: attiks commentedWe 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
Comment #27
tatarbj@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.LongFullStop' coding standard there is a lot of other issues what your patch didn't fix, btw the patches are also failed :(
Comment #28
Mile23So are we fixing coding standards, or are we including a fixer tool in core?
Comment #29
pfrenssenI 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.
Comment #30
Mile23I'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.
Comment #31
pfrenssenTestbot changes are tracked in #1266444: DrupalCI should return failed for poor code style - php.
Comment #32
YesCT CreditAttribution: YesCT commentedpostponed 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
Comment #33
MixologicIm 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.
Comment #34
pfrenssenGood 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.
Comment #35
xjmThat 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.
Comment #36
xjmAlso, 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.
Comment #37
YesCT CreditAttribution: YesCT commentedthe 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)
Comment #38
alexpottRe #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.
Comment #39
andypostI think that better to enable the integration to drupalci already to prevent accepting patches that break already fixed at least
Comment #40
alexpott@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.
Comment #41
pfrenssenThere 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.
Comment #42
elachlan CreditAttribution: elachlan commentedThere 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.
Comment #43
alexpottCreated #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
Comment #44
YesCT CreditAttribution: YesCT commentedadding information about what is blocking this to the summary.
Comment #45
YesCT CreditAttribution: YesCT commentedComment #46
YesCT CreditAttribution: YesCT commentedComment #48
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedI 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
The list of errors alphabetically
I hope the test was done right. If so, which of these are we shooting to fix?
Comment #49
Mile23Is 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.
Comment #50
pfrenssen@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.
Comment #51
alexpott@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.
Comment #52
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedI 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
Comment #53
Mile23Work in child issues, centered on
Drupal\Component
:Comment #54
alexpott@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.
Comment #55
alexpottYet another issue filed and patched... #2710081: Fix 'Drupal.Formatting.SpaceInlineIf' coding standard
Comment #56
alexpottJust created #2710201: Fix Drupal.Semantics.FunctionT.WhiteSpace and Drupal.Semantics.FunctionT.Concat - this one fixes real bugs!
Comment #57
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedI 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.
Comment #58
Mile23@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.
Comment #59
alexpott@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.
Comment #60
Mile23Right, 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/.
Comment #61
alexpott@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 ./
Comment #62
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedI 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.
Comment #63
Mile23Here'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.
For instance, I configured
core/phpcs.xml
to sniff forDrupal.Semantics
, and this is the report for that sniffspace: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
Comment #64
Mile23Also this one: #2714829: Fix 'Generic.Functions.FunctionCallArgumentSpacing' coding standard
Comment #65
pfrenssenFor 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):
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedSeveral fix coding standards in core
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedOh no, it was by manual and seems i have forgotten about the transfer line at the end, sorry. This is bonus
Comment #68
alexpott@vaplas this is issue is a plan issue - each coding standard has its own issue. See the related issues at the top.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedExcuse me for this flood, I was drunk. Hopefully these patches will fall into good hands. But be careful, example 8bbdb57..6f73a79 commit clearly wrong
Comment #70
andypostsomehow current 8.2.x has
EDIT #2719657: Fix 'Drupal.Commenting.InlineComment.NotCapital' coding standard
Comment #71
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedThere are a few patches that are posted under the sub-issues under the FunctionComment fix plan issue - #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard. Would be great if somebody could review those.
Comment #72
Mile23#2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
Comment #73
pfrenssenFollowing up on #2747073: Fix Drupal CS regressions for Coder 8.2.8, added new child issues:
Comment #75
pfrenssenThere is a new release of the Coder module. I created #2803779: Update Coder to 8.2.10 and fix errors from improved sniffs.
Comment #77
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedChanging version to 8.4.x-dev
Comment #78
clemens.tolboomUsing commands from #65 I get
Comment #79
mfernea CreditAttribution: mfernea at AmeXio commentedFrom 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.
Comment #80
Mile23phpcs 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:
Comment #81
clemens.tolboomThanks 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.
Comment #83
xjmHm there's some incorrect info in the summary here; this is not blocked. Also removing the beta eval since that's no longer relevant.
Comment #84
mfernea CreditAttribution: mfernea at AmeXio commentedI 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:
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:
Comment #85
mfernea CreditAttribution: mfernea at AmeXio commentedI modified the issue summary:
Comment #86
mfernea CreditAttribution: mfernea at AmeXio commentedI added info about what phpcs.xml.dist should contain eventually and how we should use Drupal CS ruleset.xml.
Comment #87
mfernea CreditAttribution: mfernea at AmeXio commentedDrupal.NamingConventions.ValidVariableName.LowerStart should be fixed in #2903331: Fix 'Drupal.NamingConventions.ValidVariableName.LowerStart' coding standard.
Comment #88
Mile23See: #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking"
Comment #89
mfernea CreditAttribution: mfernea at AmeXio commentedI removed from Remaining tasks all the sniffs related to Drupal.Commenting.FunctionComment. They are handled in #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.
Comment #90
mfernea CreditAttribution: mfernea at AmeXio commentedThere are some child issues that need a review and are quite easy. Like:
Anyone? :)
Comment #91
mfernea CreditAttribution: mfernea at AmeXio commentedI think we should focus next on these low complexity issues:
Comment #92
mfernea CreditAttribution: mfernea at AmeXio commentedI added #2909364: [meta] Fix 'Drupal.Commenting.VariableComment' coding standard, a plan for all Drupal.Commenting.VariableComment sniffs, and child issues.
Comment #93
andriyun CreditAttribution: andriyun as a volunteer and at Bellcom, Drupal Ukraine Community for Drupal Ukraine Community commentedI'm concerned about content of phpcs.xml.dist file
and created follow up issue about approaches used for define and exclude rules.
https://www.drupal.org/node/2912811
Comment #94
mfernea CreditAttribution: mfernea at AmeXio commentedI selected (in the order of complexity) another bunch of tickets on which, I think, we should focus on next:
Comment #95
neclimdulI'm a bit tired of re-rolling this ever couple of hours... I'm considering searching out all the cs fixes going on in core and postponing them until this backwards compatibility break and regression is fixed #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking"
Comment #96
mfernea CreditAttribution: mfernea at AmeXio commentedI agree it's not fun chasing head with that issue.
But how soon that patch will be merged once re-rolled and RTBCed? Don't we loose too much time waiting for it? Currently there are about 10-15 issues with recent activity.
Comment #97
neclimdulUpdating here since #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking" will change the script in the IS. the easiest way to run phpcs after its committed will probably be
Comment #98
andypostCommited change may need a lot of rerolls #2902381: Revert phpcs.xml.dist changes from "Ensure that we're using the right ruleset for coding standards checking"
Comment #99
neclimdulLooked through the first page of the coding standards tagged issues and think I got things rerolled that needed it.
Comment #100
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #102
Sparrow_1601 CreditAttribution: Sparrow_1601 commentedRemoved Drupal.NamingConventions.ValidVariableName.LowerCamelName from Remaining tasks.
Starting to work on issue #2937514: Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName in tests
Comment #103
eltori CreditAttribution: eltori as a volunteer commentedRemoved Drupal.Commenting.DocComment.TagGroupSpacing from Remaining tasks.
Starting to work on #2937513: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard
Comment #104
SherFengChong CreditAttribution: SherFengChong commentedRemoved Drupal.Array.Array.ArrayClosingIndentation from the remaining tasks and starting to work on #2937515: Fix Drupal.Array.Array.[ArrayClosingIndentation, ArrayIndentation] coding standard.
Comment #105
eltori CreditAttribution: eltori as a volunteer commentedRemoved Drupal.Commenting.DocComment.TagsNotGrouped from Remaining tasks and starting to work on #2937552: Fix 'Drupal.Commenting.DocComment.TagsNotGrouped' coding standard.
Comment #106
RoSk0Removed Squiz.PHP.NonExecutableCode from remaining tasks and added sub-issue with patch for fixing it. #2937844: Fix 'Squiz.PHP.NonExecutableCode' coding standard
Comment #107
SherFengChong CreditAttribution: SherFengChong commentedRemoved Drupal.Formatting.MultipleStatementAlignment from remaining tasks and added sub-issue with patch. #2937853: Fix 'Drupal.Formatting.MultipleStatementAlignment' coding standard
Comment #108
SherFengChong CreditAttribution: SherFengChong commentedRemoved Drupal.Commenting.DocCommentAlignment from remaining tasks and added sub-issue #2937858: Fix 'Drupal.Commenting.DocCommentAlignment' coding standard.
Comment #109
RoSk0Updated instructions on how to get current state overview.
Also forgot to mention before, in last couple sub-issue created, like #2937844: Fix 'Squiz.PHP.NonExecutableCode' coding standard and #2937858: Fix 'Drupal.Commenting.DocCommentAlignment' coding standard, I have changed issue description because code sniffer and coder are now part of development dependencies of core. So instructions are slightly simplified.
Comment #110
RoSk0Removed Drupal.Classes.PropertyDeclaration from remaining tasks and added sub-issue with patch #2937882: Fix 'Drupal.Classes.PropertyDeclaration' coding standard.
Comment #111
RoSk0Removed Drupal.Commenting.DocComment.LongNotCapital from remaining tasks and added sub-issue with patch #2937888: Fix 'Drupal.Commenting.DocComment.LongNotCapital' coding standard.
Comment #113
Chi CreditAttribution: Chi commentedComment #116
peterkokot CreditAttribution: peterkokot as a volunteer and at Agiledrop - Your Trusted Drupal Teammates commentedThe 'Drupal.Commenting.ClassComment.Missing' is being fixed via the https://www.drupal.org/project/drupal/issues/3105950
Comment #117
jungleAdded a subsection
Comments
underRemaining tasks
to easy tracking comments relevant ones, with remains tasks exacting from core/phpcs.xml.dist file which were excluded and child issues linked.Comment #118
jungleCreated 6 more new issues for the rest of the remaining tasks and updated links to IS
Comment #119
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThere are many more child 'fix' issues in the sidebar which are active and are not listed in "Remaining Tasks". From previous posts it looked like when an issue was created the sniff was deleted from the list in the IS. So I think that's how it is meant to work. All those new issues are set as children to this, so removing them from the summary.
Comment #120
Mile23Added child: #3088730: Include 'composer' directory in phpcs scans
Comment #123
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAdded #3183673: Fix 'Drupal.Commenting.DocComment.ShortFullStop' coding standard - part 1 auto-fixes
The comment in #84 above associated #2572635: Fix 'Drupal.Commenting.DocComment.LongFullStop' coding standard with three sniffs, but this (correctly) now only deals with
LongFullStop
.ParamGroup
now has #3123058: Fix 'Drupal.Commenting.DocComment.ParamGroup' coding standard butShortFullStop
was missed.Comment #124
quietone CreditAttribution: quietone as a volunteer commentedAdded how to change drupalci.yml to not run the tests as suggested by jonathan1055 in #3180696#comment-3.
Comment #126
quietone CreditAttribution: quietone as a volunteer commentedI needed a bit more order to the remaining issues so I added the child issues to the issue summary.
Comment #127
quietone CreditAttribution: quietone as a volunteer commentedUpdate IS
Comment #129
quietone CreditAttribution: quietone as a volunteer commentedUpdating list of issues in the IS.
Comment #130
quietone CreditAttribution: quietone as a volunteer commentedAdd new meta, #3268741: [meta] Fix Drupal.Commenting.DocComment.ShortSingleLine
Comment #132
quietone CreditAttribution: quietone as a volunteer commentedAdd related issue
Comment #133
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAll of the child issues here appear to fix
Drupal.
standards. Is there a separate issue which works onDrupalPractice.
sniffs? Or should those also be added here? Searched for a drupalPractice meta issue but could not find it.edit
I have now created #3310127: [META] Fix DrupalPractice best practice in Core
Comment #134
quietone CreditAttribution: quietone at PreviousNext commentedRemove completed issues and add error numbers and number of files for some of the issues.
Comment #136
quietone CreditAttribution: quietone at PreviousNext commentedMaking this just about PHP and re-parent.
Comment #137
quietone CreditAttribution: quietone at PreviousNext commentedComment #138
quietone CreditAttribution: quietone at PreviousNext commentedComment #139
quietone CreditAttribution: quietone at PreviousNext commentedComment #140
quietone CreditAttribution: quietone at PreviousNext commentedCreated a Meta for Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName
Comment #142
quietone CreditAttribution: quietone at PreviousNext commented