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
#27 2572643-27.patch51.16 KBneerajsingh
#22 2572643-2.22.patch42.06 KBalexpott
#20 2572643-2-19.patch42.02 KBalexpott
#20 15-19-interdiff.txt6.53 KBalexpott
#15 2572643.15.patch41.41 KBalexpott
#15 13-15-interdiff.txt535 bytesalexpott
#13 2572643.13.patch41.12 KBalexpott
#13 9-13-interdiff.txt3.37 KBalexpott
#10 2572643.9.patch39.94 KBalexpott
#10 8-9-interdiff.txt1.05 KBalexpott
#8 2572643.8.patch38.61 KBalexpott
#5 findcomments.sh_.txt384 bytesJelle_S
#2 Drupal.Commenting.FileComment.patch16.5 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
Drupal.Commenting.FileComment.patch16.5 KB

Status: Needs review » Needs work

The last submitted patch, 2: Drupal.Commenting.FileComment.patch, failed testing.

The last submitted patch, 2: Drupal.Commenting.FileComment.patch, failed testing.

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S at Attiks commented
FileSize
findcomments.sh_.txt384 bytes

I also wrote a little script to detect wrong @file comments (either punctuation or wrong classname in the @file comment. Not sure if the script is fool-proof or if it can be improved (it probably can, I'm not really a shell scripter) but it detected these issues:


./core/lib/Drupal/Core/Cache/CacheableJsonResponse.php: * Contains \Drupal\Core\Cache\CacheableResponse.


./core/lib/Drupal/Core/Render/Placeholder/SingleFlushStrategy.php: * Contains \Drupal\Core\Render\Placeholder\SingleFlushStrategy

./core/lib/Drupal/Core/Render/PlaceholderGenerator.php: * Contains \Drupal\Core\Render\Placeholder.

./core/modules/ban/src/Plugin/migrate/destination/BlockedIp.php: * Contains \Drupal\ban\Plugin\migrate\destination\BlockedIP.

./core/modules/ban/tests/src/Unit/Plugin/migrate/source/d7/BlockedIpsTest.php: * Contains \Drupal\Tests\ban\Unit\Plugin\migrate\source\d7\BlockedIps.

./core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php: * Contains \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageacheSubscriber.

./core/modules/field/tests/src/Unit/Plugin/migrate/process/d7/FieldSettingsTest.php: * Contains \Drupal\Tests\field\Unit\Plugin\migrate\process\d7\FieldInstanceSettingsTest.

./core/modules/migrate/src/Event/MigratePreRowSaveEvent.php: * Contains \Drupal\migrate\Event\MigratePreSaveEvent.

./core/modules/migrate/src/Tests/MigrateStatusTest.php: * Contains \Drupal\migrate\Tests\MigrateStatusTest

./core/modules/migrate/src/Tests/SqlBaseTest.php: * Contains \Drupal\migrate\Tests\SqlBaseTest

./core/modules/migrate/src/Tests/TemplateTest.php: * Contains \Drupal\migrate\Tests\TemplateTest

./core/modules/system/src/Tests/System/UncaughtExceptionTest.php: * Contains \Drupal\system\Tests\System\UncaughtExceptionTest

./core/modules/system/src/Tests/Update/SiteBrandingConvertedIntoBlockUpdateTest.php: * Contains \Drupal\system\Tests\Update\LocalActionsAndTasksConvertedIntoBlocksUpdateTest.

./core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php: * Contains \Drupal\system\Tests\Update\UpdatePathTestBaseTest.php

./core/modules/system/src/Tests/Update/UpdatePathTestJavaScriptTest.php: * Contains \Drupal\system\Tests\Update\UpdatePathTestJavaScriptTest.php

./core/modules/system/tests/modules/early_rendering_controller_test/src/CacheableTestDomainObject.php: * Contains \Drupal\early_rendering_controller_test\AttachmentsTestDomainObject.

./core/modules/system/tests/modules/form_test/src/Form/FormTestDetailsForm.php: * Contains \Drupal\form_test\Form\FormTestGroupContainerForm.

./core/modules/taxonomy/src/Tests/Views/TaxonomyFieldAllTermsTest.php: * Contains \Drupal\taxonomy\Tests\Views\TaxonomyFieldAllTerms.
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
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2572643.8.patch38.61 KB

Here's an updated patch against core using Drupal coder 8.2.6.

One problem we have is that it wants to make the following changes...

diff --git a/core/scripts/db-tools.php b/core/scripts/db-tools.php
index 316c62f..0e0aada 100644
--- a/core/scripts/db-tools.php
+++ b/core/scripts/db-tools.php
@@ -1,3 +1,9 @@
+<?php
+
+/**
+ * @file
+ */
+?>
 #!/usr/bin/env php
 <?php
 
diff --git a/core/scripts/dump-database-d8-mysql.php b/core/scripts/dump-database-d8-mysql.php
index c4f7507..568581b 100755
--- a/core/scripts/dump-database-d8-mysql.php
+++ b/core/scripts/dump-database-d8-mysql.php
@@ -1,3 +1,9 @@
+<?php
+
+/**
+ * @file
+ */
+?>
 #!/usr/bin/env php
 <?php
 
diff --git a/core/scripts/generate-proxy-class.php b/core/scripts/generate-proxy-class.php
index d5f0e43..a091bbf 100644
--- a/core/scripts/generate-proxy-class.php
+++ b/core/scripts/generate-proxy-class.php
@@ -1,3 +1,9 @@
+<?php
+
+/**
+ * @file
+ */
+?>
 #!/usr/bin/env php
 <?php

Which is nuts.

Status: Needs review » Needs work

The last submitted patch, 8: 2572643.8.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
8-9-interdiff.txt1.05 KB
2572643.9.patch39.94 KB

Ah... we can make it pass just by adding an @file in the correct place. Also we should fix 8.1.0 and I think we should leave 8.0.x alone.

Status: Needs review » Needs work

The last submitted patch, 10: 2572643.9.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review

Re-testing #9 as DrupalCI was feed the wrong branch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
9-13-interdiff.txt3.37 KB
2572643.13.patch41.12 KB

Some files had @file doc blocks below the use statements. Patch attached uses the original @file doc blocks in HEAD.

The last submitted patch, 10: 2572643.9.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
13-15-interdiff.txt535 bytes
2572643.15.patch41.41 KB

Fixing the test fail.

The last submitted patch, 13: 2572643.13.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

I took a careful look through this patch.

I don't understand why it would pass a @file sniffer, because there are a couple of @file blocks (noted below) that do not have any descriptions in them at all, and that is not correct. Maybe the file sniffer needs work?

  1. +++ b/core/modules/simpletest/tests/modules/simpletest_test/simpletest_test.install
@@ -1,6 +1,11 @@
+ * Test fixture.

    What is a "test fixture"? Maybe it should just say "Install hooks for test module"? Hm. Berdir pointed me to this page that explains what a test fixture is:
    https://github.com/junit-team/junit/wiki/Test-fixtures

    I don't think a .install file is a "test fixture" at all. Most of the things in this patch that are labeled as "test fixture" are not test fixtures... I don't think they should be labeled as such. Some of them are .install files, so they can be labeled as "install hooks for test module" or something like that. A few are generic .inc files in test modules, so they can be labeled as "include file for test module" or something like that.

    I have marked a few "test fixture" comments below but not by far all of them. I think they all need a second look, and only a very few of them are actually test fixtures. 

  2. +++ b/core/modules/system/tests/fixtures/update/drupal-8.broken_routing.php
@@ -1,5 +1,10 @@
+ * Test fixture.

    See, this makes sense to me to be "test fixture" because it is in a directory test/fixtures, and I think it's providing base data for a test. 

  3. +++ b/core/modules/system/tests/modules/entity_test/entity_test.views.inc
@@ -1,6 +1,10 @@
 /**
+ * @file
+ */

    This @file block is missing a description. 

  4. +++ b/core/modules/system/tests/modules/module_test/module_test.implementations.inc
@@ -1,6 +1,11 @@
+ * Test fixture.

    Again, not a fixture. 

  5. +++ b/core/modules/system/tests/modules/theme_test/theme_test.inc
@@ -1,6 +1,11 @@
+ * Test fixture.

    Again, not a fixture. 

  6. +++ b/core/modules/views/tests/fixtures/update/duplicate-field-handler.php
@@ -1,5 +1,9 @@
+ * @file
+ */

    Missing description in @file here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

I wrote test fixture because coming up with a different version of "Install hooks for test module" for all the different things bugged me, and, I think that all test modules / themes whatever are all test fixtures. They only exist to test code in core. But OTOH I don't think this is worth a big discussion so I'll fix it. And yes the file doc sniff needs work but that shouldn't stop us here.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Fair enough. It just seemed weird that the .module files said "test module" and the others that were part of the module said "test fixture". I'm not hung up on changing it. They're obviously parts of test modules, not a big deal.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
15-19-interdiff.txt6.53 KB
2572643-2-19.patch42.02 KB

Patch addresses all of #17 - thanks for the careful review.

Running the phpcs using --sniffs=Drupal.Commenting.FileComment shows no fails.

Status: Needs review » Needs work

The last submitted patch, 20: 2572643-2-19.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2572643-2.22.patch42.06 KB

Same patch as #20 - just created with --binary

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Fixed 
+++ b/core/lib/Drupal/Core/Archiver/ArchiveTar.php
@@ -1,4 +1,5 @@
+// @codingStandardsIgnoreFile

Yeah... This whole file is crap as far as coding and documentation standards go... Issues get filed on it all the time.

Maybe we should add a few more comment lines at the top to make it human-readable that we don't want these issues to be filed on this file any more? Anyway. Sorry. That is just me ranting.

Anyway, all of these updates look fine to me. Since this is alexpott's patch and it's all docs and totally non-controversial, I just went ahead and committed/pushed this to 8.1.x it rather than delaying this process any more. Thanks!

  • jhodgdon committed d87ade3 on 8.1.x 
    Issue #2572643 by alexpott, attiks, Jelle_S, jhodgdon: Fix Drupal....

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Closed (fixed) » Active

Um, wait. I think I should not have marked this fixed -- we didn't actually get rid of the @file doc blocks yet.

neerajsingh’s picture

neerajsingh
Primary language English
Location IN 🇮🇳
CreditAttribution: neerajsingh commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Active » Needs review
FileSize
2572643-27.patch51.16 KB

As per coding standard(PSR-4) documentation at 'https://www.drupal.org/coding-standards/docs', @file tag docblock should not be there in the files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension.

The @file doc block MUST be present for all PHP files, with one exception: files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block.

Attaching the patch here...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work

We need to work out why the current phpcs rule is not picking these up.

+++ b/core/lib/Drupal/Component/Assertion/Handle.php
@@ -1,10 +1,4 @@
- *
- * For PHP 5 this contains \AssertionError as well.

This needs to move somewhere.

+++ b/core/lib/Drupal.php
@@ -1,10 +1,5 @@
-/**
- * @file
- * Contains \Drupal.
- */
-

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/source/d7/FileTest.php
--- a/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
+++ b/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php

+++ b/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
+++ b/core/modules/migrate/tests/src/Kernel/SqlBaseTest.php
@@ -1,10 +1,5 @@

@@ -1,10 +1,5 @@
 <?php
 
-/**
- * @file

This one is problematic... we need to fix the phpcs rule cause atm this will cause a false positive.

FILE: /Volumes/devdisk/dev/drupal/core/lib/Drupal.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
   |       |     (Drupal.Commenting.FileComment.Missing)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue tags: +Needs followup

I don't think we should re-open this issue. We need to adjust coder's FileCommentSniff. The reason these files aren't being detected by the rule is that they all have multiple classes/traits/interfaces - hence all being test code. We don't enforce the no @file rule in this instance - perhaps we should.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Fixed
Issue tags: -Needs followup

Ah. How's this for a follow-up -- in the Coder project:
#2735143: @file doc blocks for test classes with an additional class in the file

Putting this back to Fixed (rather than Closed / Fixed, so that it still shows up in issue lists for a while and people can see the follow-up).

Status: Fixed » Closed (fixed)

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