Follow-up to #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard

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

Approach

We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.

Step 1: Add the coding standard to the whitelist

Every coding standard is identified by a "sniff". For example, an imaginary coding standard that would require all llamas to be placed inside a square bracket fence would be called the "Drupal.AnimalControlStructure.BracketedFence sniff". There are dozens of such coding standards, and to make the work easier we have started by only whitelisting the sniffs that pass. For the moment all coding standards that are not yet fixed are simply skipped during the test.

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 PHP CodeSniffer and the ruleset from the Coder module

Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:

$ composer require drupal/coder squizlabs/php_codesniffer
$ ./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.Commenting.InlineComment.DocBlock"/>

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
#53 interdiff_2719653_51-53.txt5.08 KBkarishmaamin
#53 2719653-53.patch45.13 KBkarishmaamin
#51 interdiff_46-51.txt32.33 KBsuresh prabhu parkala
#51 2719653-51.patch45.39 KBsuresh prabhu parkala
#46 2719653-46.patch7.73 KBandypost
#46 interdiff.txt775 bytesandypost
#44 2719653-43_44.txt737 bytesgauravvvv
#44 2719653-44.patch7.77 KBgauravvvv
#43 diff_reroll_2719653_35-43.txt3.25 KBankithashetty
#43 2719653-43.patch7.77 KBankithashetty
#35 2719653-35.patch7.76 KBlongwave
#25 interdiff-2719653-24-25.txt1.74 KBmfernea
#25 drupal-coding-standards-2719653-25.patch88.46 KBmfernea
#24 interdiff-2719653-21-24.txt2.4 KBmfernea
#24 drupal-coding-standards-2719653-24.patch87.49 KBmfernea
#21 drupal-coding-standards-2719653-21.patch87.17 KBsavkaviktor16@gmail.com
#19 drupal-coding-standards-2719653-19.patch86.61 KBsavkaviktor16@gmail.com
#16 drupal-coding-standards-2719653-15.patch87.36 KBmfernea
#10 fix-2719653-10.patch547.26 KBbasil_snowman
#5 Drupal.Commenting.InlineComment.DocBlock-2719653-5.patch140.07 KBvprocessor

Issue fork drupal-2719653

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

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Parent issue: #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard » #2571965: [meta] Fix PHP coding standards in core, stage 1
pashupathi nath gajawada’s picture

pashupathi nath gajawada commented
Assigned: Unassigned » pashupathi nath gajawada

I would fix it.

vprocessor’s picture

vprocessor commented
Assigned: pashupathi nath gajawada » vprocessor

I am on it

vprocessor’s picture

vprocessor commented
Assigned: vprocessor » Unassigned
Status: Active » Needs review
StatusFileSize
new Drupal.Commenting.InlineComment.DocBlock-2719653-5.patch140.07 KB

done

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll 
$ git apply Drupal.Commenting.InlineComment.DocBlock-2719653-5.patch 
error: patch failed: core/modules/field/src/Tests/FormTest.php:237
error: core/modules/field/src/Tests/FormTest.php: patch does not apply
error: core/modules/filter/tests/src/Kernel/FilterUnitTest.php: No such file or directory
error: patch failed: core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php:124
error: core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php: patch does not apply
error: core/modules/system/src/Tests/Common/TableSortExtenderUnitTest.php: No such file or directory
error: core/modules/system/src/Tests/File/DirectoryTest.php: No such file or directory
error: core/modules/system/src/Tests/File/UrlRewritingTest.php: No such file or directory
error: core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php: No such file or directory
error: core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php: No such file or directory
error: core/modules/system/src/Tests/Plugin/Condition/RequestPathTest.php: No such file or directory
error: core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php: No such file or directory
error: patch failed: core/modules/views/src/Plugin/views/filter/Combine.php:120
error: core/modules/views/src/Plugin/views/filter/Combine.php: patch does not apply
error: patch failed: core/phpcs.xml.dist:69
error: core/phpcs.xml.dist: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php:299
error: core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php: patch does not apply
prabhurajn654’s picture

prabhurajn654 commented
Assigned: Unassigned » prabhurajn654

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vermauv’s picture

