The Symfony2 DependencyInjection Component has made my life and work as a developer a lot easier. Choosing the right way to use it however can be a bit difficult sometimes. Knowing what a/the service container can do, helps a lot, and also thinking about how you would do it with just PHP can put you back on track. To be able to recognize some problems related to dependency injection in your own code, I will describe a few "dependency injection smells" below (a term derived from "code smells", used by Kent Beck, Martin Fowler and the likes).

Injecting too many dependencies

When you choose constructor or setter injection (I would not recommend any other way), you would recognize this problem by a large number of injected objects, i.e. services:

class UnfocusedClass
{
    public function __construct(
        EntityManager $entityManager,
        DocumentManager $documentManager,
        NewsFeedGenerator $generator,
        DelayedEmailSender $emailSender,
        RouterInterface $router,
        LoggerInterface $logger,
    ) {
        // put them all into private properties
    }
}

When you look at a class and its dependencies it should already be clear what the class can do, but also that it does not do too much. Looking at the list of dependencies above, this class clearly does too much and knows all too well about all these things. It has, in other words, no single responsibility (not even two responsibilities). Cut this thing into several other classes, and leave all the implementation details to them.

Injecting dependencies just for passing them through

This happens when a class receives a dependency just to use it later on when instantiating some other class (or otherwise to pass as an argument to a method of another object). For instance:

use Symfony\Component\Translation\Translator;

class MenuItem
{
    private $translator;

    public function __construct(Translator $translator)
    {
        $this->translator = $translator;
    }

    public function createView()
    {
        return new MenuItemView($this, $this->translator);
    }
}

There are several problems with the design of this class:

1. The MenuItem is now tightly coupled to the MenuItemView class. This is because we used the new operator. It would not be so easy for other developers to modify the behavior of this class because he can't use his own view class.

2. Injecting the translator does not look too bad at first sight, but now we know that it's only used in the createView() method: what if the MenuItemView class needs more dependencies in the future? Should we keep injecting them all into the constructor? This is bad - MenuItem itself does not need these dependencies, MenuItemView does!

The solution would be to create a factory for creating the view items. The factory should know about MenuItem and MenuViewItem, and MenuItem will know nothing about creating a view item anymore:

use Symfony\Component\Translation\Translator;

class MenuItemViewFactory
{
    private $translator;

    public function __construct(Translator $translator)
    {
        $this->translator = $translator;
    }

    public function create(MenuItem $menuItem)
    {
        return new MenuItemView($menuItem, $this->translator);
    }
}

class MenuItem
{
    // contains only the data for the menu item,
    // no translator, no createView() method
}

Injecting implementations instead of interfaces

You may have already noticed this injection problem when you looked at the MenuItemViewFactory class. Its constructor expects a Translator object. We should have used the TranslatorInterface instead, since Translator just implements that interface:

use Symfony\Component\Translation\TranslatorInterface;

class MenuItemViewFactory
{
    public function __construct(TranslatorInterface $translator)
    {
        // ...
    }
}

"Why?" would you say, "I'm never going to use anything else then the standard Translator bundled with the framework!" You could be right about that, but there are many other cases where you will change implementation later on. But that's not really the point: you only really need a dependency to implement the interface. In almost all cases it does not matter what implementation of a translator you get, as long as it can translate some string and replace some variables within it (exactly the expected behaviour of TranslatorInterface::trans()). The interface is a contract, and each implementation promises to provide what the interface defines. So you should trust the interface and use it when injecting dependencies.

Making all services public (or forgetting to make them private)

When you dump your service container (using app/console container:debug), and you see many of your services that should not or do not need to be used stand-alone, you should mark them as private. By default, all services you define are publicly available. This means that every part of the application can retrieve them by calling (when $this->container is the service container):

$this->container->get('public_service);

Marking services as private (in fact, you should set the "public" attribute of the service to "false") will clean things up a lot. It will also make it clear to other developers what the main entry points of your bundle are.

Before making all your services private (except one or so...), remember that some lazy-load mechanisms require a service to be public. For instance, all form types registered using the form.type tag must be public, since they are retrieved from the service container only when needed, by using the get() method of the container.

Having an unreadable service definition file

Both Yaml and XML service definition files should be as readable as they can be. This means: add some extra lines between each service definition, group services and their dependencies with comments and put them in a readable order (and be consequent from them on).

Also, when some logical subdivisions can be made in your service definition file (be it services.xml or services.yml), it may also help maintainability to split it into multiple files. Of course, these new files should then also be loaded from your bundle extension:

namespace Matthias\SomeBundle\DependencyInjection;

use Symfony\Component\HttpKernel\DependencyInjection\Extension;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\Config\FileLocator;

class SomeExtension extends Extension
{
    public function load(array $config, ContainerBuilder $container)
    {
        $locator = new FileLocator(__DIR__.'/../Resources/config');
        $loader = new XmlFileLoader($container, $locator);
        $loader->load('services.xml');
        $loader->load('form.xml');
    }
}

This way, you could even make the file optional, by wrapping the $loader->load() command in an if-statement.

In conlusion

Two things: service definition files are also part of your project, which means they should be readable and maintainable. And: just because injecting dependencies is so easy these days, think well about where and why you inject them.

These were some of the more important "dependency injection smells". I'm happy to hear of any other "smells" you have encountered yourself.