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: Add the coding standard to the whitelist

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 only whitelisting the sniffs that pass. 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 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 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
$ ./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.Array.Array.CommaLastItem"/>

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.

CommentFileSizeAuthor
#60 2572613-60.patch504.47 KBidebr
#60 interdiff-59-60.txt621 bytesidebr
#59 2572613-59.patch507.78 KBidebr
#59 interdiff-57-59.txt393 bytesidebr
#57 2572613-57.patch507.4 KBidebr
#57 interdiff-55-57.txt551 bytesidebr
#55 2572613-55.patch505.24 KBidebr
#52 interdiff-2572613-47-52.txt15.25 KBrajeevk
#52 2572613-52.patch474.24 KBrajeevk
#47 drupal-coding-standards-2572613-47.patch479.83 KBMalevi4
#48 interdiff-26-47.txt48.12 KBMalevi4
#13 2572613_13.patch413.32 KBMile23
#16 2572613_16.patch417.14 KBMile23
#24 2572613-24.patch466.59 KBpfrenssen
#26 2572613-26.patch466.53 KBpfrenssen
#26 interdiff.txt393 bytespfrenssen
#33 interdiff-26-33.txt48.49 KBMalevi4
#33 drupal-coding-standards-2572613-33.patch481.46 KBMalevi4
#35 interdiff-26-35.txt48.63 KBMalevi4
#35 drupal-coding-standards-2572613-35.patch481.6 KBMalevi4
#39 interdiff-26-39.txt48.12 KBMalevi4
#39 drupal-coding-standards-2572613-39.patch481.1 KBMalevi4
#42 interdiff-26-41.txt48.12 KBMalevi4
#42 drupal-coding-standards-2572613-41.patch480.46 KBMalevi4
#45 drupal-coding-standards-2572613-41.patch480.46 KBMalevi4
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
Issue summary: View changes
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 commented
Status: Postponed » Active

#2572587: Extend array sniff to short array syntax was fixed, unpostponing.

andriyun’s picture

andriyun CreditAttribution: andriyun at Skilld commented

I got the next notice
PHP Notice: Undefined index: parenthesis_closer in ~/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/Drupal/Sniffs/Array/ArraySniff.php on line 162

when I try to check core code regarding sniff Drupal.Array.Array

phpcs --standard=Drupal --sniffs=Drupal.Array.Array --ignore=vendor,assets/vendor -p core/
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen 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.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue summary: View changes
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue summary: View changes
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.1.x-dev » 8.2.x-dev 
    127 Drupal.Array.Array.ArrayClosingIndentation
    369 Drupal.Array.Array.ArrayIndentation
    906 Drupal.Array.Array.CommaLastItem
   2375 Drupal.Array.Array.LongLineDeclaration
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Fix 'Drupal.Array.Array' coding standard » Fix 'Drupal.Array.Array.CommaLastItem' coding standard
Status: Active » Needs review
FileSize
2572613_13.patch413.32 KB

I thought Drupal.Array.Array.CommaLastItem might be easy to review.

It's 462 affected files, but auto-fix does a good job.

Status: Needs review » Needs work

The last submitted patch, 13: 2572613_13.patch, failed testing.

The last submitted patch, 13: 2572613_13.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2572613_16.patch417.14 KB

Redid the work. It turns out the sniffer has false positives for arrays with heredoc strings in them.

This patch fixes the last element commas, but not the heredoc, meaning phpcs will spit back two errors.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Filed an issue on coder: #2761019: False positive for Drupal.Array.Array.CommaLastItem on heredoc strings

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Postponed

I guess this should really be postponed on the issue in Coder.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Postponed » Needs work

Coder issue is fixed. Thanks @klausi!

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Postponed

Postponed until the fix in Coder makes it into a release.

Version: 8.3.x-dev » 8.4.x-dev

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

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Postponed » Needs work

The fix has been accepted in Coder in the meantime so this is no longer blocked. Coding standards fixes are still allowed for the 8.3.x branch.

Setting back to "Needs work" because this will need to be rerolled against latest HEAD.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review
FileSize
2572613-24.patch466.59 KB

100% autofixed by phpcbf.

Status: Needs review » Needs work

The last submitted patch, 24: 2572613-24.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Needs review
FileSize
2572613-26.patch466.53 KB
interdiff.txt393 bytes

Patch failed to apply since it touches a fixture containing a UTF-16 database export which is treated as a binary file by git. Re-exported using the --binary option.

Status: Needs review » Needs work

The last submitted patch, 26: 2572613-26.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented

I guess the ref attribute value should be like: ../vendor/drupal/coder/...

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Related issues: +#2901306: Fix Drupal.Array.Array.LongLineDeclaration coding standard, +#2902636: Fix Drupal.Array.Array.ArrayIndentation coding standard
pazhyn’s picture

pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commented
Assigned: Unassigned » pazhyn
Issue summary: View changes
Issue tags: +kharkiv2017
pazhyn’s picture

pazhyn CreditAttribution: pazhyn as a volunteer and at AnyforSoft, Drupal Ukraine Community for AnyforSoft commented
Assigned: pazhyn » Unassigned
Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
interdiff-26-33.txt48.49 KB
drupal-coding-standards-2572613-33.patch481.46 KB

Status: Needs review » Needs work

