Part of meta-issue #2571965: [meta] Fix 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

mfernea’s picture

Issue summary: View changes
mfernea’s picture

Assigned: mfernea » Unassigned
Status: Active » Needs review
FileSize
100.75 KB

Here is the patch.

Status: Needs review » Needs work

The last submitted patch, 3: drupal-coding-standards-2878211-3.patch, failed testing. View results

mfernea’s picture

Status: Needs work » Needs review
FileSize
100.74 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-coding-standards-2901562-5.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
100.4 KB

Re-rolled.

3ssom’s picture

Assigned: Unassigned » 3ssom
Issue tags: +Vienna2017

Hello, I'm from Vienna Drupalcon, I'm working on this issue.

3ssom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Hello,

This patch #7 doesn't apply

it needs re-roll

error: core/modules/comment/src/Tests/CommentThreadingTest.php: No such file or directory
error: patch failed: core/modules/node/src/Tests/NodeRevisionsTest.php:165
error: core/modules/node/src/Tests/NodeRevisionsTest.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Component/FileCache/FileCacheFactoryTest.php:122
error: core/tests/Drupal/Tests/Component/FileCache/FileCacheFactoryTest.php: patch does not apply

Thank you

3ssom’s picture

Assigned: 3ssom » Unassigned
mfernea’s picture

Assigned: Unassigned » mfernea
mfernea’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
101.01 KB

Re-roll.

mfernea’s picture

Assigned: mfernea » Unassigned
3ssom’s picture

Assigned: Unassigned » 3ssom
3ssom’s picture

Assigned: 3ssom » Unassigned
Status: Needs review » Needs work

Hello,

so I ran the test and I found a one error:

FILE: ...s/vienna85/core/lib/Drupal/Core/Render/Element/RenderElement.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 271 | ERROR | [x] Functions must not contain multiple empty lines in a
     |       |     row; found 2 empty lines
     |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Thank you

mfernea’s picture

Status: Needs work » Needs review
FileSize
101.61 KB
477 bytes

Indeed, it can also be seen in the test results. New patch and interdiff.

Status: Needs review » Needs work

The last submitted patch, 16: drupal-coding-standards-2901562-16.patch, failed testing. View results

3ssom’s picture

Hello,

Sorry but patch #16 failed to apply ..

re-roll please ..

Thank you

mfernea’s picture

Status: Needs work » Needs review
FileSize
101.61 KB

Already?! :)
Re-rolled.

3ssom’s picture

Hello,

the patch applied successfully but are you are about the same error I mentioned earlier because after this the same error showed up:

FILE: ...s/vienna85/core/lib/Drupal/Core/Render/Element/RenderElement.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 271 | ERROR | [x] Functions must not contain multiple empty lines in a
     |       |     row; found 2 empty lines
     |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

:)

mfernea’s picture

Are you sure? :) I looked at the patch again and that line is removed. I've also tested the patch and no cs issues pop up.

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -269,7 +269,6 @@ public static function preRenderAjaxForm($element) {
       $element['#attributes']['data-disable-refocus'] = "true";
     }
 
