[FrameworkBundle] Fix test-container on kernel reboot, revert to returning the real container from Client::getContainer() #27501
Conversation
f73bda1 to
e749e91
Compare
|
|
||
| private function getPublicContainer() | ||
| { | ||
| $this->kernel->boot(); |
There was a problem hiding this comment.
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.
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.
|
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 to
6764d4e
Compare
|
sounds good! Is there going to be accompanying documentation regarding what you (@nicolas-grekas) mentioned about |
|
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()
|
@arderyp the PR introducing the test container made it available in 2 different ways: |
|
awesome! So, for clarity's sake, should be replaced with: Is that right? |
|
This line is not needed: |
|
So no need to |
|
Yes you need it! I only told about the 2nd line. |
|
Sorry, I'm an idiot... read too quickly, I understand now. Thanks again. |
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.