@mnapoli opened this Pull Request on October 16th 2014 Member

This is an attempt at refactoring the EventDispatcher class towards dependency injection.

Since we want dependency injection, we need to get rid of singletons. The EventDispatcher is one.

For this refactoring, I've started by simply creating a new Piwik\EventDispatcher\EventDispatcher class that is not a singleton and moving all the logic inside.
Then the old Piwik\EventDispatcher is just a singleton proxy (i.e. a singleton whose getInstance() returns an instance of the new class).

I also took the opportunity for a refactoring:

  • I added EventDispatcherInterface so that later, when injecting the dispatcher, we can type-hint against interfaces and thus easily replace the implementation (e.g. if we want to use a 3rd party library instead, of even for injecting mocks in tests)
  • I tried to have the EventDispatcher component completely separated from the rest of the Piwik codebase and that wasn't too hard. The only coupling is to the plugins: plugins declare events they want to subscribe too.

This reminded me a lot of how Symfony's event dispatcher works, so I went a similar way.
The event dispatcher was handling plugins and observers (callbacks), now it handles subscribers and observers.

A subscriber is a class that declares which events it wants to subscribe to. And hey, that's just exactly what plugins do ;) cool!

interface SubscriberInterface
{
    public function getListHooksRegistered();
}
// ...
class Plugin implements SubscriberInterface
{
    public function getListHooksRegistered()
    {
        return array();
    }
}

So now the base Plugin class implements that interface, and plugins are already compatibles. The only thing I wish we could do is rename that method to something more consistent, like getSubscribedEvents() but that would totally break BC so let's do that in 3.0 or something. We could also write adapters but is that worth it?

So now that plugins implement that interface, we want the event dispatcher to know about them without directly using the PluginManager.

That's where there's a new concept of event subscribers provider (i.e. it provides event subscribers). Yes the name is long :/, maybe we can find better. So there's one implementation of SubscriberProviderInterface which is in the Piwik\Plugin namespace and which uses the plugin manager to find all the plugins. All that is easy code, it just takes a bit of time to visualize how it all fits together.

TL/DR: It works like before, except now there's no singleton, the event dispatcher is totally decoupled from Piwik and there's a bit more interfaces. Maybe I've gone too far and it's too abstract, let me know honestly what you think.

We could also move those classes to a separate component/repository, but in that case it would be better to rename some methods and change their signatures and that would break BC.

PS: why not using Symfony's dispatcher? Because we would need to write one object per event, which is not cool.

TODO:

  • [ ] write unit tests (which didn't exist previously because of the singleton)
@diosmosis commented on October 16th 2014 Member

Couple thoughts:

  • Do you think you could get rid of Piwik::addAction and replace w/ use of Subscribers w/ this pull request?
  • Will interfaces be necessary for DI objects? If so, do you think there's a way to reorganize core so there ends up being less clutter (ie, if every injectable type requires an interface)?
  • I think the 'SubscriberProvider' class may be unnecessary. When there is a DI container, we can simply gather all objects in the container that implement SubscriberInterface (correct?) and use those. I can't think of a use case for replacing this logic that couldn't be done a simpler way (eg, adding new subscribers can be done by just adding them to the DI container, and filtering subscribers can be done by allowing filtering logic to be injected).
@mnapoli commented on October 16th 2014 Member

@diosmosis

Do you think you could get rid of Piwik::addAction and replace w/ use of Subscribers w/ this pull request?

I'm not sure we can easily because subscribers are classes which declare the events they want to subscribe to. Observers are just callbacks registered manually (and addAction registers observers). So that would be a complete break of compatibility to remove altogether the concept of observers. And that would require changing all the calls to addAction to add new method to the classes etc.

I'm not sure it makes sense conceptually too. At its simplest form, an events triggers listeners which are callbacks. If there's a concept too much it's the subscriber one. But subscriber is extremely practical, removing it would be painful to use events in plugins. I would be a step backward for sure.

So I think that observers + subscribers is good, even though it's a bit confusing. We also have to consider that Symfony does exactly the same, so that means that this is not too bad.

Will interfaces be necessary for DI objects? If so, do you think there's a way to reorganize core so there ends up being less clutter (ie, if every injectable type requires an interface)?

No it's not necessary. Now that you mention it maybe we can remove the EventDispatcherInterface. The 2 other interfaces are however useful to decouple the event component from the rest of Piwik (though see the next paragraph).

I think the 'SubscriberProvider' class may be unnecessary. When there is a DI container, we can simply gather all objects in the container that implement SubscriberInterface (correct?) and use those.

We could simply add subscribers to the dispatcher when it is created. (that would be done by some class somewhere, probably a factory)
But as it was previously coded, plugins (i.e. subscribers) were re-fetched everytime (on each event) so that new plugins would be taken into account by the dispatcher.

So if we actually don't need to remove/add subscribers at runtime, then yes maybe we can get rid of it.

If we do, then either we use a concept of provider (so that subscribers can come from the plugin manager, or a container, or whatever), either we have a "factory" that creates the event dispatcher will all the subscribers and adds/removes subscribers when plugins are added/removed.

@diosmosis commented on October 16th 2014 Member

I'm not sure we can easily because subscribers are classes which declare the events they want to subscribe to.

I was referring to a change like this:

1) replace existing Piwik::addAction calls from

Piwik::addAction('Whatever.whatever', function () { ... });

to

class MyMeaningfullyNamedWhatever implements SubscriberInterface
{
    public function getListHooksRegistered()
    {
        return array('Whatever.whatever', 'thisIsMyCodeForWhatever');
    }

    public function thisIsMyCodeForWhatever()
    {
        ...
    }
}

2) then deprecate Piwik::addAction

The use of Piwik::addAction is hard to test and is prone to causing side effects for pretty much all of Piwik, so I'd welcome it's deprecation.

No it's not necessary.

That's good to know, though I was already thinking core was getting cluttered w/o the interfaces and there should be some new organizational principle. I have no problems w/ using meaningful interfaces, I was just hoping you'd been thinking something similar and maybe had some ideas ;)

So if we actually don't need to remove/add subscribers at runtime, then yes maybe we can get rid of it.

This would be useful in the long term (say if we eventually support an application server). BUT, if we get the instances from a DI container, then adding objects to the DI container at runtime would solve this issue, no?

If there's an issue w/ not having to search through the entire container on each event, we can add an internal caching layer to the DI container. So, if we get the objects by:

$diContainer->getObjectsImplementing('SubscriberInterface');

then add a new one somewhere w/

$diContainer->addObject(...);

The addObject method can internally unset the cache entry that was created when calling getObjectsImplementing(...).

In this example, there's no need to inject the logic to get subscribers.

@mnapoli commented on October 21st 2014 Member

I've removed the EventDispatcherInterface, will answer your points more thoroughly eventually. In the meantime, I'll wait to see how #6487 goes before maybe starting discussing this here

@diosmosis commented on October 21st 2014 Member

Like I said, I don't mind using interfaces (I use them in this branch for instance: https://github.com/piwik/piwik/tree/5363_cron_archive_full_concurrency ). I think eventually we will have to reorganize core if injectable types end up having related interfaces, just wanted to start a conversation on that.

@mnapoli commented on November 4th 2014 Member

Closing this PR for now

This Pull Request was closed on November 4th 2014
Powered by GitHub Issue Mirror