Generic/ForLoopShouldBeWhileLoop: improve sniff code coverage + fix potential PHP 8.3 deprecation notice

[rodrigoprimo]

Description

This PR improves the code coverage for the Generic/ForLoopShouldBeWhileLoop sniff.

There was just one block of code that was not covered by tests. It is a defensive code that bails early if the for keyword is not followed by an opening parenthesis. I also added a few more test cases that were already handled properly by the sniff, but they seemed different enough from the other test cases to justify their inclusion to avoid regressions in the future.

Two questions that occurred to me while working on this PR:

• One of the test cases that I added is for (;;). This could be replaced with while as well, but I'm not sure if the sniff is not covering this case intentionally or not. Should we update the sniff to trigger a warning for for(;;) or leave it as is? Judging by the commit message that introduced this sniff, it seems it was inspired by a PHP Mess Detector rule, but I was not able to find such a rule to check how it handles for (;;).

• The sniff relies on finding the closing parenthesis associated with the for keyword. But there is no code to explicitly handle missing closing parenthesis. The sniff still works as without the closing parenthesis, $end is null, and $next <= $end always evaluates to false. I wonder if we should update the sniff to bail early if there is no closing parenthesis?

Code coverage for the Generic/ForLoopShouldBeWhileLoop sniff before this PR:

https://coveralls.io/builds/64633852/source?filename=src%2FStandards%2FGeneric%2FSniffs%2FCodeAnalysis%2FForLoopShouldBeWhileLoopSniff.php

And after this PR:

https://coveralls.io/builds/64858806/source?filename=src%2FStandards%2FGeneric%2FSniffs%2FCodeAnalysis%2FForLoopShouldBeWhileLoopSniff.php

Related issues/external references

Part of #146

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@rodrigoprimo Thanks for continuing your work on these type of PRs.

I also added a few more test cases that were already handled properly by the sniff, but they seemed different enough from the other test cases to justify their inclusion to avoid regressions in the future.

👍🏻

As I'm looking over your shoulder at the tests now anyway, may I suggest adding some tests with a for loop using alternative syntax as well ?

* One of the test cases that I added is `for (;;)`. This could be replaced with `while` as well, but I'm not sure if the sniff is not covering this case intentionally or not. Should we update the sniff to trigger a warning for `for(;;)` or leave it as is? Judging by the commit message that introduced this sniff, it seems it was inspired by a PHP Mess Detector rule, but I was not able to find such a rule to check how it handles `for (;;)`.

I don't have an answer for that. You may be able to find more info on the website of the original PMD project on which the PHP Mess Detector is based.

Alternatively, maybe @manuelpichler remembers ? Or one of the current maintainers of the PHPMD project, like @tvbeek, could advise ?

