Php_codesniffer: Provide phpcbf parameter `loop-limit=N` to stop (and save) after N loops/passes

[caugner]

While migrating a legacy code base to PSR-2 using phpcbf, I had a couple of files that couldn't be fixed automatically due to conflicting sniffs. That is, when running phpcbf with verbose output (-v or -vv respectively), I found that phpcbf was able to fix most sniff violations (in my example: 574 of 575), before turning into an infinite loop:

(...)

=> Fixing file: 1/575 violations remaining [made 11 passes]...  * fixed 1 violations, starting loop 12 *
PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\FunctionCallArgumentSpacingSniff (line 111) replaced token 2559 (T_WHITESPACE) "                    ," => ","

=> Fixing file: 1/575 violations remaining [made 12 passes]...  * fixed 1 violations, starting loop 13 *
**** PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 881) has possible conflict with another sniff on loop 11; caused by the following change ****
**** replaced token 2559 (T_COMMA) "," => "                    ," ****
**** ignoring all changes until next loop ****

=> Fixing file: 0/575 violations remaining [made 13 passes]...  * fixed 0 violations, starting loop 14 *
PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 881) replaced token 2559 (T_COMMA) "," => "                    ,"

=> Fixing file: 1/575 violations remaining [made 14 passes]...  * fixed 1 violations, starting loop 15 *
**** PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\FunctionCallArgumentSpacingSniff (line 111) has possible conflict with another sniff on loop 13; caused by the following change ****
**** replaced token 2559 (T_WHITESPACE) "                    ," => "," ****
**** ignoring all changes until next loop ****

=> Fixing file: 0/575 violations remaining [made 15 passes]...  * fixed 0 violations, starting loop 16 *
PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\FunctionCallArgumentSpacingSniff (line 111) replaced token 2559 (T_WHITESPACE) "                    ," => ","

=> Fixing file: 1/575 violations remaining [made 16 passes]...  * fixed 1 violations, starting loop 17 *
**** PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff (line 881) has possible conflict with another sniff on loop 15; caused by the following change ****
**** replaced token 2559 (T_COMMA) "," => "                    ," ****
**** ignoring all changes until next loop ****

(...)

Unfortunately phpcbf doesn't persist its fixes until it reaches 0 remaining violations. Having said this, I would have preferred phpcbf to stop and persist when the loop was detected, so that I can manually fix the remaining violation (). A parameter loop-limit would support this (with default value 0 for unlimited loops and N for stopping after loop *N).

(As a workaround I temporarily excluded the Generic.Functions.FunctionCallArgumentSpacing.SpaceBeforeComma sniff using a custom ruleset.xml, which eliminated the loop.)

PS

(*) = I found two sets of problems, both having to-do with misplaced comments:

/**
 * Problem 1
 */

// Description of firstCondition.
if (firstCondition) {
  // ...
}
// Description of secondCondition.
elseif (secondCondition) {
  // ...
}
// Description of fallback behavior.
else {
 // ...
}

/**
 * Solution (manual fix)
 */

if (firstCondition) {
    // Description of firstCondition.
    // ...
} elseif (secondCondition) {
    // Description of secondCondition.
    // ...
} else {
    // Description of fallback behavior.
    // ...
}

/**
 * Problem 2a
 */

sprintf("Some pattern with parameters %s, %s and %s"
  , firstParameterExpression // Description of first parameter.
  , secondParameterExpression // Description of second parameter.
  , thirdParameterExpression // Description of third parameter
);

/**
 * Problem 2b
 */

sprintf("Some pattern with parameters %s, %s and %s"
  // Description of first parameter.
  , firstParameterExpression 
  // Description of second parameter.
  , secondParameterExpression
  // Description of third parameter
  , thirdParameterExpression
);

/**
 * Solution (manual fix)
 */

sprintf(
    "Some pattern with parameters %s, %s and %s",
    // Description of first parameter.
    firstParameterExpression,
    // Description of second parameter.
    secondParameterExpression,
    // Description of third parameter
    thirdParameterExpression,
);

[caugner]

(Actually, phpcbf already stops after 50 passes. Therefore a parameter that makes phpcbf save after those 50 passes would suffice.)

[gsherwood]

The reason it doesn't apply the changes when it doesn't exit cleanly (i.e., it gets into an infinite loop) is that it isn't sure that the file is actually clean and the standard is valid.

I'm not really sure that it should just go ahead and apply the changes for you. I suspect people just wont notice that the file didn't get fixed completely and then either never fix their standard, or blame PHPCS for strange fixes.

[cscott]

I would also like this feature. It would be fine to exit with non-zero error code and/or an ugly message on standard error; the feature is useful when you are manually cleaning up some code and the error message indicates where to look next.

[coudenysj]

Any thoughts on how to resolve this issue?

Now I don't have a clue what the problems in the files are :(.

[coudenysj]

So, the approach I took was to run phpcbf with -vvv and see what sniffs keep popping up before failures.

I temporary disabled those, and do some manual fixing after phpcbf succeeded.

[gsherwood]

Closing as I think it's a bad idea to save a file that PHPCBF doesn't think it correctly fixed

[JLuc]

Ok closed, but is there a parameter to increase loop limits in case it's enough to fix the issue ?

Sidenote : strange is that cutting the file in little bits enables the fixer to fix the issues.

[gsherwood]

Ok closed, but is there a parameter to increase loop limits in case it's enough to fix the issue ?

If you cant fix the file in 50 loops, it's not going to be fixable. There must be a conflict in there.

Sidenote : strange is that cutting the file in little bits enables the fixer to fix the issues.

This is typically because the fixer conflict is happening when PHPCBF is processing the entire file structure. To debug, I always start with the entire file and keep removing bits until the fixer starts working. Then I can figure out which bit of code I removed to stop the failure. In every single case, this turns out to be just a few lines of code that creates a very specific structure that is causing the conflict. It's not that the 50 loops is suddenly enough to make that code snippet work.

[JLuc]

I've also had a situation where it was enough to split in bits for the fixer to fix, without any manual fix. That's why i thought it could be related to size or n-loops.
But ok it could have been related to comments or something in between one of the cutting points.

[coudenysj]

If you just run it with -vvv you see the exact location where phpcbf struggles.