Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice #235

rodrigoprimo · 2024-01-05T16:52:36Z

Description

As discussed in #226 (comment), this PR fixes the following PHP 8.3+ deprecation notice that affected the Generic.CodeAnalysis.ForLoopWithTestFunctionCall sniff:

Decrement on type null has no effect, this will change in the next major version of PHP

src/Standards/Generic/Sniffs/CodeAnalysis/ForLoopWithTestFunctionCall.php:69

While working on this change, I also added a commit to improve test coverage for the sniff.

I'm marking this PR as a bug fix. That made me wonder if I should update #226 to mark it as a bug fix as well and include a changelog or if, since this just fixes a deprecation notice, it is fine to exclude the changelog entry from this PR and not mark it as a bug fix. Let me know what is the best approach. Yes, this PR should be marked as a bug fix. Answered here: #226 (review)

I opted to add a comment to the top of the existing test case file (I decided to use this file for code examples using the ¨regular" for syntax and created a separate file for examples using the alternative syntax). This means that I had to update the line numbers in the array returned by ForLoopWithTestFunctionCallUnitTest::getWarningList(). I believe this is ok in this particular case since there were only two elements in the original array. Please let me know if you disagree, and I can remove the comment from the PR before you review it.

Suggested changelog entry

Generic/ForLoopWithTestFunctionCall: fix PHP 8.3+ deprecation notice

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 · 2024-01-05T22:15:51Z

@rodrigoprimo Why did you add the tests for the alternative syntax in a separate test case file ?

rodrigoprimo · 2024-01-08T13:21:08Z

@rodrigoprimo Why did you add the tests for the alternative syntax in a separate test case file ?

@jrfnl, I checked #196 and saw that you created a separate test case file for short open echo tags. I liked the idea of separating the test cases into different files and decided to do the same here hoping that it would help to better organize the test cases.

That being said, I think it would make more sense to separate the test cases into different files if there was an easier way to see which test cases each test case file contained without having to open the file and read the comment (maybe a suffix in the file name?).

Now that you asked, I also realized that the criterion that I used to separate the tests is a bit different than what you used. In #196, each of the main test case files contains test cases for different tokens that trigger the sniff, while that is not the case here.

Happy to move the test cases for the alternative syntax to the main test case file if you prefer.

jrfnl · 2024-01-17T11:15:48Z

@rodrigoprimo Why did you add the tests for the alternative syntax in a separate test case file ?

@jrfnl, I checked #196 and saw that you created a separate test case file for short open echo tags.

The only reason I did that is because that sniff has conditions to handle the first and last open tags in a file differently, which is why this needs a separate test case file to allow for this to be tested properly.
The PR/commit descriptions mention this a couple of times.

I liked the idea of separating the test cases into different files and decided to do the same here hoping that it would help to better organize the test cases.

While I like better test organization, the way the tests are set up, every test case file being added leads to a slow down in the tests (due to the overhead of setup/tokenizing), so extra test case files should only be added when they are needed to test a particular aspect of the sniff.

That being said, I think it would make more sense to separate the test cases into different files if there was an easier way to see which test cases each test case file contained without having to open the file and read the comment (maybe a suffix in the file name?).

That is currently not an option with how the test suite is organized. I'll consider it as a suggestion for #25.

Happy to move the test cases for the alternative syntax to the main test case file if you prefer.

Yes please.

This commit moves the tests for the alternative `for` syntax to the main test case file following a request during the PR review: PHPCSStandards#235 (comment) It also renames the subsequent test case files to keep the filenames consistent.

rodrigoprimo · 2024-01-17T14:29:19Z

While I like better test organization, the way the tests are set up, every test case file being added leads to a slow down in the tests (due to the overhead of setup/tokenizing), so extra test case files should only be added when they are needed to test a particular aspect of the sniff.

That is good to know. I will keep it in mind when creating future PRs. Thanks.

That is currently not an option with how the test suite is organized. I'll consider it as a suggestion for #25.

Sounds good to me.

Yes please.

I just pushed a new commit moving the alternative for syntax tests to the main test case file.

jrfnl

Thanks for this PR @rodrigoprimo. I've now done a proper review and left some comments inline.

I like the extra variations in function calls (and the variable one which was previously handled, but untested). I can think of some more nasty code variations still (function call on new object (new MyClass)->callMe(), variable variable call ${$var}()), but can already tell some of those wouldn't be handled correctly in the sniff as it is... 😈

Would it also be an idea to add some tests where the first/second/third part of the for condition is empty ?
Like if ($i = 0; ; $--$i) {} ?

By the looks of it, this will be handled correctly, but it would add a safeguard to ensure future changes to the sniff doesn't break that.

src/Standards/Generic/Tests/CodeAnalysis/ForLoopWithTestFunctionCallUnitTest.2.inc

jrfnl · 2024-01-17T19:34:08Z

src/Standards/Generic/Tests/CodeAnalysis/ForLoopWithTestFunctionCallUnitTest.1.inc

+    echo $it->current();
+}
+
+for ($i = 0; MyClass::staticMethod($i); $i++) {


Outside the scope of this PR, but I kind of think that the sniff should not trigger for this line (nor for line 21, 39 and 43) as the function call receives $i as a parameter and $i is the variable being incremented in the third part of the condition.

For now, I think it would be better to just change the parameter passed to the function call in each of these code samples to something other than $i.

If you agree that if the variable being in/decremented in the third part of the condition and it is being used as a parameter in the function call in the second part of the condition the sniff, should not flag the function call (or flag it with a different error code, which would allow for excluding the error code), I suggest opening an issue about this.

Good catch! I failed to consider this when creating the tests. I just updated them in a new commit.

I tend to agree with you that the behavior of the sniff should be different. and using $i in the cases that you highlighted should not trigger the sniff or it should raise a different error. I will create an issue to continue this discussion as you suggested.

This commit changes the variable name in a few tests where there is a function call in the second part of the `for` condition and the variable being passed to the function is the same that is incremented in the third part of the `for` condition. The sniff is triggered in the examples that were changed, but there is a discussion that potentially it should not be triggered. Changing the code to use variable names that for sure should trigger the sniff. Discussion here: PHPCSStandards#235 (comment)

rodrigoprimo · 2024-01-18T17:28:04Z

Thanks for the super comprehensive and valuable reviews, @jrfnl! I appreciate it a lot.

I can think of some more nasty code variations still (function call on new object (new MyClass)->callMe(), variable variable call ${$var}()), but can already tell some of those wouldn't be handled correctly in the sniff as it is... 😈

I added a test for function call on new object. Should I create an issue for a variable variable call as the sniff does not handle that properly? I guess the sniff should support it since it is valid syntax.

Would it also be an idea to add some tests where the first/second/third part of the for condition is empty ?
Like if ($i = 0; ; $--$i) {} ?

Good point. I added a few more tests covering different scenarios with one or more conditions empty. I don't know if I went too far, as I added a significant number of tests that are similar and then duplicated them with the alternative syntax. Happy to adjust this if needed.

This PR is ready for another review.

jrfnl · 2024-01-22T02:13:13Z

I don't know if I went too far, as I added a significant number of tests that are similar and then duplicated them with the alternative syntax. Happy to adjust this if needed.

Well, kind of... this is definitely more comprehensively tested now than is needed. Then again, better safe than sorry, so I'm okay with leaving those tests be.

jrfnl

Thanks for this PR @rodrigoprimo !

I think this is good to go.

If I look very critically, I'd say that you've gone a bit overboard with the alternative syntax tests (but you recognized that yourself already).
I can also imagine one more situation which is currently untested (though would not be problematic given the current code): a for with a function call in the third expression, but not in the second.
The current tests have fors with a function call in the first expression, in the second expression or in all three, but no test with only a function call in the third (or in the first and third and not in the second expression).

Either way, I'm okay with merging this as-is, though if you don't mind, I will rebase the PR and re-organize the commits to atomic ones.

Only thing that remains would then be to open those two issues for enhancements to the sniff.

rodrigoprimo · 2024-01-22T15:19:56Z

Thanks again for taking another look at this PR, @jrfnl.

I can also imagine one more situation which is currently untested (though would not be problematic given the current code): a for with a function call in the third expression, but not in the second.
The current tests have fors with a function call in the first expression, in the second expression or in all three, but no test with only a function call in the third (or in the first and third and not in the second expression).

While adding tests with function calls in different parts of the for loop, I wrongly came to the conclusion that adding a function to the third part would not work. That is why I did not add such tests. After seeing your comment, I noticed my mistake and added two more tests as you suggested.

Either way, I'm okay with merging this as-is, though if you don't mind, I will rebase the PR and re-organize the commits to atomic ones.

I don't mind at all. Thanks for taking care of this. Happy to do it myself if you prefer.

Only thing that remains would then be to open those two issues for enhancements to the sniff.

Here are the issues: #297 and #298

Doing this to create more test case files in subsequent commits.

This commit adds more test cases related to the Generic/ForLoopWithTestFunctionCall sniff including test cases for the alternative for syntax, more types of function calls in the test part of the for loop and a test case to check defensive code when the for loop doesn't have a opening parenthesis.

This commit fixes an issue in the sniff that could result in the following E_DEPRECATED error when running PHP 8.3: ``` Decrement on type null has no effect, this will change in the next major version of PHP src/Standards/Generic/Sniffs/CodeAnalysis/ForLoopWithTestFunctionCall.php:69 ``` This sniff relies on finding the position of the open and closing parentheses for a given `for` loop. However, the problem was that there was no defensive code for cases when the closing parenthesis is missing. The sniff would still work when running PHP >= 8.2, but on PHP 8.3 it would throw the deprecated message above. This would happen because since there is no closing parenthesis `$end` is set to null, and $next <= $end always evaluates to false (https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/84acf4e56f110db8e75cb9a575c5727df637643c/src/Standards/Generic/Sniffs/CodeAnalysis/ForLoopWithTestFunctionCallSniff.php#L73). The issue was fixed by bailing early if the closing parenthesis is missing. A test with a `for` loop without the closing parenthesis was added.

jrfnl · 2024-01-22T23:36:23Z

Thanks for those additional tests and opening those issues @rodrigoprimo !

I've now gone ahead and reorganized the commits to atomic ones + added one commit with a small docs fix for something I noticed while reviewing.

Merging when the build has passed.

github-actions bot added Type: bug Standard: Generic Status: triage labels Jan 5, 2024

rodrigoprimo mentioned this pull request Jan 5, 2024

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

Merged

11 tasks

rodrigoprimo changed the title ~~Generic/ForLoopWithTestFunctionCall: fix PHP 8.3+ deprecation notice~~ Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice Jan 5, 2024

jrfnl added the Status: awaiting feedback label Jan 5, 2024

jrfnl removed the Status: awaiting feedback label Jan 12, 2024

jrfnl added the Status: awaiting feedback label Jan 17, 2024

jrfnl requested changes Jan 17, 2024

View reviewed changes

jrfnl removed the Status: triage label Jan 17, 2024

rodrigoprimo force-pushed the fix-deprecated-error-for-loop-with-test-function-call branch from 72d03ff to 6d2b42b Compare January 18, 2024 17:25

jrfnl removed the Status: awaiting feedback label Jan 19, 2024

jrfnl added this to the 3.9.0 milestone Jan 22, 2024

jrfnl approved these changes Jan 22, 2024

View reviewed changes

rodrigoprimo and others added 4 commits January 23, 2024 00:18

Generic/ForLoopWithTestFunctionCall: rename test case file

e3dcefe

Doing this to create more test case files in subsequent commits.

Generic/ForLoopWithTestFunctionCall: minor doc fix

817a1ef

jrfnl force-pushed the fix-deprecated-error-for-loop-with-test-function-call branch from 1d01107 to 817a1ef Compare January 22, 2024 23:33

jrfnl merged commit e0e03f0 into PHPCSStandards:master Jan 22, 2024

rodrigoprimo deleted the fix-deprecated-error-for-loop-with-test-function-call branch January 23, 2024 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice #235

Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice #235

rodrigoprimo commented Jan 5, 2024 •

edited

Loading

jrfnl commented Jan 5, 2024

rodrigoprimo commented Jan 8, 2024

jrfnl commented Jan 17, 2024

rodrigoprimo commented Jan 17, 2024

jrfnl left a comment

jrfnl Jan 17, 2024

rodrigoprimo Jan 18, 2024 •

edited

Loading

rodrigoprimo commented Jan 18, 2024

jrfnl commented Jan 22, 2024

jrfnl left a comment •

edited

Loading

rodrigoprimo commented Jan 22, 2024

jrfnl commented Jan 22, 2024

Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice #235

Generic/ForLoopWithTestFunctionCall: fix potential PHP 8.3+ deprecation notice #235

Conversation

rodrigoprimo commented Jan 5, 2024 • edited Loading

Description

Suggested changelog entry

Types of changes

PR checklist

jrfnl commented Jan 5, 2024

rodrigoprimo commented Jan 8, 2024

jrfnl commented Jan 17, 2024

rodrigoprimo commented Jan 17, 2024

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl Jan 17, 2024

Choose a reason for hiding this comment

rodrigoprimo Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

rodrigoprimo commented Jan 18, 2024

jrfnl commented Jan 22, 2024

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

rodrigoprimo commented Jan 22, 2024

jrfnl commented Jan 22, 2024

rodrigoprimo commented Jan 5, 2024 •

edited

Loading

rodrigoprimo Jan 18, 2024 •

edited

Loading

jrfnl left a comment •

edited

Loading