* The sniff relies on [finding the closing parenthesis](https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/84acf4e56f110db8e75cb9a575c5727df637643c/src/Standards/Generic/Sniffs/CodeAnalysis/ForLoopShouldBeWhileLoopSniff.php#L65) associated with the `for` keyword. But there is no code to explicitly handle missing closing parenthesis. The sniff still works as without the closing parenthesis, `$end` is `null`, and [`$next <= $end`](https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/84acf4e56f110db8e75cb9a575c5727df637643c/src/Standards/Generic/Sniffs/CodeAnalysis/ForLoopShouldBeWhileLoopSniff.php#L74) always evaluates to false. I wonder if we should update the sniff to bail early if there is no closing parenthesis?

Good catch and yes, most definitely! If for no other reason than that the current code will throw a "Decrement on type null has no effect, this will change in the next major version of PHP" deprecation notice on PHP 8.3.

I guess, I don't need to ask you to add a test for that too ? 😉

Code coverage for the Generic/ForLoopShouldBeWhileLoop sniff before this PR:

For the future - I appreciate you adding these links, but they are not needed. Each PR contains a direct link to the Coveralls report in the status check block (and I check PRs locally anyway):

[tvbeek]

* One of the test cases that I added is `for (;;)`. This could be replaced with `while` as well, but I'm not sure if the sniff is not covering this case intentionally or not. Should we update the sniff to trigger a warning for `for(;;)` or leave it as is? Judging by the commit message that introduced this sniff, it seems it was inspired by a PHP Mess Detector rule, but I was not able to find such a rule to check how it handles `for (;;)`.

I don't have an answer for that. You may be able to find more info on the website of the original PMD project on which the PHP Mess Detector is based.

Alternatively, maybe @manuelpichler remembers ? Or one of the current maintainers of the PHPMD project, like @tvbeek, could advise ?

For PHPMD we have a rule about CyclomaticComplexity ( https://phpmd.org/rules/codesize.html#cyclomaticcomplexity ) but not anything about the for(;;) notation. Maybe it was a missed point during creation of the sniff. I think triggering a warning is a good point (or make it configurable but that means more code to maintain).

[rodrigoprimo]

Thanks for checking this PR, @jrfnl.

As I'm looking over your shoulder at the tests now anyway, may I suggest adding some tests with a for loop using alternative syntax as well ?

Sure, I just added a new commit with a few tests using the alternative syntax.

For PHPMD we have a rule about CyclomaticComplexity ( https://phpmd.org/rules/codesize.html#cyclomaticcomplexity ) but not anything about the for(;;) notation. Maybe it was a missed point during creation of the sniff. I think triggering a warning is a good point (or make it configurable but that means more code to maintain).

Thanks for your input, @tvbeek.

@jrfnl, I defer to you on whether or not it makes sense to change the sniff, on a separate PR, to trigger a warning when it finds for(;;).

Good catch and yes, most definitely! If for no other reason than that the current code will throw a "Decrement on type null has no effect, this will change in the next major version of PHP" deprecation notice on PHP 8.3.

I guess, I don't need to ask you to add a test for that too ? 😉

I added the test for that case. Would you like me to work on fixing the sniff as well? In a separate PR?

For the future - I appreciate you adding these links, but they are not needed.

👍

[rodrigoprimo]

@jrfnl, and here is the warning that you mentioned that is causing the tests to fail when running PHP 8.3: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7410876425/job/20164113044?pr=226#step:9:57. I failed to consider that this would obviously happen before posting my previous comment.

Let me know if you prefer that I fix the sniff in this PR or remove the test from it and add it together with the fix on a separate PR (that is, if you want me to work on this fix).

[rodrigoprimo]

Just documenting here that the sniff ForLoopWithTestFunctionCall will also throw a "Decrement on type null has no effect, this will change in the next major version of PHP" deprecation notice on PHP 8.3 if the closing parenthesis is missing.

[jrfnl]

@rodrigoprimo

I defer to you on whether or not it makes sense to change the sniff, on a separate PR, to trigger a warning when it finds for(;;).

Let's leave that out of this PR for now, if for no other reason than that that change would need to go in a minor release while this PR as-is can go into a patch release.

You might want to open an issue to see if other people are interested in this change ?

Let me know if you prefer that I fix the sniff in this PR or remove the test from it and add it together with the fix on a separate PR (that is, if you want me to work on this fix).

I'm perfectly fine with you fixing it in this PR, preferably the test and the fix should be in the same commit though, so this PR would then ideally be three commits:

1. The test file rename
2. The extra tests for covering more syntax variations and existing logic (current commits 2, 3 and 4)
3. The fix + associated test (current commit 5)

Just documenting here that the sniff ForLoopWithTestFunctionCall will also throw a "Decrement on type null has no effect, this will change in the next major version of PHP" deprecation notice on PHP 8.3 if the closing parenthesis is missing.

Sounds like a nice one for you to tackle next once this one is finished ;-)

[rodrigoprimo]

You might want to open an issue to see if other people are interested in this change ?

Sure, here is the issue: #230

I'm perfectly fine with you fixing it in this PR, preferably the test and the fix should be in the same commit though, so this PR would then ideally be three commits

Done, this PR is ready to be reviewed again. Thanks.

Sounds like a nice one for you to tackle next once this one is finished ;-)

Sounds good :-)

[jrfnl]

Thanks for making those updates @rodrigoprimo ! LGTM.

Two practical points:

1. Could you squash those last two commits into the PHP 8.3 fix commit ?
2. As this PR now contains a small bugfix, the "Changelog" section in the PR template could be brought back.
   You don't need to worry about that, I'll come up with something, but maybe something to keep in mind for the future.

[rodrigoprimo]

Could you squash those last two commits into the PHP 8.3 fix commit ?

Sure, I just did it.

As this PR now contains a small bugfix, the "Changelog" section in the PR template could be brought back.
You don't need to worry about that, I'll come up with something, but maybe something to keep in mind for the future.

This answers the question that I asked in #235 (I should probably have asked it here instead) :-)

Thanks for taking care of the changelog.