-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fixed entity manager resetting #7
Conversation
Btw, I employed the solution mentioned here: dflydev/dflydev-doctrine-orm-service-provider#61 (comment) . I know that defining a orm.ems.factory key into the container should be implemented in the DoctrineOrmServiceProvider, but that project seems to be dead and hopefully this one is not. Thus the quickest fix is this one and if it happens that the original project implements it, there's an if that avoids redefining the factory if it's already defined. |
@dominikzogg bump |
/** | ||
* @var EntityManager[] | ||
*/ | ||
protected $resetManagers = []; |
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.
Hello @ghola
Thank you for your pull request. And the passion.
The only thing that i do not understand why you need originalManagers and resetManagers. I looks more complex to me as it used to.
Regards @dominikzogg
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.
Hi there,
The previous $mangers member was not an array as the docblock suggests but a container (because $this->managers = $this->container['orm.ems'];
and the DoctrineOrmServiceProvider
defines 'orm.ems'
as being $ems = new Container(); ... return $ems
). Since container services are frozen once they get instantiated once, it means that I was not able to replace the old service with a new one when needing to reset the service.
Making the previous $manages member be an actual array instead of a container was not a solution because that would have instantiated all the EMs from the 'orm.ems'
container which defeats the purpose of a container.
So my solution was to leave the old $managers member as a container and rename it to $originalManagers and add the $resetManagers and when serving an EM I would pull from these giving priority to the $resetManagers member.
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.
Hello @ghola
Makes sense, does it work resetting the same manager multiple times? I can't see a test for this, thats why i ask.
Regards @dominikzogg
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.
@ghola thank you for your contribution. if that get solved within the orm provider itself, we should look into this again. |
@dominikzogg thank you for the merge and the new version tag. |
@ghola your welcome |
Fixes issue #6.
Since pimple services get frozen once they have been fetched, actually setting a new EM into the container to replace the old one is impossible. The solution was to maintain a separate list of reset EMs into the registry and serve from that list when necessary.