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

Deprecate custom ObjectRepository implementations #9533

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

derrabus
Copy link
Member

Alternative approach to #9531.

Although the documentation never mentions it, it is currently possible to use an implementation of ObjectRepository as repository class that does not extend EntityRepository. A caller of EntityManager::getRepository() will probably not expect not getting an EntityRepository as return value.

This is why I'd like to deprecate that possibility. This will also allow us to safely use EntityRepository as a native return type in 3.0.

@derrabus derrabus force-pushed the deprecate/object-repository branch 2 times, most recently from 2ef0669 to 1563960 Compare February 24, 2022 10:20
SenseException
SenseException previously approved these changes Feb 24, 2022
@derrabus derrabus force-pushed the deprecate/object-repository branch 2 times, most recently from d28e9e1 to 71a70c5 Compare February 25, 2022 09:28
@kbond
Copy link
Contributor

kbond commented Feb 25, 2022

I'd like to voice an objection to this. In my apps, I have my entity repositories implement \Countable. This is not possible when extending EntityRepository because of it's count() method. Edit: I was wrong about this - I can override as count(array $criteria = []).

Still, I have a few concerns:

  1. I'd like to keep the extra EntityRepository public methods out of my public API (ie createQueryBuilder).
  2. I can have my custom ObjectRepository's implement ServiceEntityRepositoryInterface in a way that keeps the constructor clean for injecting additional services (using setter injection with the Required attribute).

Admittedly, these are both minor - my main concern was being able to implement \Countable.

@derrabus
Copy link
Member Author

Indeed, Countable can be implemented by repositories, so that should not hold you back.

Regarding the other concerns:

  • I don't see a big problem with a repository having a createQueryBuilder() method. In fact, somone experienced with the ORM who joins your project might be surprised if your repositories don't have that method.
  • That might be a matter of taste, but imho there's nothing clean about injecting a mandatory dependency in a setter. Symfony's service entity repositories can alaredy have any dependencies you want.

Looking at the alternatives (#9531 mainly), this PR still seems to be the best option to me.

@kbond
Copy link
Contributor

kbond commented Feb 26, 2022

For a bit of context, I've been working on a library (very wip) that provides an opinionated entity repository (iterable, countable, read-only vs writeable implementations). It's optimized for large data sets.

I don't see a big problem with a repository having a createQueryBuilder() method.

Excluding was an attempt to lock user's out of "doing whatever they want" outside of the repo itself.

...there's nothing clean about injecting a mandatory dependency in a setter.

Clean was a poor word choice on my part. My library's "repository" classes are abstract and force you implement ObjectRepository::getClassName() and a protected em(): EntityManagerInterface method. How you do so is up to you but I provide an optional DX helper (that uses the setter injection) to make it easier to use with Symfony.

This change throws a bit of a wrench in my vision but isn't something I can't work around.

@derrabus derrabus force-pushed the deprecate/object-repository branch from 71a70c5 to 06c95ed Compare April 4, 2022 23:43
@derrabus derrabus requested a review from greg0ire April 4, 2022 23:44
@derrabus derrabus merged commit d9e8e83 into doctrine:2.12.x Apr 6, 2022
@derrabus derrabus deleted the deprecate/object-repository branch April 6, 2022 11:51
derrabus added a commit to derrabus/orm that referenced this pull request Apr 6, 2022
* 2.12.x:
  Deprecate custom ObjectRepository implementations (doctrine#9533)
  Fix types on walkLiteral() and walkLikeExpression() (doctrine#9566)
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.

4 participants