Faulty core fixes: ScopeIndentSniff doesn't align a multiline comment correctly

[pento]

Given the following example:

<?php

function a() {
/*
 * A comment.
 */
}

The comment will be indented, but the 2nd and 3rd line are missing the space to align their * correctly.

[pento]

There's a similar issue with docblocks, given this example:

<?php

function a() {
/**
 * A docblock
 */
}

The /** line of the docblock is indented, but subsequent lines are not.

[jrfnl]

I'd need to check, but I seem to remember that is covered by one of the comment related sniffs in the Docs standard.

In case that would be a stand-alone sniff, should it be moved to the Core standard ?

[pento]

Yah, I think it would need to be moved - the current Core behaviour is incorrect. The other option would be to include selected rules from Docs in the phpcs.xml.dist file, but that seems like it'll cause problems for anyone who wants to use Core rules for their own project, but doesn't copy the core phpcs.xml.dist.

[jrfnl]

@pento I've been looking into this. There is no isolated enough upstream sniff which can be used for the multi-line comments, however, for the docblock comments - as long as these are file/function/class/interfaces/property docblocks (i.e. not hook documentation docblocks) - the Squiz.Commenting.DocCommentAlignment sniff could be added to the core phpcs.xml.dist file to fix this.

Want to have a try with that ?

I'll have a think about writing a WPCS native sniff for multi-line comment alignment. My current line of thinking is: don't touch the first line (leave that to the ScopeIndent sniff to sort out) and align the subsequent lines based on the alignment of the comment opener. Independently of the comment style (/* */ vs /** */ - not //).

How does that sound ?

[pento]

Unfortunately, Squiz.Commenting.DocCommentAlignment isn't a huge help - there are a few places in tests it fixes, but all of the problems in src are either hook docblocks, or multiline comments.

I like the sound of the WPCS sniff behaviour.

[jrfnl]

@pento Ok, thanks for the response. Leave it with me.

[jrfnl]

@pento @DrewAPicture I have a question regarding multi-line non-docblock comments:
The only example in the docs handbook has every line prefixed with a star. Is this a hard rule ? Soft rule ? Just an example, but not a rule ?
I.e. should stars at the start of each line of a multi-line non-docblock comment be enforced or not ?

Ref: https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#5-2-multi-line-comments

[DrewAPicture]

I would consider it a hard rule. I actually can't think of any examples in core that omit the leading asterisks in terms of PHP code unless it's illustrating example code outside a docblock (can't say that I've seen any though).

[JDGrimes]

I think that the leading asterisks make things more readable, FWIW.

[jrfnl]

@DrewAPicture @JDGrimes Thanks for your responses. I'm in favour of it being a hard-rule as well - even when it's for commented out code, which is where I most often see non-star /* */ comments -. Will implement like that. We can always fine-tune once the sniffs are ready & merged.

[jrfnl]

@DrewAPicture Just wondering - should the handbook be adjusted to make it explicit that multi-line non-docblock comments should always have stars at the start of each line ?

[jrfnl]

@pento FYI: part 1 (docblocks) is finished. Now working on part 2 (multi-line non-docblock comments).
I expect I'll pull this tomorrow.

As we're working on the 0.15.0 release which will be focused solely on reorganizing WPCS (see #1157), the sniff will not be merged until 0.15.0 is out the door, but the PR-branch can be used to create a new patch for WP Core.
Is that workable for you ?

[pento]

Yup, that works for me!

[jrfnl]

@DrewAPicture Another question - regarding docblocks this time -:
Should the below format be allowed (tolerated) ?

/** @var Database $mockedDatabase */
public $mockedDatabase;

Or should it be fixed to:

/**
 * @var Database $mockedDatabase
 */
public $mockedDatabase;

[Edit] Just thinking - even if not allowed in Core, I imagine some people in userland may prefer this format, so maybe we should by default auto-fix it, but allow that check to be bypassed with a $allow_short = true property ?

[GaryJones]

I find that:

/**
 * @var Database $mockedDatabase
 */

is not currently sufficient for CS, and nor is:

/**
 * @var Database $mockedDatabase Description.
 */

It needs to be:

/**
 * Description.
 *
 * @var Database $mockedDatabase
 */

Not sure if that's intentional or not.

(And it may just be some checks that PhpStorm is doing, outside of WPCS.)

What the single-line formatting allows, is type hinting for the purposes of the IDE, when the variable appears in a method.

[jrfnl]

@GaryJones In the Docs ruleset are other sniffs which would complain about there not being a short description for the property.

I do not think this is something which can be auto-fixed. Moving the description up or adding an additional short description should be done by a dev.

For now, I'm working on sniffs which will focus solely on the formatting of a docblock/ multi-line comment, not on the content.
Replacing/Improving of the doc content related sniffs is a future project.

FYI - the formatting rules I'm currently implementing (based on the handbook and an inventerisation of this which I did nearly a year ago and which @DrewAPicture verified at the time):

Docblocks:

• A docblock comment should not be empty.
• A docblock should have one blank line above it and no blank line below it.
• The docblock opener and closer should each be on their own line. (can be turned off using the allow_short property, but only if it's all on one line)
• Each line in the docblock should start with a star.
• Stars should be aligned with the first star in the docblock opener.
• There should be a minimum of one space after each star (unless it is an empty comment line).

Multi-line comments /* */:

• • A multi-line comment should not be empty.
• • The comment opener and closer should each be on their own line.
• • Each line in the comment should start with a star.
• • Stars should be aligned with the first star in the comment opener.
• • There should be a minimum of one space after each star (unless it is an empty comment line).

[pento]

A couple of notes:

A docblock should have one blank line above it and no blank line below it.

When the docblock is in a location that requires no blank lines (for example, inside a function call), this causes clashes - DocblockFormatSniff adds a new line, FunctionCallSignatureSniff takes it away.

The docblock opener and closer should each be on their own line. (can be turned off using the allow_short property, but only if it's all on one line)

Short docblocks are allowed for duplicate hook dockblocks.

[jrfnl]

When the docblock is in a location that requires no blank lines (for example, inside a function call), this causes clashes - DocblockFormatSniff adds a new line, FunctionCallSignatureSniff takes it away.

I'll look into this.

Short docblocks are allowed for duplicate hook dockblocks.

I've changed the default value for allow_short to true. That should sort this out. I'll need to adjust the unit tests to match.

[jrfnl]

FYI: The WIP branch which currently only contains the DocblockFormat sniff has just been successfully used for the Core patch.

The BlockComment sniff is a little more complicated as we don't have separate tokens to work with and I'm stretched for time.

I can pull the two sniffs separately, but these should be milestoned for 0.16.0 anyway while we're working out what will go into 0.15.0.