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

Close connections on shutdown #415

Merged

Conversation

supersmile2009
Copy link
Contributor

This PR addresses issue #400.
SncRedisBundle::shutdown() function will be called on kernel shutdown in-between unit tests. The same solution is implemented in DoctrineBundle.

I also had to declare snc_redis.session.handler as a public service to be able to fetch it from container in Symfony 4+.

@curry684
Copy link
Collaborator

Solution is fine, but I don't think we should be making a service public just for testing purposes. Don't we have some way to do that better?

@supersmile2009
Copy link
Contributor Author

This is the most elegant and simple solution as to my knowledge.

There is a way to fetch private services from container in tests env, but it's Symfony 4.1 feature: link1, link2

There is a service resetting feature in Symfony 3.4+ symfony/symfony#24155, but it runs on kernel.terminate event, which is not the case with kernel shutdown.
We could reproduce similar feature here by extending a listener from above mentioned Symfony's PR, adding a compiler pass for it, adding some kind of 'kernel.shutdown' event. But it will require people using this bundle to override Symfony\Bundle\FrameworkBundle\Test\KernelTestCase::ensureKernelShutdown() in their tests to dispatch that event in order to benefit from that feature. Too much effort with not a really good result.

The perfect solution would probably be adding existing service resetting feature into kernel shutdown - but that's an issue to be discussed in Symfony repo. And I guess it's going to be Symfony 4.1 feature, I don't think they'll merge it into 4.0.

@dkarlovi
Copy link

dkarlovi commented May 3, 2018

You can add an env-specific service and prefix it with test..

@curry684
Copy link
Collaborator

curry684 commented May 7, 2018

I'll merge this into 2.1 but keep #400 open so we can fix it better in 3.x.

@curry684 curry684 merged commit 1fc324a into snc:2.1 May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants