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. 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 or remove the line if it's currently excluded. The sniff name is in the issue title. Make sure your patch will include the addition / removal of this line.
Step 2: 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 -ps
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. The -s
flag shows the sniffs when displaying results.
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 |
---|---|---|---|
#45 | diff_42-45.txt | 903 bytes | Deepak Goyal |
#45 | 2824935-45.patch | 125.71 KB | Deepak Goyal |
Comments
Comment #2
pfrenssenComment #3
pfrenssenI had a first pass at this. It was actually possible to autofix the entire
Squiz.ControlStructures.SwitchDeclaration
sniff in one go.There was one single instance where I needed to make a manual intervention, and this is not automatable. One of the code examples in Migrate was missing a
case:
statement:In here I also saw some anomalous code style which is not yet detected. I'll make an issue for this in Coder so we can fix this in a followup:
Comment #4
pfrenssenComment #6
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI tested the '2824935-3.patch' patch and it fail in some files
cit008490cpsubuntu:drupal progestag* 8.4.x$ find . -type f -name '*rej'
./core/modules/views/views.tokens.inc.rej
./core/modules/views/src/Plugin/views/display/Page.php.rej
./core/modules/migrate/src/MigrateExecutable.php.rej
./core/modules/search/src/Tests/SearchRankingTest.php.rej
./core/modules/block/src/Plugin/migrate/source/Block.php.rej
./core/modules/taxonomy/taxonomy.module.rej
./core/modules/rest/src/Tests/RESTTestBase.php.rej
Comment #7
hgunicamp CreditAttribution: hgunicamp at CI&T commentedI inserted a new patch to fix the rejected changes.
Comment #9
mfernea CreditAttribution: mfernea at AmeXio commentedNo sniffs should be excluded from Squiz.ControlStructures.SwitchDeclaration. As I understand from the issue title and #3 we should fix all Squiz.ControlStructures.SwitchDeclaration sniffs.
Comment #10
mfernea CreditAttribution: mfernea at AmeXio commentedThis is the report for the 8.5.x branch:
There are many sniffs with a lot of issues, so I think we should start creating issues for individual sniffs.
Comment #11
mfernea CreditAttribution: mfernea at AmeXio commentedI transformed the issue into a "plan". I will add child issues for each sniff.
I also updated the instructions.
Comment #12
mfernea CreditAttribution: mfernea at AmeXio commentedActually, I was wrong.
Drupal CS' ruleset.xml has the following code:
Currently we have:
So, indeed, it can be fixed in one go.
phpcs.xml.dist should contain the exact same code from Drupal CS' ruleset.xml related to this sniff.
Comment #13
mfernea CreditAttribution: mfernea at AmeXio commentedHere is the patch.
Comment #14
andypostThis is a good step & sane patch size. Maybe it needs follow-up to get rid of warnings as well
this is a good compromise!
Comment #15
mfernea CreditAttribution: mfernea at AmeXio commented@andypost Why do you think we need a follow-up? We shouldn't go beyond Drupal CS. If it's excluding some sniffs, we should exclude them too in core's phpcs.xml.dist. The patch doesn't exclude more or less than Drupal CS' ruleset.xml does. So we should be fine with this patch only.
Comment #16
andypost@mfernea I think final polishing suppose no warnings & removal of
severity
)Comment #17
mfernea CreditAttribution: mfernea at AmeXio commentedActually, no. The goal of the parent meta is to make Drupal core follow Drupal CS. So we should not worry about sniffs / warnings that Drupal CS excludes (using severity) in its ruleset.xml.
Comment #19
mfernea CreditAttribution: mfernea at AmeXio commentedComment #20
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #22
mfernea CreditAttribution: mfernea at AmeXio commentedComment #24
mfernea CreditAttribution: mfernea at AmeXio commentedComment #25
andypostCI still shows 1 coding standard issue but it's not related to the issue
Comment #26
mfernea CreditAttribution: mfernea at AmeXio commentedIndeed, it's not related and it's fixed now. I launched a new test now just to make it clearer.
Comment #27
alexpottI'm not sure about this change. Surely the right thing to do here is to remove the switch? Perhaps we should file an issue to just discuss how to change this because otherwise we're going to have context issue. This change needs to be reviewed wrt to migrate not coding standards.
Comment #29
idebr CreditAttribution: idebr at ezCompany commentedFiled an issue in the migration system: #2972045: Drupal\field\Plugin\migrate\process\d7\FieldInstanceDefaults contains a switch with only a default statement
I suggest we postpone this issue until it is resolved.
Comment #30
idebr CreditAttribution: idebr at ezCompany commented#2972045: Drupal\field\Plugin\migrate\process\d7\FieldInstanceDefaults contains a switch with only a default statement has been committed to 8.6.x, so this issue is no longer postponed.
The patch will need a reroll to account for the change in #2972045: Drupal\field\Plugin\migrate\process\d7\FieldInstanceDefaults contains a switch with only a default statement
Comment #31
idebr CreditAttribution: idebr at ezCompany commentedComment #32
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #33
borisson_The testbot gives us 23 new failures. So those need to be fixed as well.
Comment #34
idebr CreditAttribution: idebr at ezCompany commentedFixed the remaining failures with phpcbf
Comment #36
idebr CreditAttribution: idebr at ezCompany commentedSetting status back to 'Needs review' after #2973992: Permission issue in Nightwatch step marks all full testruns as unstable
Comment #38
idebr CreditAttribution: idebr at iO commentedReroll against 8.7.x
Comment #42
longwaveRecreated for 9.1.x.
Comment #43
longwaveSelf-review:
The breaks here can be removed.
Comment #44
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #45
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi @longwave
As you suggested removed breaks and created patch for same please review.
Comment #46
daffie CreditAttribution: daffie commentedWhich PHPCS rule are we changing here. The patch does not contain any changes to
core/phpcs.xml.dist
.Comment #47
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi @daffie,
No fixable errors were found in core/phpcs.xml.dist file.
Comment #48
longwave@daffie The latest patch does contain phpcs.xml.dist?
Comment #49
daffie CreditAttribution: daffie commentedDeepak Goyal & @longwave: Sorry, I missed the phpcs part in the 125 KB patch.
Tested on my local machine with only the
core/phpcs.xml.dist
part of the patch and I got a whole lot of errors with the message: "Case breaking statements must be followed by a single blank line".Tested on my local machine with the whole patch and no erros where returned.
Reviewed the patch and all changes look good to me.
Almost all changes are the adding of empty lines and in a couple of places unnecessary
break;
are removed.For me it is RTBC.
Comment #50
alexpottCommitted and pushed 9d1faa52f4 to 9.1.x and a4cc432a2e to 9.0.x. Thanks!
I backported the changes to 8.9.x without the phpcs.xml.dist change and without the changes to core/lib/Drupal/Core/Database/Connection.php because there were conflicts. This helps us backport bugfixes to 8.9.x which pointless conflicts.
Comment #55
xjm