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

Update DefaultRepositoryFactory.php #1159

Closed
wants to merge 1 commit into from
Closed

Update DefaultRepositoryFactory.php #1159

wants to merge 1 commit into from

Conversation

alexanderwink
Copy link

getRepository() does not support different databases for the same entity class due to the lookup in repositoryList. By adding the database name (or any other unique name for the specific database/connection) the lookup works for identical entities in different databases.

getRepository() does not support different databases for the same entity class due to the lookup in repositoryList. By adding the database name (or any other unique name for the specific database/connection) the lookup works for identical entities in different databases.
@Ocramius
Copy link
Member

We don't support multiple DBs per single EntityManager unless you abstract them away into a single aggregate Connection instance.

The edit looks invalid to me.

@alexanderwink
Copy link
Author

Sorry. Not multiple DBs per EntityManager. Multiple EntityManagers.

$tRepo = $app["orm.ems"][0]->getRepository("some\namespace\Transfer");

$tRepo2 = $app["orm.ems"][1]->getRepository("some\namespace\Transfer");

The second repo will be the same as the first one due to the if-statement on (original) line 46.

@Ocramius
Copy link
Member

Seems more like a hack to me, as we do not have a reliable way to differentiate EntityManager instances except for their object hash

@alexanderwink alexanderwink deleted the patch-1 branch October 13, 2014 11:09
@Ocramius
Copy link
Member

@akexakex why not just using the spl_object_hash($entityManager) as actual index prefix?

@stof
Copy link
Member

stof commented Oct 13, 2014

In this case, I think you should use a separate repository factory for each entity manager

@Ocramius
Copy link
Member

@stof the problem still stands: the API doesn't protect from misuse (and it should)

@stof
Copy link
Member

stof commented Oct 13, 2014

@Ocramius reusing the same instances for several entity managers will break in many other places as well (I'm not even sure it is safe to reuse the same Configuration instance)

@Ocramius
Copy link
Member

I'm not even sure it is safe to reuse the same Configuration instance

That's exactly why it exploded in this case. I think we should provide a fix :-)

@alexanderwink
Copy link
Author

Some background info:
There are two identical databases. They differ only in content due to regulatory requirements.

I configure two EntityManagers, one for each database. I then get a repository (entity is called Transfer) from each EntityManager.
How is this considered misuse? What would the appropriate use be?

Personally I easily resolved this by implementing my own RepositoryFactory. I just wanted to inform you of a potential bug.

@Ocramius
Copy link
Member

I'm working on a cleaner fix+test.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 13 October 2014 18:47, akexakex notifications@github.com wrote:

Some background info:
There are two identical databases. They differ only in content due to
regulatory requirements.

I configure two EntityManagers, one for each database. I then get a
repository (entity is called Transfer) from each EntityManager.
How is this considered misuse? What would the appropriate use be?

Personally I easily resolved this by implementing my own
RepositoryFactory. I just wanted to inform you of a potential bug.


Reply to this email directly or view it on GitHub
#1159 (comment).

@stof
Copy link
Member

stof commented Oct 13, 2014

@akexakex you should not share the same Configuration instance between both EntityManager instances. doctrine does not take this case into account currently

@Ocramius
Copy link
Member

@stof I actually have few systems relying on this as well, so I'll have to go through the different use-cases here.

@alexanderwink
Copy link
Author

I have two configuration instances as well. The problem is with the default RepositoryFactory implementation which uses an internal lookup-array that only uses the Entity as key.

@stof
Copy link
Member

stof commented Oct 13, 2014

@akexakex are you sharing the same RepositoryFactory instance between your 2 configurations ?

@Ocramius
Copy link
Member

It is shared by default

@stof
Copy link
Member

stof commented Oct 13, 2014

@Ocramius this will not share it between different Configuration instances

@stof
Copy link
Member

stof commented Oct 13, 2014

and your link is not even related to the RepositoryFactory

@alexanderwink
Copy link
Author

Yes the RepositoryFactory is shared.

@stof
Copy link
Member

stof commented Oct 13, 2014

@akexakex as I said previously, doctrine does not support the sharing between entity managers properly for its stack yet. You should use 2 instances for now

@Ocramius Ocramius self-assigned this Oct 13, 2014
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…toryFactory` caches instantiated repositories locally
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…toryFactory` considers custom repository class from metadata when instantiating repositories
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…ould create different repositories for different entity managers
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…eps separate caches per entity manager used to build repositories
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…Factory` API by making it `final` and its `protected` members `private`
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
…y\DefaultRepositoryFactory` becoming `final`
Ocramius added a commit to Ocramius/doctrine2 that referenced this pull request Oct 13, 2014
@Ocramius
Copy link
Member

See #1160

deeky666 added a commit that referenced this pull request Oct 19, 2014
…anagers-per-repository-factory

#1159 - multiple entity managers per repository factory should be supported
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.

None yet

3 participants