-
     // Add a reasonable default event handler if none was specified.
     if (isset($element['#ajax']) && !isset($element['#ajax']['event'])) {
       switch ($element['#type']) {
Mile23’s picture

Status: Needs review » Needs work
+++ b/core/phpcs.xml.dist
@@ -225,8 +225,9 @@
-  <!-- Zend sniffs -->
+    <!-- Zend sniffs -->

Out of scope change.

Just one error using these commands:

$ git fetch
$ git pull origin 8.5.x
$ git apply [patch]
$ composer install
$ cd core
$ ../vendor/bin/phpcs -ps

The error:

FILE: ...alTests/Entity/ContentEntityFormFieldValidationFilteringTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 114 | ERROR | [x] Functions must not contain multiple empty lines in a
     |       |     row; found 2 empty lines
     |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 6 mins, 33.54 secs; Memory: 104.01Mb

I'd wager it's a commit since the last patch.

Also, *SO* very ready for #2911280: RectangleTest.php takes a very long time to scan for coding standards to be committed... :-)

3ssom’s picture

Hello,

I just did it again to be sure the same result came up!

FILE: ...s/vienna85/core/lib/Drupal/Core/Render/Element/RenderElement.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 271 | ERROR | [x] Functions must not contain multiple empty lines in a
     |       |     row; found 2 empty lines
     |       |     (Squiz.WhiteSpace.SuperfluousWhitespace.EmptyLines)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 1 mins, 45.81 secs; Memory: 120.5Mb
jofitz’s picture

Status: Needs work » Needs review
FileSize
765 bytes
101.8 KB

Removed empty line.

Mile23’s picture

@3ssom: Are you sure you're fetching and pulling the 8.5.x branch?

Status: Needs review » Needs work

The last submitted patch, 24: 2901562-24.patch, failed testing. View results

mfernea’s picture

Status: Needs work » Needs review
FileSize
4.22 MB

Re-roll.

mfernea’s picture

Assigned: Unassigned » mfernea
Status: Needs review » Needs work

Bad re-roll.

mfernea’s picture

Assigned: mfernea » Unassigned
Status: Needs work » Needs review
FileSize
103.1 KB

Re-roll.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I didn't look at all the files in the patch individually but I reviewed what happened after I applied the patch.

git diff --ignore-blank-lines core/ only lists the changes in the phpcs.xml file.
git diff --shortstat core/ lists that only 2 added lines, and 275 deletions.

This makes me confident that the patch does only what it says it does.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: drupal-coding-standards-2901562-29.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
102.53 KB

Re-rolled.

borisson_’s picture

Status: Needs review » Needs work

I reviewed this again and it looks like there's an unneeded change in phpcs.xml.dist, that comment shouldn't be changed.

-  <!-- Zend sniffs -->
+    <!-- Zend sniffs -->
   <rule ref="Zend.Files.ClosingTag"/>
yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
103.05 KB
335 bytes

Done changes addressed in #33 & also added an interdiff.

zaporylie’s picture

Status: Needs review » Reviewed & tested by the community

Yep, #35 is good to go. Thanks all!

  • xjm committed 6dc062e on 8.5.x
    Issue #2901562 by mfernea, Jo Fitzgerald, Yogesh Pawar, 3ssom, Mile23,...
xjm’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.5.x. Thanks!

It'd definitely be valuable to backport this since it could introduce needless merge conflicts in a lot of files. To do so, we need to reroll the patch without the changed rule as mentioned in #2919983: Restore the 8.4.0 phpcs.xml.dist on 8.4.x. Marking "Patch (to be ported)" for that.

zaporylie’s picture

Patch is against 8.4.x HEAD

What I did:

  1. added <rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/> to phpcs.xml.dist
  2. phpcbf
  3. phpcs -> no CS errors found
  4. removed <rule ref="Squiz.WhiteSpace.SuperfluousWhitespace"/> from phpcs.xml.dist
zaporylie’s picture

Status: Patch (to be ported) » Needs review
mfernea’s picture

You could have also used a phpcs.xml file (which is ignored).

mfernea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me:

  1. no cs issues
  2. all changes relate to blank lines
  3. no changes to phpcs.xml.dist
xjm’s picture

Status: Reviewed & tested by the community » Fixed

You could have also used a phpcs.xml file (which is ignored).

We've had a couple occasions where these files get staged but I see that it is indeed now in the core/ directory's gitingore. :) Thanks for the tip!

I scanned through it and then also confirmed that git diff --staged --ignore-blank-lines is empty. Ran phpcs against the codebase again as well to make sure there were no other regressions.

So, committed the backport to 8.4.x. Thanks!

  • xjm committed 18fa4fc on 8.4.x
    Issue #2901562 by mfernea, Jo Fitzgerald, Yogesh Pawar, zaporylie, 3ssom...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.