Php-cs-fixer: BracesFixer - comment indented wrongly removed

[pablorsk]

:warning: EDIT: the problem is braces on v 2.10.0. :point_right: https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/3490#issuecomment-362238724

The PHP version you are using ($ php -v):

=> PHP 7.0.22

PHP CS Fixer version you are using ($ php-cs-fixer -V):

=> 2.10.0

The command you use to run PHP CS Fixer:

=> vendor/bin/php-cs-fixer fix --config=.php_cs.dist

The configuration file you are using, if any:

'multiline_whitespace_before_semicolons' => ['strategy' => 'no_multi_line'],

If applicable, please provide minimum samples of PHP code (as plain text, not screenshots):

• before running PHP CS Fixer (no changes):

             if (true) {       
                 Log::error('No se encontro el proximo precio (futuro) de tipo
                     al mismo precio.');
                 //Si existe remmplazo el producto por el precio que corresponde
                 //debo borrar los dos future prices
             }

• with unexpected changes applied when running PHP CS Fixer:

             if (true) {       
                 Log::error('No se encontro el proximo precio (futuro) de tipo
                     al mismo precio.');
             //Si existe remmplazo el producto por el precio que corresponde
                 //debo borrar los dos future prices
             }

• with the changes you expected instead (the same :/):

             if (true) {       
                 Log::error('No se encontro el proximo precio (futuro) de tipo
                     al mismo precio.');
                 //Si existe remmplazo el producto por el precio que corresponde
                 //debo borrar los dos future prices
             }

[SpacePossum]

Hi and thanks for reporting,

multiline_whitespace_before_semicolons should not make changes to your sample code as there is no spacing at all before any semicolon in it. I've tried various releases of the fixer on the sample but never see the change applied.
In order to bug hunt more I would like to know if you've any other rules (or sets) enabled through your configuration file (maybe you've a default set as well?). Secondly, could you check if the indentation in the sample is made up only out of spaces or tabs (and not mixed) and matches the configuration for identation you use?
Thanks!

[pablorsk]

You are rigth @SpacePossum, the problem is not this rule. The problem was migration from 2.9.0 to 2.10.0

2.9.0

$ vendor/bin/php-cs-fixer -V
PHP CS Fixer 2.9.0 Speechless by Fabien Potencier and Dariusz Ruminski
$ git diff
# no results
$ vendor/bin/php-cs-fixer fix  --path-mode=intersection --rules=braces app/Builder.php
Loaded config default from "/home/pablorsk/myproject/.php_cs.dist".
Using cache file ".php_cs.cache".

Fixed all files in 0.129 seconds, 10.000 MB memory used
$ git diff
# no results

2.10.0

$ vendor/bin/php-cs-fixer -V
PHP CS Fixer 2.10.0 Bowling Bear by Fabien Potencier and Dariusz Ruminski
$ git diff
# no results
$ vendor/bin/php-cs-fixer fix  --path-mode=intersection --rules=braces app/Builder.php
Loaded config default from "/home/pablorsk/myproject/.php_cs.dist".
Using cache file ".php_cs.cache".
   1) app/Builder.php

Fixed all files in 0.140 seconds, 10.000 MB memory used
$ git diff
diff --git a/app/Builder.php b/app/Builder.php
index f1f2850..dfc2b1f 100644
--- a/app/Builder.php
+++ b/app/Builder.php
@@ -94,7 +94,7 @@ class JsonApiResourceBuilder
             } elseif ($modelOrCollection === null) {
                 // @todo, we need to know if is a Collection or a Resource
                 $ret[$typealias] = ['data' => []];
-                // $ret[$typealias] = ['data' => null];
+            // $ret[$typealias] = ['data' => null];
             } else {
                 throw new \Exception(

[SpacePossum]

Thanks for the additional details!
With it I could create a minimal sample and find the lowest maintained branch with this issue;

<?php

if ($a) {
    echo 2;
    // abc
} else {
    echo 1;
}

[12:42 PM]-[possum@zoo1]-[/home/possum/work/PHP-CS-Fixer]-[git 2.2] 
$ php php-cs-fixer fix --dry-run --rules=braces tmp/MyTest.php  --diff
Loaded config default from "/home/possum/work/PHP-CS-Fixer/.php_cs.dist".
Paths from configuration file have been overridden by paths provided as command arguments.
   1) tmp/MyTest.php
      ---------- begin diff ----------
--- Original
+++ New
@@ @@
 <?php

 if ($a) {
     echo 2;
-    // abc
+// abc

      ----------- end diff -----------

[keradus]

this change is by purpose. there is no other code after // abc comment in it's scope.
Comments are attached for next code instructions. Here, next instruction is else control statement. For that, comment is aligned to it, not to previous code.

you are documenting? always document before code or inline, never after.
you are commenting out dead code? drop it instead.

[pablorsk]

@keradus, thanks for you explanation.

On version 2.9.0, when the next line of a comment is a block closing (}), indent is like before line. When the next line, is not a closing block, indent like next line its OK.

Example:

Original :thinking:

if (true) {
    // some comment
    echo 'true';
} else {
    echo '1';
    echo '2';
    // echo '3'; // temporarily disabled 
}
echo 'done';

Fixed with 2.10.0 :disappointed:

if (true) {
    // some comment
    echo 'true';
} else {
    echo '1';
    echo '2';
// echo '3'; // temporarily disabled
}
echo 'done';

Fixed with 2.9.0 :smile:

if (true) {
    // some comment
    echo 'true';
} else {
    echo '1';
    echo '2';
    // echo '3'; // temporarily disabled
}
echo 'done';

We also work with php_codesniffer, and works like 2.9.0.

[keradus]

in 2.10 we extended fixer to better cover the real case,
when you are documenting next line (that } else {)

how it handle temporarily disabled code doesn't matter in the end, as you shall never commit it to the repo.

also, different IDEs are commenting them differentially,

-    echo 3;
+    // echo 3;

vs

-    echo 3;
+//  echo 3;

[SpacePossum]

I'm +0 here for this case.
I don't think the comment should be re-indented as comments should be allowed to go about everywhere (so not like PHPDoc's), however such a arbitrary statement makes it hard to make a fixer. In the end, it would be nice to split the braces-fixer and leave it up to the end-user, but I don't see that happen soon.

[gharlan]

I don't like the new behavior and it prevents me from upgrading php-cs-fixer.
So I would prefer to make this configurable or split this from braces fixer.

[gharlan]

But at least it should also reindent the second comment line (in the example in first issue comment).

[C-Duv]

you are documenting? always document before code or inline, never after.
-- @keradus

What about documenting _post-operation_ states?
Sometimes you add a comment after a line of code called a function, only to explain how the state of you application changed after the execution of the function.

Examples:

$array = ['foo', 'bar'];
sort($array);
// $array is now sorted

$object = new MyClass();
$object->doStuff(MyClass::SPECIAL_ACTION);
// $object should now contain foo attribute, should now be ready for xxx, should not be serialized, etc.

$object1 = new MyClass();
$object2 = new MyClass();
$comparison = $object1->compareTo($object2);
// $comparison = 0 if $object1 is as cheap as $object2, = -1 if $object1 is cheaper than $object2, = 1otherwise.