3.3 Best Practices for DI Changes #7933
Conversation
best_practices/business-logic.rst
Outdated
| class: AppBundle\Utils\Slugger | ||
| # ... | ||
|
|
||
| # use the class name as the service id |
There was a problem hiding this comment.
I think everybody can understand that "class name" in this case refers to "the FQCN" or "the namespace + the class name" ... but I'd like to mention this in case we find another way to be more specific about what's the new service id.
|
|
||
| .. best-practice:: | ||
|
|
||
| Don't use ``$this->get()`` or ``$this->container->get()`` to fetch services |
There was a problem hiding this comment.
Here and in the previous paragraph we don't explain why you should do that. Should we add something or link to some other article in the docs?
best_practices/business-logic.rst
Outdated
| { | ||
| // ... | ||
|
|
||
| // you can also fetch a public service like this, but is not the best-practice |
There was a problem hiding this comment.
Seems incomplete, that's not the best practice?
best_practices/controllers.rst
Outdated
| ----------------- | ||
|
|
||
| If you extend the base ``Controller`` class, you can access services directly from | ||
| the container via ``$this->container->get()`` or ``$this->get()``. Instead, you |
best_practices/controllers.rst
Outdated
| /** | ||
| * @Route("/comment/{postSlug}/new", name = "comment_new") | ||
| */ | ||
| public function newAction(Request $request, $postSlug) | ||
| public function newAction(Request $request, $postSlug, EntityManagerInterface $em) |
There was a problem hiding this comment.
Is this change really needed? This increase the learning curve for not much gain imo.
There was a problem hiding this comment.
I've been making this change throughout the docs to avoid $this->get() calls in the container. But for the EntityManager specifically, it's not as clear - since $this->getDoctrine() is not $this->get() (and we're still recommending all other controller shortcuts be used - e.g. $this->createForm() instead of injecting the FormFactory).
So, wdyt? It might be safer to change back to preferring $this->getDoctrine() throughout the docs?
There was a problem hiding this comment.
What about suggest to extend from AbstractController with common features needed in controllers and use $this->getDoctrine() & Co.?
There was a problem hiding this comment.
I talked with Nicolas about this about a month ago, we decided (for now) to recommend using Controller... it's less of a big change because we're not taking away the "escape hatch" of using the container as a service locator. We can (likely will) change that recommendation in the future as people get more accustomed to not using $this->get().
There was a problem hiding this comment.
But the original question still remains... and even though I've been the one adding the EntityManagerInterface type-hints everywhere, I kind of think we should revert to using $this->getDoctrine() as the main way.
There was a problem hiding this comment.
The problem is that if we use getDoctrine() then we are using some new features and some old features. We should use only new features or only old features. In any case, I agree that EntityManagerInterface looks and feels terrible.
There was a problem hiding this comment.
$this->getDoctrine() is not especially discouraged whereas $this->get() is.
If we provide this shortcut, it's to simplify code and here it's the case. Of course it's an old feature but as long as it looks better than newer features, I don't see the problem of using it.
There was a problem hiding this comment.
I don't have strong opinion on it, but if type hinting on the interface become the best practice, maybe should we consider to deprecate getDoctrine method in 4.x? I'm not disturbed by the interface, it's only an interface you know it's not that terrible :)
There was a problem hiding this comment.
EntityManagerInterface looks and feels terrible
We may ultimately decide in the future to allow non-interface autowiring where it makes sense - e.g. simply EntityManager $em.
$this->getDoctrine() is not especially discouraged whereas $this->get() is
We can definitely change whatever the recommendation is in the future. But, as @GuilhemN said, there's really nothing wrong with $this->getDoctrine() or the other shortcut methods. The advantage to getDoctrine() is discoverability (I get auto-complete with $this->getD). The disadvantage is that it's not transferrable to a service, which is something we're trying to reduce (i.e. make controllers look more like services... while balancing DX).
I'm quite mixed on which to use...
There was a problem hiding this comment.
I'm in favor to keep $this->getDoctrine()->getManager() it's a very common Controller feature that we need frequently, this feels to me like $this->render(), $this->generateUrl(), etc. I can't imagine having to inject these services too for Controllers.
Some situations:
- Using custom entity manager (
$this->getDoctrine()->getManager('custom')or type-hint and update theservices.ymlto inject the custom manager alias?) - Getting entity manager from entity class (use
$this->getDoctrine()for these cases only?) - Getting entity repository (
$this->getDoctrine()->getRepository()or Inject EM and then$em->getRepository()?).
I wondering If can I use $this->getDoctrine() for some situations why it's best type-hinting the EM instead of ->Manager()? To inject EM every time looks a bit overkill IMHO :)
Just I would recommend always use $this->getDoctrine()->getManager() instead of $this->get('doctrine.orm.default_entity_manager').
This PR was squashed before being merged into the 3.3 branch (closes #7933). Discussion ---------- 3.3 Best Practices for DI Changes Hi guys! This updates the best practices for the 3.3 DI changes. The specific changes are: A) Use class names as service ids (except when a class has multiple services, then use normal snake-case) B) Do not use the container as a service locator - use DI (also, make services private) Thanks! Commits ------- 33647a6 Tweaks after review! adadb5a updating best practices for DI 3.3 changes
Hi guys!
This updates the best practices for the 3.3 DI changes. The specific changes are:
A) Use class names as service ids (except when a class has multiple services, then use normal snake-case)
B) Do not use the container as a service locator - use DI (also, make services private)
Thanks!