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

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="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/>

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
#34 2901745-34.patch550 byteslongwave
#33 interdiff_26-33.txt1022 bytesridhimaabrol24
#33 2901745-33.patch2.36 KBridhimaabrol24
#26 2901745-26.patch3.11 KBvacho
#21 interdiff-17-21.txt690 bytesMerryHamster
#21 2901745-21.patch3.65 KBMerryHamster
#17 2901745-17.patch3.62 KBkostyashupenko
#15 2901745-15.patch2.99 KBkostyashupenko
#13 2901745-13-sniff-only.patch550 bytesandypost
#12 2901745-12.patch2.45 KBkostyashupenko
#5 2901745-5.patch19.31 KBpazhyn
#4 2901745-4.patch18.89 KBpazhyn

Comments

mfernea created an issue. See original summary.

pazhyn’s picture

pazhyn commented
Assigned: Unassigned » pazhyn
Issue tags: +kharkiv2017
pazhyn’s picture

pazhyn commented
Issue summary: View changes
pazhyn’s picture

pazhyn commented
Status: Postponed » Needs review
StatusFileSize
new 2901745-4.patch18.89 KB

There are some rows wrapped with 

// @codingStandardsIgnoreStart
// @codingStandardsIgnoreEnd

Because there are several classes in one file and use is declared after next classes in the middle of the files.
So it should be ok.

pazhyn’s picture

pazhyn commented
StatusFileSize
new 2901745-5.patch19.31 KB

Added rule <rule ref="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/> into phpcs.xml.dist

mfernea’s picture

mfernea commented
Assigned: pazhyn » Unassigned
Status: Needs review » Needs work

The patch remove blank lines and although this it's correct to do so, it's not related to this sniff.
I'm not sure that ignoring those lines is a good solution.

mfernea’s picture

mfernea commented
Status: Needs work » Postponed

