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).

@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.

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.

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

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.

Closing this PR for now