-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inconsistent behaviour findPrevious() vs findNext() #1319
Comments
FYI: I've opened PRs for both options. Only one of the two should be merged. |
Either would be considered a BC break and would require all sniffs (core and custom) to be changed make use of it. I'm for inclusive option (the
|
Not necessarily - for the core sniffs, I've already made the pertinent changes needed and included them with the PRs for this issue. Turned out only a few sniffs really needed changing to account for the change (at least based on the unit test results). To be very specific: For both options, usage of the For option 1:Any usage of For example: // Test code snippet
function foobar(){} $next = $phpcsFile->findNext(T_WHITESPACE, ($tokens[$stackPtr]['close_parenthesis'] + 1), $tokens[$stackPtr]['scope_opener'], true); The above returns if ( $next === false) { // <- this would be problematic
return;
}
if ( $next !== false && $tokens[$next]['code'] === T_SOMETHING) { // <- this would be fine
// do something;
} For option 2:Any changes which option 1 necessitated would need to be applied. Oh and of course, any work-arounds for this issue can be removed |
My intention was actually to have these methods not be inclusive. If you are looking for a token before/after a specific one, and you want to stop early, I didn't think the stop token should be included. Obviously, I've accidentally coded these differently. So a bigger question is: should findPrevious() actually be changed to match findNext()? I could be swayed either way - it's just that findNext() was written first and I tend to think of them both working in that way. I'd like to know what others think. Whatever happens, they should be made consistent. But it is a BC break and I'd like to keep it in 3.0 if possible. I'm trying (trying) to keep the BC breaks down for sniff developers in 2.x because it's getting towards the end, and 3.0 is a big BC break in itself. |
In my opinion they should (obviously) both be inclusive. For sniff devs to always have to remember to add Regarding the BC break - I'd like to suggest the following: what about including option 1 in the last 2.x release and option 2 in the 3.x branch ? Option 2 is the real BC break, option 1 is more a "plaster to stop the bleeding" and will more than anything just prevent those infinite loops. The branch against which the PRs are pulled can be changed easily, so let me know if there's anything I can do in that regards and/or regarding a more in-depth review of the potential BC-break cases in the sniffs which are natively included. |
👎 for this, because:
Custom standard developers like myself can't guarantee that everyone using the standard would have specific phpcs version installed. |
Sorry, should have read "one of the last"
As I outlined, that is only for Option2 which wouldn't go in the 2.x branch
I completely concur, that's why I proposed the two step approach with the real BC break only in v3 |
If no BC break (even minor one), then I'm OK with merging to 2.x. |
@aik099 There is only a very very very minor BC break with Option 1 as outlined in more detail in #1319 (comment) Conditions in which Option 1 would constitute a break:
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#8 |
I've been tripped up by this a number of times already, so putting this out here for consideration.
If the
$start
and$end
token positions you pass tofindPrevious()
are the same, the function will work as expected. However, the same can not be said about thefindNext()
method as in that case it will always returnfalse
.This is caused by the
findPrevious()
method treating the end position as inclusivefor ($i = $start; $i >= $end; $i--)
, while thefindNext()
method treats it as exclusivefor ($i = $start; $i < $end; $i++)
.There are two possible solutions for this:
$start
and$end
being the same and adding 1 to the$end
in that case before entering the loop. This will lead to the least difference with current behaviour. - PR Issue #1319: findnext inconsistency, option 1 #1320findNext()
inclusive as well. This might lead to more breakage, but would make the functions more consistent in use. - PR Issue #1319: findnext inconsistency, option 2 #1321The text was updated successfully, but these errors were encountered: