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

done

updated with parent patch

rerolled & fixed

$ git apply Drupal.Commenting.InlineComment.SpacingBefore-2719649-10.patch 
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/phpcs.xml.dist:63
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

Added a rerolled patch.

I think this patch also includes spaceAfter fixes too which belong to other issue (https://www.drupal.org/node/2572659).

eg.

FILE: ....2-git/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 6 WARNINGS AFFECTING 6 LINES
----------------------------------------------------------------------
 200 | WARNING | [x] There must be no blank line following an inline
     |         |     comment

The above standard is fixed in above patch which should be part of other meta issue.

@crazyrohila do I need to remove the extra fixes from the above patch?

Rerolled against 8.3.x.

Re-roll.

Modifications to the phpcs.xml.dist are not correct.

fixed the phpcs.xml.dist and re-rolled to 8.5.x

What is the difference between #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard and this?

Edit: Answer:

This is a NoSpaceBefore violation:

function helloWorld() {
  //prints "hello world"
  echo("Hello world");
}

and this is a SpacingBefore violation:

function helloWorld() {
// prints "hello world"
  echo("Hello world");
}

The modifications to phpcs.xml.dist should be:

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

Doing so, we are not overlapping with #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard, although we'll have merge conflicts for phpcs.xml.dist, which is expected.
So, please make sure that the changes in the patch are only fixing SpacingBefore issues and nothing else.

@mfernea

fixed as per your comment.

Modifications to phpcs.xml.dist look good.
But if I look at the patch I see modifications for NoSpaceBefore sniff (eg Diff.php or DiffEngine.php). It should not. The patch should only contain fixes for SpacingBefore sniff.

This might be the right changeset

Edit: wait, no it messed up a bunch of indentation; sorry.

+++ b/core/modules/node/tests/src/Functional/NodeAccessLanguageAwareTest.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessLanguageAwareTest.php
@@ -88,12 +88,12 @@ protected function setUp() {

@@ -88,12 +88,12 @@ protected function setUp() {
 
     // Create six nodes:
     // 1. Four Hungarian nodes with Catalan translations
-    //   - One with neither language marked as private.
+    //    - One with neither language marked as private.
     //   - One with only the Hungarian translation private.
     //   - One with only the Catalan translation private.
     //   - One with both the Hungarian and Catalan translations private.
     // 2. Two nodes with no language specified.
-    //   - One public.
+    //    - One public.
     //   - One private.
     $this->nodes['both_public'] = $node = $this->drupalCreateNode([
       'body' => [[]],

Seems to add the wrong indentation in this comment - or incompletely add the correct one.

The tests results show that there are still some cs issues.

@mfernea

in core/tests/Drupal/Tests/Component/Graph/GraphTest.php

there is graph diagram in the comment section do we need to fix it.

Not really sure what to do with the comment in core/tests/Drupal/Tests/Component/Graph/GraphTest.php which is failing the test. A quickfix would be to add // @code and // @endcode. It looks like the Drupal CS doesn't raise an issue anymore, but @code and @endcode are meant for docblocks and not inline comments. And I think we have to use this for all other inline commented code in this patch.

Suggestions are welcome.

Drupal CS allows @code @endcode in inline comments and does that on purpose, it's not just a coincidence.
So whenever we have to deal with commented code or comments like those in GraphTest.php it's best to use these tags.
Also please pay attention to // @todo vs // @todo: or other variations. Only the first one is correct and allows the next lines to be indented with 2 spaces.

If I understand correctly, in #46 you are suggesting the troublesome comment in GraphTest.php should be surrounded by @code ... @endcode so that is what I have done (and re-rolled because the patch no longer applies cleanly, hence no interdiff).

// @todo vs // @todo: issue is not solved yet.
We should use @code @endcode tags whenever we deal with code like comments (eg ConfigImporter.php).

Replaced all occurrences of // @todo: with // @todo.

If we use // @todo we can have a 2 space indent for the lines below.
We still have to fix the code like comments using @code and @endcode.

Let's postpone this until #2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard gets in.

#2715485: Fix 'Drupal.Commenting.InlineComment.NoSpaceBefore' coding standard is merged. We can start working again on this one.

Small fix

diff --git a/core/modules/simpletest/src/TestDiscovery.php b/core/modules/simpletest/src/TestDiscovery.php
index 9e6c32c327..3b434eb173 100644
--- a/core/modules/simpletest/src/TestDiscovery.php
+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -193,7 +193,7 @@ public function getTestClasses($extension = NULL, array $types = []) {
       // unavailable modules. TestDiscovery should not filter out module
       // requirements for PHPUnit-based test classes.
       // @todo Move this behavior to \Drupal\simpletest\TestBase so tests can be
-      //       marked as skipped, instead.
+      // marked as skipped, instead.
       // @see https://www.drupal.org/node/1273478
       if ($info['type'] == 'Simpletest') {
         if (!empty($info['requires']['module'])) {

https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Prefered style:

// @todo Move this behavior to \Drupal\simpletest\TestBase so tests can be
//   marked as skipped, instead.

Still passes CS, but less nice:

// @todo Move this behavior to \Drupal\simpletest\TestBase so tests can be
// marked as skipped, instead.

Doesn't pass CS:

// @todo Move this behavior to \Drupal\simpletest\TestBase so tests can be
//       marked as skipped, instead.

Added patch

@harsha012 Can you give us more details about your patch please? What changes have you made since the previous patch in #49?

It is also helpful if you can provide an interdiff when you upload a patch, if possible. That makes it far easier to review.

I am surprised to see that your patch is about a third of the size of the previous patch. This might suggest you have missed something.

1. +++ b/core/lib/Drupal/Core/Render/RenderCache.php
   @@ -121,9 +121,9 @@ public function set(array &$elements, array $pre_bubbling_elements) {
   -      //   '#cache_redirect' => TRUE,
   +      // '#cache_redirect' => TRUE,
   

   this break the formatting of a code example. I think we should wrap this with @code and @endcode comments, then it should not complain? Can you test if that works?

2. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -416,7 +416,7 @@ protected function runDbTasks() {
        // @todo: Saving the account before the update is problematic.
   -    //   https://www.drupal.org/node/2560237
   +    // https://www.drupal.org/node/2560237
   

   do we need to fix this? I think it should work under the @todo like it is.

I think a lot of cases here could be improved with @code tags then Coder should not complain.

The change in phpcs.xml.dist is missing?

Reroll the patch.
@klausi the required changes for phpcs.xml.dist is already added in drupal 9.

9.1.x phpcs.xml.dist still excludes the Drupal.Commenting.InlineComment.SpacingBefore rule:

  <rule ref="Drupal.Commenting.InlineComment">
    <!-- 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>

Yep, please remove the exclude in phpcs.xml.dist.

There's a few validations that feel overly strict. For example, in some places "Note:" is used to start a continuation indent, but the sniff does not allow this.

    // Note: this cannot be checked in ::supportsDenormalization() because at
    //       that time we only have the field item class. ::hasSerializeColumn()
    //       must be able to call $field_item->schema(), which requires a field
    //       storage definition. To determine that, the entity type and bundle
    //       must be known, which is contextual information that the Symfony
    //       serializer does not pass to ::supportsDenormalization().
 

In another place, it's @see:

    // Double open angle brackets.
    // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#Double_open_angle_brackets
    // @see http://ha.ckers.org/blog/20060611/hotbot-xss-vulnerability/ to
    //      understand why this is a vulnerability.
    $data[] = ['<iframe src=http://ha.ckers.org/scriptlet.html <', '<iframe src="http://ha.ckers.org/scriptlet.html">'];

Also in a ton of places, a @todo is marked as an invalid continuation indent because it is written as "@todo: ..." rather than "@todo ..." - the colon causes the sniffer to not recognize it. That does seem like something we want to be consistent on, but I'm not sure if removing the colon from all of those is within the scope of this issue.

Did a new pass.

This one seems to satisfy the sniff, and fixes some of the changes from the previous patch (eg indentation stripped instead of adding @code/@endcode). There were also some non-SpacingBefore fixes in the previous patch.

I fixed the "@todo:" issue by removing the colon from all @todo items that got flagged by the sniff, and changed @TODO and @fixme to @todo in a few cases.

But there are lots and lots more of those that didn't get flagged because they're single line. If we want to apply a consistent rule to those, that's definitely a separate issue.

Filed #3150259: Consistent style for "@todo" comments as a separate issue.

Rerolled and fixed two more cases.

Needed a reroll

I've put 2719649-76.patch in to an Merge Request, just for easier commenting on each change.

Feel free to post your answer as a patch and/or in the MR.

Am I right that the unresolved issues are about the link in the todos?
The example in the standard does not include 'see' or '@see' for @todos.

* @todo Remove this in D8. http://www.drupal.org/node/1234567
* @todo Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
*   nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed
*   diam voluptua.

@quiteone

The example in the standard does not include 'see' or '@see' for @todos.

Ah, you're absolutely right. Resolved all my comments that were about those @sees.

Only three unresolved issues left. :)

Rebased MR on 9.3.x.

Hiding old 9.2.x patches

A few minor comments but this is looking good overall.

Thanks @longwave and his eagle eyes.

Thanks, all points addressed, this is ready to go.

Committed/pushed to 9.3.x, thanks! Also committed the same patch, minus the phpcs.dist.xml change to 9.2.x.

This one is borderline for what might need to be scheduled in a beta/rc, but it's a tangible documentation as well as code style improvement.