Skip to content
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
157 changes: 157 additions & 0 deletions coder_sniffer/Drupal/Sniffs/Commenting/TodoCommentSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
<?php
/**
* Parses and verifies comment language.
*
* @category PHP
* @package PHP_CodeSniffer
* @link http://pear.php.net/package/PHP_CodeSniffer
*/

namespace Drupal\Sniffs\Commenting;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;
use PHP_CodeSniffer\Config;

/**
* Parses and verifies that comments use the correct @todo format.
*
* @category PHP
* @package PHP_CodeSniffer
* @link http://pear.php.net/package/PHP_CodeSniffer
*/
class TodoCommentSniff implements Sniff
{

/**
* Show debug output for this sniff.
*
* Use phpcs --runtime-set todo_debug true
*
* @var boolean
*/
private $debug = false;


/**
* Returns an array of tokens this test wants to listen for.
*
* @return array<int|string>
*/
public function register()
{
if (defined('PHP_CODESNIFFER_IN_TESTS') === true) {
$this->debug = false;
}

return [
T_COMMENT,
T_DOC_COMMENT_TAG,
T_DOC_COMMENT_STRING,
];

}//end register()


/**
* Processes this test, when one of its tokens is encountered.
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
* in the stack passed in $tokens.
*
* @return void
*/
public function process(File $phpcsFile, $stackPtr)
{
$debug = Config::getConfigData('todo_debug');
if ($debug !== null) {
$this->debug = (bool) $debug;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not initialize this variable on every single process() call? Can we do that once in the constructor instead?

Ah, I see that this approach is copied from ScopeIndentSniff, so also not ideal there. Anyway, then I think it is fine to keep this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and I also used it in Commenting/DeprecatedSniff Would you like me to start a new issus to change all three of these, when this is committed?

}

$tokens = $phpcsFile->getTokens();
if ($this->debug === true) {
echo "\n------\n\$tokens[$stackPtr] = ".print_r($tokens[$stackPtr], true).PHP_EOL;
echo 'code = '.$tokens[$stackPtr]['code'].', type = '.$tokens[$stackPtr]['type']."\n";
}

// Standard comments and multi-line comments where the "@" is missing so
// it does not register as a T_DOC_COMMENT_TAG.
if ($tokens[$stackPtr]['code'] === T_COMMENT || $tokens[$stackPtr]['code'] === T_DOC_COMMENT_STRING) {
$comment = $tokens[$stackPtr]['content'];
if ($this->debug === true) {
echo "Getting \$comment from \$tokens[$stackPtr]['content']\n";
}

$this->checkTodoFormat($phpcsFile, $stackPtr, $comment);
} else if ($tokens[$stackPtr]['code'] === T_DOC_COMMENT_TAG) {
// Document comment tag (i.e. comments that begin with "@").
// Determine if this is related at all and build the full comment line
// from the various segments that the line is parsed into.
$expression = '/^@to/i';
$comment = $tokens[$stackPtr]['content'];
if ((bool) preg_match($expression, $comment) === true) {
if ($this->debug === true) {
echo "Attempting to build comment\n";
}

$index = ($stackPtr + 1);
while ($tokens[$index]['line'] === $tokens[$stackPtr]['line']) {
$comment .= $tokens[$index]['content'];
$index++;
}

if ($this->debug === true) {
echo "Result comment = $comment\n";
}

$this->checkTodoFormat($phpcsFile, $stackPtr, $comment);
}//end if
}//end if

}//end process()


/**
* Checks a comment string for the correct syntax.
*
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
* @param int $stackPtr The position of the current token
* in the stack passed in $tokens.
* @param string $comment The comment text.
*
* @return void
*/
private function checkTodoFormat(File $phpcsFile, $stackPtr, string $comment)
{
if ($this->debug === true) {
echo "Checking \$comment = '$comment'\n";
}

$expression = '/(?x) # Set free-space mode to allow this commenting
^(\/|\s)* # At the start optionally match any forward slashes and spaces
(?i) # set case-insensitive mode
(?=( # start a positive non-consuming look-ahead to find all possible todos
@+to(-|\s|)+do # if one or more @ allow spaces and - between the to and do
| # or
to(-)*do # if no @ then only accept todo or to-do or to--do, etc, no spaces
))
(?-i) # Reset to case-sensitive
(?! # Start another non-consuming look-ahead, this time negative
@todo\s # It has to match lower-case @todo followed by one space
(?!-|:)\S # and then any non-space except - or :
)/m';

if ((bool) preg_match($expression, $comment) === true) {
if ($this->debug === true) {
echo "Failed regex - give message\n";
}

$comment = trim($comment, " /\r\n");
$phpcsFile->addWarning("'%s' should match the format '@todo Some task'", $stackPtr, 'TodoFormat', [$comment]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comments should end with a dot, also todo comments. So the example should be something like @todo Fix problem X here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be really cool to have a fixer here, that would help many people auto-fix this standard (also in Drupal core). I think it should be possible, we should have all info in the regex and could try a preg_replace().

Not a blocker for this pull request, we can merge this as first step.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, doc comments not ending with a period is checked for. But currently @ tags are not considered within that group, so no warning is given if they have no period at the end. I agree that if they should end in a dot then this example should include the dot. But then we should also include @ tags in the general sniff for no ending period. @see should not be included, we discussed that on the deprecation sniffs, the url should not end in a dot. Maybe this is for another issue?

Regarding a fixer, yes it should be possible. @ adamzimmermann did suggest that, but we wanted to wait until we knew you were interested in this sniff

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, very considerate of you! Yep, I absolutely support this sniff.

Also yep to the ending dot discussion, should be a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also yep to the ending dot discussion, should be a separate issue.

I have raised https://www.drupal.org/project/coder/issues/3183156 to check for doc tags not ending with a period.

}

}//end checkTodoFormat()


}//end class
64 changes: 64 additions & 0 deletions tests/Drupal/Commenting/TodoCommentUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

/**
* Test file for the todo standard.
*
* These are valid examples.
* @todo Valid.
* @todo valid with lower-case first letter
* @todo $can start with a $
* @todo \also with backslash
*
* These are all incorrect.
* @TODO Error
* @ToDo Error
* @TODo Error
* @ToDO Error
* @todo: Error
* @todo : Error
* @to-do Error
* @TO-DO Error
* @To-Do Error
* @TO do Error
* @to do Error
* @todo- Error
* @todo - Error
* @todoError
* @todo Error
* todo Error
* TODO Error
* ToDo Error
* to-do Error
*/

function foo() {
// These are valid examples.
// @todo Valid.
// @todo valid with lower-case first letter
// @todo $can start with a $
// @todo \also with backslash

// This is not a todo tag. It is a general comment and we do not want
// to do the standards checking here.

// These are all incorrect.
// @TODO Error
// @ToDo Error
// @TODo Error
// @ToDO Error
// @todo: Error
// @todo : Error
// @to-do Error
// @TO-DO Error
// @To-Do Error
// @TO do Error
// @to do Error
// @todo Error
// @todo- Error
// @todo - Error
// @todoError
// todo Error
// TODO Error
// ToDo Error
// to-do Error
}
46 changes: 46 additions & 0 deletions tests/Drupal/Commenting/TodoCommentUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<?php

namespace Drupal\Test\Commenting;

use Drupal\Test\CoderSniffUnitTest;

class TodoCommentUnitTest extends CoderSniffUnitTest
{


/**
* Returns the lines where errors should occur.
*
* 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 file being tested.
*
* @return array<int, int>
*/
protected function getErrorList(string $testFile): array
{
return [];

}//end getErrorList()


/**
* Returns the lines where warnings should occur.
*
* 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 file being tested.
*
* @return array<int, int>
*/
protected function getWarningList(string $testFile): array
{
$warningList = (array_fill_keys(range(13, 31), 1) + array_fill_keys(range(45, 63), 1));
return $warningList;

}//end getWarningList()


}//end class
8 changes: 4 additions & 4 deletions tests/Drupal/good/good.php
Original file line number Diff line number Diff line change
Expand Up @@ -1185,10 +1185,10 @@ function test6(array $names) {
/**
* Some short description.
*
* @todo TODOs are allowed here.
*
* @param string $x
* Some parameter.
*
* @todo These are allowed here.
*/
function test7($x) {

Expand All @@ -1205,8 +1205,8 @@ class ListContainsTest extends RulesIntegrationTestBase {}
/**
* Provides a 'Delete any path alias' action.
*
* @todo: Add access callback information from Drupal 7.
* @todo: Add group information from Drupal 7.
* @todo Add access callback information from Drupal 7.
* @todo Add group information from Drupal 7.
*
* @Action(
* id = "rules_path_alias_delete",
Expand Down