-
Notifications
You must be signed in to change notification settings - Fork 103
Add integration tests for ODM features #39
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 integration tests for ODM features #39
Conversation
4facb3f
to
02e81e7
Compare
The methods `EntityManagerInterface#getReference()` and `EntityManagerInterface#getPartialReference()` have nullable return. That's properly documented for `getReference()` but had to be fixed for `getPartialReference()`.
So that things would be compatible with both ORM and ODM.
With the necessary dependencies on Travis-CI to ensure the build will run properly. Following the same approach as https://github.com/doctrine/mongodb/blob/1.6.3/.travis.yml#L61 (for PHP 7+ installation).
02e81e7
to
af7bb42
Compare
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.
This PR is incomplete: there needs to be more distinction as to the types returned. For example, the repository extension always returns an EntityRepository
type, even for ODM which isn't correct. See #36 for a more complete implementation. I would suggest combining the tests from this PR with the changes from #36. BC preserving changes (e.g. not removing the Entity*
classes can be dropped from the other PR. Opinion @ondrejmirtes?
Hi, I'm not able to try this out because of:
I think it shouldn't be necessary to have installed |
Because of the tests: they need to import ODM classes. You could remove the dependency, skip the tests if ODM isn’t installed and only install it on travis. However, as changing something might break ODM functionality, I would recommend to keep this as is. This extension is supposed to cover ORM and ODM, so it’s expected to have both as a dev dependency. |
Is it just because of some |
All the tests run fine if I remove |
Correct, because you're pretending that the |
Superceded by #48. |
Which kicks-off the support for #26
Depends on #38