[FrameworkBundle] Fix service reset between tests #58240
Conversation
...rameworkBundle/Tests/Functional/Bundle/TestBundle/TestServiceContainer/ResettableService.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/TestServiceContainer/services.yml
Outdated
Show resolved
Hide resolved
fad3d3e to
549ada5
Compare
| if ($container instanceof ResetInterface) { | ||
| if ($container->has('services_resetter')) { | ||
| $container->get('services_resetter')->reset(); | ||
| } elseif ($container instanceof ResetInterface) { | ||
| $container->reset(); |
There was a problem hiding this comment.
$container->reset() must still be called, to reset the instances hold by the container itself (see the code of line 302).
Btw, if you ensure that the services_resetter service is instantiated, the container reset call will then call it (as ServiceResetter implements the ResetInterface).
The reason it was not called is because Container::reset only resets instantiated resettable services
There was a problem hiding this comment.
@stof Fixed, see https://github.com/symfony/symfony/compare/549ada5257c771da92d51aff76e224e1ac5d31f4..5491aa54a3205acfaf5ba62c134bddc26d794a36
Initially, I wanted to just instantiate the services_resetter and let Container::reset() call reset on it, but that didn’t work because the services_resetter only resets instantiated services. Because of line 302, by the time it’s called, there aren’t any instantiated services left.
The downside of the current approach is that some services will get reset twice, but I don’t see a better solution at the moment.
There was a problem hiding this comment.
ah indeed. The container resets itself before resetting the services.
I think this might actually be an issue if the reset method of a service triggers the instantiation of some other service. I think the resetting of the container state should probably be moved after the loop.
There was a problem hiding this comment.
5491aa5 to
53d0f56
Compare
53d0f56 to
fb1ae1a
Compare
|
Isn’t the test failure on PHP 8.2 related? |
@xabbuh Yes, but that's high-deps and if I'm not mistaking, that'll get resolved when merged upstream. |
|
Thank you @HypeMC. |
…nel is shut down (jderusse) This PR was merged into the 5.4 branch. Discussion ---------- [FrameworkBundle] Do not access the container when the kernel is shut down | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT #58240 play with the container after the kernel was shut down. This is an issue when the kernel performs a cleanup operation. For instance, the `@Nyholm` TestBundle [deletes cache files](https://github.com/SymfonyTest/symfony-bundle-test/blob/241bf0e2f00f28f7327113570ab20e24a5828829/src/TestKernel.php#L184-L199) as a result, the service is not accessible anymore and triggers errors. This PR move the call to `container->get` before `kernel::shutdown` Commits ------- e68f88c [FrameworkBundle] Do not access the container when the kernel is shut down
Currently, not all services are reset between tests. If a service uses the kernel.reset tag but does not implement the ResetInterface, it never gets reset.