Problem/Motivation

In #2571965: [meta] Fix PHP coding standards in core, stage 1 we are gradually fixing the coding standards in the Drupal standard. This issue is all about fixing the best practices in the DrupalPractice standard.

There are 40 sniffs in the DrupalPractice standard, and they can be listed using:

$ cd /path/to/drupal/root/
$ composer install
$ ./vendor/bin/phpcs --standard=./vendor/drupal/coder/coder_sniffer/DrupalPractice -e

This produces

The DrupalPractice standard contains 40 sniffs
DrupalPractice (40 sniffs)
--------------------------
  DrupalPractice.CodeAnalysis.VariableAnalysis
  DrupalPractice.Commenting.AuthorTag
  DrupalPractice.Commenting.CommentEmptyLine
  DrupalPractice.Commenting.ExpectedException
  DrupalPractice.Constants.GlobalConstant
  DrupalPractice.Constants.GlobalDefine
  DrupalPractice.FunctionCalls.CheckPlain
  DrupalPractice.FunctionCalls.CurlSslVerifier
  DrupalPractice.FunctionCalls.DbQuery
  DrupalPractice.FunctionCalls.DbSelectBraces
  DrupalPractice.FunctionCalls.DefaultValueSanitize
  DrupalPractice.FunctionCalls.FormErrorT
  DrupalPractice.FunctionCalls.InsecureUnserialize
  DrupalPractice.FunctionCalls.LCheckPlain
  DrupalPractice.FunctionCalls.MessageT
  DrupalPractice.FunctionCalls.TCheckPlain
  DrupalPractice.FunctionCalls.Theme
  DrupalPractice.FunctionCalls.VariableSetSanitize
  DrupalPractice.FunctionDefinitions.AccessHookMenu
  DrupalPractice.FunctionDefinitions.FormAlterDoc
  DrupalPractice.FunctionDefinitions.HookInitCss
  DrupalPractice.FunctionDefinitions.InstallT
  DrupalPractice.General.AccessAdminPages
  DrupalPractice.General.ClassName
  DrupalPractice.General.DescriptionT
  DrupalPractice.General.ExceptionT
  DrupalPractice.General.FormStateInput
  DrupalPractice.General.LanguageNone
  DrupalPractice.General.OptionsT
  DrupalPractice.General.VariableName
  DrupalPractice.InfoFiles.CoreVersionRequirement
  DrupalPractice.InfoFiles.Description
  DrupalPractice.InfoFiles.NamespacedDependency
  DrupalPractice.Objects.GlobalClass
  DrupalPractice.Objects.GlobalDrupal
  DrupalPractice.Objects.GlobalFunction
  DrupalPractice.Objects.StrictSchemaDisabled
  DrupalPractice.Objects.UnusedPrivateMethod
  DrupalPractice.Variables.GetRequestData
  DrupalPractice.Yaml.RoutingAccess

The core 10.1 phpcs.xml.dist file does not add the complete "DrupalPractice" ruleset but instead specifically includes just five rules:

DrupalPractice.CodeAnalysis.VariableAnalysis
DrupalPractice.Commenting.ExpectedException
DrupalPractice.General.ExceptionT
DrupalPractice.InfoFiles.NamespacedDependency
DrupalPractice.Objects.GlobalFunction

The other 35 best practices are not tested for at all. The initial tasks are therefore:

  1. Determine which best practices core code adheres to
  2. Add these into phpcs.xml.dist to ensure that we don't get any regression and introduce faults for existing passing sniffs.
  3. On a sniff-by-sniff basis add child issues for the remaining failing sniffs
  4. Decide if Core can (or should) adhere to them and fix the code or ignore the sniff as appropriate

Issue fork drupal-3310127

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

The full set of sniffs in the DrupalPractice standard can be tested on the /core folder using:

$ ./vendor/bin/phpcs -p -s --colors --standard=./vendor/drupal/coder/coder_sniffer/DrupalPractice --report-source core/

This produces

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
-------------------------------------------------------------------------------
SOURCE                                                                    COUNT
-------------------------------------------------------------------------------
Internal.Tokenizer.Exception                                              1272
DrupalPractice.Objects.GlobalFunction.GlobalFunction                      975
DrupalPractice.Commenting.CommentEmptyLine.SpacingAfter                   423
DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable               378
DrupalPractice.Objects.GlobalDrupal.GlobalDrupal                          274
DrupalPractice.FunctionCalls.InsecureUnserialize.InsecureUnserialize      140
DrupalPractice.General.OptionsT.TforValue                                 96
DrupalPractice.Constants.GlobalConstant.GlobalConstant                    43
DrupalPractice.General.DescriptionT.DescriptionT                          29
DrupalPractice.Variables.GetRequestData.SuperglobalAccessed               22
DrupalPractice.Objects.GlobalClass.GlobalClass                            19
DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedUnsetVariable       15
DrupalPractice.FunctionCalls.Theme.ThemeFunctionDirect                    8
DrupalPractice.Objects.StrictSchemaDisabled.StrictConfigSchema            8
DrupalPractice.Variables.GetRequestData.SuperglobalAccessedWithVar        7
DrupalPractice.Objects.UnusedPrivateMethod.UnusedMethod                   4
DrupalPractice.Commenting.AuthorTag.AuthorFound                           1
-------------------------------------------------------------------------------
A TOTAL OF 3714 SNIFF VIOLATIONS WERE FOUND IN 17 SOURCES
-------------------------------------------------------------------------------

