Part of meta-issue #2571965: [meta] Fix coding standards in core

Step 1: Preparation

Open the file core/phpcs.xml.dist and add yml as file extention. Make sure your patch will include this addition.

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: Remove phpcs.xml file

When adding a new file extenstion all tests should run so no need for phpcs.xml file. PHPCS will use phpcs.xml.dist.

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.

CommentFileSizeAuthor
#14 2901653-14.patch17.37 KBidebr
#14 interdiff-11-14.txt698 bytesidebr
#11 2901653-11.patch17 KBidebr
#11 interdiff-10-11.txt437 bytesidebr
#10 2901653-10.patch16.7 KBidebr
#10 interdiff-5-10.txt491 bytesidebr
#5 app-yml-files-2901653-5.patch20.69 KBankitjain28may
#2 drupal-coding-standards-2901653-2.patch18.24 KBmfernea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Assigned: mfernea » Unassigned
Status: Active » Needs review
FileSize
drupal-coding-standards-2901653-2.patch18.24 KB

Here is the patch.

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

ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented

The above patch failed to apply on drupal 8.6.x. Here is the patch for it.

ankitjain28may’s picture

ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented
FileSize
app-yml-files-2901653-5.patch20.69 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

Patch is not correct, after I applied this and ran composer phpcs I got this:

FILE: ...web/core/node_modules/babel-plugin-add-header-comment/circle.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
3 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I think this means that we have to update the phpcs script in composer to ignore the node_modules directory?

ankitjain28may’s picture

ankitjain28may CreditAttribution: ankitjain28may as a volunteer and at Google Summer of Code commented

@borisson_ Yes, we should ignore node_modules dir in phpcs script.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work

NW based on #6 and #7.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language Malayalam
Location Manchester
CreditAttribution: rakesh.gectcr as a volunteer and at CTI Digital commented
Issue tags: +Nwdug_may18
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
FileSize
interdiff-5-10.txt491 bytes
2901653-10.patch16.7 KB

Attached patch includes the following changes:

  1. Exclude scanning node_modules directory. #2329453: Ignore front end vendor folders to improve directory search performance also excludes the bower_components directory, but I left this out since Drupal does not ship with bower.
  2. Rerolled against 8.6.x, specifically #2959148: Enable PHPCS for info.yml files
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
FileSize
interdiff-10-11.txt437 bytes
2901653-11.patch17 KB

We can expand on the existing exclude pattern for *.info.yml files to bypass "No PHP" errors in yml files.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

That looks great!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Yeah this looks like a sensible widening. I'm not a huge fan of coding standards checking other languages with PHPCS because generally there are better tools for each language. But in the case of yml this makes sense.

+++ b/core/phpcs.xml.dist
@@ -7,10 +7,11 @@
+  <exclude-pattern>*/node_modules/*</exclude-pattern>

Let's copy the exclude in settings.php ie.

$settings['file_scan_ignore_directories'] = [
  'node_modules',
  'bower_components',
];

So that this is consistent. And add a comment saying the this excludes the directory based on what's in settings.php.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
FileSize
interdiff-11-14.txt698 bytes
2901653-14.patch17.37 KB

#13.1 Added an exclude-pattern for the "bower_components" folder

#13.2 Added a comment referencing the file_scan_ignore_directories option in settings.php

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks @idebr, the changes in #14 seems to resolve the issue @alexpott mentioned in #13. I just ran composer phpcs again and there are no new failures.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 1b07d55 and pushed to 8.6.x. Thanks!

  • alexpott committed 1b07d55 on 8.6.x 
    Issue #2901653 by idebr, ankitjain28may, mfernea, borisson_: Add yml...

Status: Fixed » Closed (fixed)

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