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.Array.Array.CommaLastItem"/>
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 |
---|---|---|---|
#60 | 2572613-60.patch | 504.47 KB | idebr |
#60 | interdiff-59-60.txt | 621 bytes | idebr |
Comments
Comment #2
attiks CreditAttribution: attiks commentedComment #3
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 #4
pfrenssen#2572587: Extend array sniff to short array syntax was fixed, unpostponing.
Comment #5
andriyun CreditAttribution: andriyun at Skilld commentedI got the next notice
PHP Notice: Undefined index: parenthesis_closer in ~/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal/Sniffs/Array/ArraySniff.php on line 162
when I try to check core code regarding sniff Drupal.Array.Array
Comment #6
pfrenssenComment #8
pfrenssenComment #9
pfrenssenComment #10
pfrenssenComment #11
andypostComment #12
pfrenssenComment #13
Mile23I thought Drupal.Array.Array.CommaLastItem might be easy to review.
It's 462 affected files, but auto-fix does a good job.
Comment #16
Mile23Redid the work. It turns out the sniffer has false positives for arrays with heredoc strings in them.
This patch fixes the last element commas, but not the heredoc, meaning phpcs will spit back two errors.
Comment #17
Mile23Filed an issue on coder: #2761019: False positive for Drupal.Array.Array.CommaLastItem on heredoc strings
Comment #18
Mile23I guess this should really be postponed on the issue in Coder.
Comment #20
Mile23Coder issue is fixed. Thanks @klausi!
Comment #21
Mile23Postponed until the fix in Coder makes it into a release.
Comment #23
pfrenssenThe fix has been accepted in Coder in the meantime so this is no longer blocked. Coding standards fixes are still allowed for the 8.3.x branch.
Setting back to "Needs work" because this will need to be rerolled against latest HEAD.
Comment #24
pfrenssen100% autofixed by
phpcbf
.Comment #26
pfrenssenPatch failed to apply since it touches a fixture containing a UTF-16 database export which is treated as a binary file by git. Re-exported using the
--binary
option.Comment #29
mfernea CreditAttribution: mfernea at AmeXio commentedI guess the ref attribute value should be like: ../vendor/drupal/coder/...
Comment #30
mfernea CreditAttribution: mfernea at AmeXio commentedComment #31
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commentedComment #32
pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commentedComment #33
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #35
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #37
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedI tried add some changes to core/modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php file in 8.4.x. But I discover some problem. My git output diff for this file like binary. I think this happens because on line 43 we have unknown symbols in serialised string ({s:16:"[]*[]config_prefix") and etc.
Comment #38
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #39
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedJust remove /modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php from patch.
Comment #40
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #42
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #43
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #45
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #47
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #48
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #49
Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commentedComment #50
joshmillerMalevi4, this patch no longer applies to 8.x-5.x ... Errors below:
Comment #51
rajeevkComment #52
rajeevkThis is what I have done --
Thanks !
Comment #54
borisson_Patch no longer applies.
Comment #55
idebr CreditAttribution: idebr at ezCompany commentedComment #57
idebr CreditAttribution: idebr at ezCompany commentedChanges in #57:
Comment #58
borisson_According to the testbot, there is one remaining failure in
modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php
Comment #59
idebr CreditAttribution: idebr at ezCompany commentedFixed the violation reported in core/modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php. Git struggles to generate a meaningful interdiff since it is treated as a binary file.
Comment #60
idebr CreditAttribution: idebr at ezCompany commentedUpdated the sniff in core/phpcs.xml.dist to it sniffs the parent and excludes the other sniffs:
Comment #61
borisson_The testbot agrees with this patch, and I ran composer phpcs locally and got no remaining errors.
Looks great!
Comment #62
alexpottCommitted be6d515 and pushed to 8.6.x. Thanks!