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

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
#21 reroll_diff_19-21.txt1.46 KBSpokje
#21 2902540-21.patch2.14 KBSpokje
#18 2902540-19.patch2.14 KBlongwave
#11 2902540-11.patch1.9 KBguilhermevp
#10 2902540-10.patch1.87 KBguilhermevp
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea created an issue. See original summary.

mfernea’s picture

mfernea CreditAttribution: mfernea at AmeXio commented
Status: Active » Postponed
Related issues: +#2902536: $config in $coreGlobals from Drupal.NamingConventions.ValidGlobal

We should wait for #2902536: $config in $coreGlobals from Drupal.NamingConventions.ValidGlobal to be fixed.
I'm not really sure what to do with global $efq_test_metadata;

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.

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.

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.

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.

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Postponed » Active

This issue this was postponed on is fixed.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Status: Active » Needs review
FileSize
2902540-10.patch1.87 KB

Phpcs Summary: 

 FILE: /home/guilhermevp/Ambientes/Drupal9Dev/drupal/core/modules/field/tests/modules/field_test/field_test.module
--------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------
 172 | ERROR | global variables should start with a single underscore followed by the module and another underscore
--------------------------------------------------------------------------------------------------------------------


FILE: /home/guilhermevp/Ambientes/Drupal9Dev/drupal/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
--------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------
 777 | ERROR | global variables should start with a single underscore followed by the module and another underscore
--------------------------------------------------------------------------------------------------------------------

Time: 1 mins, 11.46 secs; Memory: 42MB

Sending patch, please review.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
FileSize
2902540-11.patch1.9 KB

Better organized patch.

Status: Needs review » Needs work

The last submitted patch, 11: 2902540-11.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@guilhermevp, thanks for working on this.

It isn't clean to me what 'better organized patch' means in the context of a coding standard issue. What change was made and why does it result in a 'better organized patch'? Thx.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented

Hi @quietone!

The better patch would about the placement of the rule in phpcs.xml.dist, but then I found about the issue #3135933: Sort sniffs/rules in phpcs.xml.dist and write test to keep them sorted.
Can the patch #10 be the reviewed one?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review

Hi! Yes, the patch in #10 can be the one to review. I would add that to the Remaining Task in the Issue Summary and a big obvious comment. Since these coding standards have a unique Issue Summary we will just have to rely on the comment. I've done that and changed the status. Cheers.

To all reviewers, Review the patch in #10! Ignore the patch in #11. Thanks.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented

Thanks @quietone!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work 
+++ b/core/phpcs.xml.dist
@@ -120,6 +120,7 @@
   <rule ref="Drupal.Semantics.FunctionT">
+  <rule ref="Drupal.NamingConventions.ValidGlobal"/>
     <exclude name="Drupal.Semantics.FunctionT.NotLiteralString"/>
   </rule>

The new line is in the wrong place, we have <rule> nested inside another <rule>.

I also think we should be able to get rid of that global entirely, somehow.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
2902540-19.patch2.14 KB

Fixed both points in #18.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Needs work

I came to review.

This no longer applies to 9.2.x or 9.3.x. Since this is small and I am not familiar with field_test_memorize, I rerolled it locally and read the docs and played with the test. I get what this is doing and it looks fine to me, it is ready for RTBC, just needs a patch that applies to 9.3.x.

Needs a reroll.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review
FileSize
2902540-21.patch2.14 KB
reroll_diff_19-21.txt1.46 KB

Rerolled patch #19 for 9.3.x

quietone’s picture

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

Spokje, thanks.

This is ready now.

  • catch committed 91b2327 on 9.3.x 
    Issue #2902540 by guilhermevp, Spokje, longwave, quietone, mfernea: Fix...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +9.3.0 release notes

Committed 91b2327 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

pameeela’s picture

pameeela CreditAttribution: pameeela commented
Issue tags: +Needs release note

Would someone be able to add a release note snippet summarising any disruptions this change may cause?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: -Needs release note

9.3.0 shipped about a year ago. Release notes are irrelevant now 😅