Problem/Motivation

https://www.drupal.org/project/coder/releases/8.3.9 released on 8 May 2020 at 18:31 CST

Proposed resolution

Remember checking potential coding standard violations 

$ composer update drupal/coder squizlabs/php_codesniffer
$ composer run phpcs -- -ps

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

The development dependency drupal/coder has been updated to 8.3.9. New coding standards rules are now available with this update.

CommentFileSizeAuthor
#3 3134472-2.patch1000 bytesjungle
#4 3134731-2.patch2.05 KBjungle
#10 3134731-10.patch14.59 KBjungle
#10 all-sniffers-coder-8.3.9.txt6.03 KBjungle
#14 3134318-14.patch1.43 KBjungle
#14 raw-interdiff-10-14.txt6.72 KBjungle
#15 3134731-15.patch14.25 KBjungle
#15 raw-interdiff-10-15.txt6.72 KBjungle
#26 3134731-9.x-26.patch2.05 KBjungle
#26 3134731-8.9.x-26.patch1.71 KBjungle

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Version: 8.9.x-dev » 9.1.x-dev
Parent issue: #3121885: Update coder to 8.3.8 »
Related issues: +#3121885: Update coder to 8.3.8
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Active » Needs review
StatusFileSize
new 3134472-2.patch1000 bytes
  1. Run $ composer update drupal/coder squizlabs/php_codesniffer get coder updated only
    +--------------+-------+-------+
| Dev Changes  | From  | To    |
+--------------+-------+-------+
| drupal/coder | 8.3.8 | 8.3.9 |
+--------------+-------+-------
  2. Run $ composer run phpcs -- -ps no coding standard violations.
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new 3134731-2.patch2.05 KB

Again, a wrong patch uploaded, sorry!

andypost’s picture

andypost
he/him
Primary language Russian
commented

Btw maybe default core list of sniffers needs update with newly added to coder?

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

#5, good point, thank you @andypost!

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs review » Needs work
jonathan1055’s picture

jonathan1055 commented

For background, the reason for this new Coder release 8.3.9 (8th May) so soon after 8.3.8 (24th March) is primarily to allow #3094454: Fix remaining @deprecated manually and enable the coding standard to make progress at core 8.9. Work has already been done at Core 9.1 and 9.0 and they are completely clean with regard to @deprecated standards so there should be no problem in committing 8.3.9 there. Then it can be back-ported to Core 8.9 and that's the aim, so that we can fix all @deprecated text layouts and then enable the main deprecated sniff.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@jonathan1055 thanks for pointer, looks like better to file follow-up to add new sniffers

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Status: Needs work » Needs review
StatusFileSize
new 3134731-10.patch14.59 KB
new all-sniffers-coder-8.3.9.txt6.03 KB

  1. All sniffers available in coder 8.3.9 generated with the command below and attached.
    ./vendor/bin/phpcs --standard=Drupal,DrupalPractice --runtime-set installed_paths vendor/drupal/coder/coder_sniffer -e > all-sniffers-coder-8.3.9.txt

  2. Note for the reviewer(s): If you are using PHPStrom, you can split editor tabs vertically. (Window > Editor Tabs > Split Vertically). One tab opens
    all-sniffers-coder-8.3.9.txt, another tab opens core/phpcs.xml.dist comparing the two files side by side.

  3. Key changes:

    - PSR2.Classes.PropertyDeclaration, <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
+ Drupal.Classes.PropertyDeclaration,  <exclude name="Drupal.Classes.PropertyDeclaration.Underscore"/>

- Generic.Arrays.DisallowLongArraySyntax
+ Drupal.Arrays.DisallowLongArraySyntax

- Generic.CodeAnalysis.EmptyPHPStatement

    Generic.CodeAnalysis.EmptyPHPStatement is not available in the new version.

Thanks!

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Btw maybe default core list of sniffers needs update with newly added to coder?

Forgot to mention that #10 is to address #5 and

Add two other changes

  1. Sniffers sorted by alphabet
  2. Sniffers' totals per type are in comments
jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Found a workaround to get coder installed from packagist explicitly

  1.      "repositories": [
-        {
-            "type": "composer",
-            "url": "https://packages.drupal.org/8"
-        },

    Remove Drupal composer repository temporarily first in composer.json

  2. Run composer update drupal/coder squizlabs/php_codesniffer -vvv to get the new version installed from packagist.
  3. Revert the removal of Drupal composer repository
  4. Run composer update --lock to update the content-hash value in composer.lock.
andypost’s picture

andypost
he/him
Primary language Russian
commented

Nice! #12 is ready to be added to some docs page

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new 3134318-14.patch1.43 KB
new raw-interdiff-10-14.txt6.72 KB

Rerollted patch from #10 against 8.9.x

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new 3134731-15.patch14.25 KB
new raw-interdiff-10-15.txt6.72 KB

Please ignore the patch in #14, again and again, it's a wrong patch uploaded, sorry!

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

To reviewer(s):

  1. The patch in #10 is for 9.1.x and 9.0.x
  2. The patch in #15 is for 8.9.x

Thanks!

jonathan1055’s picture

jonathan1055 commented

Thanks @jungle. Are the changes to phpcs.xml.dist meant to result in all rules being listed, and now including the ones (commented out) that core does not yet meet. That's a good idea, but I think some are missing from the 8.9 patch.

Howevere, I am finding it hard to review/follow the differences when you have also re-sorted the sniffs alphabetically. I know this is a good idea, but can it be done in a separate patch later so that we can review th actual differences first? Then when we agree on the changes, the lines can be re-ordered.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented 
  1. +++ b/core/phpcs.xml.dist
@@ -22,23 +22,35 @@
+  <!-- Internal（1 sniff) -->
+  <rule ref="Internal.NoCodeFound">
+    <!-- No PHP code in *.yml -->
+    <exclude-pattern>*.yml</exclude-pattern>
+  </rule>

@@ -168,16 +185,10 @@
-  <!-- Internal sniffs -->
-  <rule ref="Internal.NoCodeFound">
-    <!-- No PHP code in *.yml -->
-    <exclude-pattern>*.yml</exclude-pattern>
-  </rule>

    Moved 

  2. +++ b/core/phpcs.xml.dist
@@ -22,23 +22,35 @@
+  <rule ref="Drupal.Arrays.DisallowLongArraySyntax"/>

@@ -145,12 +166,8 @@
-  <rule ref="Generic.Arrays.DisallowLongArraySyntax"/>

    renamed 

  3. +++ b/core/phpcs.xml.dist
@@ -22,23 +22,35 @@
+  <rule ref="Drupal.CSS.ClassDefinitionNameSpacing"/>
+  <rule ref="Drupal.CSS.ColourDefinition"/>
...
-  <rule ref="Drupal.CSS.ClassDefinitionNameSpacing"/>
-  <rule ref="Drupal.CSS.ColourDefinition"/>

    moved 

  4. +++ b/core/phpcs.xml.dist
@@ -22,23 +22,35 @@
+  <rule ref="Drupal.Classes.PropertyDeclaration">
+    <exclude name="Drupal.Classes.PropertyDeclaration.Underscore"/>
+  </rule>

@@ -222,17 +230,15 @@
-  <!-- PSR-2 sniffs -->
-  <rule ref="PSR2.Classes.PropertyDeclaration">
-    <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>
-  </rule>

    renamed 

  5. +++ b/core/phpcs.xml.dist
@@ -78,14 +91,7 @@
-  <rule ref="Drupal.Commenting.VariableComment">
-    <!-- Sniff for: DuplicateVar, EmptyVar, InlineVariableName -->
-    <exclude name="Drupal.Commenting.VariableComment.IncorrectVarType"/>
-    <exclude name="Drupal.Commenting.VariableComment.Missing"/>
-    <exclude name="Drupal.Commenting.VariableComment.MissingVar"/>
-    <exclude name="Drupal.Commenting.VariableComment.VarOrder"/>
-    <exclude name="Drupal.Commenting.VariableComment.WrongStyle"/>
-  </rule>

@@ -95,11 +101,20 @@
+  <rule ref="Drupal.Commenting.VariableComment">
+    <!-- Sniff for: DuplicateVar, EmptyVar, InlineVariableName -->
+    <exclude name="Drupal.Commenting.VariableComment.IncorrectVarType"/>
+    <exclude name="Drupal.Commenting.VariableComment.Missing"/>
+    <exclude name="Drupal.Commenting.VariableComment.MissingVar"/>
+    <exclude name="Drupal.Commenting.VariableComment.VarOrder"/>
+    <exclude name="Drupal.Commenting.VariableComment.WrongStyle"/>
+  </rule>

    moved 

  6. +++ b/core/phpcs.xml.dist
@@ -95,11 +101,20 @@
-  <rule ref="Drupal.ControlStructures.ElseIf"/>
...
+  <rule ref="Drupal.ControlStructures.ElseIf"/>

    moved 

  7. +++ b/core/phpcs.xml.dist
@@ -145,12 +166,8 @@
-  <!-- Drupal Practice sniffs -->
-  <rule ref="DrupalPractice.Commenting.ExpectedException"/>

@@ -337,7 +354,46 @@
+  <!-- Drupal Practice (38 sniffs) -->
...
+  <rule ref="DrupalPractice.Commenting.ExpectedException"/>

    moved 

  8. +++ b/core/phpcs.xml.dist
@@ -145,12 +166,8 @@
-  <rule ref="Generic.CodeAnalysis.EmptyPHPStatement" />

    no more existed 

  9. +++ b/core/phpcs.xml.dist
@@ -192,9 +203,6 @@
-  <rule ref="PEAR.Functions.ValidDefaultValue"/>

@@ -222,17 +230,15 @@
+  <rule ref="PEAR.Functions.ValidDefaultValue"/>

    moved 

  10. +++ b/core/phpcs.xml.dist
@@ -291,6 +306,15 @@
+  <!--<rule ref="Squiz.ControlStructures.SwitchDeclaration"/>-->
+  <rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
+    <properties>
+      <property name="equalsSpacing" value="1"/>
+    </properties>
+  </rule>
+  <rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforeArg">
+    <severity>0</severity>
+  </rule>

@@ -308,15 +332,8 @@
-  <rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
-    <properties>
-      <property name="equalsSpacing" value="1"/>
-    </properties>
-  </rule>
-  <rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing.NoSpaceBeforeArg">
-    <severity>0</severity>
-  </rule>

    moved

Only one rule deleted, Generic.CodeAnalysis.EmptyPHPStatement, as it does not exist any more.

The rest of the deletion operations are related to move/rename operations.

I realised it's hard to review, but that was in the middle. I should make and submit a reordered patch first. If you think it's necessary, I am ok to do it in a separate issue. or do it again to make a reordered patch to make review easier.

Thank you @jonathan1055!

jonathan1055’s picture

jonathan1055 commented

Thanks @jungle, now I can see what is going on. No need to do a separate issue. Others may have an opinion on which is easier to review first, but I would say leave all the rules where they are for now, so we can see the differences, then when the renames/additions/deletions are agreed we can look at moving the rules.

There is one discrepancy which needs to be addressed - above the 'Drupal' rules is the comment

<!-- Only include specific sniffs that pass. This ensures that, if new sniffs are added, HEAD does not fail.-->

and only the sniffs which pass are listed. Sub-sniffs that fail are excluded, but rules which fail completely are not present.

However, with your patch, in the 'DrupalPractice' section you have added many rules, all commented out, so this is different from how it is done for the 'Drupal' section above. I think we need a consistent approach. Others will have a view on this too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Agree that the diff is hard to read now, can we sort these in a separate issue with no other changes?

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented

