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.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 |
---|---|---|---|
#44 | 2572619-44.patch | 14.27 KB | alexpott |
#43 | 2572619-43.patch | 14.29 KB | andypost |
#40 | 2572619-40.patch | 408.56 KB | pfrenssen |
Comments
Comment #3
attiks CreditAttribution: attiks commentedCoder error
Comment #5
tatarbjI've used the latest patch which didn't fix these problems:
Attached a patch which solves these as well.
Comment #8
attiks CreditAttribution: attiks commentedComment #9
tatarbjI'm reattach the patch because it was broken with one already changed system module part.
Comment #10
DuaelFrAs agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.
Comment #11
attiks CreditAttribution: attiks at Attiks commentedFiled patch upstream #2575407: Drupal.Classes.ClassDeclaration fixer breaks code, uploading auto generated patch for testing
Comment #12
andriyun CreditAttribution: andriyun at Skilld commentedPatch is outdated. Need reroll
Comment #13
andriyun CreditAttribution: andriyun at Skilld commentedComment #14
andriyun CreditAttribution: andriyun at Skilld commentedRerolled patch.
Comment #17
andypostwtf?
wrong indent, class inside namespace wrapper
questionable
Comment #18
andypostManually fixed after upgrading coder to dev after #2575407: Drupal.Classes.ClassDeclaration fixer breaks code
core/tests/Drupal/Tests/Core/Database/Stub/Driver/Schema.php
core/tests/Drupal/Tests/Core/DrupalKernel/DrupalKernelTest.php
...and filed #2611258: Drupal.Classes.ClassDeclaration fixer breaks indent
The interesting moment that sniffer is dumb on it
Comment #19
andriyun CreditAttribution: andriyun at Skilld commentedphpcbf with class declaration sniffer also added extra line after fixing emty class.
#2611238: Drupal.Classes.ClassDeclaration fixer added extra line
Comment #20
pfrenssenComment #21
andriyun CreditAttribution: andriyun at Skilld commentedpatch is outdated
Comment #22
andriyun CreditAttribution: andriyun at Skilld commentedthis change blocked on #2610984: Add Archive Tar via Composer, with BC shim
Comment #24
andypostthis no more depends on related issue, just skip ArchiverTar
Comment #25
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #26
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #27
vprocessor CreditAttribution: vprocessor at Skilld commentedrerolled & refixed with phpcbf
Comment #28
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #29
vprocessor CreditAttribution: vprocessor at Skilld commentedSorry, forgot to remove changes in ArchiveTar
Now it's ready
Comment #30
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #31
pfrenssenI reviewed the entire thing. At some point I thought it was never going to end. But it looks great! Everything is perfect.
Interesting to see how some specific whitespace mistakes proliferate through sections the codebase, you can see how much copy/pasting is going on.
Still need to run a full test with PHPCS to see if everything has been caught.
NaughtyRecursiveLogger? :)
This is my all-time favourite.
Comment #32
pfrenssenThis was the result. This was actually a file that was present on my installation from an old issue I was working on, this file is NOT present in core. The actual code in core is perfectly fine.
This is looking good, setting to RTBC!
Comment #33
andypostRTBC++
no false-positives
phpcs -p --sniffs=Drupal.Classes.ClassDeclaration
Time: 59.58 secs; Memory: 124.25Mb
Comment #34
pfrenssenComment #35
pfrenssenComment #36
alexpottThis file is not our coding standards and already has the ignore.
All of these too. I don't think phpcbf has been properly run here. Please don't apply and reroll coding standards patches - the correct way to make a coding standards patch is too add the rule to phpxml.dist.xml and run the fixer if possible.
Comment #37
pfrenssenComment #38
pfrenssenHere's the raw result from phpcbf + interdiff. The files that @alexpott mentioned that are exempt are now correctly reverted, but there are now a few problems coming up which are not caught. These were fixed manually before.
The code above is incorrectly indented, causing this sniff to fail. I guess this is acceptable for now. When the incorrect indenting is fixed in the respective issue this will be correctly detected and fixed.
Same as above. The preceding code is incorrectly indented, and this cascades to this sniff working incorrectly. When the indenting is fixed, this will be fixed along with it.
I think this is not a coding standards violation. This is an empty class. If the comment would not be there then we also wouldn't have a space there. So this is actually an improvement over the previous patch.
This is a bug in coder. This should be indented by two spaces, since this is a class which is wrapped by a namespace. This is also like the most edgy of all edge cases. I would report this in Coder, but not hold up this issue on it.
Comment #40
pfrenssenThe drop is on the move again. The conflict seems to have been in the context. There was no visible interdiff.
Comment #41
alexpott@pfrenssen can we split this issue up into specific errors defined by
Drupal.Classes.ClassDeclaration
this way the issue becomes reviewable. See the current phpcs.xml.dist for how to do this - I committed #2707641: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation (part 2) which does this.Comment #42
pfrenssenSure! I won't have time for it today though.
Comment #43
andypostFiled #2719695: Fix 'Drupal.Classes.ClassDeclaration.CloseBraceAfterBody' coding standard
Comment #44
alexpottFixing all the sub-sniffs that are left.
Doing a git diff -w
Comment #45
alexpottThis file is a copy of the symfony test so ignoring coding standards is a good idea.
Comment #46
klausiLooks good.
Comment #47
alexpottCommitted 92187c5 and pushed to 8.1.x and 8.2.x. Thanks!