Problem/Motivation
Part of meta-issue #2571965: [meta] Fix PHP coding standards in core
Drupal core doesn't match Squiz.Functions.MultiLineFunctionDeclaration coding standars.
Following these steps to reproduce the issue:
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.
Proposed resolution
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 |
---|---|---|---|
#30 | output.txt | 9.91 KB | Finn Lewis |
#28 | interdiff.txt | 1.03 KB | gmario |
#28 | drupal-coding-standards-2901726-28.patch | 153.67 KB | gmario |
#11 | drupal-coding-standards-2901726-10.patch | 155.64 KB | mfernea |
#7 | drupal-coding-standards-2901726-7.patch | 157.68 KB | mfernea |
Comments
Comment #2
mfernea CreditAttribution: mfernea at AmeXio commentedHere is the patch.
Comment #3
mfernea CreditAttribution: mfernea at AmeXio commentedStatus change.
Comment #4
mfernea CreditAttribution: mfernea at AmeXio commentedPart of this is also targeted in #2572795: Fix coding standard for closures - Drupal.WhiteSpace.ScopeClosingBrace and Generic.Functions.OpeningFunctionBraceKernighanRitchie.
Comment #6
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #7
mfernea CreditAttribution: mfernea at AmeXio commentedPatch update for the new issues.
Comment #8
andriyun CreditAttribution: andriyun as a volunteer and at Drupal Ukraine Community for Drupal Ukraine Community commentedPatch is not applicable
need reroll
Comment #9
mfernea CreditAttribution: mfernea at AmeXio commentedComment #10
mfernea CreditAttribution: mfernea at AmeXio commentedRe-roll.
Comment #11
mfernea CreditAttribution: mfernea at AmeXio commentedComment #12
gmario CreditAttribution: gmario commentedI'm in DrupalCon in Vienna - i'm working on this
Comment #13
robertoperuzzoI'm at DrupalCon Vienna and I'm working on it with gmario.
Comment #14
robertoperuzzoI tested patch #11. Result is ok without errors.
My local env:
MacBook Pro (Retina, Mid 2012)
OSX El Capitain 10.11.6
PHP 7.0.0
Comment #15
gmario CreditAttribution: gmario commentedI have tested the patch #11 in a Linux box, PHP 5.6.18 - and it works.
I have followed the steps in description, before apply the patch:
after patch is been applied:
Comment #16
Finn Lewis CreditAttribution: Finn Lewis at Agile Collective commentedI'm mentoring at Drupalcon Vienna and have been working with gmario and robertoperuzzo to test this patch.
All looking good, adding to the 'spreadsheet of awesomeness' for further review!
Comment #17
mradcliffeI was able to apply the patch as well successfully on 8.5.x.
I do not have time to review the actual patch because it is quite long, but that is one remaining step that I haven't seen any one due since the patch in comment #11 was posted.
Comment #18
rachel_norfolkCan we re-format the Issue Summary Template according to the Issue Summary Template Standards, please?
This makes it much easier for the potential Core Committer to understand the issue and assess whether to commit.
Thanks
Comment #19
robertoperuzzoOk rachel_norfolk, I'm going to do that.
Comment #20
robertoperuzzoUpdated. Do we need to keep original report by mfernea?
Comment #21
rachel_norfolkComment #22
rachel_norfolkNo, it's fine to replace the IS rather than add to it - a version history is kept.
Comment #23
robertoperuzzoOk ,fine.
Comment #24
mfernea CreditAttribution: mfernea at AmeXio commentedThanks for the review! Just a hint: for these types of patches which should only introduce whitespace differences you can also use
git diff --color-words
like command to review. Doing so, you can easily spot other modifications.Comment #25
xjmThanks @mfernea!
This issue is adding a core/phpcs.local.xml file and I don't believe it should be. Can we check why that was added? It may have been staged accidentally.
Comment #26
Finn Lewis CreditAttribution: Finn Lewis at Agile Collective commentedWe've had some feedback that the patch appears to be adding a core/phpcs.local.xml which was perhaps added by mistake in the patch in #11
gmario is taking a look now.
Comment #27
mfernea CreditAttribution: mfernea at AmeXio commentedYes, my bad! I'll let @gmario create a new patch and interdiff then.
Comment #28
gmario CreditAttribution: gmario commentedHere the news patch and the interdiff with that in #10.
Comment #29
gmario CreditAttribution: gmario commentedComment #30
Finn Lewis CreditAttribution: Finn Lewis at Agile Collective commentedPatch applies cleanly.
OSX 10.10.5
PHP 7.1.7
and no longer creates core/phpcs.local.xml
Running ../vendor/bin/phpcs -p now shows no errors or warnings.
Attaching output of ../vendor/bin/phpcs -p to confirm.
I think this is now RTBC
Comment #31
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!