Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTidDepth.php
   @@ -54,7 +54,7 @@ public function query() {
   -      $operator = 'IN';# " IN (" . implode(', ', array_fill(0, sizeof($this->value), '%d')) . ")";
   +      $operator = 'IN';// " IN (" . implode(', ', array_fill(0, sizeof($this->value), '%d')) . ")";
        }
   

   IMHO we should move that onto the previous line

2. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -127,7 +127,7 @@ public function testArgumentDefaultFixed() {
      /**
       * @todo Test php default argument.
       */
   -  //function testArgumentDefaultPhp() {}
   +  // function testArgumentDefaultPhp() {}
   

   Let's remove this code

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
   @@ -64,7 +64,7 @@ function testEnableTargetLogging() {
   -    db_query('SELECT age FROM {test} WHERE name = :name', array(':name' => 'Ringo'), array('target' => 'replica'));//->fetchCol();
   +    db_query('SELECT age FROM {test} WHERE name = :name', array(':name' => 'Ringo'), array('target' => 'replica'));// ->fetchCol();
   

   Let's remove this pointless comment as well

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -104,7 +104,7 @@ public function testEntityTypeUpdateWithoutData() {
   -    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected); //, 'EntityDefinitionUpdateManager reports the expected change summary.');
   +    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected); // , 'EntityDefinitionUpdateManager reports the expected change summary.');
   

   Move on top

rerolled & fixed according with dawehner comment #7

rerolled

To be clear, removing this test is 100% fine, we don't have this PHP argument_default plugin anymore.

Novice to fix the issue pointed out by mile23

Tasks that remain:

1. Make the fix suggested by mile23, which is to remove the argument default test and move the doc block.

Added patch for 8.3.x

Re-uploading. Missed phpcs.xml.dist changes.

Just noticed. Missed out @dawehner comments #7

Final patch from my end now.

@malavya Ensure to create interdiffs in the future.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTidDepth.php
   @@ -54,7 +54,8 @@ public function query() {
   -      $operator = 'IN';# " IN (" . implode(', ', array_fill(0, sizeof($this->value), '%d')) . ")";
   +      // " IN (" . implode(', ', array_fill(0, sizeof($this->value), '%d')) . ")";
   +      $operator = 'IN';
   

   Not really useful to keep this comment without explaining what this even is? I would remove it.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
   @@ -104,7 +104,8 @@ public function testEntityTypeUpdateWithoutData() {
   -    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected); //, 'EntityDefinitionUpdateManager reports the expected change summary.');
   +    // , 'EntityDefinitionUpdateManager reports the expected change summary.');
   +    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected);
   

   That comment is not really useful, let's just remove it?

So this seems to detect a whole bunch of commented out code that is left in our code base. Let's make some followups to get rid of it.

Yes but coding standards fixes are acceptable right up to 8.3.0-RCx.

/v/w/h/drupal83> 
git apply --index 2715485-20.patch
error: patch failed: core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php:104
error: core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php: patch does not apply

Rerolled and made changes from #24

Thanks, looks like you forgot to fix CommentResourceTestBase.php which shows up with 6 errors after I run phpcs on core.

1. +++ b/core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php
   @@ -172,7 +172,7 @@ public function testEditorWithCustomMetadata() {
   -    $this->assertEqual($expected, $metadata); //, 'The correct metadata (including custom metadata) is generated.');
   +    $this->assertEqual($expected, $metadata); // , 'The correct metadata (including custom metadata) is generated.');
   

   Comments should not appear after statements. I think we should move the commented out message just as normal comment to the line before.

Here is the patch that i made. I also changed, this line

// , 'The correct metadata (including custom metadata) is generated.');

to

// 'The correct metadata (including custom metadata) is generated.');

I don't understand what's the need for , here.

The comment was just commented out code from the assertion call where you can pass a third parameter as message.

+++ b/core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php
@@ -172,7 +172,8 @@ public function testEditorWithCustomMetadata() {
-    $this->assertEqual($expected, $metadata); // , 'The correct metadata (including custom metadata) is generated.');
+    // 'The correct metadata (including custom metadata) is generated.');
+    $this->assertEqual($expected, $metadata);

Cool, can you also remove "');" at the end?

Here is the new patch.

+++ b/core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php
@@ -172,7 +172,7 @@ public function testEditorWithCustomMetadata() {
-    // 'The correct metadata (including custom metadata) is generated.');
+    // , 'The correct metadata (including custom metadata) is generated.'.

No, you should remove the quotes and the "," to make it look like a normal comment.

And I don't see the required changes to CommentResourceTestBase in this patch. Can you run ../vendor/bin/phpcs in the core directory and make sure its output is empty before you post the next patch. Thanks!

This needs work as the required phpcs.xml.dist are not correct.

fixed the phpcs.xml.dist

Please check the test results. There are 378 cs violations.

Actually we are not excluding enough sniffs. I would go for these lines in phpcs.xml.dist:

  <rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Commenting/InlineCommentSniff.php">
    <!-- Sniff for: NoSpaceBefore, WrongStyle -->
    <exclude name="Drupal.Commenting.InlineComment.DocBlock"/>
    <exclude name="Drupal.Commenting.InlineComment.InvalidEndChar"/>
    <exclude name="Drupal.Commenting.InlineComment.NotCapital"/>
    <exclude name="Drupal.Commenting.InlineComment.SpacingAfter"/>
    <exclude name="Drupal.Commenting.InlineComment.SpacingBefore"/>
  </rule>

Other notes:

• Please don't remove public from functions' definitions (ForumTest and AliasTest 2 places)
• Make the comment from MetadataGeneratorTest look like a real comment - no quotes. That text was the initial assertEqual's third param ($message).
• I don't think that modifications from ArgumentDefaultTest.php are ok. I would just add the required space.
• In EntityDefinitionUpdateTest keep the comment just like in MetadataGeneratorTest.

@mfernea not sure about the below line

I don't think that modifications from ArgumentDefaultTest.php are ok. I would just add the required space.

fixed 378 cs issue and also fixed the other point.

Hmm... actually you should have only modified phpcs.xml.dist as you did, but not fix the 378 cs issues. They wouldn't have been raised again. Also, they are not related to the sniffs we are fixing in this issue, mentioned in the title.
My suggestion would be to start from the patch at #37, do the modifications from #39 and see what else pops up. Also post an interdiff between the new patch and #37.

Looking at the interdiff between #37 and #40 regarding the issues mentioned in #39 the modifications look good.
Regarding ArgumentDefaultTest.php:
I would only add the space after "//": //function testArgumentDefaultPhp() {} -> // function testArgumentDefaultPhp() {}

@mfernea

fixed as per your comment.

Looks good.

In the latest patch testArgumentDefaultNode function's comment was removed and it triggers a cs error. The comment was:

/**
 * Test node default argument.
 */

Also, if I look at the test results I notice one extra cs issue.

@mfernea

fixed as per the comment

Test failure from #2857843: Random fail in Drupal\KernelTests\Core\Entity\ContentEntityChangedTest::testChanged.

Is this identical to #2719649: Fix 'Drupal.Commenting.InlineComment.SpacingBefore' coding standard? The patch seems to be targeting the same lines.

Good work! The patch looks fine. Applies cleanly. No cs errors.

@harsha012: Maybe one more thing, but less important. Add back the empty line after:

// function testArgumentDefaultPhp() {}

@cburschka: If I apply this patch there are still a lot of SpacingBefore issues. SpacingBefore focuses on indentation.

Quick re-roll. I also added back the blank line mentioned at #50.

Re-rolled patch from #46 and re-added the blank line mentioned in #50 (same as was attempted in #52).

Comments should start with capital letter and end with full stops.

-        //empty directory - returns false
+        // Empty directory - returns false.

@tiago.urbano We are not targeting those sniffs in this patch. As you can see in the phpcs.xml.dist Drupal.Commenting.InlineComment.InvalidEndChar and Drupal.Commenting.InlineComment.NotCapital are excluded. They are handled in other issues.

Patch looks good. No cs issues. RTBC.

So this patch mixes several different kinds of fixes to comments and fixes several rules in a single patch, which isn't the best issue scope per https://www.drupal.org/core/scope. It's mostly adding a space between the double slash and words, but there are some places that are moved to the line before and several places that are removing commented code entirely. We shouldn't do that -- each place where there is commented code should probably have a git blame and be discussed separately.

Keep in mind that coding standards issues are often best reviewed with word diffs. git diff --color-words would show nothing for the majority of this patch where it's just fixing whitespace, but then there are a number of places where it shows something and then the reviewer has to switch contexts to think about the lines and do research into why they are being removed.

+++ b/core/modules/comment/src/Plugin/views/sort/Thread.php
@@ -16,14 +16,14 @@ class Thread extends SortPluginBase {
-      //@todo is this secure?
+      // @todo is this secure?

Aside: This is a terrifying comment to read.

Can we make this patch a patch that just fixes the comment initial whitespace rule, have a different issue that fixes the "comment on its own line" rule, and have separate issues that discuss removing the commented-out code rather than doing it in this patch, with research into why the commented code is there in the first place?

I've removed issue credit for @tiago.urbano because that review is out of scope for this issue, and attaching screenshots of patches applied to code is not helpful. I've also removed credit for @RajeevK since they only attached a patch that did not apply and did not address feedback. In the future, please read the issue carefully to ensure you are helping make progress on it, so that you can receive issue credit. Thanks!

Thanks everyone for your ongoing work to clean up these coding standards and enable our coding rules for checking; it's really appreciated and will make it so we don't have to worry about these sorts of issues in the future.

I update the issue title and opened #2909163: Fix 'Drupal.Commenting.InlineComment.WrongStyle' coding standard as a follow-up.
For commented code we can add the required space when missing.

I'm not sure what to do with commented code, as there are a lot of them. How to create new issues? Grouped or individual? Both options have downsides :).

Here is the updated patch.

I would lean toward one issue per API for the commented code followups. This adds some overhead for issue creation etc., but then we can use git blame, relate the past issues that commented the code, file followups if needed, and get input from the relevant subsystem maintainer. I think maybe a meta is a good idea. Then we could file issues just for the two we decided to remove here, and let the meta take care of others since this is only tangentially connected to this coding standard.

The updated patch looks like a good scope to me if some others want to review/confirm for RTBC ('cause then I can commit it). Thanks!

Actually, the modifications that started the discussion belong to #2901572: Fix 'Drupal.Commenting.PostStatementComment' coding standard and not to #2909163: Fix 'Drupal.Commenting.InlineComment.WrongStyle' coding standard. I will add comments there on how to approach those issues.

Patch is pretty clear for me and contains only issue sniff related changes.
Agree that we should fix other issues related to inline comment issues in proper sniff treads
Let's be sure that we create all following issues.
There is the list related follow-up issues:

• #2572659: Fix 'Drupal.Commenting.InlineComment.SpacingAfter' coding standard
• #2721309: Fix Drupal.Commenting.FunctionComment.ParamCommentFullStop
• #2901572: Fix 'Drupal.Commenting.PostStatementComment' coding standard

+1 to RTBC

Alright, thanks, this is the patch I'd expect.

• Reviewed with a word diff and confirmed that there are no non-whitespace changes other than a couple places where we removed a tripled slash.
• Changes in the patch all look correct.
• There aren't any comments that would need to be rewrapped because adding the single space put them over 80 chars. (One is already over 80 chars as well as the comments at the ends of lines, but those are out of scope here.
• Ran ../vendor/bin/phpcs -p on 8.5.x with the patch applied and confirmed there were no failures.
• Reintroduced a failure and confirmed it would fail.
• Ran it again on the 8.4.x backport in case the branches were different.

Committed to 8.5.x and cherry-picked to 8.4.x as a coding standards cleanup during RC.

Posted #2909362: [meta] Commented-out code in Drupal to address commented-out code. We can probably redirect some of the other issues' followups there as well.

Thanks everyone! See you in the other issues. It will be helpful if you can mention in the summaries of those issues who from this issue should also receive credit (if they participated in the discussion for how to fix that part). Thanks!

Oh darnit. This one also has diff component files changes in it. Let's please remove those hunks before committing. Thanks!

I will handle this shortly.

@xjm: you reverted the revert on 8.5.x, I guess you meant to revert from both branches, no?

This would be the patch if the commit was reverted, not reverted twice. So, I'm not setting the NR status yet.

#fail. Thanks @mfernea. That'll teach me to use relative refs rather than the actual commit hash. :P

#73 is kind of an interdiff. Let's get the rest of the patch, without that bit. :) Thanks!

Or, we could keep going for more reverts of reverts of reverts... ;) but probably best not.

Here is the updated patch.

Ok
No more changes for Diff component in patch.

git diff --color-words
show phpcs.xml.dist and few removed /

So move issue to RTBC

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
@@ -115,7 +115,7 @@ public function testEntityTypeUpdateWithoutData() {
       ],
     ];
-    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected); //, 'EntityDefinitionUpdateManager reports the expected change summary.');
+    $this->assertEqual($this->entityDefinitionUpdateManager->getChangeSummary(), $expected); // , 'EntityDefinitionUpdateManager reports the expected change summary.');
 

We should remove the inline comment here - it's just commenting out the assertion message. So either kill the message altogether or uncomment it.

We agreed with @xjm to just fix the cs issues here and open child issues of #2909362: [meta] Commented-out code in Drupal for these lines. So for the above mentioned line we have #2911166: Undo accidental commenting of message in EntityDefinitionUpdateTest. Isn't it ok?

Yep, back to RTBC. :) Sorry for the confusion!

Fair enough.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Hi guys I have additional changes but for 8.5.x, please verify it.

This issue is fixed.
@Malevi4: changes in your patch relate to the sniff fixed in #2719649: Fix 'Drupal.Commenting.InlineComment.SpacingBefore' coding standard.

Thanks, I will move it to #2719649: Fix 'Drupal.Commenting.InlineComment.SpacingBefore' coding standard.