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

As agreed between the mentors at Drupalcon, according to issues to avoid for novices, I am untagging this issue as "Beginner". This issue contains changes across a very wide range of files and might create too many other patches to need to be rerolled at this particular time. This patch has an automated way to be rerolled later so better to implement it after Drupalcon.

Updated title and summary. Working on a patch.

Automated patch generated by phpcbf. It's basically un-reviewable....

I suggest we split this up into a bunch of patches: core/lib, core/includes, core/modules, and core/tests.

Should we make other issues or knock them down one at a time here?

I spent a few minutes reviewing the mega patch, and didn't get very far, so +1 for smaller issues I think.

I found a lot in a short bit:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -618,8 +618,7 @@ function hook_install_tasks_alter(&$tasks, $install_state) {
   +  // function hook_update_N() {.
   

   This doesn't look quite right :)

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -334,7 +334,7 @@ public function buildForm($form_id, FormStateInterface &$form_state) {
   +    // .
   
   @@ -513,7 +513,7 @@ public function retrieveForm($form_id, FormStateInterface &$form_state) {
   +    // .
   

   Or these.

3. +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -184,11 +184,11 @@ protected function doGenerate(array $variables, array $defaults, array $tokens,
   +    // [ [ 0 => 'text', 1 => '/admin/config' ] ].
   

   Many of these would probably benefit from being wrapped in @code blocks, but that seems out of scope here.

4. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -242,7 +242,7 @@ public function stream_eof() {
   +    // Fseek returns 0 on success and -1 on a failure.
   

   False positive. Should probably be "`fseek`"?

5. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
   @@ -49,7 +49,7 @@ public static function cases(ContainerInterface $container = NULL, AccountInterf
   +      [], // ['#type' => 'status_messages'],.
   

   More false positives.

Yeah we definitely need to chunk this, it's not realistic to sign off on a humongous patch like this. Splitting by path will probably still be too large.

Shall we do it in chunks of 20kb max? That would be easy to review + commit. We can iterate here, and once we get to the final chunk we update the ruleset.

Concentrating on core/lib/Drupal/Component.

phpcbf does pretty well as a starting point, but does require review.

Code that's obviously been commented out gets deleted. :-)

The sniffer misses the finer points of grammar (adding a period in the wrong place).

phpcs also gives some false positives on inline /** @var */ comments. See: #2305593: [policy] Set a standard for @var inline variable type declarations

We see a forbidden use of var in DiffFormatter: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Component/Diff/Di...

...which triggers of a bunch of other issues including seeing that property's docblock as an inline comment.

So either I change it here so it's not a var, or we put this off until properties are properly declared. Ahh, scope, you cruel mistress.

Added #2706753: Fix 'PSR2.Classes.PropertyDeclaration.ScopeMissing/VarUsed' coding standard which covers the var problem.

Patch from #13 looks good.

+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -607,7 +607,7 @@ public static function strcasecmp($str1 , $str2) {
-      $chunk_size = 47; // floor((75 - strlen("=?UTF-8?B??=")) * 0.75);

We should not drop this comment, this has helpful information in there.

Thanks.

Added that comment back, as a proper inline comment, which is a bit ugly. Please suggest a better replacement.

It's not the most beautiful line of documentation encountered by mankind, but it is very clear and legible :) Back to RTBC, thanks!

1. +++ b/core/lib/Drupal/Component/Utility/Tags.php
   @@ -20,7 +20,9 @@ class Tags {
        // This regexp allows the following types of user input:
   +    // @code
        // this, "somecompany, llc", "and ""this"" w,o.rks", foo bar
   +    // @endcode
   

   Is user input code? Tricky.

2. +++ b/core/lib/Drupal/Component/Utility/Unicode.php
   @@ -607,7 +607,8 @@ public static function strcasecmp($str1 , $str2) {
   +      // Chunk size: floor((75 - strlen("=?UTF-8?B??=")) * 0.75); .
   

   Looks odd

3. +++ b/core/lib/Drupal/Component/Utility/UserAgent.php
   @@ -40,10 +40,15 @@ public static function getBestMatchingLangcode($http_accept_language, $langcodes
   +    // @code
        //   Accept-Language = "Accept-Language" ":"
        //                  1#( language-range [ ";" "q" "=" qvalue ] )
        //   language-range  = ( ( 1*8ALPHA *( "-" 1*8ALPHA ) ) | "*" )
   -    // Samples: "hu, en-us;q=0.66, en;q=0.33", "hu,en-us;q=0.5"
   +    // @endcode
   ...
   +    // @code
   +    //   "hu, en-us;q=0.66, en;q=0.33", "hu,en-us;q=0.5"
   +    // @endcode
   

   I don't think code should be indented and is this code?

Also how many fails are there to fix if we apply this rule to all of core... and maybe it'd be better to fix by sub -sniff in core entirely?

Auto fix for all core phpcbf --sniffs=Drupal.Commenting.InlineComment core

drop is moving

We really should limit scope here because, for instance, we have to review the patch in #24 and find things like this:

+++ b/core/includes/batch.inc
@@ -118,7 +118,6 @@ function _batch_progress_page() {
   else {
     // This is one of the later requests; do some processing first.
-
     // Error handling: if PHP dies due to a fatal error (e.g. a nonexistent
     // function), it will output whatever is in the output buffer, followed by

That's a fix that introduces another error: Word wrap at 80.

But as for #21, I really wish we had some better direction on what change is needed for:

+      // Chunk size: floor((75 - strlen("=?UTF-8?B??=")) * 0.75); .

suppose we need to split the issue into parts (tests and code)
it really pita to review such size of patches

So what we can do is not enable the entire rule. See #2707641-7: Ensure core compliance to Drupal.Commenting.FunctionComment.ParamCommentIndentation (part 2) For an example of how we can break this rule up into small amounts of work by excluding specific errors from the rule.

So let's do Drupal.Commenting.InlineComment.SpacingAfter

1. +++ b/core/includes/batch.inc
   @@ -118,7 +118,6 @@ function _batch_progress_page() {
        // This is one of the later requests; do some processing first.
   -
        // Error handling: if PHP dies due to a fatal error (e.g. a nonexistent
   
   +++ b/core/includes/pager.inc
   @@ -199,7 +199,6 @@ function template_preprocess_pager(&$variables) {
      // End of marker calculations.
   -
      // Prepare for generation loop.
   

   There are plenty of examples like this where we break wrapping... hmmm tricky.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
   @@ -66,7 +66,6 @@ public function load($file)
            // Not supported.
            //$this->container->addResource(new FileResource($path));
   -
            // empty file
            if (null === $content) {
                return;
   @@ -75,7 +74,6 @@ public function load($file)
   
   @@ -75,7 +74,6 @@ public function load($file)
            // imports
            // Not supported.
            //$this->parseImports($content, $file);
   -
            // parameters
            if (isset($content['parameters'])) {
                if (!is_array($content['parameters'])) {
   @@ -90,7 +88,6 @@ public function load($file)
   
   @@ -90,7 +88,6 @@ public function load($file)
            // extensions
            // Not supported.
            //$this->loadFromExtensions($content);
   -
            // services
            $this->parseDefinitions($content, $file);
   

   Not our standards so perhaps we should mark this file to be ignored?

Let's do this against 8.2.x first.

Just a heads-up that I made a little script to help figure out how to work on some of these things: #2571965-63: [meta] Fix PHP coding standards in core, stage 1

Filed related patch to #2168241: Type hints for optional methods in StatementInterface (D8) / DatabaseStatementInterface (D7)

+++ b/core/phpcs.xml.dist
@@ -18,6 +18,14 @@
+  <rule ref="Drupal.Commenting.InlineComment">
+    <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.SpacingBefore"/>
+    <exclude name="Drupal.Commenting.InlineComment.WrongStyle"/>
+  </rule>

This fixes only SpacingAfter

      1 Drupal.Commenting.InlineComment.WrongStyle
     53 Drupal.Commenting.InlineComment.NoSpaceBefore
     99 Drupal.Commenting.InlineComment.SpacingBefore
    114 Drupal.Commenting.InlineComment.DocBlock
    151 Drupal.Commenting.InlineComment.NotCapital
    269 Drupal.Commenting.InlineComment.SpacingAfter
    703 Drupal.Commenting.InlineComment.InvalidEndChar

Added issues for the rest

The latest patch no longer contains a phpcs.xml.dist change. The issue summary describes how to fix CS issues.

Added phpcs.xml file

@rasikap We shouldn't add a new phpcs.xml file but instead amend the existing phpcs.xml.dist.

@tstoeckler, added the ruleset in phpcs.xml.dist itself.

The change in phpcs.xml.dist is on the wrong line, please insert it alphabetically with the other Durpal standard sniffs.

Then the fixes for the newly added sniff are missing? The patch should include those. While running this sniff I found a false positive, opened a Coder issue: #2851518: False positives in Drupal.Commenting.InlineComment.SpacingBefore for multi line comments.

Drupal 8.4.x currently does not even pass its own configured rules so I also filed #2851510: Fix phpcs regressions by running phpcbf.

This was solved in #2716685: Part 2: Fix several errors in the 'Drupal.Commenting.DocComment' coding standard.

I was wrong. #2716685: Part 2: Fix several errors in the 'Drupal.Commenting.DocComment' coding standard fixed Drupal.Commenting.DocComment.SpacingAfter and this one targets Drupal.Commenting.InlineComment.SpacingAfter.

Started again from scratch.

If I look at the test results there is still one issue to be solved.

I don't know how that one slipped through. Fixed.

While the patch is correct from the technical point of view, I have some mixed feelings about some type of changes like:

1. +++ b/core/includes/batch.inc
   @@ -331,7 +330,6 @@ function _batch_process() {
        // Gather progress information.
   -
        // Reporting 100% progress will cause the whole batch to be considered
   
   +++ b/core/includes/errors.inc
   @@ -230,7 +230,6 @@ function _drupal_log_error($error, $fatal = FALSE) {
            // Without verbose logging, use a simple message.
   -
            // We call SafeMarkup::format() directly here, rather than use t() since
   

   Title like comments.

2. +++ b/core/includes/pager.inc
   @@ -199,7 +199,6 @@ function template_preprocess_pager(&$variables) {
      // End of marker calculations.
   -
      // Prepare for generation loop.
   
   @@ -213,7 +212,6 @@ function template_preprocess_pager(&$variables) {
      // End of generation loop preparation.
   -
      // Create the "first" and "previous" links if we are not on the first page.
   

   Comment for end of code section.

Maybe it's best to remove the comments for the end of code sections. I'm not sure.

#60 While I agree with your line of thought, for these type of issues it is typically best to leave the code as is and make the smallest change possible. Once you start refactoring, the issue will quickly escalate and become hard to actually get committed :)

Patch no longer applies to 8.6.x, so it will need to be rerolled.

Re-rolled

Tested the #64, it applies cleanly and I think the comments look good now.

+++ b/core/phpcs.xml.dist
@@ -82,6 +82,7 @@
+    <exclude name="Drupal.Commenting.InlineComment.WrongStyle"/>

The change in phpcs.xml.dist should make it stricter. This change removes a sniff instead.

The correct change to phpcs.xml.dist was in #59:

<rule ref="../vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Commenting/InlineCommentSniff.php">
  <!-- Sniff for: SpacingAfter -->
  <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.SpacingBefore"/>
  <exclude name="Drupal.Commenting.InlineComment.WrongStyle"/>
</rule>

Review the patch.

+++ b/lib/Drupal/Component/DependencyInjection/Container.php
@@ -416,7 +416,7 @@ class Container implements ContainerInterface {
-   * @param array|\stdClass $arguments
+   * @param array|object $arguments

We should only fix Drupal.Commenting.InlineComment.SpacingAfter, not other things.

Please provide patches that only fix this rule and nothing else here.

Tagging for Drupal Global Contribution Weekend

No interdiff as #69 did unexpected changes. it‘s meaningless to provide interdiff

BTW, all fixed by phpcbf, no manual changes.

I looked through the diff and it seems to remove all empty lines after inline comments, which is what was required.

1. +++ b/core/modules/ban/tests/src/Functional/IpAddressBlockingTest.php
   @@ -81,7 +81,6 @@ public function testIPAddressValidation() {
        // $edit['ip'] = \Drupal::request()->getClientIP();
        // $this->drupalPostForm('admin/config/people/ban', $edit, t('Save'));
        // $this->assertText(t('You may not ban your own IP address.'));
   -
        // Test duplicate ip address are not present in the 'blocked_ips' table.
   

   This removal is odd because the commented out code is wrong - we need to find out why the code is commented out and resolve that.

2. +++ b/core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
   @@ -160,7 +160,6 @@ public function testBigPipe() {
        // @see performMetaRefresh()
   -
        $this->drupalGet(Url::fromRoute('big_pipe_test'));
   

   Removals like this are good.

3. +++ b/core/modules/editor/editor.module
   @@ -512,7 +512,6 @@ function editor_file_download($uri) {
      // about them.
      // @see file_file_download()
   -
      // Find out if any editor-backed field contains the file.
   

   Changes like this look wrong to me. The spacing here has meaning. The comments are not connected. I think that the rule needs fixing to allow a space between two inline comments.

4. +++ b/core/modules/editor/tests/src/Kernel/EditorManagerTest.php
   @@ -31,7 +31,6 @@ protected function setUp() {
        // Install the Filter module.
   

   The filter module is already installed. This comment should be removed.

5. +++ b/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php
   @@ -56,7 +56,6 @@ public function providerTestFilterXss() {
        // All cases listed on https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet
   -
        // No Filter Evasion.
        // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_Filter_Evasion
        $data[] = ['<SCRIPT SRC=http://ha.ckers.org/xss.js></SCRIPT>', ''];
   

   This is a common pattern where the space has meaning. The first comment is about the entire coming section. and the next comment is specific to the following line of code. The new line has meaning and should not be removed.

tldr; personally I think we need to check whether this coding standard is actually a coding standard. As far as I can see it is not - https://www.drupal.org/node/1354

Thanks @alexpott for reviewing.

+++ b/core/modules/editor/editor.module
@@ -512,7 +512,6 @@ function editor_file_download($uri) {
   // about them.
   // @see file_file_download()
-
   // Find out if any editor-backed field contains the file.

Changes like this look wrong to me. The spacing here has meaning. The comments are not connected. I think that the rule needs fixing to allow a space between two inline comments.

I think the first comment could be moved out as a part of the comment of the function/method.

+++ b/core/modules/editor/tests/src/Unit/EditorXssFilter/StandardTest.php
@@ -56,7 +56,6 @@ public function providerTestFilterXss() {
     // All cases listed on https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet
-
     // No Filter Evasion.
     // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_Filter_Evasion
     $data[] = ['<SCRIPT SRC=http://ha.ckers.org/xss.js></SCRIPT>', ''];

This is a common pattern where the space has meaning. The first comment is about the entire coming section. and the next comment is specific to the following line of code. The new line has meaning and should not be removed.

The same, the first could be moved out as a part of the function/method comment. if one function/method has multiple sections, maybe it's a code smell. One way is to split it into small functions/methods. It's better that one function does one thing. Maybe, to move the inline comments of all sections out to the description of the function/method is reasonable. Code should be self-explained. It's untrue that the more comments, the better.

So about #77.3 and 5, my suggestion is to move out the first comment to the description of the function/method or to refactor the code itself.

@jungle the more I think about it the more I think this coding standard is incorrect - at least in implementation. Sure the following case could be flagged:

     // @see performMetaRefresh()
-
     $this->drupalGet(Url::fromRoute('big_pipe_test'));

But doing something like

     // All cases listed on https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet
-
     // No Filter Evasion.
     // @see https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#No_Filter_Evasion
     $data[] = ['<SCRIPT SRC=http://ha.ckers.org/xss.js></SCRIPT>', ''];

Is wrong. Space has meaning and that's a good thing. I think we should file an issue issue to either fix the coder coder so that we only flag this is the after the blank line there's code - or we file an issue to remove the rule from coder.

+++ b/core/modules/editor/tests/src/Kernel/EditorManagerTest.php
@@ -31,7 +31,6 @@ protected function setUp() {
// Install the Filter module.

I removed this comment. Plz review patch

+1 For the comment #79 from @alexpott. A lot of changes from the patch look wrong.

Reroll the patch and worked on #79

Needs reroll after commited #2623718: Fix 'Drupal.Commenting.HookComment' coding standard

Working on this.

Rerolled the patch for 9.2.x.

Latest patches missing Step 3 from issue summary - update default sniffers in core/phpcs.xml.dist with <rule ref="Drupal.Commenting.InlineComment.SpacingAfter"/>

See https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/phpcs.xml.di...

PS: updated IS with valid sniff name

I am in agreement with #60/#77.3/#77.5/#79 in that sometimes these blank lines are useful for readability and to split up groups of comments, so just removing them all is not the right answer and perhaps this isn't and shouldn't be a standard for us to follow, at least not with the sniff in it's current form.

I came here to review. It is not helpful that these coding standard issue do not have a Remaining Task section in the IS. It just makes it harder for a reviewer.

I thought I would apply the latest patch and have a look. It does not have changes to phpcs.xml. I then searched backwards and found that it it does not appear to be in patch form #85 onwards.

Now, reading the comments from the top everything look like typical work on an issue. However, in #77-#79 there are serious questions raised about using this sniff. Despite those concerns prabha1997, nikitagupta, ayushmishra206 made patches which are not helping to advance the issue (and do not have a phpcs.xml file as mentioned).

Going back to the serious questions. In #79 alexpott states

tldr; personally I think we need to check whether this coding standard is actually a coding standard. As far as I can see it is not - https://www.drupal.org/node/1354

I agree, Drupal standards for in-line code comments states "Comments should be on a separate line immediately before the code line or block they reference. For example:" So, there isn't a strict requirement.

My two cents, when writing some tests I recall spending extra time to make in line comments that would pass this Drupal Practice standard. It was frustrating at the time but in the end I think the result was better readability. So, even though this sniff isn't a strict requirement I would be OK with it. Now, having that said that I will go with the consensus.

And the consensus seems to be to modify the sniff. Where does that work happen?

Setting to NR because the patch does not need work.

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

The comment #92, #93 and #94 still need to be addressed.

10.1.x Reroll for #97

#102 appears to be missing a number of changes from #97 can it be documented why?

Also the interdiff is the same size of the patch?

Thanks for the interest in this work. However, as pointed out in several comments the work here is to address comments #92, #93, #94. And #93 suggests that we don't implement this sniff at all!

I am changing the status to active so emphasize that the work here is to resolve #92, #93, #94. Also, updating credit per How is credit granted for Drupal core issues.

I checked #94 and I totally agree that we need to update code sniff rule Drupal.Commenting.InlineComment.SpacingAfter to work with examples provided in #79.
Do we have issue for that?

Do I understand it right, before we have agreement or code sniff rule will be update, no further patches should be attached to this issue?
Do we need to update IS?

@zniki.ru, thanks for the interest in coding standard issues! For some background, there is a Coding Standards project where the community discusses changes to the Drupal Coding standards. Changes agreed to there usually require a change to the Coder project to implement a new sniff or create a new one. Once, that is done, then we have issues it the core queue to implement the change. There is also the #coding_standards channel in Drupal Slack where coding standards are also discussed.

There is an issue in the Coding Standards project discussing changing this standard, #2464123: Remove the requirement that no blank line follow an inline comment. That issue is being worked on so maybe we should postpone this on that issue. I do want to avoid double work. I am adding that as a relating issue and postponing this on that issue.