Skip to content

Conversation

@alcaeus
Copy link

@alcaeus alcaeus commented Aug 6, 2018

The ObjectManager interface is shared across multiple projects, so assuming every object manager returns an entity repository is wrong and breaks using the extension with MongoDB ODM.

In addition to that, I would suggest renaming the generic extensions to ObjectManager... since they affect all object managers, not just an EntityManager. I wasn't sure if that would be considered a BC break so I left the refactoring out. Would you be open to renaming the classes?

The ObjectManager interface is shared across multiple projects, so assuming every object manager returns an entity repository is wrong and breaks using the extension with MongoDB ODM.
@ondrejmirtes
Copy link
Member

Hi, the fact that this stops working is a bigger BC break than the extension renaming 😊

Anyway, this goes against #17. It would be better to send a PR that has ObjectManager returning ObjectRepository and EntityManagerInterface returning EntityRepository.

@alcaeus
Copy link
Author

alcaeus commented Aug 6, 2018

It would be better to send a PR that has ObjectManager returning ObjectRepository and EntityManagerInterface returning EntityRepository.

ObjectManager already returns ObjectRepository: https://github.com/doctrine/persistence/blob/af1ec238659a83e320f03e0e454e200f689b4b97/lib/Doctrine/Common/Persistence/ObjectManager.php#L108.

@ondrejmirtes
Copy link
Member

But I mean here, in the context of the extension, equivalent of this type: https://github.com/phpstan/phpstan-doctrine/blob/master/src/Type/Doctrine/EntityRepositoryType.php

@alcaeus
Copy link
Author

alcaeus commented Aug 6, 2018

Forgive my ignorance, but why? Wouldn't PHPStan normally read it from the Docblock, so no additional type handling is necessary?

@ondrejmirtes
Copy link
Member

The additional type is there for knowing what findBy etc. methods return: https://github.com/phpstan/phpstan-doctrine/blob/master/src/Type/Doctrine/EntityRepositoryDynamicReturnTypeExtension.php

@alcaeus
Copy link
Author

alcaeus commented Aug 6, 2018

Now I got it - I thought you wanted an equivalent of EntityManagerGetRepositoryDynamicReturnTypeExtension for ObjectManager - I'll create an ObjectRepositoryDynamicReturnTypeExtension to handle ObjectRepository return types 👍

@alcaeus alcaeus deleted the fix-odm-discrepancies branch August 6, 2018 09:29
@ondrejmirtes
Copy link
Member

You will need to create:

  • ObjectRepositoryDynamicReturnTypeExtension
  • ObjectManagerGetRepositoryDynamicReturnTypeExtension
  • ObjectRepositoryType

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.

2 participants