How to handle dependency injection? #6548

mnapoli · 2014-10-28T23:55:07Z

OK this is a RFC to discuss how we want to implement dependency injection and use the container in our codebase.

The options

We have several options with PHP-DI that I'll detail below. If you want to read more on this, there's the getting started guide, or also the reference on how to define injections.

Autowiring and constructor injection

namespace Piwik\Plugins\MobileMessaging;

class API {
    private $smsProvider;

    public function __construct(SMSProvider $smsProvider) {
        $this->smsProvider = $smsProvider;
    }

    public function sendSMS() {
        $this->smsProvider->...();
    }
}

Pros:

this is the proper way to do dependency injection, it lets us write code that is decoupled from the container
definitely appropriate for unit testing, we can inject mocks
no configuration needed, PHP-DI can figure out what to inject using the type-hints (here: SMSProvider class) -> that's called autowiring

Cons:

a bit verbose, we have to write constructors which can be a pain when we use a lot of dependencies
we can't inject non-objects (e.g. configuration values, anything that is string/int/…): we need to configure the injection in a configuration file

Annotations

namespace Piwik\Plugins\MobileMessaging;

class API {
    /**
     * @Inject
     * @var SMSProvider
     */
    private $smsProvider;

    public function sendSMS() {
        $this->smsProvider->...();
    }
}

PHP-DI supports injecting with annotations, like Spring in Java, or ZF2, or a few other containers. It can inject in protected and private properties too.

In this example, @Inject will inject the type defined in @var. You can also define explicitly which entry you want to see injected if you can't do it with @var (e.g. it's a configuration value): @Inject('the.name.of.my.entry')

Pros:

the least verbose solution, it's really easy and practical to inject stuff
you can inject everything: objects, primitive types, … (e.g. configuration values…)

Cons:

couples the code to PHP-DI because annotations are not standard stuff
more difficult to unit test: the class can't be created without PHP-DI
on some setups, phpdoc can be stripped off and thus remove annotations: in total I've had 1 bug report of this, it's usually an APC or OPcache config that is non default so I think people that have that optimization enabled have admin access to disable it. All in all annotations are pretty well supported in the PHP world when you look at Symfony, Doctrine, PHPUnit…
some people hate annotations, that's not an argument per se but it's worth being aware of that :)

Configuration

In the case where we don't want any magic to happen and we want complete control over everything, we can define injections manually in config files:

return array(
    'Piwik\Plugins\MobileMessaging\API' => DI\object()
        ->constructor(DI\link('Piwik\Plugins\MobileMessaging\SMSProvider')),
);

namespace Piwik\Plugins\MobileMessaging;

class API {
    private $smsProvider;

    public function __construct(SMSProvider $smsProvider) {
        $this->smsProvider = $smsProvider;
    }

    public function sendSMS() {
        $this->smsProvider->...();
    }
}

The syntax of the PHP config is explained here: defining injections. If you are curious as to why it's not yaml/why it looks like that/etc. have a look here: Why YAML was a bad choice.

So obviously this option is extremely verbose. It's the path chosen by Symfony for example: define ALL the things! And in my experience that sucks a lot, especially when you have a large codebase. As you can see it's pretty dumb code, and can be avoided most of the time.

Pros:

this is the proper way to do dependency injection, it lets us write code that is decoupled from the container
definitely appropriate for unit testing, we can inject mocks
no magic, everything is explicit
you can inject everything (like in annotations): objects, primitive types (e.g. configuration values)…

Cons:

verbose
verbose
verbose, and very redundant with the actual code (when type-hints exist for example)
config hell
hard to maintain injections in the configuration with refactorings/changes in the code

Mixing up options

The good news is that we can mix all 3 options and use each where it makes sense :)

If you have a look at the best practices guide, that's the approach I usually recommend.

Performances

There is no performance difference between all these options if we use a cache.

However, given we have the plugin architecture where plugins are enabled/disabled at will, and given that maybe in the future plugins can have their own DI config, maybe a cache is incompatible with that? Or maybe we could simply clear the DI cache when a plugin is enabled/disabled?

My suggestion

Maybe we can differentiate between:

code that should be unit tested
code that shouldn't be unit tested because it's just "glue" code or application code (i.e. VC in MVC)

Code that needs to be unit tested needs to use proper dependency injection, i.e. either autowiring or configuration. Of course, I'd tend towards autowiring and using configuration only when it's necessary (i.e. inject config values for example).

Code like controllers, menus… don't need to be unit tested in theory because they are just application code and don't contain a lot of logic. Covering them with system/ui tests is usually enough. So usually it's best to be as productive and practical as possible so annotations can help a lot here. I've used annotations in the past (in controllers) and it's just fantastic to use.

