BracesFixer - handle comment for content outside of given block

[keradus]

No description provided.

[demiankatz]

@keradus, I'm seeing some odd behavior in the latest php-cs-fixer release that seems to be related to this PR. If I have a series of comments inside an if..else, the first one gets de-indented. So, for example, I end up with something like this:

if ($condition) {
    echo "This is code.";
// This is the first line of comments
    // This is the second line of comments
} else {
    echo "This is different code.";
}

This isn't really causing major problems for me, but it seems like it is not the desired behavior, so I thought you might be interested to know. Please let me know if you would like any additional information.

[keradus]

2nd commend shall be de-indented as well, as those multiple T_COMMENT are one comment for dev.

Would you mind to send a PR ?

[demiankatz]

@keradus, it may take me a little while to find time to put together a PR, but I will try to do so. Thanks!

[keradus]

Thanks, I would appreciate that !

[demiankatz]

@keradus, I wanted to start by creating a test case to demonstrate the problem, and to this end I began trying to find a way to set up an example of my problem in BracesFixerTest. Unfortunately, I don't think I understand exactly what all these arrays of examples are supposed to do, and I ran out of time before achieving success. Do you have any pointers on a place in the test suite where I can simply provide input and expected output in order to begin this project?

[keradus]

Sure @demiankatz
test case is the following array: [$expected, $input = null, array $configuration = null]
$input is code piece before fixing, then $expected is the code we want to have after fix
we could also have only $expected, if we want to confirm that fixer won't change anything in already valid code (no $input)
optionally, $config could be added if we want test rule configured differently (some of the rules allows for configuration).

Now, I can see that we didn't properly handle this PR. Test scenarios we have added are having expected values, but not input ones (only one element in array).
For that, I do believe that we shall add next case build on top of https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/3359/files#diff-6ee2b7ebb0faee78657d4e8d3fb3e10dR2781 . Basically, copy it as next case scenario and modify it to cover newly discovered issue - by both, extending it to cover multiline comments and to have that missing $input version

[demiankatz]

Thanks, @keradus, that is extremely helpful. I was able to create a failing test here:

demiankatz@8b5f664

Not sure if I'm qualified to actually fix the bug, though -- I've been poking at the code a bit, but it's quite complex, and I'm not sure if this problem is going to require a small tweak or a more significant rewrite. I welcome advice if you're willing to share. In any case, thanks for your time so far!

[keradus]

you welcome ;)

I do agree that braces rule is huge and hard to maintain, but hey, it's monster for sad reasons, that it's super complex to handle all of those braces in one place without creating conflicts with other rules and have all "sub-rules" of braces working well together :( #472 for reference - see amount of commits, fixes, referred issues...

Your test-case looks pretty valid and I believe it nicely showing the issue we are facing now.
Please, open it as work-in-progress PR, so it would be easier to actually discuss and help on it, also we would have CI services processing the changes !

[demiankatz]

Thanks, @keradus, I've opened #3573 to continue this conversation. And yes, I do understand that some of these things are inherently complicated. Keeps us from getting bored, I guess! :-)