Part of meta-issue #2571965: [meta] Fix PHP 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 |
---|---|---|---|
#8 | interdiff-2901727-6-8.txt | 1.14 KB | iainp999 |
#8 | 2901727-8.patch | 13.42 KB | iainp999 |
#6 | 2901727-6.patch | 13.46 KB | iainp999 |
#3 | 2901727-3.patch | 13.22 KB | iainp999 |
Comments
Comment #2
iainp999 CreditAttribution: iainp999 as a volunteer commentedI had a quick look at this, and a small number of issues (14) can be fixed with phpcbf. However, there are others that possibly require the input of someone closer to that code. For example :-
FILE: ...drupal/core/modules/views/src/ViewExecutable.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
1005 | WARNING | Method name "_preQuery" should not be prefixed with an
| | underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1020 | WARNING | Method name "_postExecute" should not be prefixed with
| | an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1038 | WARNING | Method name "_initHandler" should not be prefixed with
| | an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1059 | WARNING | Method name "_buildArguments" should not be prefixed
| | with an underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
1344 | WARNING | Method name "_build" should not be prefixed with an
| | underscore to indicate visibility
| | (Drupal.Methods.MethodDeclaration.Underscore)
----------------------------------------------------------------------
Comment #3
iainp999 CreditAttribution: iainp999 as a volunteer commentedFor the moment, I've attached a patch that exclude the sniff for methods beginning with underscores as a starting point.
Comment #4
mfernea CreditAttribution: mfernea at AmeXio commentedThe patch looks good. I'm not sure we'll be able to fix Drupal.Methods.MethodDeclaration.Underscore as it is breaking backwards compatibility. I opened a new issue for this #2906262: Fix 'Drupal.Methods.MethodDeclaration.Underscore' coding standard and let's focus on this issue on the rest of the sniffs from Drupal.Methods.MethodDeclaration. I changed the title accordingly. Please add in phpcs.xml.dist which sniffs are we targeting:
If that is done I would mark this as RTBC.
Comment #5
mfernea CreditAttribution: mfernea at AmeXio commentedActually, as per my comment in #2906262: Fix 'Drupal.Methods.MethodDeclaration.Underscore' coding standard, Drupal.Methods.MethodDeclaration.Underscore is not part of Drupal CS because of these lines in ruleset.xml:
So, for modifications to phpcs.xml.dist I would go for something like:
Comment #6
iainp999 CreditAttribution: iainp999 as a volunteer commentedthanks @mfernea
updated patch is attached
Comment #7
mfernea CreditAttribution: mfernea at AmeXio commentedWhen a patch is uploaded, the status should be changed to 'Needs review'. This also triggers the tests to run.
When adjusting an existing patch, it's best to upload an interdiff too (https://www.drupal.org/documentation/git/interdiff). Not really needed in this case, being a small patch, but it's easier to review.
Regarding to phpcs.xml.dist modifications, I would only add the code from #5. The "Sniff for" comment is not needed anymore. Also, please note the ref value which is different than the one in the patch.
Thanks for the help!
Comment #8
iainp999 CreditAttribution: iainp999 as a volunteer commentednp @mfernea, thanks for the feedback.
patch and interdiff attached!
Comment #9
iainp999 CreditAttribution: iainp999 as a volunteer commentedComment #10
mfernea CreditAttribution: mfernea at AmeXio commentedThanks for the work done! ;)
RTBC as:
Comment #11
iainp999 CreditAttribution: iainp999 as a volunteer commentedthanks @mfernea :)
Comment #14
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!