Part of meta-issue #2571965: [meta] Fix PHP coding standards in core, stage 1

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.

Comments

mfernea created an issue. See original summary.

mfernea’s picture

Status: Active » Needs review
StatusFileSize
new160.62 KB

Here is the patch.

Status: Needs review » Needs work

The last submitted patch, 2: drupal-coding-standards-2901730-2.patch, failed testing. View results

mfernea’s picture

Status: Needs work » Needs review
StatusFileSize
new160.35 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-coding-standards-2901730-4.patch, failed testing. View results

mfernea’s picture

Status: Needs work » Needs review
StatusFileSize
new159.55 KB

Re-roll.

mfernea’s picture

StatusFileSize
new160.03 KB

Update patch for new issues.

Status: Needs review » Needs work

The last submitted patch, 7: drupal-coding-standards-2901730-7.patch, failed testing. View results

mfernea’s picture

Status: Needs work » Needs review
StatusFileSize
new157.01 KB

Re-roll.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, I'm assuming all the newlines are ok. I haven't looked at all of those, so I reviewed this by applying the patch and running git diff --ignore-blank-lines core/. Those changes are very minimal and they all look great.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: drupal-coding-standards-2901730-9.patch, failed testing. View results

mfernea’s picture

Assigned: Unassigned » mfernea
Issue tags: +Needs reroll
mfernea’s picture

Assigned: mfernea » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new157.03 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, 13: drupal-coding-standards-2901730-13.patch, failed testing. View results

borisson_’s picture

I don't understand why the testbot doesn't like this patch. But we should figure out why it's failing (I retested and it came back with the same failure). I applied the same patch (now on a fresh version of drupal core) and did the same checking I did in #10 and the patch still looks good to me.

mfernea’s picture

Looking at https://dispatcher.drupalci.org/job/drupal_patches/ it seems that all the jobs are failing due to drupal/coder issue. I can't find any issue related to this. I'll keep looking. Anyway, let's retest this after a while.

mfernea’s picture

borisson_’s picture

In that case, this one is good to go, will re-rtbc tomorrow.

borisson_’s picture

Status: Needs work » Reviewed & tested by the community

Back to rtbc.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

These three rules would probably be better in separate patches; two of them are very small and one is massive, but also could be easily reviewed programmatically with a porcelain diff.

mfernea’s picture

Title: Fix 'Squiz.WhiteSpace' coding standard » Fix 'Squiz.WhiteSpace.FunctionSpacing' coding standard
Issue summary: View changes
Status: Needs review » Needs work
mfernea’s picture

Status: Needs work » Needs review
StatusFileSize
new150.18 KB

Here is the patch that only fixes Squiz.WhiteSpace.FunctionSpacing.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ankitjain28may’s picture

StatusFileSize
new3.47 MB

I have fixed the phpcs error for Squiz.WhiteSpace.FunctionSpacing, Please review.

Status: Needs review » Needs work

The last submitted patch, 24: drupal-coding-standard-2901730-24.patch, failed testing. View results

ankitjain28may’s picture

Status: Needs work » Needs review
StatusFileSize
new154.38 KB

That patch was the wrong one, I fixed it again, please review

borisson_’s picture

Status: Needs review » Needs work

Patch no longer applies

ankitjain28may’s picture

Ok, i will re-roll the patch.

idebr’s picture

Status: Needs work » Needs review
StatusFileSize
new1.24 KB
new153.13 KB
  1. Rerolled against 8.6.x
  2. Moved the new Squiz.WhiteSpace.FunctionSpacing sniff in phpcs.xml.dist so it is sorted alphabetically, in line with the Drupal ruleset.xml in drupal/coder
  3. Fixed a new violation found in core/tests/Drupal/KernelTests/Core/Theme/TwigEnvironmentTest.php
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

The testbot agrees with this patch, and I ran composer phpcs locally and got no remaining errors.

Looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b8f93d5 and pushed to 8.6.x. Thanks!

  • alexpott committed b8f93d5 on 8.6.x
    Issue #2901730 by mfernea, ankitjain28may, idebr: Fix 'Squiz.WhiteSpace....

Status: Fixed » Closed (fixed)

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