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

Add type hint for entity manager #1482

Merged
merged 1 commit into from
Sep 4, 2015
Merged

Conversation

bocharsky-bw
Copy link
Contributor

No description provided.

@DHager
Copy link
Contributor

DHager commented Jul 29, 2015

Looks good, but I've previously had a similar change rejected for being a backwards-compatibility break. (Which is technically is, even if it only breaks code that's already pretty broken.)

@bocharsky-bw
Copy link
Contributor Author

@DHager Could we use ObjectRepository interface for type hint?
I see some tests failed. Maybe we could fix them? Or there are another reasons don't do that?

@stof
Copy link
Member

stof commented Jul 30, 2015

@bocharsky-bw EntityManager is not an ObjectRepository at all

and this change is wrong as it forbids to pass other implementations of EntityManagerInterface

@bocharsky-bw
Copy link
Contributor Author

@stof Sorry, I don't know how could I wrote this.
I mean ObjectManager instead of ObjectRepository.

@stof
Copy link
Member

stof commented Jul 30, 2015

We don't accept any object manager here. It must be an entity manager

@bocharsky-bw
Copy link
Contributor Author

Could we use EntityManagerInterface?

@beberlei
Copy link
Member

Yes EntityManagerInterface could work, my question is why do you want to change this code anyways? I don't see a reason to.

@bocharsky-bw
Copy link
Contributor Author

@beberlei I think it will be clearer, won't it? In PHP we have type hint for objects and it's a good practice to use it I think. Also it will help to find bugs on initial level.

@TomasVotruba
Copy link
Contributor

👍

@Ocramius
Copy link
Member

Ocramius commented Sep 4, 2015

Accepting the change, I thought a bit about possible BC breaks, but PHP doesn't care about constructor override signature changes.

Ocramius added a commit that referenced this pull request Sep 4, 2015
Add type hint for entity manager
@Ocramius Ocramius merged commit 2d00a9b into doctrine:master Sep 4, 2015
@Ocramius
Copy link
Member

Ocramius commented Sep 4, 2015

👍

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.

7 participants