vermauv commented
Assigned: prabhurajn654 » vermauv
basil_snowman’s picture

basil_snowman commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new fix-2719653-10.patch547.26 KB

patch added

Status: Needs review » Needs work

The last submitted patch, 10: fix-2719653-10.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.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.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mfernea’s picture

mfernea commented

Here we should only target Drupal.Commenting.InlineComment.DocBlock sniff. Previous patches contain unrelated fixes.

mfernea’s picture

mfernea commented
Assigned: vermauv » Unassigned
Status: Needs work » Needs review

Here is the patch.

mfernea’s picture

mfernea commented
StatusFileSize
new drupal-coding-standards-2719653-15.patch87.36 KB

As in, here.

mfernea’s picture

mfernea commented 
+++ b/core/lib/Drupal/Core/Link.php
@@ -10,9 +10,7 @@
-  /**
-   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
-   */
+  // @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   use LinkGeneratorTrait;

I'm not sure about this modification, as @deprecated should be used for declarations not for usages. So, we can also remove the comment.

savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com commented
Issue tags: +Needs reroll
savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com commented
Issue tags: -Needs reroll
StatusFileSize
new drupal-coding-standards-2719653-19.patch86.61 KB

re-rolled

Status: Needs review » Needs work

The last submitted patch, 19: drupal-coding-standards-2719653-19.patch, failed testing. View results

savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com commented
StatusFileSize
new drupal-coding-standards-2719653-21.patch87.17 KB

re-rolled again

savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com commented
Status: Needs work » Needs review
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Link.php
@@ -10,9 +10,7 @@
 class Link implements RenderableInterface {
 
-  /**
-   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
-   */
+  // @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   use LinkGeneratorTrait;

    So is Link deprecated or its use of LinkGeneratorTrait deprecated? If it's Link that's deprecated, then this deprecation message is in the wrong place. LinkGeneratorTrait is deprecated itself, so maybe this is marking that. Basically, it's unclear. 

  2. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1209,10 +1209,8 @@ public function query($get_count = FALSE) {
+    // An optimized count query includes just the base field instead of all the fields.
+    // Determine of this query qualifies by checking for a groupby or distinct.

    Wrap at 80 chars.

mfernea’s picture

mfernea commented
Status: Needs work » Needs review
StatusFileSize
new drupal-coding-standards-2719653-24.patch87.49 KB
new interdiff-2719653-21-24.txt2.4 KB
  1. LinkGeneratorTrait is deprecated
  2. Solved
mfernea’s picture

mfernea commented
StatusFileSize
new drupal-coding-standards-2719653-25.patch88.46 KB
new interdiff-2719653-24-25.txt1.74 KB

InlineCommentSniff is added twice in the patch from #24. I fixed that.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Link.php
@@ -10,9 +10,7 @@
-  /**
-   * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
-   */
+  // @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
   use LinkGeneratorTrait;

Sorry I wasn't clearer earlier. If it's deprecated then remove the comment because it has its own deprecation message. If the usage of this trait is deprecated here and it's going to be removed from Link, then change it to a @todo with a follow-up issue.

mfernea’s picture

mfernea commented

This comment was introduced in #2529560: Expand support for link objects.
Few comments related to this:

ivan berezhnov’s picture

ivan berezhnov commented
Issue tags: +CSKyiv18

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2719653-35.patch7.76 KB

I guess the sniff has been changed here at some point as many hunks of the previous patches in this thread are no longer needed for phpcs to pass. This patch only contains the changes required for the sniff to pass, no interdiff as this was rolled from scratch.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Not clear how to find coder's results at CI...

+++ b/core/modules/taxonomy/tests/src/Kernel/TermKernelTest.php
@@ -95,17 +95,16 @@ public function testTaxonomyVocabularyTree() {
+    // ------ term[3] | depth: 3
+    //
     // Count $term[1] parents with $max_depth = 1.

I bet it should trigger other rule

quietone’s picture

quietone commented

@andypost, do you mean the one about space after inline comments? That is still in discussion, particularly 2572659#comment-79. Or are you referring to something else?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I mean wrong indent should be caught by other sniffer

quietone’s picture

quietone commented

@andypost, I still do not understand. What is the 'other sniffer' you are referring to? And what action is to be taken here in this patch?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

PHPCS results are at https://dispatcher.drupalci.org/job/drupal_patches/77111/consoleFull

Search for "commit-code-check", this is the output from commit-code-check.sh that you can also run locally.

@andypost those lines look indented correctly to me, if you mean the empty comment line then that will have to be handled in #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard where there are other similar cases already.

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.

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 2719653-43.patch7.77 KB
new diff_reroll_2719653_35-43.txt3.25 KB

Just re-rolled the patch in #35, thanks!

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 2719653-44.patch7.77 KB
new 2719653-43_44.txt737 bytes

Re-rolled patch #43, Attached interdiff for same.

longwave’s picture

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

The code block in RearrangeFilter looks wrong to me, and:

FILE: ...www/html/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 84 | ERROR | Comment indentation error, expected only 1 spaces
    |       | (Drupal.Commenting.InlineComment.SpacingBefore)
----------------------------------------------------------------------
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt775 bytes
new 2719653-46.patch7.73 KB

Let's see another array syntax

daffie’s picture

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

All code changes look good to me.
The rule change is in phpcs.xml.dist.
To me it is RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 2719653-46.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Reviewed & tested by the community

unrelated test failure

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

https://www.drupal.org/pift-ci-job/2188125 shows that this introduces quite a few fails when run the coding standards checks. Seems there is more to do here!

suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 2719653-51.patch45.39 KB
new interdiff_46-51.txt32.33 KB

Tried to fix cs errors. Please review.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Not all cs are fixed.

karishmaamin’s picture

karishmaamin commented
StatusFileSize
new 2719653-53.patch45.13 KB
new interdiff_2719653_51-53.txt5.08 KB

Tried to remove custom command fail. Please review

daffie’s picture

daffie commented
Status: Needs work » Needs review

@karishmaamin: I you think your patch is ready for a review, then please the status of the issue to "Needs review".

quietone’s picture

quietone commented

The changes made in the latest patch move the PHP8 attribute to inside the doc block. According to php-attributes the annotation is to be before or after the docblock.

Before and After DocBlock comments

Attributes can appear before and after DocBlock comments. There is no standard recommendation for the code style, but this surely will be ironed out in a future PSR code-style recommendation.

I confirmed that the sniff does identify these as errors. It seems like we need an issue in coder to modify the sniff?

longwave’s picture

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

Yeah, this is an issue in Coder now. All the fails in https://www.drupal.org/pift-ci-job/2188125 are for things like

  /**
   * {@inheritdoc}
   */
  #[\ReturnTypeWillChange]
  public function open($save_path, $name) {

The attribute added between the docblock and method signature causes the sniff to trigger a false positive. Moving the attribute above the docblock doesn't help either, that just produces a different error.

Marking as postponed as there is nothing we can do here, we need a fix in Coder first.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3250346: InlineComment.DocBlock sniff does not understand PHP attributes

Opened #3250346: InlineComment.DocBlock sniff does not understand PHP attributes

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.

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

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.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.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.

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.

quietone’s picture

quietone commented
Status: Postponed » Active
Related issues: +#3250986: Sniff "Drupal.Commenting.InlineComment.WrongStyle" doesn't like #[\ReturnTypeWillChange]

The sniff referred to in #57 was closed in favor of #3250986: Sniff "Drupal.Commenting.InlineComment.WrongStyle" doesn't like #[\ReturnTypeWillChange], which is fixed.

quietone opened merge request !11255

quietone’s picture

quietone commented
Status: Active » Needs review
smustgrave’s picture

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

Will admit don't get the reason for going from /** to /* but if that's the rule.

The update to the .dist shows the rule being enforced and none of the changes seems off, minus what I mentioned. Think this is fine.

nod_ made their first commit to this issue’s fork.

nod_ closed merge request !11255

  • nod_ committed 9c7b3cbf on 11.x 
    Issue #2719653 by mfernea, quietone, savkaviktor16@gmail.com, andypost,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 9c7b3cb and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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