Skip to content
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

Hotfix: correct array indices #2746

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

michalbundyra
Copy link
Contributor

I've changed the approach completely. In my opinion it much clearer.
Added tests to cover the issue described in #2745.

Fixes #2745

It solves also the problem when we had ["value_start" => false] for an empty array.

I've changed the approach completely. In my opinion it much clearer.
Added tests to cover the issue described in squizlabs#2745.

Fixes squizlabs#2745

It solves also the problem when we had `["value_start" => false]` for
an empty array.
@gsherwood gsherwood added this to the 3.5.4 milestone Dec 8, 2019
@gsherwood
Copy link
Member

This is quite a large refactor that has removed a fair bit of code. Can you provide a bit of an explanation about what changes have been made and why?

@michalbundyra
Copy link
Contributor Author

@gsherwood I just wrote it from scratch instead of "fixing" the old code.

There are two functions now: one which is finding all indices in the array and the other (getNext) which returns the next "stop character" in array - and it is either comma or double arrow.
In this method we skip all scopes - like closures, array functions, anonymous classes, nested arrays, etc... and we return the ptr of next comma/double arrow or the end of the array.

If you read just new code you will see it is all pretty clear, and we don't need all these old code, imo.
As you can also see, all tests are fine :)

@VincentLanglet
Copy link
Contributor

@michalbundyra I just wrote a Sniff checking for comma and I added the comma in the indices here #2759.

How can we add the commas with your refactor ?

@michalbundyra
Copy link
Contributor Author

@VincentLanglet It would be easy to add it here. I am not doing it as it is out of the scope right now and PR #2759 is going to be changes (as I understand from comments there).

@gsherwood
Copy link
Member

@gsherwood I just wrote it from scratch instead of "fixing" the old code.

Refactoring a sniff is fine, but I can't determine what the actual fix is here. I can't accept this PR as a bug fix, so I'll remove it from the 3.5.4 milestone.

@gsherwood gsherwood removed this from the 3.5.4 milestone Jan 12, 2020
@gsherwood
Copy link
Member

Quick update: I'm working on writing tests for the AbstractArraySniff indices processing to expose the original bug. If I get a comprehensive enough suite of tests for it, I'll come back and review this refactor for inclusion.

@gsherwood gsherwood added this to the 3.5.4 milestone Jan 14, 2020
gsherwood added a commit that referenced this pull request Jan 14, 2020
Includes fix for unit test
@gsherwood gsherwood merged commit 45f70b2 into squizlabs:master Jan 14, 2020
@gsherwood
Copy link
Member

After adding some core tests for this sniff, I've been able to verify that this refactor is working correctly. Thanks a lot for this PR. This sniff is much cleaner now, and also fixes #2745.

@michalbundyra michalbundyra deleted the hotfix/array-indices branch January 14, 2020 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbstractArraySniff wrong indices when mixed coalesce and ternary values
3 participants