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

Multiple use issues when refactoring foreach to collection #20

Closed
squatto opened this issue Nov 4, 2020 · 2 comments · Fixed by #21
Closed

Multiple use issues when refactoring foreach to collection #20

squatto opened this issue Nov 4, 2020 · 2 comments · Fixed by #21
Labels
bug Something isn't working

Comments

@squatto
Copy link
Contributor

squatto commented Nov 4, 2020

I've come across a few situations that cause "Refactor foreach to collection" to not work correctly when it decides which variables need to be in use. I've provided detail on each issue below. Thanks!


Issue 1: missing variable from use

Using the main variable $foo in the loop doesn't lead to it being included in use:

// original
foreach ($foo as $baz) {
    blah($foo, $baz);
}

// refactored
// missing `$foo` variable in `use`
collect($foo)->each(function ($baz) {
    blah($foo, $baz);
});

Issue 2: duplicate variable in use

Adding a property accessor to the main variable $foo causes the main variable to be included twice in use:

// original
foreach ($foo->bar as $baz) {
    blah($foo, $baz);
}

// refactored
// duplicate `$foo` variable in `use`
collect($foo->bar)->each(function ($baz) use ($foo, $foo) {
    blah($foo, $baz);
});

Issue 3: unnecessary variable in use

If you have a property accessor on the main variable $foo but you don't use the main variable in the loop, it is still included in use:

// original
foreach ($foo->bar as $baz) {
    blah($baz);
}

// refactored
// unnecessary `$foo` variable in `use`
collect($foo->bar)->each(function ($baz) use ($foo) {
    blah($baz);
});

If you remove the property accessor from the main variable $foo then this issue doesn't happen:

// original
foreach ($foo as $baz) {
    blah($baz);
}

// refactored
// perfect!
collect($foo)->each(function ($baz) {
    blah($baz);
});
@olivernybroe
Copy link
Owner

This is really awesome!

Thanks a lot, having the before/after is really helpful as it makes it so easy for me to create a test based on it.

@olivernybroe olivernybroe added the bug Something isn't working label Nov 5, 2020
@olivernybroe
Copy link
Owner

olivernybroe commented Nov 5, 2020

I think I got them all fixed now! If you want to try it out before I make the release (will fix the other bug you reported before making the release) you can try it out by manually installing this one
Collector-0.0.1-EAP.6.zip
https://github.com/olivernybroe/collector-intellij#installation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants