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

Throw exception in getManagerForClass when multiple entity managers manage the same entity #68

Open
alcaeus opened this issue Sep 4, 2019 · 5 comments

Comments

@alcaeus
Copy link
Member

alcaeus commented Sep 4, 2019

This came to me while reading through symfony/symfony-docs#9878. In a few of those cases, people have the same entity managed by multiple entity managers. While I'm not sure whether that constitutes a valid use-case, the manager registry is equipped to handle this by allowing to specify en entity manager to use when calling AbstractManagerRegistry::getRepository.

However, AbstractManagerRegistry::getManagerForClass handles those cases wrong IMO: it simply returns the first entity manager that manages the entity in question. In cases like the above, this can lead to undesired behaviour as we're ignoring the fact that we can't reliably say which entity manager is responsible for.a given class.

Assuming the use-case of having the same entity managed by multiple entity managers is valid, we should throw an exception in getManagerForClass if we realise that the same class is managed by more than a single entity manager.

@malarzm
Copy link
Member

malarzm commented Sep 4, 2019

I'd say that managing a single entity with multiple managers is very invalid and error-prone use case.

@faizanakram99
Copy link

faizanakram99 commented Feb 26, 2024

I'd say that managing a single entity with multiple managers is very invalid and error-prone use case.

How so?

Persistence (Entity manager) is infra detail and infra should always be pluggable, entities otoh are used as domain objects which can very well be reused across different sub-domains.

We've been using a single entity with multiple entity manager for years (with xml configuration) without any problem (mainly because we use custom repositories and don't rely on ManagerRegistry::getManagerForClass() detection)

Here is an example
https://gist.github.com/faizanakram99/8745e6b3fb1411918d0792e6b53b6a9d

Any services needing translation entity/repository rely on TranslationRepository interface and the actual implementation is provided by DI using some strategy. I only want the connection to change based on certain conditions (hence multiple entity managers), rest business logic is the same. Why should I create separate entities when it is really not a domain concern?

@malarzm
Copy link
Member

malarzm commented Feb 28, 2024

How so?

EntityManager needs to track changes in an entity, and to do so it needs entity's original data. If same entity (instance of it to be more precise) is managed by two EMs, and one of them is being flushed, the state in that EM and in database is fine. But the other EM is now inconsistent with in-memory state and also in database.

@faizanakram99
Copy link

How so?

EntityManager needs to track changes in an entity, and to do so it needs entity's original data. If same entity (instance of it to be more precise) is managed by two EMs, and one of them is being flushed, the state in that EM and in database is fine. But the other EM is now inconsistent with in-memory state and also in database.

First of all this issue is about managing two entities (not necessarily same instance but two instances of the same class). Furthermore, each entity manager uses a different database, otherwise what is the point of using different entity managers at all?

@malarzm
Copy link
Member

malarzm commented Feb 29, 2024

First of all this issue is about managing two entities (not necessarily same instance but two instances of the same class).

It's easy enough though. Been there, done that (also with multitenancy).

Anyhoo the issue was talking about throwing an exception in ManagerRegistry, something you say you don't use. Given the issue is 5 years old I don't think we'll be resurrecting it, so let's all return to whatever we were doing ;)

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

No branches or pull requests

3 participants