Add Squiz.Commenting.DocCommentAlignment sniff

[ghost]

I saw this issue in a file that wasn't picked up by any of the current sniffs:

  /**
 * Implements BackdropCacheInterface::deleteMultiple().
 */
  public function deleteMultiple(array $cids) {

This will be flagged if we add the 'Squiz.Commenting.DocCommentAlignment' sniff.

[ghost]

Hmm, but then that sniff flags the following code:

   * @param string $key
   *   A string that maps to a key within the configuration data.
   *   For instance in the following configuration array:
   *   @code
   *   array(
   *     'foo' => array(
   *       'bar' => 'baz',
   *     ),
   *   );
   *   @endcode
   *   A key of 'foo.bar' would return the string 'baz'. However, a key of 'foo'
   *   would return array('bar' => 'baz').
   *   If no key is specified, then the entire data array is returned.

It says the @code tags should be out-dented:

   * @param string $key
   *   A string that maps to a key within the configuration data.
   *   For instance in the following configuration array:
   * @code
   *   array(
   *     'foo' => array(
   *       'bar' => 'baz',
   *     ),
   *   );
   * @endcode

So might need to make our own sniff for this...

[ghost]

Ah ha! Worked out how to exclude that one sniff message that was causing issues:

<rule ref="Squiz.Commenting.DocCommentAlignment">
  <!-- Doesn't allow nested tags. -->
  <exclude name="Squiz.Commenting.DocCommentAlignment.SpaceAfterStar"/>
</rule>

I also re-arranged the ruleset.xml slightly so that all Squiz sniffs are listed together, rather than having regular ones up top and then overridden ones below (makes it easier to find what you're looking for if they're all together).

[indigoxela]

I'll try to test ASAP. Yes, some of the Squiz rules overlap with the one(s) forked from Drupal, so extra care has to be taken. At least, we don't want to produce false positives. :wink:

[indigoxela]

@BWPanda are you sure this behaves as intended? If I'm doing it wrong like:

 /**
 *
 */
function foobar(array $arr) {
  return TRUE;
}

I get:

FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 8 | ERROR | [x] Expected 2 space(s) before asterisk; 1 found
 9 | ERROR | [x] Expected 2 space(s) before asterisk; 1 found
----------------------------------------------------------------------

That seems a bit misleading, as the problem is in line 7 (indented one space, but shouldn't be).

[ghost]

This addresses issues where the docblock's opening tag is aligned properly but the rest of the docblock isn't.

For issues like your example where the opening tag of a docblock is misaligned to the function, we'll need to find a sniff for that too I guess.

But that could be a separate issue and doesn't mean this PR doesn't work.

[indigoxela]

we'll need to find a sniff for that too I guess. ... But that could be a separate issue

I partly agree, we should figure out how to address this docblock misalignment. But ... as your PR only partly addresses it, this leads to a WTF with misleading messages. Maybe we can fix that in a way, that doesn't introduce confusion?

Here's the source code of that sniff: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Squiz/Sniffs/Commenting/DocCommentAlignmentSniff.php

Maybe we find something in Drupal-land: https://github.com/pfrenssen/coder

[ghost]

@indigoxela Thanks for the tip. I found that running Drupal's sniffs on a test file flagged the error we're having. The related sniff was one of their custom ones (that they've forked from squizlabs). So I copied it to our sniffs and it seems to work (PR updated):

<?php
/**
 * @file
 * A test file for PHPCS.
 */

 /**
  * This function returns 'blah'.
  */
function tester() {
  return 'blah';
}

/**
* This function returns 'yada'.
*/
function tester2() {
  return 'yada';
}

Returns:

----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  7 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found
    |       |     1
    |       |     (Backdrop.WhiteSpace.ScopeIndent.IncorrectExact)
 15 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
----------------------------------------------------------------------

It doesn't flag the other lines in the same docblock, unless you fix just the first one then run it again. But I think that's fine (most people would notice the issue and just fix the entire docblock).

[indigoxela]

At first I was exited, but then... this flags more than just comments.

An example:

function foo() {
  db_update('i18n_string')
  ->fields($update + array(
    'type' => $source->type,
    'objectid' => $source->objectid,
    'property' => $source->property,
    'objectindex' => $source->objectindex,
  ))
  ->condition('lid', $source->lid)
  ->execute();
}

Do we have definitions for that in Backdrop coding standards? It might be that it actually makes sense that this is an error, but ...

Re the misleading nagging with misaligned function comments: if I get it right, we do not need Squiz.Commenting.DocCommentAlignment.SpaceAfterStar anymore? Or do we?

[ghost]

@indigoxela So I was a bit confused by all this, and decided to try building a standard from scratch to see what sniffs we needed and which we didn't. Here's what I've spent some time over the last few weeks doing:

• Setup a new, blank standard that includes all Generic and Squiz sniffs
• Run that over Backdrop core
• For each issue flagged that was a legitimate issue with our code, I noted that sniff/message as a good one
• For each issue flagged that isn't a problem with our code, I excluded that sniff/message (i.e. it's a bad one)

This eventually helped me to come up with a list of good and bad sniffs.

Of the ~119 sniff messages in the Generic standard, I excluded ~55 bad ones. Of the ~323 sniff messages in the Squiz standard (that weren't already included in the Generic standard), I excluded ~92 bad ones.

So as you can see, it seems to be better to include the full Generic and Squiz standards, then exclude specific sniffs/messages that don't match our coding standards (rather than just including specific sniffs as we do now).

Would you like my new Standard as a PR here, or in a new issue? I think it addresses the issues here we've been discussing here:

<?php
/**
 * @file
 * A test file for PHPCS.
 */

 /**
  * This function returns 'blah'.
  */
function tester() {
  return 'blah';
}

/**
* This function returns 'yada'.
*/
function tester2() {
  return 'yada';
}

function foo() {
  db_update('i18n_string')
    ->fields($update + array(
      'type' => $source->type,
      'objectid' => $source->objectid,
      'property' => $source->property,
      'objectindex' => $source->objectindex,
    ))
    ->condition('lid', $source->lid)
    ->execute();
}

FILE: /app/test/test.php
-----------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 4 LINES
-----------------------------------------------------------------------------------------------
  9 | ERROR | [ ] Missing @return tag in function comment (Squiz.Commenting.FunctionComment.MissingReturn)
 15 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [x] Expected 1 space(s) before asterisk; 0 found
    |       |     (Squiz.Commenting.DocCommentAlignment.SpaceBeforeStar)
 16 | ERROR | [ ] Missing @return tag in function comment (Squiz.Commenting.FunctionComment.MissingReturn)
 21 | ERROR | [x] Missing function doc comment (Backdrop.Commenting.FunctionComment.Missing)
 21 | ERROR | [ ] Missing doc comment for function foo() (Squiz.Commenting.FunctionComment.Missing)
-----------------------------------------------------------------------------------------------

[indigoxela]

Wow, thank you so much, to put all that effort here. :pray:

If you prefer to attach your PR here, it might make sense to adapt the issue description. Or start a new one - whatever seems better.

[ghost]

I decided a new issue might be better, since this is a fairly significant change. We can always close this issue later if it's no longer necessary if/when the other PR is merged.