Skip to content

Conversation

zebba
Copy link
Contributor

@zebba zebba commented May 2, 2014

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.

Q A
Doc fix? no
New docs? yes
Applies to all
Fixed tickets
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$logger instead on $Logger

@zebba
Copy link
Contributor Author

zebba commented May 3, 2014

Changed $Logger to lowercase.

Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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.

@wouterj
Copy link
Member

wouterj commented May 3, 2014

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 components/map.rst.inc, otherwise it won't show up in the list of cookbook articles.

@cordoval
Copy link
Contributor

cordoval commented May 5, 2014

👎 please remove all reference to optional dependency via setter injection since this is only about repositories, optional setter injection is a contested practice atm

@zebba
Copy link
Contributor Author

zebba commented May 5, 2014

@wouterj, where do I need to add the PR template you've mentioned?

@merk
Copy link
Contributor

merk commented May 5, 2014

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 $registry->resetService().

@stof
Copy link
Member

stof commented May 5, 2014

there is also another issue: if some code calls $em->getRepository('AcmeBlogBundle:Post') before the first place where the service is retrieved, it will receive the repository without the extra dependencies you inject in it.
I think it is a bad idea to do this in general. Having to inject other dependencies in the repository. It is generally a good indication that the logic does not belong to the entity repository anymore

@cordoval
Copy link
Contributor

cordoval commented May 5, 2014

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

@stof
Copy link
Member

stof commented May 5, 2014

@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

@merk
Copy link
Contributor

merk commented May 5, 2014

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).

@cordoval
Copy link
Contributor

cordoval commented May 6, 2014

i see, but those are kind of special repositories, thanks @merk and @stof

@zebba
Copy link
Contributor Author

zebba commented May 8, 2014

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.

@weaverryan
Copy link
Member

@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:

  1. Pass it into the method you're calling. Sometimes this makes sense, but sometimes it's a value/object you want globally on your repository and it's just not the right solution.
  2. Create a new service and put the logic there.

Here's an example. Imagine you have a method findAllEditableBlogPosts, but inside of it, you need to check security to see if you should query for all blog posts (because you're an admin) or just query for the blog posts that I have created (since you're a normal user). The problem is that you need the security.context service to do this.

So, instead of trying to get that service into your repository, just create a new service, for example: PostManager (I always call things "managers", you may have a better name for it). You would inject the security.context service AND your PostRepository (either by injecting the entity manager, or by registering your repository as a service the normal way and injecting it - see http://stackoverflow.com/questions/17228417/symfony-2-creating-a-service-from-a-repository). Then your function might look like this:

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?

@sgomez
Copy link

sgomez commented Jun 15, 2014

I have a problem. When I do $this->get('acme.blog.repository.post') I received a Doctrine\ORM\EntityRepository instance instead of a Acme\BlogBundle\Entity\EntityRepository. So I can't call any method on my own Repository. I still need to use repositoryClass annotation on entity definition if I want to change the default repository.

@wouterj
Copy link
Member

wouterj commented Jun 15, 2014

I still need to use repositoryClass annotation on entity definition if I want to change the default repository.

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.

@sgomez
Copy link

sgomez commented Jun 15, 2014

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 class="Acme\BlogBundle\Entity\PostRepository" on the service configuration I assumed that I don´t need to configure the repositoryClass. Maybe this should be explained better. Thanks and sorry for the inconvenience.

@weaverryan
Copy link
Member

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!

@weaverryan weaverryan closed this Jul 3, 2014
@merk
Copy link
Contributor

merk commented Jul 4, 2014

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.

https://gist.github.com/docteurklein/9778800

@weaverryan
Copy link
Member

@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 getRepository, then I will get an instance of my specific entity repository class. I think you're referring to some other problem entirely, but I'm not sure. Let me know :).

@merk
Copy link
Contributor

merk commented Jul 12, 2014

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

@weaverryan
Copy link
Member

@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 request object. Now we have request_stack to get around this, which interestingly enough, is very similar to getting the "entity manager" in order to retrieve your repository (thus you always get the valid repository class at that moment).

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.

@merk
Copy link
Contributor

merk commented Jul 13, 2014

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.

@stof
Copy link
Member

stof commented Jul 15, 2014

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).
for the handling of a web request, it generally does not make sense to reset the manager.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

9 participants