Squiz.Commenting.ClosingDeclarationComment - add missing .fixed file for test-suite, and fix a bug found during

[fredden]

Description

This pull request adds a missing .fixed file for an existing test for Squiz.Commenting.ClosingDeclarationComment.

While I was reviewing the automatically-created ".fixed" file (from phpcbf), I noticed a bug. I'm fixing that bug in this same pull request, but am happy to move that elsewhere if desired.

Suggested changelog entry

• Squiz.Commenting.ClosingDeclarationComment - no longer add extra newline when adding a missing comment

Related issues/external references

Related to #299

Types of changes

• Bug fix (non-breaking change which fixes an issue)

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.

[fredden]

@jrfnl is it too late to include this in the upcoming 3.9.0 release? Ref #341.

[jrfnl]

LGTM! Thanks @fredden

[jrfnl]

@jrfnl is it too late to include this in the upcoming 3.9.0 release? Ref #341.

Nah, I'll include it 😉

While reviewing this change I did notice a number of other things, but those don't need to be addressed in this PR.

• No tests for a Missing function closing comment.
• No tests with a misplaced comment with indentation.
• No tests with a misplaced comment with multiple whitespace tokens between the close brace and the comment. (multiple new lines)
• No tests for close brace at the very end of the file (without a new line) - this would need one or more extra test case files to be tested properly.
  This would also expose a (non-problematic) bug on line 96, the if (rtrim($tokens[$next]['content']) === $comment) condition is buggy as $next can be false and that's not correctly taken into account.
  This doesn't lead to problems as the contents of token 0 will be compared to the expected comment, which can only be the PHP open tag or inline HTML and only if //end ... would be found in inline HTML could this lead to false positives, though that would make for invalid HTML anyway.
• There is some dead code - the if ($closingBracket === null) code block can be removed as the $closingBracket can never be null as the sniff already bows out before setting the variable on a isset($tokens[$stackPtr]['scope_closer']) === false condition.
• I wonder if the sniff should also handle traits ? I.e. require a comment on the trait closer. Would make sense for consistency.
• Might be good to document (via a test) that anonymous classes and arrow functions do not require a close comment (same as closures, which is already documented via a test).

We should either open an issue about these or if you're up for it, open a follow-up PR to address these issues.

[fredden]

Yes, I noticed a lot of untested cases while looking for the bug that I fixed here. I decided not to fix all of issues that I noticed (which you've now documented above - thanks) in this pull request, to keep the scope small. I thought that we can re-assess this sniff in scope of #146 when we get time for that.

[jrfnl]

@fredden I agree, those other things can be addressed in a separate PR(s), but I do think they need to be addressed.

Bullet 1, 2, 3 + the last one are related to #146. The other bullets are functional concerns/changes which are needed though.

I'm going to link my comment in #146 for visibility.

[rodrigoprimo]

Hi, @fredden! I was talking with @jrfnl about things that I could do to help the PHPCS project, and she mentioned that I could work on the items from the list above. But first, she suggested I check with you to see if you are working or planning to work on those items? Please let me know if you prefer to work on those or if you would like help. Thanks.

[fredden]

@rodrigoprimo thanks for checking in. You're welcome to work on those; they're not currently on my list.

[rodrigoprimo]

Thanks, @fredden. I will work on it next week.