Part of meta-issue #2571965: [meta] Fix coding standards in core
Step 1: Preparation
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 & configure PHPCS
Install PHP CodeSniffer and the ruleset from the Coder module:
$ composer install
$ ./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.Classes.UnusedUseStatement"/>
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 |
---|---|---|---|
#39 | fix-2901562-39.patch | 100.95 KB | zaporylie |
#35 | interdiff-2901562-32-35.txt | 335 bytes | yogeshmpawar |
#35 | fix-2901562-35.patch | 103.05 KB | yogeshmpawar |
#32 | 2901562-32.patch | 102.53 KB | jofitz |
#29 | drupal-coding-standards-2901562-29.patch | 103.1 KB | mfernea |
Comments
Comment #2
mfernea CreditAttribution: mfernea at AmeXio commentedComment #3
mfernea CreditAttribution: mfernea at AmeXio commentedHere is the patch.
Comment #5
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #7
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #8
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedHello, I'm from Vienna Drupalcon, I'm working on this issue.
Comment #9
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedHello,
This patch #7 doesn't apply
it needs re-roll
Thank you
Comment #10
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedComment #11
mfernea CreditAttribution: mfernea at AmeXio commentedComment #12
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #13
mfernea CreditAttribution: mfernea at AmeXio commentedComment #14
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedComment #15
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedHello,
so I ran the test and I found a one error:
Thank you
Comment #16
mfernea CreditAttribution: mfernea at AmeXio commentedIndeed, it can also be seen in the test results. New patch and interdiff.
Comment #18
3ssom CreditAttribution: 3ssom at Ministry Of Housing for Ministry Of Housing commentedHello,
Sorry but patch #16 failed to apply ..
re-roll please ..
Thank you
Comment #19
mfernea CreditAttribution: mfernea at AmeXio commentedAlready?! :)
Re-rolled.
Comment #20
3ssom CreditAttribution: 3ssom as a volunteer commentedHello,
the patch applied successfully but are you are about the same error I mentioned earlier because after this the same error showed up:
:)
Comment #21
mfernea CreditAttribution: mfernea at AmeXio commentedAre you sure? :) I looked at the patch again and that line is removed. I've also tested the patch and no cs issues pop up.
Comment #22
Mile23Out of scope change.
Just one error using these commands:
The error:
I'd wager it's a commit since the last patch.
Also, *SO* very ready for #2911280: RectangleTest.php takes a very long time to scan for coding standards to be committed... :-)
Comment #23
3ssom CreditAttribution: 3ssom as a volunteer commentedHello,
I just did it again to be sure the same result came up!
Comment #24
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved empty line.
Comment #25
Mile23@3ssom: Are you sure you're fetching and pulling the 8.5.x branch?
Comment #27
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #28
mfernea CreditAttribution: mfernea at AmeXio commentedBad re-roll.
Comment #29
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #30
borisson_I didn't look at all the files in the patch individually but I reviewed what happened after I applied the patch.
git diff --ignore-blank-lines core/
only lists the changes in the phpcs.xml file.git diff --shortstat core/
lists that only 2 added lines, and 275 deletions.This makes me confident that the patch does only what it says it does.
Comment #32
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #33
borisson_I reviewed this again and it looks like there's an unneeded change in phpcs.xml.dist, that comment shouldn't be changed.
Comment #34
yogeshmpawarComment #35
yogeshmpawarDone changes addressed in #33 & also added an interdiff.
Comment #36
zaporylieYep, #35 is good to go. Thanks all!
Comment #38
xjmCommitted and pushed to 8.5.x. Thanks!
It'd definitely be valuable to backport this since it could introduce needless merge conflicts in a lot of files. To do so, we need to reroll the patch without the changed rule as mentioned in #2919983: Restore the 8.4.0 phpcs.xml.dist on 8.4.x. Marking "Patch (to be ported)" for that.
Comment #39
zaporyliePatch is against 8.4.x HEAD
What I did:
<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/>
to phpcs.xml.dist<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/>
from phpcs.xml.distComment #40
zaporylieComment #41
mfernea CreditAttribution: mfernea at AmeXio commentedYou could have also used a phpcs.xml file (which is ignored).
Comment #42
mfernea CreditAttribution: mfernea at AmeXio commentedLooks good to me:
Comment #43
xjmWe've had a couple occasions where these files get staged but I see that it is indeed now in the
core/
directory's gitingore. :) Thanks for the tip!I scanned through it and then also confirmed that
git diff --staged --ignore-blank-lines
is empty. Ran phpcs against the codebase again as well to make sure there were no other regressions.So, committed the backport to 8.4.x. Thanks!