-
- Notifications
You must be signed in to change notification settings - Fork 5.3k
New cookbook recipe stub: Turn Doctrine repository into service #3825
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
Conversation
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.
$logger
instead on $Logger
Changed $Logger to lowercase. |
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.
How to Turn Your Doctrine Repository into a Service
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.
your
must be lowercased to, mustn't it?
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.
hmm, indeed. I think so
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.
Though, I'm not absolutely sure. Let's see, what @weaverryan thinks about it.
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.
Hm, according to http://www.audioenglish.org/dictionary/closed-class_word.htm (possessive) pronouns are closed-class words.
Thank you for posting such an article! It looks great for a first doc PR! As you can see, I've decorated your PR a bit with some comments. Don't worry, most are small fixes or little improvements. If you have questions about one of them, don't hesitate to ask :) Can you please add the PR template to your PR description? And for the next time, this is not about a new feature, so it should be merged into the 2.3 branch (the oldest still maintained branch). You don't have to worry about it now, since we can easily cherry pick this in 2.3, but it may cause problems in your next PR. At last, you should also add a reference to this new document in |
👎 please remove all reference to optional dependency via setter injection since this is only about repositories, optional setter injection is a contested practice atm |
@wouterj, where do I need to add the PR template you've mentioned? |
He means in the PR itself, http://symfony.com/doc/current/contributing/documentation/overview.html#pull-request-format It should be noted that this cookbook entry introduces a problem for certain ways of using doctrine: these repository services become invalid if the entity manager becomes closed. @stof has previously made comments on such suggestions indicating it is a bad practice and that repositories should be retrieved at use time from the registry. In most normal apps this isn't really a problem but exposes developers to invalid services that can't be recreated if they use |
there is also another issue: if some code calls |
interesting points @stof and @merk i believe we should add a note on the docu for this warning, i know it is doctrine but symfony using doctrine should do it. Also for instance doctrine configuration allows for a customized default entity repository, if you start injecting stuff there I believe it breaks because i doubt the factory supports it. So this approach fails and makes the user use setter injection and start to fall into these bad practices. Could you comment on the alternatives? thanks |
@cordoval what sort of dependencies are you injecting in your Doctrine repositories ? I'm asking this because I never injected extra dependencies in my repositories so I miss a real use case |
I've had to do it a single time, and instead of injecting it it became a parameter for the function call. It ended up getting moved into its own service after I realised what I was doing. @cordoval the alternative is to inject the registry into your services that need repositories, and get the repositories on demand (dont set them in the constructor). |
Now while I do understand the disadvantages of injecting services into the repository, what is a clean way to get (config-)parameters in there? Passing the options on every method call surely works but I doubt it is intuitive to do it that way. |
@zebba The idea would be that if you need to do something that requires some extra services or parameters that your repository doesn't have, you have 2 options:
Here's an example. Imagine you have a method So, instead of trying to get that service into your repository, just create a new service, for example: public function findAllEditableBlogPosts() { if ($this->securityContext->isGranted('ROLE_ADMIN')) { return $this->postRepository->findAll(); } else { // using a findAllOwnedByUser method you'd write // this also assumes the user is logged in - you may want to check for that :) $user = $this->securityContext->getToken()->getUser(); return $this->postRepository->findAllOwnedByUser(); } } For me, this is the right way to solve this, and I think most people agree. So, should we add something in the docs about this? A small cookbook entry on what to do when you need access to a service/parameter inside an entity repository? |
I have a problem. When I do |
You can't change that. Doctrine will always use the default repository when the repositoryClass isn't set, that's just how Doctrine works. However, these things should be discussed in the mailing list, on Stackoverflow or in IRC, it has nothing to do with this PR or the Symfony docs. |
Sorry for that. I didn't try to resolve an issue but to probe this recipe. I think that the explanation is confusing, because when I read |
I'm closing this - as I mentioned before, turning repositories into services via this method is not the right approach. You should turn them into services using the factory-method approach: http://stackoverflow.com/questions/17228417/symfony-2-creating-a-service-from-a-repository Thanks! |
Even using factory-service/factory-method will still result in the problems described above. I do like the look of the compiler pass that replaces the repository factory inside doctrine, and an approach using such a method could work. |
@merk which problem are you referring to? If i use a factory-service that points to the entity manager and a factory-method that points to |
What happens to an instance of a repository registered as a service when your entity manager becomes closed? The entire problem relating to repositories as services is that they cant be services at all - that you cant trust you retain a reference to a valid EntityManager |
@merk That's legitimate. It is a bit of an edge case (I use repos as services and have never hit this, though I logically see how it could happen), but definitely legitimate. It of course reminds me of scopes and the Anyways, it's an interesting issue, and I'm not sure what's best. We could say: always inject the entity manager entirely, even though it makes unit testing a little bit harder and you'll violate the law of demeter. Or we could say, if you care enough, register services via factory-service, which will work 99% of the time. I'm not sure what's best, so I don't feel too compelled yet to make any big changes in the docs, but it's an interesting conversation I had never really thought-through. |
You actually need to inject the ManagerRegistry, not the EntityManager :\ And yes - I've got a large project with repositories as services and only encountered this issue once - the only time it is really an issue is if you're going to continue a request after an exception inside Doctrine, which doesnt happen that often. It is however, something that IMO should be documented clearly. |
The only actual use case I found for resetting the entity manager is when writing a long-running process, which should be able to recover from errors to handle the next iteration (my use case is a RabbitMQ consumer). The proper fix would actually be to allow resetting an existing EntityManager entirely in Doctrine instead of requiring to create a new one. But this could be quite complex to implement in Doctrine in a BC way (i.e. being able to have it without waiting for 3.0) |
I remember stumbling upon some blog post that said: well, just make the repository a service and you'll be good. Somebody else will find this surely usefull.
Since I am not experienced with the rst-format yet the code sections could need some love.
Kind regards.