However, I said in theory because some controllers or API classes can contain a lot of code that we might want to test. So annotations could be a pain. So do we want to unit test controllers and API classes and all? Should we refactor them to move business logic inside model classes/services?

Anyway the discussion is open, I'm open to not use annotations too if that's too confusing to have 2 ways of doing injections.

See also #4917

The text was updated successfully, but these errors were encountered:

diosmosis · 2014-10-29T03:13:37Z

My thoughts:

Piwik functionality is meant to be provided by plugins, so plugins must be able to override/change injections. I think this requires having a config. Ideally, it'd be nice if users could modify this config as well to suit their own needs.

Autowiring should still be used since configuration is only required to override Piwik's behavior. So default behavior should be specified via auto-wiring and this should be overridden if the config specifies it.

I'd prefer annotations. We can always add getters/setters to make a class more testable.

mnapoli · 2014-10-29T03:54:16Z

Piwik functionality is meant to be provided by plugins, so plugins must be able to override/change injections. I think this requires having a config. Ideally, it'd be nice if users could modify this config as well to suit their own needs.

Yeah totally agree with that. I didn't go into detail with the config files, but in any case we will have some config files.

And at first I thought we could start simple and have like one or two file, then we let plugins have their own config files too later on (so that they can expand or override the main config). What I had in mind looks a lot like what you're suggesting and what already exists with ini configs I guess:

global config (versioned in the repo)
user config (gitignored)

And I'm +1 with the rest of your comment.

diosmosis · 2014-10-29T05:08:12Z

+1000 if you can replace INI config w/ DI config :)

mnapoli · 2014-10-29T05:52:51Z

Yeah that's one of the goals too in the long term :) We'll have to keep BC though, but I think that can easily be done by implementing a DefinitionSource for the container that would read .ini files.

mattab · 2014-10-30T11:04:54Z

Nice discussion, thanks for explaining!

I have no preference but it sounds good to use both, hopefully it will be clear when to use config.php VS annotation.

Or maybe we could simply clear the DI cache when a plugin is enabled/disabled

+1

tsteur · 2014-10-30T20:50:33Z

I'm 100% for constructor injection and sometimes setter injection if needed or appropriate. Wouldn't go with the injection of private / protected properties via annotations.

a bit verbose, we have to write constructors which can be a pain when we use a lot of dependencies

This often indicates a code smell as a class seems to do too many things and is not a problem of constructor injection.

A few nice blog posts about this topic:

In general the less magic the better... more magic is seldom a good thing. Defining the arguments in a normal way is understood by any IDE and any code analysis tool etc. Although PHPStorm refactors comments as well not every IDE/Editor does it and people should have the freedom to choose any IDE they like. We would probably also add like big blocks of comments just for DI? Which can be verbose ;) Ideally, a method is so clear that is does not need a comment (unless it is plugin API). Also to expose dependencies in a clear way is quite important I think. It is also understood by everyone and it will be easier for plugin developers.

diosmosis · 2014-10-30T21:35:38Z

If constructor injection allows allowing parameters to be nullable and it's still possible to type hint constructor parameters w/o having them automatically set to objects in DI, then constructor injection seems ok to me. I would prefer annotations however.

Re the IDE issue: I am not aware of an IDE outside of PHPStorm that does any refactoring, so I'm not sure this point stands. And it's not impossible to use an IDE w/o refactoring tools.

Re the big blocks of comments: the annotations I've seen (looks like 2 per injectable field) doesn't seem to qualify as big blocks of comments.

Re clarity of constructor arguments: Having just constructor arguments does not imply they will be loaded from a DI container. They're just constructor arguments. Annotations on the other hand are very clear; @Inject has an obvious meaning.

Re ensuring code quality: Is this for plugin developers? As core developers whose time to work on things is already minimal, the code we write will never be perfect. We should strive for constant incremental improvement, not immediate perfectionism. Plugin developers will do whatever they want.

Furthermore, injecting by constructor args might encourage more granular classes, or it might encourage object spaghetti. You can't force people to think critically and have the correct insights.

mnapoli · 2014-10-30T21:38:33Z

a bit verbose, we have to write constructors which can be a pain when we use a lot of dependencies

This often indicates a code smell as a class seems to do too many things and is not a problem of constructor injection.

