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

  • PSR2.Namespaces.NamespaceDeclaration
  • PSR2.Namespaces.UseDeclaration

Except PSR2.Namespaces.UseDeclaration.UseAfterNamespace which is fixed in #2901745: Fix 'PSR2.Namespaces.UseDeclaration.UseAfterNamespace' coding standard.

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.

CommentFileSizeAuthor
#36 drupal-coding-standards-2901744-35.patch587 bytesmfernea
#30 drupal-coding-standards-2901744-30.patch47.81 KBmfernea
#28 drupal-coding-standards-2901744-28.patch48.22 KBmfernea
#25 drupal-coding-standards-2901744-25.patch48.22 KBmfernea
#21 interdiff-2901744-18-21.txt359 bytesmfernea
#21 drupal-coding-standards-2901744-21.patch48.17 KBmfernea
#18 interdiff-2901744-15-18.txt1.47 KBmfernea
#18 drupal-coding-standards-2901744-18.patch47.65 KBmfernea
#15 drupal-coding-standards-2901744-15.patch49.56 KBmfernea
#7 drupal-coding-standards-2901744-7.patch49.58 KBmfernea
#5 drupal-coding-standards-2901744-5.patch50.01 KBmfernea
#2 drupal-coding-standards-2901744-2.patch50.49 KBmfernea

Comments

mfernea created an issue. See original summary.

mfernea’s picture

mfernea commented
Status: Active » Needs review
StatusFileSize
new drupal-coding-standards-2901744-2.patch50.49 KB

Here is the patch.

mfernea’s picture

mfernea commented
Issue summary: View changes

Status: Needs review » Needs work

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

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-5.patch50.01 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-coding-standards-2901744-5.patch, failed testing. View results

mfernea’s picture

mfernea commented
StatusFileSize
new drupal-coding-standards-2901744-7.patch49.58 KB

Re-roll.

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
borisson_’s picture

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

Looks solid.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

This seems to be missing the phpcs.xml.dist change, am I missing something?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented 
+++ b/core/modules/views/tests/src/Kernel/Plugin/JoinTest.php
--- a/core/phpcs.xml.dist
+++ b/core/phpcs.xml.dist

+++ b/core/phpcs.xml.dist
+++ b/core/phpcs.xml.dist
@@ -146,6 +146,10 @@

@@ -146,6 +146,10 @@
   <rule ref="PSR2.Classes.PropertyDeclaration">
     <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
   </rule>
+  <rule ref="PSR2.Namespaces.NamespaceDeclaration" />
+  <rule ref="PSR2.Namespaces.UseDeclaration">
+    <exclude name="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/>
+  </rule>

It's in the patch, is that not the correct file @catch?

mfernea’s picture

mfernea commented
Status: Needs work » Needs review

I think it should have the Needs review status.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

No you're right, I did look twice but just could not see it this morning. Back to RTBC and apologies for the red herring.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll now though.

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-15.patch49.56 KB

Re-roll.

andriyun’s picture

andriyun commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

So the only funkiness is around:

  1. core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
  2. core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
    (Which still has a @file docblock? How did that happen? I guess because it contains two classes and so therefore isn't PSR-4. It looks like there are dozens, maybe hundreds of unit tests in core that still have @file docblocks for the same reason.)

Looking at this, the double namespace declaration for (2) dates all the way to #2121299: Migrate in Core: Drupal 6 to Drupal 8. The double namespace in (1) was added along with the file in #2549801: Improve source provider missing exception message., so probably just copied it from (2).

It also looks like there is a similar double namespace declaration in:
core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
...which was not touched by this patch.

It looks like maybe those two hunks are left over from including #2901745: Fix 'PSR2.Namespaces.UseDeclaration.UseAfterNamespace' coding standard in this patch as? Let's remove them; they pass with just the rules enabled here.

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-18.patch47.65 KB
new interdiff-2901744-15-18.txt1.47 KB

Indeed, those changes are not required. Here is the updated patch and interdiff.

andriyun’s picture

andriyun commented
Status: Needs review » Needs work 
FILE: ...les/file/tests/src/Kernel/Migrate/d7/FileMigrationSetupTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 3 | ERROR | [x] There must be one blank line after the namespace
   |       |     declaration
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

new phpcs issue was added in one of recent patches

mfernea’s picture

mfernea commented
Assigned: Unassigned » mfernea
mfernea’s picture

mfernea commented
Assigned: mfernea » Unassigned
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-21.patch48.17 KB
new interdiff-2901744-18-21.txt359 bytes

Here is the patch and interdiff.

andriyun’s picture

andriyun commented
Status: Needs review » Reviewed & tested by the community

Looks good to me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: drupal-coding-standards-2901744-21.patch, failed testing. View results

andriyun’s picture

andriyun commented
Issue tags: +Needs reroll
mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-25.patch48.22 KB

Re-roll.

mfernea’s picture

mfernea commented
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 25: drupal-coding-standards-2901744-25.patch, failed testing. View results

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-28.patch48.22 KB

Re-roll again. :D

Status: Needs review » Needs work

The last submitted patch, 28: drupal-coding-standards-2901744-28.patch, failed testing. View results

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2901744-30.patch47.81 KB

Re-roll.

andriyun’s picture

andriyun commented
Status: Needs review » Reviewed & tested by the community

no phpcs fails after patch applying
patch contains only sniff related changes

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 64fa6ad on 8.5.x 
    Issue #2901744 by mfernea: Fix 'PSR2.Namespaces' coding standard
mfernea’s picture

mfernea commented
Status: Fixed » Needs work

One error got through.

mfernea’s picture

mfernea commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review

Here is the patch.

mfernea’s picture

mfernea commented
StatusFileSize
new drupal-coding-standards-2901744-35.patch587 bytes

Status: Needs review » Needs work

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

mfernea’s picture

mfernea commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
alexpott’s picture

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

@mfernea in general it is best to create a new issue rather than re-open an already fixed issue. This keeps contribution and issue metrics sane.

  • alexpott committed 0b1741f on 8.5.x 
    Issue #2901744 by mfernea, catch: Fix 'PSR2.Namespaces' coding standard
alexpott’s picture

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

Discussed with @catch - we agreed to commit #36 and move on.

Committed 0b1741f and pushed to 8.5.x. Thanks!

mfernea’s picture

mfernea commented

Ok, I understand. I will do that in the future. Thanks!

Status: Fixed » Closed (fixed)

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