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

Code hints in foreach #5

Closed
TomMalbran opened this issue Aug 9, 2014 · 3 comments · Fixed by #8
Closed

Code hints in foreach #5

TomMalbran opened this issue Aug 9, 2014 · 3 comments · Fixed by #8
Labels

Comments

@TomMalbran
Copy link

I've been using this extension and saw this bug. I was writing something like: foreach ($fields as $f|, where | is the cursor. I then got some hints at $f and I chose $field. But then it replaced $fields, so I got foreach ($field| instead of foreach ($fields as $field|.

After some testing I saw that this bug only happens when the variable for the array starts with the same letters as the variable for the value. If I have foreach ($array as $v| the codehints do not replace $array when I select something like $value for the second variable. If I have foreach ($fields as $fo| the codehints do not replace $fields when I select something like $foo for the second variable.

@mackenza mackenza added the bug label Aug 9, 2014
mackenza added a commit that referenced this issue Aug 9, 2014
@mackenza
Copy link
Contributor

mackenza commented Aug 9, 2014

@TomMalbran it was actually worse than your test case. Basically any time you did a 2nd replacement on the same line with another word that started the same way, it would get rid of the rest of the line and insert the first occurrence again.

I switched to using lastIndexOf() and it seems to be fine now.

Can you give it a quick test with the issue-5 branch to confirm it works with your test case?

@TomMalbran
Copy link
Author

I thought it might be a bigger issue that just in the foreach, but I didn't tested with anything else.

Using lastIndexOf() seems to fix the issue.
Thanks

@TomMalbran
Copy link
Author

I guess you could also use currentToken.start?

This was referenced Aug 10, 2014
mackenza added a commit that referenced this issue Aug 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants