Add automated @todo fixing.

[adamzimmermann]

Drupal.org issue: https://www.drupal.org/project/coder/issues/3177471

This replaces: #123.

---

Running this on core returns the expected results with 1 exception. The fixer currently breaks when an empty @todo is given, for example:

function lorem() {
  // @todo
  $node = Node::load(1);
}

Results in the following

function lorem() {
  // @todo $node = Node::load(1);
}

Could you add a test for the fixer? That way we can automatically validate the fixer. This can be achieved by adding the TodoCommentUnitTest.inc fixed file, containing the expected result of the autofix based on the TodoCommentUnitTest.inc file.

---

I fixed the new line creation and added a test for @todo statements by themselves. I added logic that marks them as unfixable since we can't know what the description should be.

[adamzimmermann]

Awaiting the Travis checks, but unit tests and code style checks were passing locally.

[jonathan1055]

This looks good. Leaving the empty @todo as unfixable is a good solution. I will make my own branch and try this out locally. @klausi will also want a test to prove the fixing it dones as expected, see comments above. You need to add a new file TodoCommentUnitTest.inc.fixed which is a copy of TodoCommentUnitTest.inc but with all the automated fixes applied.

[jonathan1055]

Tested locally and I confirm that the problem @klausi reported with the empty todo causing the next line to be appended up is fixed. However, when there is no text in the todo tag but the formatting of the todo can be fixed, eg @to-do or @todo:, etc then this is marked as fixable (which is partly true). Running the fixer correct them to be @todo but adds a space afterwards, and because there is no content/text this is actually introducing a new coding standards fault (trailing space) so the fixer is adding a new problem.

I tried to investigate this, but something unexpected is happening. Even when $fix is false the file is still being altered. Does the line $fix = $phpcsFile->addFixableWarning(" ... actually make a change? I thought it should not. But that line is making the alteration to my test file.

[jonathan1055]

Did some more investigation and I am even more at a loss as to what is happening. When running phpcbf and calling $phpcsFile->addFixableWarning I get from my own added debug:

---addFixableWarning()
$recorded = 1
$this->fixer->enabled = 
returning false

Two problems: fixer->enabled should be true, because I have called phpcbf. Secondly, even when it returns false and $fix is false the file is still changed. But the fixer code in the sniff is not being executed, because $fix is false. I am stumped. I must have some major mis-understanding about how phpcbf works.

[arkener]

Just to clarify, the comment on #123 in regards to the incorrect fix and the request for the TodoCommentUnitTest.inc.fixed test file were given by me.

There is currently 1 PHPStan test result which should be fixed, the others will be fixed via #126. These tests can be validated locally by running ./vendor/bin/phpstan analyse

------ --------------------------------------------------------------------- 
  Line   coder_sniffer/Drupal/Sniffs/Commenting/TodoCommentSniff.php          
 ------ --------------------------------------------------------------------- 
  126    Method Drupal\Sniffs\Commenting\TodoCommentSniff::checkTodoFormat()  
         has parameter $tokens with no value type specified in iterable type  
         array. 

@jonathan1055 Your understanding of phpcbf is correct. $phpcsFile->fixer->enabled should return true while running the phpcbf command and should return false while running the phpcs command. addFixableWarning shouldn't alter the content of the file regardless of the status of $this->fixer->enabled as this method should only display the message. The contents of the test file should only be altered when the fixer is enabled and $fix is true on a rule.

I'm unsure why these issues are happening in your environment. I'm currently getting the expected result while running phpcs and phpcbf, and viewing the value of $phpcsFile->fixer->enabled via xdebug. I'm also currently unable to reproduce the alteration of the test file when not running in "fix" mode. Could you add your test file and the command you're running so we can validate this issue?

[adamzimmermann]

However, when there is no text in the todo tag but the formatting of the todo can be fixed, eg @to-do or @todo:, etc then this is marked as fixable (which is partly true). Running the fixer correct them to be @todo but adds a space afterwards, and because there is no content/text this is actually introducing a new coding standards fault (trailing space) so the fixer is adding a new problem.

Looking into this next.

[jonathan1055]

Reading @arkener 's comments there may not be anything wrong or anything to investigate. It seems my environment might be doing some odd things. I have not resolved it yet, and will be interested if you can replicate what I get, as @arkener cannot and it all works fine for him.

[adamzimmermann]

I didn't fully follow/understand all of the conversation above, but I added tests for this new scenario, and everything is passing.

I also altered some unrelated files in my codebase and ran the fixer on them and got the following output for these problems:

Code:

  // @to-do
  // @to-do
  // @to-do
  // @todo
  // @todo Something

Output:

$ composer cs-fix
> composer robo job:fix-coding-standards
> robo --ansi --load-from $(pwd)/RoboFile.php 'job:fix-coding-standards'
 [ExecStack] Executing vendor/bin/phpcbf --config-set installed_paths vendor/drupal/coder/coder_sniffer
 [ExecStack] Running vendor/bin/phpcbf --config-set installed_paths vendor/drupal/coder/coder_sniffer
Using config file: /Users/adamzimmermann/www/chromatichq.com/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Config value "installed_paths" updated successfully; old value was "vendor/drupal/coder/coder_sniffer"
 [ExecStack] Done in 0.102s
 [ExecStack] Executing vendor/bin/phpcbf --standard=Drupal,DrupalPractice --extensions=css,inc,info,install,module,php,profile,scss,test,theme,txt --ignore=*/themes/chromatic/build,*/themes/chromatic/node_modules web/modules/custom web/themes/chromatic RoboFile.php
 [ExecStack] Running vendor/bin/phpcbf --standard=Drupal,DrupalPractice --extensions=css,inc,info,install,module,php,profile,scss,test,theme,txt --ignore=*/themes/chromatic/build,*/themes/chromatic/node_modules web/modules/custom web/themes/chromatic RoboFile.php

PHPCBF RESULT SUMMARY
--------------------------------------------------------------------------------------------------------------------------------
FILE                                                                                                            FIXED  REMAINING
--------------------------------------------------------------------------------------------------------------------------------
/Users/adamzimmermann/www/chromatichq.com/web/modules/custom/chromatic_copyright/chromatic_copyright.module     3      4
--------------------------------------------------------------------------------------------------------------------------------
A TOTAL OF 3 ERRORS WERE FIXED IN 1 FILE
--------------------------------------------------------------------------------------------------------------------------------

Time: 2.97 secs; Memory: 14MB


 [ExecStack]  Exit code 1  Time 3.135s
 [notice] Stopping on fail. Exiting....
 [error]  Exit Code: 1
 [error]    in task Robo\Task\Base\ExecStack


Script robo --ansi --load-from $(pwd)/RoboFile.php handling the robo event returned with error code 1
Script composer robo job:fix-coding-standards handling the cs-fix event returned with error code 1

Result:

  // @todo
  // @todo
  // @todo
  // @todo
  // @todo Something

It wasn't happy that it found rows that I marked as fixable, but then it was only able to fix some of the problems and not all of them. I'm not sure this is a bug though, it seems like the desired result.

I am not seeing the trailing space being added though.

So I think all issues have been addressed unless I'm told otherwise. 🤞

[jonathan1055]

So I think all issues have been addressed unless I'm told otherwise

I made a couple of comments in the review below. I thought I had submitted these, but seems it was still pending. Just changes to test data rows order, and additional comments on the tests.

[jonathan1055]

I think it might be helpful if you change "These are all incorrect" to say "These are all incorrect but can be fixed automatically" and then move the bad un-fixable cases to a group at the end, with a comment "These are incorrect and not auto-fixable" or something like that.

[jonathan1055]

Also add cases // @to-do and // @TODO with no text, to demonstrate what happens to them. These were the ones I had difficulty with.

[adamzimmermann]

Updated the test cases and comments per the requests above.

[jonathan1055]

Looks good now. Thanks