Add dynamic return type extensions for getRepository() methods

[tsufeki]

Extract repository class from @Entity annotation and use it as return type of getRepository() methods on EntityManager and ManagerRegistry.

[afurculita]

Neat. IMO this can be merged. Ping @ondrejmirtes

[ondrejmirtes]

Hi, my current stance is that this does not work very well because of two things:

1. It reads and parses annotations above an entity class, but what about other metadata drivers? This should use a completely different approach, asking configured instances of Doctrine classes.
2. Determining that the return type of a getRepository call is not very useful itself, because the main thing that the developer wants is that the getRepository(Xxx::class)->findOneBy(...) returns Xxx|null, which is not yet possible because of missing generics support in PHPStan.

Please discuss whether I'm right or wrong and why should I merge this despite these points. Thank you.

[bendavies]

@ondrejmirtes my usecase would be fixing getRepository(Xxx::class)->findXXX where findXXX is a custom method on my repository.

[ondrejmirtes]

@bendavies Yep, I understand, but your solution does not always work because of point 1.

At least for now I added support for correct return type from getRepository(Xxx::class)->find* methods.

[afurculita]

@ondrejmirtes you're completely right about 1. My use case is like the one from @bendavies : I have custom methods in repositories, but they do not necessarily start with find. Something like getAllUsersWithSomething.

[ondrejmirtes]

@afurculita I understand but that's much harder to solve. It'd mean that PHPStan would have to know about part of dependency graph of your application in order to initialize RepositoryFactory, class metadata etc.

I recommend you to access your repositories not through $em->getRepository() but injecting the concrete class directly:

/** @var ArticleRepository */
private $articleRepository;

It will lead to a better code not only for PHPStan, but for your tests as well, you won't have to mock as much.

[koftikes]

Hello,
I tested this solution and its look good.
Do you planning apply pull request?
Thank you in advance.

[Ocramius]

Looks wrong to me: the return type of getRepository() is rigid, and if something else is needed, just avoid using getRepository().

I know we added a repository factory and customization to doctrine internals, but that was our mistake that we should fix.

[ondrejmirtes]

Besides the point #1 from my comment here, if you're in need of this extension, you shouldn't call $em->getRepository(), but inject the specific EntityRepository (or a child class) in your class directly via dependency injection, as @Ocramius said.

BTW: I'm not a fan of extending EntityRepository at all. It encourages huge god objects with many methods because of 1:1 mapping between repositories and entities. I like much more specific repositories for specific use-cases of an entity. For example one repository for getting translations, another repository for tree manipulation etc.

So I reserve my right to be opinionated and close this PR.

[soullivaneuh]

Can this PR be reconsidered as an option of the plugin?

I fully understand that we should not have lot of custom repositories and the fact that getRepository "is" rigid, but it is not and this is working like that.

At least for old codebase following old conventions, having this PR possible with an option would be a real plus IMHO. 👍

[ondrejmirtes]

@soullivaneuh If someone finished #18, I'd merge it. The advantage of the more recent PR is that it uses class metadata and is not hardcoded to read annotations.

[curry684]

@tsufeki any idea why this doesn't also catch #27? I have all annotations in place.

Edit: duh didn't see this never got merged.

[ondrejmirtes]

This is completely unrelated and also this code wasn’t merged.

On Sat, 19 May 2018 at 11:41, Niels Keurentjes ***@***.***> wrote: @tsufeki <https://github.com/tsufeki> any idea why this doesn't also catch #27 <#27>? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuNRW2HgM_1cenRwUjUpJpL9KxOyUks5tz-jDgaJpZM4PQOqE> .

-- Ondřej Mirtes