Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Squiz/ClosingDeclarationComment: fix a non-problematic bug plus some other small improvements #381

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,10 @@ public function process(File $phpcsFile, $stackPtr)

$closingBracket = $tokens[$stackPtr]['scope_closer'];

if ($closingBracket === null) {
// Possible inline structure. Other tests will handle it.
return;
}

$data = [$comment];
if (isset($tokens[($closingBracket + 1)]) === false || $tokens[($closingBracket + 1)]['code'] !== T_COMMENT) {
$next = $phpcsFile->findNext(T_WHITESPACE, ($closingBracket + 1), null, true);
if (rtrim($tokens[$next]['content']) === $comment) {
if ($next !== false && rtrim($tokens[$next]['content']) === $comment) {
// The comment isn't really missing; it is just in the wrong place.
$fix = $phpcsFile->addFixableError('Expected %s directly after closing brace', $closingBracket, 'Misplaced', $data);
if ($fix === true) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,35 @@ enum MissingClosingComment {

enum HasClosingComment {
}//end enum

function misplacedClosingCommentWhitespace() {
} //end misplacedClosingCommentWhitespace()

function misplacedClosingCommentMultipleNewlines() {
}


//end misplacedClosingCommentMultipleNewlines()

function missingClosingComment() {
}

function commentHasMoreIndentationThanFunction() {
}
//end commentHasMoreIndentationThanFunction()

class Foo {
function commentHasLessIndentationThanFunction() {
}
//end commentHasLessIndentationThanFunction()

function misplacedClosingCommentWithIndentation() {
}
//end misplacedClosingCommentWithIndentation()
}//end class

// Anonymous classes don't need end comments.
$anon = new class {};

// Arrow functions don't need end comments.
$arrow = fn($a) => $a;
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,29 @@ enum MissingClosingComment {

enum HasClosingComment {
}//end enum

function misplacedClosingCommentWhitespace() {
}//end misplacedClosingCommentWhitespace()

function misplacedClosingCommentMultipleNewlines() {
}//end misplacedClosingCommentMultipleNewlines()

function missingClosingComment() {
}//end missingClosingComment()

function commentHasMoreIndentationThanFunction() {
}//end commentHasMoreIndentationThanFunction()

class Foo {
function commentHasLessIndentationThanFunction() {
}//end commentHasLessIndentationThanFunction()

function misplacedClosingCommentWithIndentation() {
}//end misplacedClosingCommentWithIndentation()
}//end class

// Anonymous classes don't need end comments.
$anon = new class {};

// Arrow functions don't need end comments.
$arrow = fn($a) => $a;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

// Intentional parse error (missing opening bracket).
// This should be the only test in this file.
// Testing that the sniff is triggered.

class MissingOpeningBracket
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

// Intentional parse error (missing closing bracket).
// This should be the only test in this file.
// Testing that the sniff is triggered.

class MissingClosingBracket {
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is a good test to have anyway, this test doesn't actually highlight the problem with the missing $next !== false condition.

We looked at this during our call and the following additional test should be added to safeguard the fix:

//end closingBraceAtEndOfFileMissingComment()

<?php

function closingBraceAtEndOfFileMissingComment() {
}

This test will show that without that extra condition the wrong error is being thrown (though an error is still being thrown).

Note: as the test framework as-is doesn't safeguard that the correct error code is being thrown for each line, it's not a hard safeguard, but having the test in place will allow us to safeguard this properly if/when the test framework gets updates to check per error code/per line.


// This should be the only test in this file.
// Testing that the sniff is triggered when the closing bracket is
// the last character in the file (no newline after it).

function closingBraceAtEndOfFile() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

// This should be the only test in this file.
// Testing that the sniff is triggered when the closing bracket is
// the last character in the file (no newline after it).

function closingBraceAtEndOfFile() {
}//end closingBraceAtEndOfFile()
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//end closingBraceAtEndOfFileMissingComment()

<?php

// This should be the only test in this file.
// Testing that the sniff is triggered and the fixer works when the closing bracket is
// the last character in the file (no newline after it) and the content of the first token
// is a "//end closingBraceAtEndOfFileMissingComment()" comment.

function closingBraceAtEndOfFileMissingComment() {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//end closingBraceAtEndOfFileMissingComment()

<?php

// This should be the only test in this file.
// Testing that the sniff is triggered and the fixer works when the closing bracket is
// the last character in the file (no newline after it) and the content of the first token
// is a "//end closingBraceAtEndOfFileMissingComment()" comment.

function closingBraceAtEndOfFileMissingComment() {
}//end closingBraceAtEndOfFileMissingComment()
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,41 @@ final class ClosingDeclarationCommentUnitTest extends AbstractSniffUnitTest
* The key of the array should represent the line number and the value
* should represent the number of errors that should occur on that line.
*
* @param string $testFile The name of the test file being tested.
*
* @return array<int, int>
*/
public function getErrorList()
public function getErrorList($testFile='')
{
return [
13 => 1,
17 => 1,
31 => 1,
41 => 1,
59 => 1,
63 => 1,
67 => 1,
79 => 1,
83 => 1,
];
switch ($testFile) {
case 'ClosingDeclarationCommentUnitTest.1.inc':
return [
13 => 1,
17 => 1,
31 => 1,
41 => 1,
59 => 1,
63 => 1,
67 => 1,
79 => 1,
83 => 1,
89 => 1,
92 => 1,
98 => 1,
101 => 1,
106 => 1,
110 => 1,
];

case 'ClosingDeclarationCommentUnitTest.4.inc':
return [8 => 1];

case 'ClosingDeclarationCommentUnitTest.5.inc':
return [11 => 1];

default:
return [];
}//end switch

}//end getErrorList()

Expand All @@ -51,11 +71,23 @@ public function getErrorList()
* The key of the array should represent the line number and the value
* should represent the number of warnings that should occur on that line.
*
* @param string $testFile The name of the test file being tested.
*
* @return array<int, int>
*/
public function getWarningList()
public function getWarningList($testFile='')
{
return [71 => 1];
switch ($testFile) {
case 'ClosingDeclarationCommentUnitTest.1.inc':
return [71 => 1];

case 'ClosingDeclarationCommentUnitTest.2.inc':
case 'ClosingDeclarationCommentUnitTest.3.inc':
return [7 => 1];

default:
return [];
}

}//end getWarningList()

Expand Down