Then I would suggest filing 3 more issues:

  1. Step1: Get phpcs.xml.dist sorted (Issue #1 to be filed)
  2. Step2: Keep sniffers in sync with all sniffers from the previous version -- 8.3.8. (Issue #2 to be filed)
  3. Setp3: Reroll the patch here, and keep sniffers in sync again with 8.3.9 (#3134731)
  4. Step4: Clean up the issue queue, adding @todo with filed issue links to corresponding sniffers/rules in phpcs.xml.dist (Issue #3 to be filed)

And about the principles, I would suggest that all sniffers should be listed in phpcs.xml.dist and sorted alphabetically.

  1. For fixed ones, listing means enabled
  2. For pending fixing ones:
    a) if it's a sub-sniffer, it should be excluded and sorted alphabetically.
    b) if it's not a sub-sniffer, it should be listed, commented and sorted alphabetically as well.
  3. For won't fix ones, give them inline-comments above to remind contributors that do not waste time on it.

I will wait for agreement before proceeding. Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think that is a reasonable approach, I guess splitting it that way makes the most sense although will take longer to get committed because of the multiple issues. I also think that once the sniffs are sorted and we have all sniffs in the phpcs.xml.dist (commented out or not) that we should add a test to ensure that they stay sorted and that all rules are in place, so future upgrades of Coder will tell us via test failure that we have missed new (or removed) sniffs.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Alternatively: is it possible to say something like

<rule ref="Drupal">
<exclude name="..." />
<exclude name="..." />
<exclude name="..." />
<exclude name="..." />
</rule>

so we only explicitly exclude sniffs we know about? Then when we upgrade Coder and get new sniffs, they automatically apply, and we have to either fix or exclude them?

jonathan1055’s picture

jonathan1055 commented

although will take longer to get committed ...

Yes. This issue must focus solely on upgrading to Coder 8.3.9 as that is what it was created for. Then the other things can be done afterwards in preparation for when Coder 8.3.10 is released. Hence back at patch 2 in comment #4 which is ready for review at 9.0 and 9.1

I also think that once the sniffs are sorted and we have all sniffs in the phpcs.xml.dist (commented out or not) that we should add a test to ensure that they stay sorted and that all rules are in place,

That's a good idea

[edit, cross post]

... so we only explicitly exclude sniffs we know about? Then when we upgrade Coder and get new sniffs, they automatically apply, and we have to either fix or exclude them?

That's an even better idea, as it reduces work, and will also allow new sniffs which already pass to be automatically included with no extra work. If that works, then why was it not done like that before? There must be a catch somewhere.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

@jonathan1055: OK so I read the earlier issue comments again and agree with #8 that we should get this upgrade done first, the reordering of rules etc can be done in a separate issue so as not to hold up this upgrade going back to 8.9.x. Therefore #4 is RTBC and @jungle is free to create separate issues as followups.

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
StatusFileSize
new 3134731-9.x-26.patch2.05 KB
new 3134731-8.9.x-26.patch1.71 KB

Thanks, @longwave and @jonathan1055 for your comments!

Reuploading the patch in #4 as 3134731-9.x-26.patch for 9.x, and rerolled a patch from #15 for 8.9.x

jungle’s picture

jungle
He/Him
Location Chongqing, China
commented
Related issues: +#3135933: Sort sniffs/rules in phpcs.xml.dist and write test to keep them sorted, +#3135935: Keep all sniffs in phpcs.xml.dist in sync with the locked version of Coder

2 issues field as discussed.

>Step4: Clean up the issue queue, adding @todo with filed issue links to corresponding sniffers/rules in phpcs.xml.dist (Issue #3 to be filed)

This is optional, so no issue filed.

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 2838d78eba to 9.1.x and e7bf981a81 to 9.0.x. Thanks!

Committed 86f2bc2bc4 and pushed to 8.9.x. Thanks!

Backported to 8.9.x so that dependencies are up-to-date as much as possible.

  • alexpott committed 2838d78 on 9.1.x 
    Issue #3134731 by jungle, jonathan1055, longwave, andypost: Update coder...

  • alexpott committed e7bf981 on 9.0.x 
    Issue #3134731 by jungle, jonathan1055, longwave, andypost: Update coder...

  • alexpott committed 86f2bc2 on 8.9.x 
    Issue #3134731 by jungle, jonathan1055, longwave, andypost: Update coder...
jonathan1055’s picture

jonathan1055 commented

Thank you for the fast turn-around Alexpott. Now I can finish off #3094454: Fix remaining @deprecated manually and enable the coding standard and #3122186: Fix @deprecated versions and enable Commenting.Deprecated.DeprecatedVersionFormat sniff

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Issue tags: +8.9.0 release notes, +9.0.0 release notes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2055851: Remove translation of exception messages

The dependent issue unblocked #2055851: Remove translation of exception messages

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Status: Fixed » Closed (fixed)

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