Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer()#27501
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f73bda1 toe749e91Compare| privatefunctiongetPublicContainer() | ||
| { | ||
| $this->kernel->boot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Haven't we made a change in 4.1 implying that booting an already booted kernel is not a no-op ? That's would be a bad side-effect then here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I reverted the side-effect so this should be OK, but to be safe and strict, I replaced this call by an exception when the container is called while the kernel is not booted.
stof commentedJun 5, 2018
Btw, this also solves the issue we faced in#27037 as the only way to access private services now is through a new API (so any 3.4 deprecation is an actual one) |
…rning the real container from Client::getContainer()
e749e91 to6764d4eComparearderyp commentedJun 6, 2018
sounds good! Is there going to be accompanying documentation regarding what you (@nicolas-grekas) mentioned about |
pamil left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me, fixes the BC break and works with Sylius :)
fabpot commentedJun 6, 2018
Thank you@nicolas-grekas. |
…ert to returning the real container from Client::getContainer() (nicolas-grekas)This PR was merged into the 4.1 branch.Discussion----------[FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer()| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | -| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27494| License | MIT| Doc PR | -By making `Client::getContainer()` return the new test container, we broke BC, as spotted in linked issue.Always use `static::$container` in your tests instead.While reverting to returning the real container, I noticed we have a serious design issue in the way the test container currently works: because the kernel can be rebooted, we cannot inject the container directly, but have to go through the kernel all the time. Fixing this forces doing a BC break on the constructor of `TestContainer`. Since this is a new class and since it's mostly internal, I think we should do it now. I've marked the class as internal to further strengthen this.Commits-------6764d4e [FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer()
stof commentedJun 6, 2018
@arderyp the PR introducing the test container made it available in 2 different ways: |
arderyp commentedJun 6, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
awesome! So, for clarity's sake, should be replaced with: Is that right? |
nicolas-grekas commentedJun 6, 2018
This line is not needed: |
arderyp commentedJun 6, 2018
So no need to |
nicolas-grekas commentedJun 6, 2018
Yes you need it! I only told about the 2nd line. |
arderyp commentedJun 6, 2018
Sorry, I'm an idiot... read too quickly, I understand now. Thanks again. |
Uh oh!
There was an error while loading.Please reload this page.
By making
Client::getContainer()return the new test container, we broke BC, as spotted in linked issue.Always use
static::$containerin your tests instead.While reverting to returning the real container, I noticed we have a serious design issue in the way the test container currently works: because the kernel can be rebooted, we cannot inject the container directly, but have to go through the kernel all the time. Fixing this forces doing a BC break on the constructor of
TestContainer. Since this is a new class and since it's mostly internal, I think we should do it now. I've marked the class as internal to further strengthen this.