However this does not include all the problems, and also does include some sniffs on files which should be excluded. An alternative (better) way is to add the single line <rule ref="DrupalPractice"/> into core/phpcs.xml.dist, then run the command using this file as the standard:

./vendor/bin/phpcs -p -s --colors --standard=core/phpcs.xml.dist --report-source core/

This shows the accurate list of sniffs that need fixing (or ignoring) as follows:

PHP CODE SNIFFER VIOLATION SOURCE SUMMARY
---------------------------------------------------------------------------------
SOURCE                                                                      COUNT
---------------------------------------------------------------------------------
DrupalPractice.Objects.GlobalFunction.GlobalFunction                        975
DrupalPractice.Yaml.RoutingAccess.OpenCallback                              469
DrupalPractice.Commenting.CommentEmptyLine.SpacingAfter                     438
DrupalPractice.Objects.GlobalDrupal.GlobalDrupal                            274
DrupalPractice.InfoFiles.CoreVersionRequirement.CoreVersionRequirement      176
DrupalPractice.FunctionCalls.InsecureUnserialize.InsecureUnserialize        140
DrupalPractice.General.OptionsT.TforValue                                   96
DrupalPractice.Constants.GlobalConstant.GlobalConstant                      71
DrupalPractice.InfoFiles.Description.Missing                                60
DrupalPractice.General.DescriptionT.DescriptionT                            30
DrupalPractice.Variables.GetRequestData.SuperglobalAccessed                 22
DrupalPractice.Yaml.RoutingAccess.PermissionFound                           20
DrupalPractice.Objects.GlobalClass.GlobalClass                              19
DrupalPractice.FunctionCalls.Theme.ThemeFunctionDirect                      8
DrupalPractice.Objects.StrictSchemaDisabled.StrictConfigSchema              8
DrupalPractice.Variables.GetRequestData.SuperglobalAccessedWithVar          7
DrupalPractice.Constants.GlobalDefine.GlobalConstant                        5
DrupalPractice.Objects.UnusedPrivateMethod.UnusedMethod                     4
---------------------------------------------------------------------------------
A TOTAL OF 2822 SNIFF VIOLATIONS WERE FOUND IN 18 SOURCES
---------------------------------------------------------------------------------
jonathan1055’s picture

jonathan1055 commented

The differences between the two outputs above are:

    Sniffs now not reported

  • Internal.Tokenizer.Exception - these are minified files, now excluded in phpcs.xml.dist
  • DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable - these are in tests and translation files whcih are excluded in phpcs.xml.dist
  • DrupalPractice.CodeAnalysis.VariableAnalysis.UndefinedUnsetVariable - severity set to zero in phpcs.xml.dist
  • DrupalPractice.Commenting.AuthorTag.AuthorFound - file is in core/lib/Drupal/Component/Diff which is excluded

Additional sniffs that fail

DrupalPractice.Yaml.RoutingAccess.OpenCallback
DrupalPractice.InfoFiles.CoreVersionRequirement.CoreVersionRequirement
DrupalPractice.InfoFiles.Description.Missing
DrupalPractice.Yaml.RoutingAccess.PermissionFound
DrupalPractice.Constants.GlobalDefine.GlobalConstant

Number of failures changed

DrupalPractice.Commenting.CommentEmptyLine.SpacingAfter 423 -> 438
DrupalPractice.Constants.GlobalConstant.GlobalConstant 43 -> 71
DrupalPractice.General.DescriptionT.DescriptionT 29 -> 30

jonathan1055 opened merge request !2765

jonathan1055’s picture

jonathan1055 commented
Title: [META] Fix DrupalPractice best practice in Core » 3310127-drupal-practice

First commit will show all the failing DrupalPractice sniffs. This is not for commiting, just done to get a baseline before progressing.

jonathan1055’s picture

jonathan1055 commented
Title: 3310127-drupal-practice » [META] Fix DrupalPractice best practice in Core

Why does that happen? the title gets replaced by the branch name unintentionally.

jonathan1055’s picture

jonathan1055 commented

Created a second branch including the 20 DrupalPractice sniffs that already pass in Core.

One phpunit test is included, which checks the order of newly-added sniffs. This should fail because the sniffs are not in the correct order.

jonathan1055 opened merge request !2768

jonathan1055’s picture

jonathan1055 commented

The 20 added sniffs all pass and the phpcs\SortTest shows that they have been added in the correct order.
Now reset drupalci.yml so that this 3310127-sniffs-that-pass is ready for committing.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review

