Follow-up to #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard
Part of #2571965: [meta] Fix PHP coding standards in core.
Approach
We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.
Step 1: Add the coding standard to the whitelist
Every coding standard is identified by a "sniff". For example, an imaginary coding standard that would require all llamas to be placed inside a square bracket fence would be called the "Drupal.AnimalControlStructure.BracketedFence
sniff". There are dozens of such coding standards, and to make the work easier we have started by only whitelisting the sniffs that pass. For the moment all coding standards that are not yet fixed are simply skipped during the test.
Open the file core/phpcs.xml.dist
and add a line for the sniff of this ticket. The sniff name is in the issue title. Make sure your patch will include the addition of this line.
Step 2: Install PHP CodeSniffer and the ruleset from the Coder module
Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:
$ composer require drupal/coder squizlabs/php_codesniffer
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer
Once you have installed the phpcs package, you can list all the sniffs available to you like this:
$ ./vendor/bin/phpcs --standard=Drupal -e
This will give you a big list of sniffs, and the Drupal-based ones should be present.
Step 3: Prepare the phpcs.xml file
To speed up the testing you should make a copy of the file phpcs.xml.dist
(in the core/
folder) and save it as phpcs.xml
. This is the configuration file for PHP CodeSniffer.
We only want this phpcs.xml file to specify the sniff we're interested in. So we need to remove all the rule items, and add only our own sniff's rule. Rule items look like this:
<rule ref="Drupal.Commenting.InlineComment.SpacingBefore"/>
Remove all of them, and add only the sniff from this issue title. This will make sure that our tests run quickly, and are not going to contain any output from unrelated sniffs.
Step 4: Run the test
Now you are ready to run the test! From within the core/
folder, run the following command to launch the test:
$ cd core/
$ ../vendor/bin/phpcs -p
This takes a couple of minutes. The -p
flag shows the progress, so you have a bunch of nice dots to look at while it is running.
Step 5: Fix the failures
When the test is complete it will present you a list of all the files that contain violations of your sniff, and the line numbers where the violations occur. You could fix all of these manually, but thankfully phpcbf
can fix many of them. You can call phpcbf like this:
$ ../vendor/bin/phpcbf
This will fix the errors in place. You can then make a diff of the changes using git. You can also re-run the test with phpcs and determine if that fixed all of them.
Comment | File | Size | Author |
---|
Issue fork drupal-2719649
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
andypostComment #3
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #4
vprocessor CreditAttribution: vprocessor at Skilld commenteddone
Comment #5
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #6
vprocessor CreditAttribution: vprocessor at Skilld commentedupdated with parent patch
Comment #7
andypostI think that should be removed as whole
this indent is wrong
Comment #8
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #9
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #10
vprocessor CreditAttribution: vprocessor at Skilld commentedrerolled & fixed
Comment #11
Mile23Comment #12
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #13
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedAdded a rerolled patch.
Comment #15
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #16
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #17
crazyrohila CreditAttribution: crazyrohila at Srijan | A Material+ Company commentedI think this patch also includes spaceAfter fixes too which belong to other issue (https://www.drupal.org/node/2572659).
eg.
The above standard is fixed in above patch which should be part of other meta issue.
Comment #18
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #20
rasikap CreditAttribution: rasikap at Blisstering Solutions commented@crazyrohila do I need to remove the extra fixes from the above patch?
Comment #21
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #22
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedRerolled against 8.3.x.
Comment #23
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #25
hitesh-jain CreditAttribution: hitesh-jain at Acquia commentedComment #26
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #29
mfernea CreditAttribution: mfernea at AmeXio commentedModifications to the phpcs.xml.dist are not correct.
Comment #30
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedfixed the phpcs.xml.dist and re-rolled to 8.5.x
Comment #32
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #33
cburschkaWhat is the difference between #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard and this?
Edit: Answer:
This is a NoSpaceBefore violation:
and this is a SpacingBefore violation:
Comment #34
mfernea CreditAttribution: mfernea at AmeXio commentedThe modifications to phpcs.xml.dist should be:
Doing so, we are not overlapping with #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard, although we'll have merge conflicts for phpcs.xml.dist, which is expected.
So, please make sure that the changes in the patch are only fixing SpacingBefore issues and nothing else.
Comment #35
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@mfernea
fixed as per your comment.
Comment #36
mfernea CreditAttribution: mfernea at AmeXio commentedModifications to phpcs.xml.dist look good.
But if I look at the patch I see modifications for NoSpaceBefore sniff (eg Diff.php or DiffEngine.php). It should not. The patch should only contain fixes for SpacingBefore sniff.
Comment #37
cburschkaThis might be the right changeset
Edit: wait, no it messed up a bunch of indentation; sorry.
Comment #38
cburschkaSeems to add the wrong indentation in this comment - or incompletely add the correct one.
Comment #39
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #40
mfernea CreditAttribution: mfernea at AmeXio commentedThe tests results show that there are still some cs issues.
Comment #41
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@mfernea
in core/tests/Drupal/Tests/Component/Graph/GraphTest.php
there is graph diagram in the comment section do we need to fix it.
Comment #43
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #44
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #45
mfernea CreditAttribution: mfernea at AmeXio commentedNot really sure what to do with the comment in core/tests/Drupal/Tests/Component/Graph/GraphTest.php which is failing the test. A quickfix would be to add // @code and // @endcode. It looks like the Drupal CS doesn't raise an issue anymore, but @code and @endcode are meant for docblocks and not inline comments. And I think we have to use this for all other inline commented code in this patch.
Suggestions are welcome.
Comment #46
mfernea CreditAttribution: mfernea at AmeXio commentedDrupal CS allows @code @endcode in inline comments and does that on purpose, it's not just a coincidence.
So whenever we have to deal with commented code or comments like those in GraphTest.php it's best to use these tags.
Also please pay attention to // @todo vs // @todo: or other variations. Only the first one is correct and allows the next lines to be indented with 2 spaces.
Comment #47
jofitz CreditAttribution: jofitz at ComputerMinds commentedIf I understand correctly, in #46 you are suggesting the troublesome comment in GraphTest.php should be surrounded by
@code ... @endcode
so that is what I have done (and re-rolled because the patch no longer applies cleanly, hence no interdiff).Comment #48
mfernea CreditAttribution: mfernea at AmeXio commented// @todo
vs// @todo:
issue is not solved yet.We should use @code @endcode tags whenever we deal with code like comments (eg ConfigImporter.php).
Comment #49
jofitz CreditAttribution: jofitz at ComputerMinds commentedReplaced all occurrences of
// @todo:
with// @todo
.Comment #50
mfernea CreditAttribution: mfernea at AmeXio commentedIf we use // @todo we can have a 2 space indent for the lines below.
We still have to fix the code like comments using @code and @endcode.
Comment #51
mfernea CreditAttribution: mfernea at AmeXio commentedLet's postpone this until #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard gets in.
Comment #52
mfernea CreditAttribution: mfernea at AmeXio commented#2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard is merged. We can start working again on this one.
Comment #53
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedSmall fix
Comment #54
mfernea CreditAttribution: mfernea at AmeXio commentedhttps://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
Prefered style:
Still passes CS, but less nice:
Doesn't pass CS:
Comment #55
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAdded patch
Comment #56
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #57
jofitz CreditAttribution: jofitz at ComputerMinds commented@harsha012 Can you give us more details about your patch please? What changes have you made since the previous patch in #49?
It is also helpful if you can provide an interdiff when you upload a patch, if possible. That makes it far easier to review.
I am surprised to see that your patch is about a third of the size of the previous patch. This might suggest you have missed something.
Comment #61
klausithis break the formatting of a code example. I think we should wrap this with @code and @endcode comments, then it should not complain? Can you test if that works?
do we need to fix this? I think it should work under the @todo like it is.
I think a lot of cases here could be improved with @code tags then Coder should not complain.
Comment #64
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #65
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #66
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #67
klausiThe change in phpcs.xml.dist is missing?
Comment #68
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedReroll the patch.
@klausi the required changes for phpcs.xml.dist is already added in drupal 9.
Comment #69
cburschka9.1.x phpcs.xml.dist still excludes the Drupal.Commenting.InlineComment.SpacingBefore rule:
Comment #70
klausiYep, please remove the exclude in phpcs.xml.dist.
Comment #71
cburschkaThere's a few validations that feel overly strict. For example, in some places "Note:" is used to start a continuation indent, but the sniff does not allow this.
In another place, it's @see:
Also in a ton of places, a @todo is marked as an invalid continuation indent because it is written as "@todo: ..." rather than "@todo ..." - the colon causes the sniffer to not recognize it. That does seem like something we want to be consistent on, but I'm not sure if removing the colon from all of those is within the scope of this issue.
Comment #72
cburschkaDid a new pass.
This one seems to satisfy the sniff, and fixes some of the changes from the previous patch (eg indentation stripped instead of adding @code/@endcode). There were also some non-SpacingBefore fixes in the previous patch.
I fixed the "@todo:" issue by removing the colon from all @todo items that got flagged by the sniff, and changed @TODO and @fixme to @todo in a few cases.
But there are lots and lots more of those that didn't get flagged because they're single line. If we want to apply a consistent rule to those, that's definitely a separate issue.
Comment #73
cburschkaFiled #3150259: Consistent style for "@todo" comments as a separate issue.
Comment #75
longwaveRerolled and fixed two more cases.
Comment #76
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll
Comment #79
SpokjeI've put
2719649-76.patch
in to an Merge Request, just for easier commenting on each change.Feel free to post your answer as a patch and/or in the MR.
Comment #80
quietone CreditAttribution: quietone as a volunteer commentedAm I right that the unresolved issues are about the link in the todos?
The example in the standard does not include 'see' or '@see' for @todos.
Comment #81
Spokje@quiteone
Ah, you're absolutely right. Resolved all my comments that were about those
@see
s.Only three unresolved issues left. :)
Comment #83
SpokjeRebased MR on
9.3.x
.Comment #84
SpokjeHiding old
9.2.x
patchesComment #85
longwaveA few minor comments but this is looking good overall.
Comment #86
SpokjeThanks @longwave and his eagle eyes.
Comment #87
longwaveThanks, all points addressed, this is ready to go.
Comment #92
catchCommitted/pushed to 9.3.x, thanks! Also committed the same patch, minus the phpcs.dist.xml change to 9.2.x.
This one is borderline for what might need to be scheduled in a beta/rc, but it's a tangible documentation as well as code style improvement.