The phpcs.xml.dist should be excluding all failing rules. The latest release of the the rulesets in https://www.drupal.org/proejct/coder have three failing rules - we should add them to be excluded because core fails them both.

Also we have rules about .info and vendor directories that are no longer needed.

As this is test infra this can go in the an upcoming patch release and does not have to wait for a minor.

CommentFileSizeAuthor
#4 2623680.4.patch2.95 KBalexpott
#3 2623680.3.patch2.02 KBalexpott
#2 2623680.2.patch1.99 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2623680.2.patch1.99 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2623680.3.patch2.02 KB

More to do... #2572641: Fix 'Drupal.Commenting.DocCommentStar' coding standard and #2572649: Fix 'Drupal.Commenting.HookComment' coding standard have been fixed... so let's remove those the rules from the excluded... oh actually both of them need re-doing, lol. #2572641: Fix 'Drupal.Commenting.DocCommentStar' coding standard has a regression in core/lib/Drupal/Core/Render/Element/FormElement.php and #2572649: Fix 'Drupal.Commenting.HookComment' coding standard seems like the sniff was improved or was just done wrong.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2623680.4.patch2.95 KB

Well let's fix the tiny regression in #2572641: Fix 'Drupal.Commenting.DocCommentStar' coding standard since it is simple and then we have less to do...

The last submitted patch, 2: 2623680.2.patch, failed testing.

pfrenssen’s picture

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

Tested it and it works great. 

  1. +++ b/core/phpcs.xml.dist
@@ -2,11 +2,10 @@
-  <arg name="extensions" value="inc,info,install,module,php,profile,test,theme"/>
+  <arg name="extensions" value="inc,install,module,php,profile,test,theme"/>

    Nice catch! 

  2. +++ b/core/phpcs.xml.dist
@@ -2,11 +2,10 @@
-  <exclude-pattern>./vendor/*</exclude-pattern>

    Yep, that's no longer there.

When I first ran the test it couldn't find the Drupal ruleset, so I changed this line in phpcs.xml.dist:

-  <rule ref="Drupal">
+  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal">

For the moment this doesn't matter since the Coder module is not installed in core yet. It's something we might have to address in the future.

After changing this I could run the test without problems:

$ composer require drupal/coder squizlabs/php_codesniffer:master
$ cd core/
$ ../vendor/bin/phpcs -p
...
Time: 4 mins, 39.56 secs; Memory: 539.75Mb

No violations reported.

catch’s picture

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

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 8b1db19 on 8.1.x 
    Issue #2623680 by alexpott, pfrenssen: Fix phpcs.xml.dist to work with...

  • catch committed 91a081f on 
    Issue #2623680 by alexpott, pfrenssen: Fix phpcs.xml.dist to work with...

Status: Fixed » Closed (fixed)

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