MR 3310127-sniffs-that-pass is ready for review to be committed. This will ensure that no new failures get committed for the 20 DrupalPractices sniffs that currently pass but are not tested for.

erikaagp’s picture

erikaagp commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+  <rule ref="DrupalPractice.FunctionCalls.TCheckPlain"/>
+  <rule ref="DrupalPractice.FunctionCalls.VariableSetSanitize"/>

These rules don't make any sense in the Drupal 9 / 10 context. And I'm pretty sure other one's don't either. I don't think we should be adding sniffs that check for incorrect usage of functions that no longer exist.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

I don't think we should be adding sniffs that check for incorrect usage of functions that no longer exist.

@alexpott yes that's reasonable. I can remove those two easily.

It comes down the way we use phpcs.xml in core, see
#2912811: Sniff definitions approach in phpcs.xml.dist: exclude vs severity
and
#3135935: Keep all sniffs in phpcs.xml.dist in sync with the locked version of Coder

alexpott’s picture

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

@jonathan1055 - it's not just those two. Someone should go through the entire list and work out if they are at all relevant. FWIW I'm not a huge fan of the DrupalPracticeSniffs. And question their value to core. I'd rather turn this around and work out which ones if any actually add value and decide what to include based on that.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jonathan1055 opened merge request !3536

jonathan1055’s picture

jonathan1055 commented

New branch for 10.1. Initially running all DrupalPractice sniffs to see what we get.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

Updated IS for 10.1.x

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

There are 40 sniffs in DrupalPractice. 5 are already in phpcs.xml.dist and being checked for. 15 have failures. 2 have been suggested as no longer relevant to 10.x (see comment #12). That leaves 40 - 5 - 15 -2 = 18 which already pass in core 10.1 but are not tested for. The next commit adds these 18 sniffs into phpcs.xml.dist

jonathan1055’s picture

jonathan1055 commented

That's good. The dispatcher output now shows that 23 sniffs (not 5) were checked, and they all passed. 

DrupalPractice (23 sniffs)
--------------------------
  DrupalPractice.CodeAnalysis.VariableAnalysis
  DrupalPractice.Commenting.AuthorTag
  DrupalPractice.Commenting.ExpectedException
  DrupalPractice.FunctionCalls.CheckPlain
  DrupalPractice.FunctionCalls.CurlSslVerifier
  DrupalPractice.FunctionCalls.DbQuery
  DrupalPractice.FunctionCalls.DbSelectBraces
  DrupalPractice.FunctionCalls.DefaultValueSanitize
  DrupalPractice.FunctionCalls.FormErrorT
  DrupalPractice.FunctionCalls.LCheckPlain
  DrupalPractice.FunctionCalls.MessageT
  DrupalPractice.FunctionDefinitions.AccessHookMenu
  DrupalPractice.FunctionDefinitions.FormAlterDoc
  DrupalPractice.FunctionDefinitions.HookInitCss
  DrupalPractice.FunctionDefinitions.InstallT
  DrupalPractice.General.AccessAdminPages
  DrupalPractice.General.ClassName
  DrupalPractice.General.ExceptionT
  DrupalPractice.General.FormStateInput
  DrupalPractice.General.LanguageNone
  DrupalPractice.General.VariableName
  DrupalPractice.InfoFiles.NamespacedDependency
  DrupalPractice.Objects.GlobalFunction

The 18 additional sniffs could now be committed to the phpcs.xml.dist file, as in the MR, so that we do not accidentally introduce new failures for these in future commits.

alexpott’s picture

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

There's still one in that list that make no sense. Like most of the function call ones are checking for functions that don't exist. Perhaps they should have other equivalents but someone needs to do that work. As I wrote before in #14 I think we should swap it around and chose to add each sniff based on the value it gives to us not blindly just because they pass.

A quick check reveals that DrupalPractice.FunctionCalls.CheckPlain, DrupalPractice.FunctionCalls.LCheckPlain, DrupalPractice.General.AccessAdminPages, DrupalPractice.General.VariableName are bogus for Drupal 10 and there are more...

jonathan1055’s picture

jonathan1055 commented

Thanks @alexpott

we should swap it around and chose to add each sniff based on the value it gives to us not blindly just because they pass

Yes OK, I accept that. I just wanted to make a new branch for 10.1 and update the issue to get it to the same point we had reached in 9.5.

I will go through all 35 sniffs in DrupalPractice that are not yet included in the core phpcs.xml.dist and come back with a suggested list to add, and reasons for excluding the others. I think we need to document why sniffs are excluded so that someone in future does not have to go through the same process.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think we need to document why sniffs are excluded...

We also need to do something about upstream - like I think coder shouldn't be running quite a few of these rules on Drupal 9 / 10 code

quietone’s picture

quietone commented

Yes, it will help to be sure we are running the right sniffs and to add some documentation.

I made a child issue for the remaining work of implementing DrupalPractice.CodeAnalysis.VariableAnalysis and moved the few remaining issues there.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bbrala’s picture

bbrala
Location Netherlands
commented
Issue tags: +Coding standards

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.