-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems incomplete, |
||
// $slugger = $this->get('app.slugger'); | ||
|
||
if ($form->isSubmitted() && $form->isValid()) { | ||
$slug = $this->get('app.slugger')->slugify($post->getTitle()); | ||
$post->setSlug($slug); | ||
|
@@ -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 | ||
-------------------- | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been making this change throughout the docs to avoid So, wdyt? It might be safer to change back to preferring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about suggest to extend from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that if we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We may ultimately decide in the future to allow non-interface autowiring where it makes sense - e.g. simply
We can definitely change whatever the recommendation is in the future. But, as @GuilhemN said, there's really nothing wrong with I'm quite mixed on which to use... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor to keep Some situations:
I wondering If can I use Just I would recommend always use |
||
{ | ||
$post = $this->getDoctrine() | ||
->getRepository('AppBundle:Post') | ||
$post = $em->getRepository('AppBundle:Post') | ||
->findOneBy(array('slug' => $postSlug)); | ||
|
||
if (!$post) { | ||
|
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.
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.