Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Introduce Dependency Injection #4917

Closed
tsteur opened this Issue · 16 comments

6 participants

@tsteur
Owner

Here are already some requirements I got from some core developers:

1) introduce the concept of a Component to replace current singletons. components would be objects that are automatically stored in an IoC container (ie, a class derives from Component and everything else is taken care of by the system).
2) every public method of a component would be wrapped in an event (ie, Auth.login.before/Auth.login.after or whatever)
3) there would be a global IoC container that holds all core components. it would also hold IoC containers for each plugin. plugin IoC containers would inherit from the global container (so the child container composits the parent container)
4) piwik should probably have its own IoC container class. the class essentially represents the environment Piwik code will run in, so could be called Environment. by creating new instances of such a class, we can access different Piwik's from within a Piwik (which would make the piwik cloud code cleaner & more robust)
5) dependency injection should include something similar to spring's @Autowired functionality. instead of associating components w/ strings and accessing objects by string in a container (as you would w/ pimple), it should be possible to automatically set properties of Components to the instance of other components. two-phase construction can be used to deal w/ cycles.
6) the end user should be able to configure the IoC container (though not directly). for example, plugins shouldn't decide which auth component to use, the end user should.
7) plugin objects, APIs & controllers should all be components

re 2) I think it is not directly related to this feature.

I think we do not need to have all features in the beginning. For instance we can implement 6) later on top of DI.

Next step:
Do we have more requirements? Do we maybe not need any of those requirements? Does anyone can recommend any good library to fulfill our needs or can someone recommend any lib in general?

Then:
We could start refactoring two or three components to use DI. For instance Log, Access and Config?

A problem might be backwards compatibility as current plugins access those instances via Singleton.

@mattab mattab added Major and removed c: Core Critical labels
@mnapoli
Collaborator

Here is a sum up of our discussion:

Migration from Singletons to DI

Step 1

  • refactor singleton objects into objects using dependency injection. The new classes can then be unit tested more easily (good because we want more unit tests)
  • we keep the singleton and its methods to keep Backward Compatibility. The singleton now because a static proxy to the real object, just like in Laravel (where proxies are called Facades)

Quick and dirty example:

// This is the old Singleton
// It's now just a facade/proxy
class Db
{
    public static function get()
    {
        if (self::$connection === null) {
            // Create the DB object
        }
        return self::$connection;
    }

    public static function fetchAll(...)
    {
        // Forward to the real object
        self::get()->fetchAll(...);
    }
}

// This is the new object
class Database
{
    private $connection;

    public function __construct(DbConnection $connection)
    {
        $this->connection = $connection;
    }

    public function fetchAll(...)
    {
        // Do the real stuff
    }
}

What's cool:

  • we keep BC compatibility for plugins and for the whole Core codebase: we shouldn't need to change any line outside the refactored class, so we can go with small steps
  • we don't necessarily need a container at first, the instantiation can still be in the singleton since everything else will still use the singleton (for the 1st step)
  • the new objects are fully using DI, so they are perfectly testable (and we can use mocks)
  • we have to have a different name for the classes because we keep the singleton class: that's an opportunity to maybe create completely namespaced components (e.g. maybe have the new Db object inside the Db folder, so that everything related to Db is together)?
  • we also get the opportunity to replace the code by libraries, e.g. for logging…

Step 2

  • we add a container
  • we move all the object's creation (that was in the singletons) into the container's config
  • the singletons now use the container to create the proxied objects

Step 3