I should have been clearer, I was mostly talking about controllers here (I mean I'm only advocating annotations for controllers). In Piwik we can only have 2 controllers per plugins: Controller and API. So code smell or not, that's just the way it is.

Of course if a service/model class (whatever the name) has a long constructor, that is indeed a code smell and it should probably be refactored. But I'm only advocating annotations for glue code, i.e. controllers and maybe commands.

Here are the arguments in the blog posts:

Field injections is evil… hides dependencies, instead of making them explicit.
Dependencies need to be communicated publically

In the case of controllers, the consumer is the front controller, who uses the container. So it doesn't matter if dependencies are hidden because this is the container that takes care of these dependencies.

But now when you want to test your class and want to assign e.g. a mock to the field you have to…

We don't want to unit test controllers.

Here is an extract of http://php-di.org/doc/best-practices.html:

As you can see, this solution requires very few code, is simple to understand and benefits from IDE support (autocompletion, refactoring, …).

Property injection is generally frowned upon, and for good reasons:

injecting in a private property breaks encapsulation

it is not an explicit dependency: there is no contract saying your class need the property to be set to work

if you use PHP-DI's annotations to mark the dependency to be injected, your class is dependent on the container (see the 2nd rule above)

BUT

if you follow general best practices on how to write your application, your controllers will not contain business logic (only routing calls to the models and binding returned values to view).

So:

you will not unit-test it (that doesn’t mean you won’t write functional tests on the interface though)

you will not need to reuse it elsewhere

Regarding IDE support, I tend to think the contrary, this usually gets better autocompletion and refactoring support:

class Foo {
    /**
     * @var Logger
     */
    private $logger;

    public function bar() {
        $this->logger->…
    }
}

Than this:

class Foo {
    private $logger;

    public function __construct(Logger $logger) {
        $this->logger = $logger;
    }

    public function bar() {
        $this->logger->…
    }
}

Basic IDEs can lack proper static type analysis to provide autocompletion. So then the solution is to add docblock comments for properties, but we're back to an even more verbose solution ;)

However I agree with your point about "it's magic". I've never liked magic because you need to understand it to use it. So @Inject could confuse new contributors.

And the second drawback IMO is that there would be 2 best practices: use annotations in controllers, and use constructor injection elsewhere. Confusing… (not for us, but for casual contributors)

tsteur · 2014-10-30T21:56:01Z

@diosmosis
many IDEs support refactoring but maybe not all of them also take care of comments etc. There are also command line refactoring tools such as http://qafoolabs.github.io/php-refactoring-browser/ so you could integrate it in sublime or whatever ;) Anyway, everyone should choose any IDE he likes...

If constructor injection allows allowing parameters to be nullable and it's still possible to type hint constructor parameters w/o having them automatically set to objects in DI, then constructor injection seems ok to me. I would prefer annotations however.

A constructor probably defines mandatory dependencies, optional ones as a setter.

Re the big blocks of comments: the annotations I've seen (looks like 2 per injectable field) doesn't seem to qualify as big blocks of comments.

Might be bigger for methods etc.

Re ensuring code quality: Is this for plugin developers?

I don't think I wrote about code quality. I only meant plugin developers are used to it the standard way etc and for instance annotations can add additional complexity etc.

Furthermore, injecting by constructor args might encourage more granular classes, or it might encourage object spaghetti. You can't force people to think critically and have the correct insights.

Nobody is forcing anyone. Constructor arguments can help you to see / identify problems but it doesn't have to.

@mnapoli
I didn't know we are talking only about Controllers but think in general we should have only one way. Having multiple ways to do things just makes it more confusing what to use when and where and probably ends in a big mix. It is already "hard" to define what is a "controller" and what not. For instance Widgets etc. can be considered as a controller as well. At least long term I hope to get rid of controllers completely but will take a while ;)

tsteur · 2014-10-30T21:58:01Z

It maybe also makes sense to refactor some controllers into multiple classes when introducing DI

mnapoli · 2014-10-30T22:18:58Z

Something we didn't mention yet is that controllers have a base Controller class. If we use constructor injection that might be a mess: how do we handle the dependencies of the parent class? If we have to copy paste the dependencies of the base class every time that might be boring.

Especially when we see all the dependencies of the base controller:

use Piwik\Access;
use Piwik\API\Proxy;
use Piwik\API\Request;
use Piwik\Common;
use Piwik\Config as PiwikConfig;
use Piwik\Config;
use Piwik\DataTable\Filter\CalculateEvolutionFilter;
use Piwik\Date;
use Piwik\Exceptions\HtmlMessageException;
use Piwik\FrontController;
use Piwik\Menu\MenuTop;
use Piwik\Menu\MenuUser;
use Piwik\NoAccessException;
use Piwik\Notification\Manager as NotificationManager;
use Piwik\Period\Month;
use Piwik\Period;
use Piwik\Period\Range;
use Piwik\Piwik;
use Piwik\Plugins\CoreAdminHome\CustomLogo;
use Piwik\Plugins\CoreVisualizations\Visualizations\JqplotGraph\Evolution;
use Piwik\Plugins\LanguagesManager\LanguagesManager;
use Piwik\Plugins\UsersManager\UserPreferences;
use Piwik\Registry;
use Piwik\SettingsPiwik;
use Piwik\Site;
use Piwik\Url;
use Piwik\View;
use Piwik\View\ViewInterface;
use Piwik\ViewDataTable\Factory as ViewDataTableFactory;

(some of them might not be objects to inject through the constructor though)

When we'll have moved away from singletons, we might have a long list of dependencies to inject. We should probably refactor that though? But still for example CoreAdminHome has 19 dependencies… But yeah we could also split that too.

One thing to note is that it's easy to move from constructor injection to annotations though, so we can always start with the constructor injection and see how it goes.

mnapoli · 2014-12-01T01:45:30Z

For now I have disabled the use of annotations.

We can give it a try with autowiring + config only and see how it goes. We are anyway mainly using the container as a service locator (i.e. StaticContainer::getInstance()->get('foo')), which is not ideal (it's not really dependency injection) but at least we are moving in the right direction.

Basically, we are moving from "static" -> "service locator" -> "dependency injection". Moving from SL to DI is much easier than from static/singletons, so we are on the right path.

mattab · 2015-04-02T05:36:50Z

quick update: we're continuing the work of introducing DI in Piwik which involves tackling technical debt especially in tests code: this is being done in #7601 and #7600

tassoman · 2020-06-04T12:51:22Z

In 2020 this still an actual question 🤣 because StaticContainer is marked as deprecation in the code and we have DI\Container available in the code, removable in the 3.x-dev but we're going to 4.x-dev any developers documentation about that... I'm lost 🐒

tsteur · 2020-06-04T20:08:30Z

@tassoman in Matomo 4 we have marked StaticContainer no longer as API so it is basically to be considered removed. In plugins you should always use injection through constructor method. This is also best practice for 3.X

Findus23 · 2020-06-04T21:07:09Z

@tsteur I think this means that guides like https://developer.matomo.org/guides/logging#how-to-log-messages need to be updated and maybe matomo-org/developer-documentation#67 needs to be finished.

tsteur · 2020-06-04T21:13:55Z

added a comment to #8415 @Findus23

i reckon we start preparing the docs in a month or so.

tassoman · 2020-06-05T09:00:36Z

If I'm able to tame it, I would like to contribute somehow, lemme study more now

sgiehl · 2020-06-05T10:00:43Z

@tassoman I think a good start would be to change the existing plugins to use DI instead of StaticContainer where possible. There are still some usages left that should be easy to convert. Just created a small PR to demonstrate what I mean: #16023
That should be at least possible for all Controller and API classes.
If you like to help with that, feel free to create PRs maybe for each plugin, so it's easier and faster to review.

mnapoli added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Oct 28, 2014

mnapoli self-assigned this Oct 28, 2014

mnapoli mentioned this issue Oct 29, 2014

Introduced the DI container #6555

Merged

mattab added this to the Piwik 2.10.0 milestone Nov 3, 2014

mnapoli closed this as completed Dec 1, 2014

tsteur mentioned this issue Jun 4, 2020

Update our Develop guides for Matomo 4.0.0 #8415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

How to handle dependency injection? #6548

How to handle dependency injection? #6548

mnapoli commented Oct 28, 2014

diosmosis commented Oct 29, 2014

mnapoli commented Oct 29, 2014

diosmosis commented Oct 29, 2014

mnapoli commented Oct 29, 2014

mattab commented Oct 30, 2014

tsteur commented Oct 30, 2014

diosmosis commented Oct 30, 2014

mnapoli commented Oct 30, 2014

tsteur commented Oct 30, 2014

tsteur commented Oct 30, 2014

mnapoli commented Oct 30, 2014

mnapoli commented Dec 1, 2014

mattab commented Apr 2, 2015

tassoman commented Jun 4, 2020

tsteur commented Jun 4, 2020

Findus23 commented Jun 4, 2020

tsteur commented Jun 4, 2020

tassoman commented Jun 5, 2020

sgiehl commented Jun 5, 2020

How to handle dependency injection? #6548

How to handle dependency injection? #6548

Comments

mnapoli commented Oct 28, 2014

The options

Autowiring and constructor injection

Annotations

Configuration

Mixing up options

Performances

My suggestion

diosmosis commented Oct 29, 2014

mnapoli commented Oct 29, 2014

diosmosis commented Oct 29, 2014

mnapoli commented Oct 29, 2014

mattab commented Oct 30, 2014

tsteur commented Oct 30, 2014

diosmosis commented Oct 30, 2014

mnapoli commented Oct 30, 2014

tsteur commented Oct 30, 2014

tsteur commented Oct 30, 2014

mnapoli commented Oct 30, 2014

mnapoli commented Dec 1, 2014

mattab commented Apr 2, 2015

tassoman commented Jun 4, 2020

tsteur commented Jun 4, 2020

Findus23 commented Jun 4, 2020

tsteur commented Jun 4, 2020

tassoman commented Jun 5, 2020

sgiehl commented Jun 5, 2020