Improve compatibility with framework based applications #440
Conversation
There was a problem hiding this comment.
Thanks for reporting & fixing the issue!
I however doubt the current change will be enough: it does reduce the likelihood of a conflict but it's easy to miss & forget about it and introduce back the issue. I added a comment about another possible approach which I hope is simpler
src/Command/InfectionCommand.php
Outdated
| @@ -170,29 +170,29 @@ protected function configure(): void | |||
|
|
|||
| protected function execute(InputInterface $input, OutputInterface $output) | |||
| { | |||
| $container = $this->getContainer(); | |||
| $infectionContainer = $this->getContainer(); | |||
There was a problem hiding this comment.
I don't think the renaming is necessary here
src/Command/InfectionCommand.php
Outdated
| { | ||
| $bootstrap = $config->getBootstrap(); | ||
| $bootstrap = $infectionConfig->getBootstrap(); |
There was a problem hiding this comment.
maybe instead of changing names here the bootstrap file should be included in its own scope:
$bootstrap = $config->getBootstrap();
(function ($_file) {
// Include the file in a separate scope to avoid variable shadowing
require_once $_file;
})($bootstrap);
There was a problem hiding this comment.
Or you can unset($config) before the inclusion.
There was a problem hiding this comment.
I think a closure will be safer to use: you don't have any change to do if you add a variable or parameter; but yeah ultimately both works so let's pick one and be done with it :)
* Rename config variable to avoid naming collisions * Rename container variable to avoid naming collisions
maks-rafalko
force-pushed
the
fix-config-naming
branch
from
70c65ea
to
e397752
Compare
maks-rafalko
force-pushed
the
fix-config-naming
branch
from
e397752
to
017a9b6
Compare
|
I've reverted (hope @patrickfunke you don't mind) all variables renaming and added anonymous function as was suggested. Also updated a test to proof there was an issue. @infection/core please review again |
|
thank you @patrickfunke |
* Improve compatibility with framework based applications * Rename config variable to avoid naming collisions * Rename container variable to avoid naming collisions * Require bootstrap file in the separe scope to avoid variables conflicts
|
Please excuse that I didn't react - I was on vacation. I definitely like the solution, thanks for fixing the issue so quickly! |
|
No worries. I'm so glad you didn't check github notifications during vacation and didn't do any programming. This is something I should also try when I have one :) |
When using a custom bootstrap/autoloader, this file is directly required by the InfectionCommand. Applications to be tested may also use the variable names $config and $container for their own configuration and dependency injection. This will cause either the application or Infection to use the wrong config/container. With a unique name, this collision is avoided.
This PR: