Problem/Motivation

Recently, a new top-level directory composer was introduced. This directory holds sources for code that does not need to be packaged in the drupal/core subtree split, and therefore should not be in the core directory. However, only things in the core directory are being tested with phpcs, leading to a growing collection of coding violations in the composer location.

Proposed resolution

We should ensure that the composer directory is also sniffed for conformance with the Drupal coding standards. We could do this either by copying the phpcs configuration from the core directory, or by making a relative path from within the phpcs configuration file to the composer directory. Neither option is great, but the relative path is probably preferable to the duplicated configuration.

Remaining tasks

  • Start code-sniffing the composer directory.
  • Correct the existing coding standard violations. There are many, but phpcbf can help with most of them.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

n/a

CommentFileSizeAuthor
#27 interdiff_26-27.txt2.3 KBspokje
#27 3088730-27.patch9.08 KBspokje
#26 3088730-26.patch6.96 KBspokje
#26 interdiff_23-26.txt3.18 KBspokje
#23 diff_reroll_3088730_19-23.txt975 bytesankithashetty
#23 3088730-23.patch3.89 KBankithashetty
#19 3088730-19.patch4.45 KBlongwave
#18 coding-standards.png16.8 KBgreg.1.anderson
#17 interdiff_13_17.txt1.15 KBanmolgoyal74
#17 3088730-17.patch2.96 KBanmolgoyal74
#13 3088730-13.patch3.09 KBnaresh_bavaskar
#4 3088730_4.patch3.13 KBmile23

Comments

greg.1.anderson created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

We can add a root folder which will extend core/phpcs.xml.dist.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 3088730_4.patch3.13 KB

This moves the phpcs.xml.dist file to the root directory (out of the core directory), and modifies the composer.json phpcs script to use this new location.

This will likely break the phpcs plugin from DrupalCI, because DrupalCI\Build\Codebase\CodebaseInterface::getProjectConfigDirectory().

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

Now that core's .git repo is no longer "the root of somebodies project", we can move core's config files to the root of the git repo.

However, I think we might need drupalci to be able to determine if the config file lives at /core, then use that, otherwise default to / so that we can test patches like the above.

But we also have a someday plan of #2943856: [meta] Reorganize Components so they are testable in isolation -> would that include validation steps like phpcs? Would it include things like their own drupalci.yml file? Phpunit.xml?

I filed:
https://www.drupal.org/project/drupalci_testbot/issues/3093912 but I think it bears some discovery on whether or not we want 'one phpcs to rule them all' or individual files per subtree, and how that would work going forward in conjunction with our other plans for 'testable in isolation'

greg.1.anderson’s picture

