Part of #2571965: [meta] Fix PHP coding standards in core.

Approach

We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.

Step 1: Remove the coding standard from the blacklist

Every coding standard is identified by a "sniff". For example, an imaginary coding standard that would require all llamas to be placed inside a square bracket fence would be called the "Drupal.AnimalControlStructure.BracketedFence sniff". There are dozens of such coding standards, and to make the work easier we have started by blacklisting all the sniffs. For the moment all coding standards that are not yet fixed are simply skipped during the test.

Open the file core/phpcs.xml.dist and remove the line that matches the sniff of this ticket (it's in the issue title). Make sure your patch will include the removal of this line.

Step 2: Install PHP CodeSniffer and the ruleset from the Coder module

Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:

$ composer require drupal/coder squizlabs/php_codesniffer

Step 3: Prepare the phpcs.xml file

Next we need to 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 have to configure the location of the Drupal coding standard ruleset from the Coder module that we just downloaded. To do this open the newly created file core/phpcs.xml. Find the line containing:

<rule ref="Drupal">

Change it to:

<rule ref="../vendor/drupal/coder/coder_sniffer/Drupal">

Or if you prefer the command line:

$ cd core/
$ cp phpcs.xml.dist phpcs.xml
$ perl -pi -e "s|ref=\"Drupal|ref=\"../vendor/drupal/coder/coder_sniffer/Drupal|" phpcs.xml

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:

$ ../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. Go ahead and fix them all!

CommentFileSizeAuthor
#13 2572601-13.patch7.4 KBalexpott
#7 interdiff.txt2.2 KBattiks
#5 fix_drupal_classes_classcreateinstance_coding_standard-2572601-5.patch8.42 KBtatarbj
#4 i2572601-3.patch5.75 KBattiks
#2 i2572601-2.patch5.82 KBattiks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks created an issue. See original summary.

attiks’s picture

attiks CreditAttribution: attiks commented
FileSize
i2572601-2.patch5.82 KB

Status: Needs review » Needs work

The last submitted patch, 2: i2572601-2.patch, failed testing.

attiks’s picture

attiks CreditAttribution: attiks commented
Status: Needs work » Needs review
FileSize
i2572601-3.patch5.75 KB
tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
CreditAttribution: tatarbj commented
FileSize
fix_drupal_classes_classcreateinstance_coding_standard-2572601-5.patch8.42 KB

I've applied the patch on the fresh drupal instance then i've run the following command to check Drupal.Classes.ClassCreateInstance sniff is ok, but unfortunately it throws the following lines:

FILE: ...cs/drupal/core/lib/Drupal/Core/Controller/ControllerResolver.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 76 | ERROR | Calling class constructors must always include
    |       | parentheses
----------------------------------------------------------------------


FILE: ...cations/MAMP/htdocs/drupal/core/lib/Drupal/Core/DrupalKernel.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1233 | ERROR | Calling class constructors must always include
      |       | parentheses
----------------------------------------------------------------------

FILE: ...s/drupal/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 407 | ERROR | Calling class constructors must always include
     |       | parentheses
----------------------------------------------------------------------


FILE: ...rupal/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 527 | ERROR | Calling class constructors must always include
     |       | parentheses
 546 | ERROR | Calling class constructors must always include
     |       | parentheses
----------------------------------------------------------------------

I'm attaching the new patch which contains all of the already fixed ones with the rest of it, and used the correct format of naming the patch file based on the d.org rules.

attiks’s picture

attiks CreditAttribution: attiks at Attiks commented

#5 Thanks for reviewing, but I'm afraid it's a bit soon to do it manually, since we probably need to reroll this a couple of times, I finally was able to update all my composer dependencies and will re-run the automated fixes, so I assume more things will get detected.

attiks’s picture

attiks CreditAttribution: attiks at Attiks commented
FileSize
interdiff.txt2.2 KB

Attaching an interdiff

attiks’s picture

attiks CreditAttribution: attiks at Attiks commented

Opened an issue against coder: #2573195: Not all Drupal.Classes.ClassCreateInstance violations are marked as fixable

attiks’s picture

attiks CreditAttribution: attiks at Attiks commented
Related issues: +#2573195: Not all Drupal.Classes.ClassCreateInstance violations are marked as fixable

Patch uploaded to #2573195: Not all Drupal.Classes.ClassCreateInstance violations are marked as fixable which detects and fixes all violations.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Issue tags: -Novice

As agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented
Issue summary: View changes

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: +rc eligible
FileSize
2572601-13.patch7.4 KB

Rerolled and added rule to phpcs.xml.dist. Once the patch is applied running phpcs against core reveals no new error.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looking good, thanks!

alexpott’s picture

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

Committed 84754b0 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed e62a2c7 on 8.2.x 
    Issue #2572601 by attiks, alexpott, tatarbj: Fix 'Drupal.Classes....

Status: Fixed » Closed (fixed)

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