As per issue summary, let's wait until #2901744: Fix 'PSR2.Namespaces' coding standard is merged.

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -67,8 +67,10 @@ public function testSourceProviderNotActive() {
+// @codingStandardsIgnoreStart
 use Drupal\Core\Database\Connection;
 use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+// @codingStandardsIgnoreEnd

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
@@ -130,9 +130,11 @@ public function testVariableGet() {
+// @codingStandardsIgnoreStart
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+// @codingStandardsIgnoreEnd

+++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
@@ -504,7 +504,9 @@ protected function getPath($module) {
+// @codingStandardsIgnoreStart
 use Drupal\Tests\Core\Theme\RegistryTest;
+// @codingStandardsIgnoreEnd

This needs follow-up to get rid of this cos otherwise this "use" with not be analysed at all

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.

idebr’s picture

idebr commented
Status: Postponed » Needs review

#2901744: Fix 'PSR2.Namespaces' coding standard was committed, so this is no longer postponed.

borisson_’s picture

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

@idebr is right, this can go in. But it needs a reroll first.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
StatusFileSize
new 2901745-12.patch2.45 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 2901745-13-sniff-only.patch550 bytes

Wrong re-roll so here's a rule clean-up only patch to see the number of current regressions

+++ b/core/phpcs.xml.dist
@@ -148,6 +148,7 @@
+  <rule ref="PSR2.Namespaces.UseDeclaration.UseAfterNamespace"/>

this rule needs to be added

andypost’s picture

andypost
he/him
Primary language Russian
commented

I got following warnings locally with prev patch 

FILE: /srv/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 507 | ERROR | USE declarations must go after the first namespace
     |       | declaration
     |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
----------------------------------------------------------------------


FILE: ...tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 55 | ERROR | USE declarations must go after the first namespace
    |       | declaration
    |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
----------------------------------------------------------------------


FILE: ...dules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 70 | ERROR | USE declarations must go after the first namespace
    |       | declaration
    |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
 71 | ERROR | USE declarations must go after the first namespace
    |       | declaration
    |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
----------------------------------------------------------------------


FILE: ...s/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 133 | ERROR | USE declarations must go after the first namespace
     |       | declaration
     |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
 134 | ERROR | USE declarations must go after the first namespace
     |       | declaration
     |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
 135 | ERROR | USE declarations must go after the first namespace
     |       | declaration
     |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
----------------------------------------------------------------------


FILE: ...srv/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 412 | ERROR | USE declarations must go after the first namespace
     |       | declaration
     |       | (PSR2.Namespaces.UseDeclaration.UseAfterNamespace)
----------------------------------------------------------------------

Time: 6 mins, 29.08 secs; Memory: 108.5Mb
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Issue tags: -Needs reroll
StatusFileSize
new 2901745-15.patch2.99 KB

again reroll

andypost’s picture

andypost
he/him
Primary language Russian
commented

please add fix for CryptRandomFallbackTest.php http://cgit.drupalcode.org/drupal/tree/core/tests/Drupal/Tests/Component...

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
StatusFileSize
new 2901745-17.patch3.62 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented

Great! Good to go
So only a question about how to add comments why this lines ignored left

mfernea’s picture

mfernea commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/DrupalSqlBaseTest.php
@@ -67,8 +67,10 @@ public function testSourceProviderNotActive() {
+// @codingStandardsIgnoreStart
 use Drupal\Core\Database\Connection;
 use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+// @codingStandardsIgnoreEnd

    In this case we can remove the namespace declaration and merge the use statements. 

  2. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
@@ -130,9 +130,11 @@ public function testVariableGet() {
+// @codingStandardsIgnoreStart
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+// @codingStandardsIgnoreEnd

    In this case we can remove the namespace declaration and merge the use statements. 

  3. +++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
@@ -409,7 +409,9 @@ protected function getExtensions() {
+// @codingStandardsIgnoreStart
 use Drupal\simpletest\WebTestBase;
+// @codingStandardsIgnoreEnd

    We can also use // @codingStandardsIgnoreLine to keep the modifications footprint low. 

  4. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
@@ -52,7 +52,9 @@ public function testRandomBytesFallback() {
+// @codingStandardsIgnoreStart
 use Drupal\Tests\Component\Utility\CryptRandomFallbackTest;
+// @codingStandardsIgnoreEnd

    We can also use // @codingStandardsIgnoreLine to keep the modifications footprint low. 

  5. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
@@ -504,7 +504,9 @@ protected function getPath($module) {
+// @codingStandardsIgnoreStart
 use Drupal\Tests\Core\Theme\RegistryTest;
+// @codingStandardsIgnoreEnd

    We can also use // @codingStandardsIgnoreLine to keep the modifications footprint low.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

MerryHamster’s picture

MerryHamster commented
StatusFileSize
new 2901745-21.patch3.65 KB
new interdiff-17-21.txt690 bytes

Reroll for 8.7.x

MerryHamster’s picture

MerryHamster commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Re: #19 I agree with changes 1 and 2 but for 3-5 should we switch to the braces style for the multiple namespaces declared in each file? That would mean we don't have to ignore the coding standards, I believe.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vacho’s picture

vacho commented
Issue tags: +Needs reroll
vacho’s picture

vacho commented
StatusFileSize
new 2901745-26.patch3.11 KB

Patch rerolled to 8.8.x

vacho’s picture

vacho commented
Issue tags: -Needs reroll
jmikii’s picture

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

mfernea commented
Status: Needs review » Needs work 
  1. +++ b/core/tests/Drupal/Tests/Component/Utility/CryptRandomFallbackTest.php
@@ -52,7 +52,9 @@ public function testRandomBytesFallback() {
+// @codingStandardsIgnoreStart

    We can also use // @codingStandardsIgnoreLine to keep the modifications footprint low. 

  2. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
@@ -504,7 +504,9 @@ protected function getPath($module) {
+// @codingStandardsIgnoreStart

    We can also use // @codingStandardsIgnoreLine to keep the modifications footprint low.

@longwave I'm not sure I understand what you mean can you please give some examples?

mfernea’s picture

mfernea commented
Issue summary: View changes

Remove "postponed" message.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ridhimaabrol24’s picture

ridhimaabrol24 commented
Status: Needs work » Needs review
StatusFileSize
new 2901745-33.patch2.36 KB
new interdiff_26-33.txt1022 bytes

Patch for Drupal 9.1

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 2901745-34.patch550 bytes

Locally there were no failures after enabling this sniff, although DrupalSqlBaseTest looks the same as it did before to me. Let's see what the bot thinks.

daffie’s picture

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

Removing the rule exclusion will result in no errors.
The patch only makes the rule active.
Tested this on my local machine.
All code changes look good to me.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed b5fa69586c to 9.1.x and 28b5e7cea5 to 9.0.x and b31c7541dc to 8.9.x. Thanks!

Tested on 8.9.x and it passed without issue.

  • alexpott committed b5fa695 on 9.1.x 
    Issue #2901745 by kostyashupenko, pazhyn, MerryHamster, ridhimaabrol24,...

  • alexpott committed 28b5e7c on 9.0.x 
    Issue #2901745 by kostyashupenko, pazhyn, MerryHamster, ridhimaabrol24,...

  • alexpott committed b31c754 on 8.9.x 
    Issue #2901745 by kostyashupenko, pazhyn, MerryHamster, ridhimaabrol24,...
xjm’s picture

xjm
she/her
Primary language English
commented

Just a heads-up that while the cleanups of this issue are patch-safe (and therefore allowed during RC), enabling new PHPCS rules is not. We've actually just restored the rulesets from 9.0.0-rc1 and 8.9.0-rc1 because of this:
https://git.drupalcode.org/project/drupal/-/commit/6144e3b1f69b5ff9c786c...
https://git.drupalcode.org/project/drupal/-/commit/2a5cd8c8adcdae777324d...

The rule is still enabled in 9.1.x which makes it unlikely that a regression will be committed to 9.0 or 8.9 either.

Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +9.1.0 release notes