greg.1.anderson commented 
+++ b/phpcs.xml.dist
@@ -1,26 +1,27 @@
+  <exclude-pattern>core/assets/vendor/*</exclude-pattern>

Do you know why core/assets/vendor/* is still being scanned for code style, even though this line in theory still excludes it?

We end up with 30k+ new standard violations with this patch; only a fraction of those are valid.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

The DrupalCI phpcs plugin ends up making a special case for core, where all relative paths start at core/ instead of ./

So therefore excluding a relative path means it starts at the wrong place.

I find it a little disheartening that core itself has 3150 CSS errors to begin with.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Parent issue: » #2571965: [meta] Fix PHP coding standards in core, stage 1

Added parent. #2571965: [meta] Fix PHP coding standards in core, stage 1

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

otoh, csslint isnt even the tool they want to use (stylelint is), so, maybe we should implement that change instead, and make sure it gets the right dir?

https://www.drupal.org/project/drupalci_testbot/issues/2866840

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I think this makes sense!

+++ b/phpcs.xml.dist
@@ -1,26 +1,27 @@
-  <exclude-pattern>*/node_modules/*</exclude-pattern>
-  <exclude-pattern>*/bower_components/*</exclude-pattern>
+  <exclude-pattern>core/*/node_modules/*</exclude-pattern>
+  <exclude-pattern>core/*/bower_components/*</exclude-pattern>

I think we should not change this. We also want to ignore those JS directories everywhere. That way our phpcs config file is a better template for others.

So do we need to fix the DrupalCI phpcs plugin first in #3093912: Add a shim to allow /core to move config to the root, and or, allow for multiple configs. or after this patch?

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.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3088730-13.patch3.09 KB

Just rerolled the #4 patch for 9.1.x

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.

tanubansal’s picture

tanubansal commented

Patch #13 is working fine on 9.1
This can be moved to RTBC

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work 
  1. +++ b/phpcs.xml.dist
@@ -1,26 +1,27 @@
+  <file>composer/</file>
+  <file>core/</file>

    no need for the ending slash? 

  2. +++ b/phpcs.xml.dist
@@ -1,26 +1,27 @@
-  <exclude-pattern>*/node_modules/*</exclude-pattern>
-  <exclude-pattern>*/bower_components/*</exclude-pattern>
+  <exclude-pattern>core/*/node_modules/*</exclude-pattern>
+  <exclude-pattern>core/*/bower_components/*</exclude-pattern>

    this should not be changed, we want to exclude those everywhere

Haha, I'm posting the same comment as before, but this has not been fixed yet.

We also need to clarify this: do we need to fix the DrupalCI phpcs plugin first in #3093912: Add a shim to allow /core to move config to the root, and or, allow for multiple configs. or after this patch?

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3088730-17.patch2.96 KB
new interdiff_13_17.txt1.15 KB

Addressed #16.

greg.1.anderson’s picture

greg.1.anderson commented
Status: Needs review » Needs work
StatusFileSize
new coding-standards.png16.8 KB

Note that although #17 shows as green, coding violations do not set the test results to "failed".

I suspect that the fact that this patch is changing the relative paths of files makes it appear to the test runner that an existing violation is a new violation (c.f. #6, #7). While we do not necessarily want to fix all of the existing violations throughout core here, we should fix anything in the Composer directory.

It might be useful to make a patch that turns on code sniffing only for the Composer directory and get everything that is reported there first, and then turn the other paths back on after that's finished.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3088730-19.patch4.45 KB

Do we have to move phpcs.xml.dist and restructure everything that uses it here, or is that out of scope? We can just add ../composer as a path and that seems to work.

In this patch I've also fixed all the coding standards violations in the composer directory.

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.

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

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

daffie’s picture

daffie commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3088730-23.patch3.89 KB
new diff_reroll_3088730_19-23.txt975 bytes

Rerolled the patch in #19, thanks!

longwave’s picture

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

Some new errors, I think these are newly added sniffs since the patch in #19


FILE: ...w/html/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 119 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 130 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 141 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
 150 | ERROR | Missing parameter comment
     |       | (Drupal.Commenting.FunctionComment.MissingParamComment)
----------------------------------------------------------------------


FILE: /var/www/html/composer/Plugin/Scaffold/Git.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 21 | ERROR | Parameter $io is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
 39 | ERROR | Parameter $io is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
 57 | ERROR | Parameter $io is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
----------------------------------------------------------------------


FILE: ...r/www/html/composer/Plugin/Scaffold/Operations/OperationData.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 34 | ERROR | Parameter $destination is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
----------------------------------------------------------------------


FILE: /var/www/html/composer/Plugin/Scaffold/ManageGitIgnore.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 28 | ERROR | Parameter $io is not described in comment
    |       | (Drupal.Commenting.FunctionComment.ParamMissingDefinition)
----------------------------------------------------------------------
vikashsoni’s picture

vikashsoni commented

Patch not working in drpual-9.3.x-dev

Checking patch composer/Generator/PackageGenerator.php...
error: composer/Generator/PackageGenerator.php: No such file or directory
Checking patch composer/Plugin/ProjectMessage/Message.php...
error: composer/Plugin/ProjectMessage/Message.php: No such file or directory
Checking patch composer/Plugin/Scaffold/Git.php...
error: composer/Plugin/Scaffold/Git.php: No such file or directory
Checking patch composer/Plugin/Scaffold/Operations/AppendOp.php...
error: composer/Plugin/Scaffold/Operations/AppendOp.php: No such file or directory
Checking patch composer/Plugin/Scaffold/Operations/OperationData.php...
error: composer/Plugin/Scaffold/Operations/OperationData.php: No such file or directory

Needs to re-roll

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new interdiff_23-26.txt3.18 KB
new 3088730-26.patch6.96 KB

@vikashsoni patch #23 is against 9.4.x-dev as stated in the Version select in the IS, so it not applying to 9.3.x-dev isn't an issue.

However, the failing tests that @longwave pointed out in #24 are a problem.

Attached patch tries to fix those.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new 3088730-27.patch9.08 KB
new interdiff_26-27.txt2.3 KB

New sniff, new problems.
Would love to moan about it, but I'm the one of the people that introduced the sniff...

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
daffie’s picture

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

Looks good to me.

alexpott’s picture

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

Committed and pushed 01bd426e28 to 10.0.x and b5aa3eb643 to 9.4.x and 00e831d136 to 9.3.x. Thanks!

Backported to 9.3.x to keep the code aligned and there is no functional change to run-time.

  • alexpott committed 01bd426 on 10.0.x 
    Issue #3088730 by Spokje, anmolgoyal74, ankithashetty, Mile23, longwave...

  • alexpott committed b5aa3eb on 9.4.x 
    Issue #3088730 by Spokje, anmolgoyal74, ankithashetty, Mile23, longwave...

  • alexpott committed 00e831d on 9.3.x 
    Issue #3088730 by Spokje, anmolgoyal74, ankithashetty, Mile23, longwave...

Status: Fixed » Closed (fixed)

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