Slow migration from using singletons to injecting dependencies:

  • we change the way controllers work (both web and API controllers): we need to split them because they are getting quite long sometimes, and it will only get worse with dependency injection
  • controllers can now be created by the container (we haven't discussed that in details too much yet) and there can be dependency injection in the controllers (so no need to use the singletons anymore, great!)
  • same can be done with CLI commands: they shouldn't be using the singletons anymore

Step 4

In some future version we can remove the singletons.

Architecture of plugins

We have 2 options:
- a "root" container + one child container per plugin (that would have access to the root container entries, but can't modify it)
- one container and several config files (e.g. one config per plugin) -> plugins could override Core config

We discussed both options, and kind of agreed that it's OK if plugins can override Core config. It should be recommended that plugins shouldn't do it, but leaving the door open to that can be very useful in some situations. It will be the plugins responsibility to make the right choice. So in the end one container + several config files is the simplest option.

It is also the solution is used by Symfony in its system of Bundles, and it's very powerful.

Here is for example how configs could be included:

  • Global Piwik config (default options)
  • Plugin 1 config
  • Plugin 2 config
  • Plugin … config
  • User config

The latest included config can override the previous ones. That way plugins can enrich, or override the default config, and then the user can always re-override everything.

Here is an example with Authentication:

  • global config (default):

AuthProviderInterface is bound to DatabaseAuthProvider (users logins are stored in database)

  • the LDAP plugin provides LdapAuthProvider but doesn't override AuthProviderInterface (because that's bad practice, it's better to let the user choose)

In the plugin's config file, there is the configuration for LdapAuthProvider

  • the user overrides the default config:

AuthProviderInterface is now bound the interface to LdapAuthProvider, so Piwik's authentication will now work with LDAP.

Then everywhere we want to use Auth, we inject AuthProviderInterface and the container will inject the object that was configured (so in our example, LdapAuthProvider).

Events

Regarding the events, and the fact that "Components" have to extend the interface (so that events are auto-declared), we are not all on the same page. Since this is not needed right now, we can come to a decision later on.

Container

We have discussed several options, we agree that having autowiring and annotations would be great (like in Spring), we have discussed maybe using PHP-DI (which covers that). The question is still open (I think obviously PHP-DI would fit the bill pretty nicely but I'm not objective :p).

Here are some documentation topics that are related if you want to learn more:

And if you have any question about it just ask

@JulienMoumne
Collaborator

+1 for DI in Piwik

@mnapoli
Collaborator

Hi all, here are the 2 POC we discussed for the steps described above:

  • step 1 (DI refactoring): #5946
  • step 2 (container): #5948
@diosmosis
Collaborator

Requirement #2 (and the whole event dispatching system) can be removed if DI solution makes it easy for plugins to composit existing components. For example, a plugin could create a new Auth component that composits (ie, uses) the existing injected component. The compositing object can manipulate the existing component w/o having to use events, and the problem w/ plugins & event ordering can be avoided too.

@mnapoli
Collaborator

:+1:

@diosmosis
Collaborator

If possible I think replacing INI config w/ DI config would be a good idea. Ie move all settings in INI config to DI config while still keeping it easy for end users to modify.

@mattab
Owner

fyi we must keep INI format because we don't want to update all FAQs and guides explaining how to change settings etc.

@diosmosis
Collaborator

We can keep it backwards compatible or still support INI, but DI config will be much more powerful since you should be able to configure actual objects.

For example, it should be possible (w/ correct Piwik architecture), to create a DAO object that uses a NoSQL solution and associate it w/ the selecting/aggregating (ie, map/reduce) logic, create DAO objects that use MySQL for sites, users and other relational data and create a DAO object that uses a key/value store for archive data all in a config file. It would be very hard and very ugly to represent such a configuration using only INI config and plugin event observers.

@mattab mattab added the c: Platform label
@sabl0r sabl0r referenced this issue from a commit in sabl0r/piwik
@mattab mattab Hide errors output in CI build. This could be fixed in much better wi…
…th Dependency injection refs #4917
6a85ad3
@mattab
Owner

Heya @mnapoli,
It looks like we made big progress on introducing DI to Piwik.
what is the status on this issue?

@mattab mattab modified the milestone: Piwik 2.11.0, Short term
@mattab mattab removed the Major label
@mnapoli
Collaborator

Let's close it, it's been long out of date.

@mnapoli mnapoli closed this
@mattab
Owner

For anyone interested about using Dependency Injection in Piwik see this guide: https://github.com/piwik/developer-documentation/pull/67/files?short_path=762e172#diff-762e172d280769ca6701a87863ec30f2

@kaz231

@mnapoli I have two questions:

  • I thought that StaticContainer class is deprecated, is it ?
  • Am I able to define config in plugin ?
@mattab
Owner

Not sure about our official point of view with the deprecated class StaticContainer @diosmosis what do you suggest, maybe the class is not deprecated after all, since we still use it in few places?

Am I able to define config in plugin ?

yes, you can create DI config.php files in plugins, see for example https://github.com/piwik/piwik/blob/master/plugins/Diagnostics/config/config.php

@kaz231

Thanks @mattab !

What is the order of including of such config files ? Can I override some service/class that is already defined in the main config from a plugin ?

@mattab
Owner

Yes, plugins DI config should override core DI config.

@diosmosis
Collaborator

Not sure about our official point of view with the deprecated class StaticContainer @diosmosis what do you suggest, maybe the class is not deprecated after all, since we still use it in few places?

It will be removed in 3.0. It has to be used now because Piwik dependencies are too tangled up, and because of @api static methods that can't be removed/deprecated until 3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.