Skip to content

3.3 Best Practices for DI Changes #7933

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

Merged
merged 2 commits into from
May 27, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions best_practices/business-logic.rst
Original file line number Diff line number Diff line change
Expand Up @@ -84,30 +84,41 @@ Next, define a new service for that class.

# app/config/services.yml
services:
# keep your service names short
app.slugger:
class: AppBundle\Utils\Slugger
# ...

# use the class name as the service id
Copy link
Member

Choose a reason for hiding this comment

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

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.

AppBundle\Utils\Slugger:
public: false

.. note::

Traditionally, the naming convention for a service involved following the
class name and location to avoid name collisions. Thus, the service
*would have been* called ``app.utils.slugger``. But by using short service names,
your code will be easier to read and use.
If you're using the :ref:`default services.yml configuration <service-container-services-load-example>`,
the class is auto-registered as a service.

Traditionally, the naming convention for a service was a short, but unique
snake case key - e.g. ``app.utils.slugger``. But for most services, you should now
use the class name.

.. best-practice::

The name of your application's services should be as short as possible,
but unique enough that you can search your project for the service if
you ever need to.
The id of your application's services should be equal to their class name,
except when you have multiple services configured for the same class (in that
case, use a snake case id).

Now you can use the custom slugger in any controller class, such as the
``AdminController``:

.. code-block:: php

public function createAction(Request $request)
use AppBundle\Utils\Slugger;

public function createAction(Request $request, Slugger $slugger)
{
// ...

// you can also fetch a public service like this, but is not the best-practice
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems incomplete, that's not the best practice?

// $slugger = $this->get('app.slugger');

if ($form->isSubmitted() && $form->isValid()) {
$slug = $this->get('app.slugger')->slugify($post->getTitle());
$post->setSlug($slug);
Expand All @@ -116,6 +127,16 @@ Now you can use the custom slugger in any controller class, such as the
}
}

Services can also be :ref:`public or private <container-public>`. If you use the
:ref:`default services.yml configuration <service-container-services-load-example>`,
all services are private by default.

.. best-practice::

Services should be ``private`` whenever possible. This will prevent you from
accessing that service via ``$container->get()``. Instead, you will need to use
dependency injection.

Service Format: YAML
--------------------

Expand Down
26 changes: 20 additions & 6 deletions best_practices/controllers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,16 @@ for the homepage of our app:

use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Doctrine\ORM\EntityManagerInterface;

class DefaultController extends Controller
{
/**
* @Route("/", name="homepage")
*/
public function indexAction()
public function indexAction(EntityManagerInterface $em)
{
$posts = $this->getDoctrine()
->getRepository('AppBundle:Post')
$posts = $em->getRepository('AppBundle:Post')
->findLatest();

return $this->render('default/index.html.twig', array(
Expand All @@ -115,6 +115,19 @@ for the homepage of our app:
}
}

Fetching Services
-----------------

If you extend the base ``Controller`` class, you can access services directly from
the container via ``$this->container->get()`` or ``$this->get()``. Instead, you
Copy link
Contributor

Choose a reason for hiding this comment

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

But instead?

should use dependency injection to fetch services: most easily done by
:ref:`type-hinting action method arguments <controller-accessing-services>`:

.. best-practice::

Don't use ``$this->get()`` or ``$this->container->get()`` to fetch services
Copy link
Member

Choose a reason for hiding this comment

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

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?

from the container. Instead, use dependency injection.

.. _best-practices-paramconverter:

Using the ParamConverter
Expand Down Expand Up @@ -164,13 +177,14 @@ manually. In our application, we have this situation in ``CommentController``:

.. code-block:: php

use Doctrine\ORM\EntityManagerInterface;

/**
* @Route("/comment/{postSlug}/new", name = "comment_new")
*/
public function newAction(Request $request, $postSlug)
public function newAction(Request $request, $postSlug, EntityManagerInterface $em)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change really needed? This increase the learning curve for not much gain imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@yceruto yceruto May 22, 2017

Choose a reason for hiding this comment

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

What about suggest to extend from AbstractController with common features needed in controllers and use $this->getDoctrine() & Co.?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@yceruto yceruto May 26, 2017

Choose a reason for hiding this comment

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

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 the services.yml to 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').

{
$post = $this->getDoctrine()
->getRepository('AppBundle:Post')
$post = $em->getRepository('AppBundle:Post')
->findOneBy(array('slug' => $postSlug));

if (!$post) {
Expand Down
7 changes: 4 additions & 3 deletions best_practices/security.rst
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,9 @@ more advanced use-case, you can always do the same security check in PHP:
/**
* @Route("/{id}/edit", name="admin_post_edit")
*/
public function editAction($id)
public function editAction($id, EntityManagerInterface $em)
{
$post = $this->getDoctrine()->getRepository('AppBundle:Post')
$post = $em->getRepository('AppBundle:Post')
->find($id);

if (!$post) {
Expand Down Expand Up @@ -328,7 +328,8 @@ the same ``getAuthorEmail()`` logic you used above:
}
}

Your application will :ref:`autoconfigure <services-autoconfigure>` your security
If you're using the :ref:`default services.yml configuration <service-container-services-load-example>`,
your application will :ref:`autoconfigure <services-autoconfigure>` your security
voter and inject a ``AccessDecisionManagerInterface`` instance in it thanks to
:doc:`autowiring </service_container/autowiring>`.

Expand Down