The last submitted patch, 33: drupal-coding-standards-2572613-33.patch, failed testing. View results

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
interdiff-26-35.txt48.63 KB
drupal-coding-standards-2572613-35.patch481.6 KB

Status: Needs review » Needs work

The last submitted patch, 35: drupal-coding-standards-2572613-35.patch, failed testing. View results

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented

I tried add some changes to core/modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php file in 8.4.x. But I discover some problem. My git output diff for this file like binary. I think this happens because on line 43 we have unknown symbols in serialised string ({s:16:"[]*[]config_prefix") and etc.

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Assigned: Unassigned » Malevi4
Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
FileSize
interdiff-26-39.txt48.12 KB
drupal-coding-standards-2572613-39.patch481.1 KB

Just remove /modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php from patch.

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: drupal-coding-standards-2572613-39.patch, failed testing. View results

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
FileSize
interdiff-26-41.txt48.12 KB
drupal-coding-standards-2572613-41.patch480.46 KB
Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 42: drupal-coding-standards-2572613-41.patch, failed testing. View results

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
drupal-coding-standards-2572613-41.patch480.46 KB

Status: Needs review » Needs work

The last submitted patch, 45: drupal-coding-standards-2572613-41.patch, failed testing. View results

Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
Status: Needs work » Needs review
FileSize
drupal-coding-standards-2572613-47.patch479.83 KB
Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
FileSize
interdiff-26-47.txt48.12 KB
Malevi4’s picture

Malevi4 CreditAttribution: Malevi4 at Drupal Ukraine Community commented
joshmiller’s picture

joshmiller
he/him
Primary language English
Location Indianapolis, Indiana, USA
CreditAttribution: joshmiller at Acro Commerce commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Needs review » Needs work

Malevi4, this patch no longer applies to 8.x-5.x ... Errors below:

error: patch failed: core/modules/action/src/Form/ActionAdminManageForm.php:71
error: patch failed: core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php:88
error: core/modules/block_content/src/Tests/BlockContentTypeTest.php: No such file or directory
error: core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php: No such file or directory
error: patch failed: core/modules/filter/tests/src/Functional/FilterAdminTest.php:140
error: patch failed: core/modules/locale/locale.batch.inc:242
error: patch failed: core/modules/node/src/NodeForm.php:146
error: patch failed: core/modules/node/src/Plugin/views/wizard/Node.php:36
error: patch failed: core/modules/node/src/Plugin/views/wizard/NodeRevision.php:35
error: core/modules/page_cache/src/Tests/PageCacheTest.php: No such file or directory
error: patch failed: core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php:169
error: patch failed: core/modules/system/src/Form/ThemeSettingsForm.php:212
error: patch failed: core/modules/system/src/Plugin/views/field/BulkForm.php:361
error: patch failed: core/modules/system/tests/src/Unit/Menu/MenuLinkTreeTest.php:134
error: patch failed: core/tests/Drupal/Tests/Component/Assertion/InspectorTest.php:159
error: patch failed: core/tests/Drupal/Tests/Core/Render/RendererTest.php:182
rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at Sirius commented
Assigned: Malevi4 » rajeevk
rajeevk’s picture

rajeevk
Location London
CreditAttribution: rajeevk as a volunteer and at Sirius commented
Status: Needs work » Needs review
FileSize
2572613-52.patch474.24 KB
interdiff-2572613-47-52.txt15.25 KB

This is what I have done --

  • Cleaned patch for failing chunks from #47 and applied.
  • Applied failed chunks manually on different branch & re-created new patch along with successful ones from #47 (attaching for test).
  • Created interdiff & attaching.

Thanks !

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.

borisson_’s picture

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

Patch no longer applies.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2572613-55.patch505.24 KB

Status: Needs review » Needs work

The last submitted patch, 55: 2572613-55.patch, failed testing. View results

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
FileSize
interdiff-55-57.txt551 bytes
2572613-57.patch507.4 KB

Changes in #57:

  1. Restored core/modules/system/tests/fixtures/HtaccessTest/access_test.module.orig
  2. Removed an unrelated change in core/phpcs.xml.dist
borisson_’s picture

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

According to the testbot, there is one remaining failure in modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs work » Needs review
FileSize
interdiff-57-59.txt393 bytes
2572613-59.patch507.78 KB

Fixed the violation reported in core/modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php. Git struggles to generate a meaningful interdiff since it is treated as a binary file.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
FileSize
interdiff-59-60.txt621 bytes
2572613-60.patch504.47 KB

Updated the sniff in core/phpcs.xml.dist to it sniffs the parent and excludes the other sniffs:

  <rule ref="Drupal.Array.Array">
    <!-- Sniff for these errors: CommaLastItem -->
    <exclude name="Drupal.Array.Array.ArrayClosingIndentation"/>
    <exclude name="Drupal.Array.Array.ArrayIndentation"/>
    <exclude name="Drupal.Array.Array.LongLineDeclaration"/>
  </rule>
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
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

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

Committed be6d515 and pushed to 8.6.x. Thanks!

  • alexpott committed be6d515 on 8.6.x 
    Issue #2572613 by Malevi4, idebr, pfrenssen, Mile23, RajeevK: Fix '...

Status: Fixed » Closed (fixed)

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