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

getManagerForClass should try to retrieve the default manager #374

Closed
wants to merge 3 commits into from

Conversation

elernonelma
Copy link

getManagerForClass should try to retrieve the default manager instead of the first one found in the list.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-290

We use Jira to track the state of pull requests and the versions they got
included in.

@TomasVotruba
Copy link
Contributor

Could you also provide tests?

@elernonelma
Copy link
Author

The comprehension of the actual test was painful. Not great here (the test itself doesn't test a lot), but it covers the non regression of existing test. If you think something can be done better easily, I will try it but without a refactoring of the actual test, I think it will be hard.

@elernonelma
Copy link
Author

To abort the context of use, I have N managers for my entity, and I choose a default one. I supposed then that this method would return the default one, and not the first one declared. It may have a reason to this?

@BobWeb
Copy link

BobWeb commented Jul 23, 2015

I encounter the same problem as elernonelma. Do you have information about the pull request's validation, because it would really help !

@Ocramius
Copy link
Member

I have N managers for my entity

This assumption seems wrong: the registry should map each entity to a separate manager.

@elernonelma
Copy link
Author

How can it be wrong? My entity must be used by N different databases, handled by N manager.

@Ocramius
Copy link
Member

Yes, but this class assumes that only one manager is used per type of class. The idea is to bridge different domains together via different ORM/ODM instances.

@elernonelma
Copy link
Author

Yes, it is exactly how it works and exactly what i expect it to. But I would expect that it would try to use the default manager first, instead of the first one found in the declaration.

@Ocramius
Copy link
Member

The "default" one is supposed to work as a fallback

@elernonelma
Copy link
Author

I dont expected it to work as a fallback, as it's default I expect it to works as a default. Did not find any information about it. Where did you see that?
Here, it does not works as a fallback.

@Majkl578
Copy link
Contributor

Hello,
we've recently split Persistence component into a separate doctrine/persistence repository.
If your PR is still relevant, please open new one to the new repository.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants