Generic/InlineControlStructure: small bug when fixing a control structure without body

rodrigoprimo commented 4 weeks ago

Describe the bug

The Generic.ControlStructures.InlineControlStructure sniff removes a newline from the code when adding braces to an inline control structure without body and with a trailing comment.

Code sample

if ($emptyBody); // comment

$unrelatedCode = 1;

To reproduce

Steps to reproduce the behavior:

Create a file called test.php with the code sample above.
Run phpcbf --standard=Generic --sniffs=Generic.ControlStructures.InlineControlStructure test.php
Check the modified file

Expected behavior

I would expect the modified file to be:

if ($emptyBody) { 
// comment
}

$unrelatedCode = 1;

But instead it is:

if ($emptyBody) { 
// comment
}
$unrelatedCode = 1;

Note there is no newline between the if and the variable assignment.

Versions (please complete the following information)


Operating System	Ubuntu 24.04
PHP version	8.3
PHP_CodeSniffer version	master
Standard	Generic
Install type	git clone

Additional context

I found this while working on finalizing the items discussed in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/482#pullrequestreview-2062765716, specifically the sections about commits 6 and 7. I can work on a PR to fix this small issue. Since @jrfnl mentioned the current behavior of the sniff regarding how to handle inline control structures without body, I'm not planning to change it anymore as I did in commit 6 or remove part of the fixer code as I did in commit 7.

Please confirm

[x] I have searched the issue list and am not opening a duplicate issue.
[x] I have read the Contribution Guidelines and this is not a support question.
[x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
[x] I have verified the issue still exists in the master branch of PHP_CodeSniffer.

jrfnl commented 4 weeks ago

@rodrigoprimo It is unclear to me what you are reporting here or what you are proposing as a fix.

if ($emptyBody); // comment

... is valid code (not useful code, but that's not the concern of the sniff), so this code should not be flagged by the sniff nor touched by the fixer, which is exactly what I suggested in my feedback on commit 6 in PR 482.

And if the code is no longer flagged by the sniff, the new line issue doesn't come into play.

rodrigoprimo commented 3 weeks ago

@jrfnl, since you mentioned in your feedback in PR 482 that in the past there was a decision not to handle those inline control structures without a body, I was planning on dropping commit 6. I should have mentioned this in this issue or before opening this issue. Without commit 6, I believe we should just fix this tiny issue in the fixer. Does this help clarify things?

what you are proposing as a fix

I'm proposing that the sniff fixes

if ($emptyBody); // comment

$unrelatedCode = 1;

As

if ($emptyBody) { 
// comment
}

$unrelatedCode = 1;

But let me know if you prefer that I work on improving commit 6 to address the points that you raised. If that is the case, we can close this issue.

jrfnl commented 3 weeks ago

But let me know if you prefer that I work on improving commit 6 to address the points that you raised.

Yes.

rodrigoprimo commented 3 weeks ago

Ok, closing as not planned (I wrongly closed as completed initially).

PHPCSStandards / PHP_CodeSniffer