Refactor

[bakura10]

Hi everyone,

PS: Everyone is free to fork my branch and submit PR to it!

Motivation

Event manager has always been, with service manager, the slowest parts of ZF2, but the most widely used one internally. It was therefore highly important to make them faster.

Also, the event manager was a complex piece of software with a lot of obscure features, a strange shared manager concept and complex signatures.

Breaking changes

Massive performance improvements

I copy-paste some of the benchmarks I've made (the PHP7 build was an old one, so expect at least 10-15% improvments on PHP7):

ZF2 EVM

PHP 5.6

Bench\EventManagerBenchmark
Method Name          Iterations    Average Time      Ops/second
------------------  ------------  --------------    -------------
attach20Listeners : [1,000     ] [0.0005693485737] [1,756.39326]
trigger20Listeners: [1,000     ] [0.0010452888012] [956.67341]

PHP 7

Bench\EventManagerBenchmark
Method Name          Iterations    Average Time      Ops/second
------------------  ------------  --------------    -------------
attach20Listeners : [1,000     ] [0.0000875046253] [11,427.96734]
trigger20Listeners: [1,000     ] [0.0001518340111] [6,586.13965]

ZF3 EVM

PHP 5.6

Bench\EventManagerBenchmark
Method Name          Iterations    Average Time      Ops/second
------------------  ------------  --------------    -------------
attach20Listeners : [1,000     ] [0.0001252155304] [7,986.22980]
trigger20Listeners: [1,000     ] [0.0002773256302] [3,605.86939]

PHP 7

Bench\EventManagerBenchmark
Method Name          Iterations    Average Time      Ops/second
------------------  ------------  --------------    -------------
attach20Listeners : [1,000     ] [0.0000128707886] [77,695.31713]
trigger20Listeners: [1,000     ] [0.0000429670811] [23,273.63123]

No more shared event manager

I've explain myself why I think the shared event manager was a bad idea (although @Ocramius tried to show me the contrary, I think the event manager should stay simple as introduced in this PR).

As I used EVM in my app, I realized I ONLY used the Shared event manager. But the SEM is SLOW AS HELL and complex (because of identifiers, expensive merging...). I think that in most use cases, people should have a global EVM used throughout their applications, and have more precise event name.

Simplifications

Previously, trigger and triggerUntil were very complex. Depending on the order and types of parameters, you could use it in a LOT of different ways. Now everything has been enforced in interfaces, so there is one possible use case, which make things clearer and more efficient (less checks).

Event no longer has name

I've simplified the EventInterface so that it no longer has a getName method. I'm not against reverting it, but to me, we should trigger event like this:

$evm->trigger('event.name', $eventObject);

As an event object could be used for various event names, I think it makes more sense to be used like this (and avoid to re-add logic to check if the first parameter is a string or object... that kind of things).

propagationIsStopped has been renamed to isPropagationStopped.

Lazy listeners

One main issue with ZF2 was the inability to have lazy listeners. This means taht when you attach listeners, they are created, with all their dependencies, on every requests, even if you don't need them.

From my app, this can add a non-negligible overhead, and solutions like proxies are annoying to setup.

Some example usage:

<?php
    // The idea is that in ZF3, there will be one "application-wide" event manager registered. Nothing prevents you to
    // create your own local event manager, but to replicate the shared manager behaviour, you should retrieve the
    // application-wide evm

    // Attaching a simple listener:

    $eventManager = new EventManager();
    $eventManager->attach('foo', function() {}, $priority);

    // Triggering a listener
    $eventManager->trigger('foo', new Event());

    // Triggering a listener with a callable that will stop propagation if return to true
    $eventManager->triggerUntil('foo', new Event(), function() {});

    // Attaching to wildcard
    $eventManager->attach('*', function() {});

    // Attaching a listener and detaching it
    $listener = $eventManager->attach('foo', function() {});
    $eventManager->detach('foo', $listener);

    // Detaching all listeners from a given event
    $eventManager->detach('foo');


    // Using listener aggregate to aggregate multiple events, lazily
    class MyListenerAggregate implements ListenerAggregateInterface
    {
        public static function attachAggregate(EventManager $evm)
        {
            // No more confusion of getting the shared manager. Note the method is static, and
            // that we pass a "spec" without instantiating the listener, as it will be created only
            // on demand
            $evm->attach('foo', [static::class, 'myMethod']);
        }

        public function myMethod(Event $event)
        {

        }
    }

    // In the module class
    $evm = $serviceLocator->get(EventManager::class);
    MyListenerAggregate::attachAggregate($evm); // Maybe should be called "bind" instad of attachAggregate

    // No more identifiers, no more SEM... Of course, events must now be scoped, so no "created" event anymore,
    // but "user.created", "application.dispatch", "controller.dispatch"...

?>

[bakura10]

It has been rebased.

[Slamdunk]

Hi, I'm using ZF2 since 2012 for our core business app (bank credit reporting), and we use it as a whole framework.
I am not aware of the current module ecosystem, and flexibility is not a concern for my business, so my next thoughts are biased this way.

The main issue I found on current EVM+SEM is that setIdentifiers is not a standard nor a public contract, and so not reliable at all.
For example AbstractController::setIdentifiers differs greatly from ModuleManager::setIdentifiers.

Identifiers are just strings: when (seldom) my listener needs a strong bind, I still use Interface hinting:

public function attach(EventManagerInterface $events)
{
    $event->attach('i_choose_the_event', function(Event $e){
        if (! $e->getTarget() instanceof MyInterface) {
            // skip
            return;
        }

        // do stuff
    });
}

Otherwise, the fact that listener choose itself the event name is enough.

Morover EventManager::setSharedManager typehints SharedEventManagerInterface and not the singleton StaticEventManager (thanks god).
So in the end:

1. EVM+SEM used in Zend\Mvc are already acting as a single EVM
2. The fact that the SEM used is not a singleton, forces to fetch the current EVM instance (plus ->getSharedManager()) that will be triggered, while attaching listeners
3. And this is exactly the same as injecting the right EVM after class construcion

The ControllerManager setSharedManager is just another way to see the only-one-EVM + injecting.

The current ViewManager onBootstrap to me sums up all the flaws the current approach has: why manual attaching, when the those listener are ListenerAggregateInterface? I know the answer, but likely not everyone does.

@weierophinney

What many are unaware of at this point is the amount of research that went into the current solution, which incorporates aspects of event systems, intercepting filters, signal slots, and aspect oriented programming.

You are absolutely right: as a reader (and learner from) zf2 source code since 2012 I am still unaware, so how can a new developer knowledge it when he faces the confusion over ViewManager example?

Working on medium team with ZF2 and a big application, the only way we found to ensure a complex EVM-using application to be easly understood and developed and mainteined is:

• Always delegate to listener the choice of the $eventName (or specification)
• Only one trigger call per $eventName (so split EVENT_DISPATCH)
• If needed, do Interface typehint within the callback
• Many small events are better than one event with identifier-based splitting of the listener

Maintainability before flexibility.

Doing so, the only way an unexpected error can raise, or hide, is in the correct EVM propagation, that is much easier to control.

Yet the backward compatibility is a problem: luckily I'm not in charge to take this decision, but as a user of the framework in a core business app I do not expect that nothing changes between big version numbers. The opposite: if a better design is reached and can help my business design to be better, it's ok.
2 to 3 should not be just a bugfix release + component split.

[Ocramius]

Reminder: if the SharedEventManager is gone, then I see no point in using Zend\EventManager: I'd rather use Symfony\Component\EventDispatcher or doctrine/common instead.

The SEM is the only reason why I use this dispatcher instead of others.

That's all I have to say on the SharedEventManager discussion.

[codeliner]

@Ocramius Do the other dispatchers provide a triggerUntil functionality?

[Slamdunk]

I realize that I talked too much about Zend Framework as a whole instead of Zend\EventManager as a single component.

I should clarify that:

1. EVM+SEM+ids may be very powerful and useful for someone / in some context
2. There is no need to strictly change Zend\EventManager component: if you don't want to use SEM, you can ignore it (I do and I live happy with normal EVM in my business)
3. I see no need to use SEM in Zend\Mvc, but as long as zf2#3651 is still a requirement, it can't be changed, although I think if you inject a custom EVM you don't want the Zend\Mvc::SEM, and if you want it you can previously fetch the Zend\Mvc::EVM to solve the injection issue

But probably I have a too strict POV.

[weierophinney]

propagationIsStopped has been renamed to isPropagationStopped

Why?

The original follows natural language better:

if ($event->propagationIsStopped()) {
    // ...
}

(Read it allowed: "if event propagation is stopped.")

[weierophinney]

we should trigger event like this:

$evm->trigger('event.name', $eventObject);

This is a massive BC break.

• Users now are required to create the event instance.
  • Which means importing an Event class into their code.
  • Which means adding logic to instantiate the event instance to their code.
• What happens if they wanted to add a callable for short-circuiting?

As per previous comments, I'm okay with BC breaks for major versions. However, they MUST be accompanied by a detailed migration plan. I'm afraid this particular change will not have a trivial migration plan, which makes it essentially untenable for many developers/organizations. We want people to migrate. People will not migrate if doing so requires massive changes in their applications.

We must always keep an eye on migration. I know it's not fun. But if we want people to migrate and actually use our code, it's a requirement. (It's also a fun challenge!)

[weierophinney]

I liked the idea of the SharedEventManager and the concept of providing some kind of aspect oriented programming style. But I think @bakura10 is right. We pay a high price for that namely a very slow central compoent slowing down the entire system.

@codeliner and @bakura10 — I understand your concerns. But the changes proposed mean nobody will be able to migrate reasonably to the new architecture. Perhaps instead of focusing on a change that is not only non-BC but impossible to migrate to, effort should be put into improving the performance of the existing codebase?

Here is why I'm concerned: The combination of removing the SEM and changing how events are triggered impacts not only the MVC, but any listeners written for ZF2, and any code written for ZF2 that triggers events. In other words, anything remotely event-related now needs to be rewritten. Just about every module in the ecosystem uses the SEM. Many modules are triggering their own events (look at ZF-Commons or zfcampus modules, as examples).

Additionally, the SEM iterates over all identifiers present, which means that a single trigger call can invoke listeners on a wide variety of identifiers. As an example, let's consider a REST controller from Apigility; in the current EM incarnation, a single trigger will invoke listeners attached to any of the following identifiers:

• Zend\Stdlib\DispatchableInterface
• Zend\Controller\AbstractController
• Zend\Controller\AbstractActionController
• Zend\Controller\AbstractRestfulController
• ZF\Rest\RestController
• and, finally, the actual controller name.

This gives a huge amount of flexibility, while retaining the simplicity of a single call when triggering. The current proposal changes this immensely. If I call trigger, I can offer only a single named event, and because that name is tied to a single FQCN, I don't have the kind of granularity I had before. As an example, given the dispatch event, I cannot have a listener that only listens to ZF\Rest\RestController dispatch events. This breaks a ton of functionality — particularly as the proposed changes do not incorporate any capability of querying the target (the object triggering the event); I simply cannot know if I was triggered because I'm a ZF\Rest\RestController, or if I'm a generic AbstractController. As such, many of the most useful paths for using events are no longer possible.

To all those saying there's no need to use the SEM in the MVC, the above is a concrete counter-argument.

This proposal has no defined way for developers to migrate, and, I'd argue, there's no way to script it, either. Additionally, with the granularity of triggering removed, it makes the EM solution far less useful and flexible.

[codeliner]

@weierophinney 👍 I agree. Thx for the detailed use case. I will play a bit with my idea stated above to see if I can provide a solution.

[bakura10]

Hi :),

I'm sorry but I'm still not able to understand the use case (sorry...). I still don't understand which value the identifiers bring. You are giving me an example from Apigility, but once again if your event is called "mvc.dispatch", and that all controllers attach to it using a global event manager (which is what I suggest), this will have the exact same result, without the introduced complexity.

As I said, I can't see yet a single benefits of the identifier mechanisms (at least not in the MVC). Also, your example still suffer from one major issue I see from identifiers: if someone ever decide to extend one of your controller, and override the identifiers, you have broken everything. Identifiers is really a fragile concept that is better achieved using namespaced event names.

[codeliner]

@bakura10 The limitation of namespaced events is that they don't work well for inheritance chains like shown by @weierophinney. The "mvc.dispatch" event is triggered by Zend\Controller\AbstractController. ZF\Rest\RestController extends Zend\Controller\AbstractController and therefor triggers the exact same mvc.dispatch. How would a content negotiation listener be able to attach to ZF\Rest\RestController.mvc.dispatch events only?

I see two possibilties:

1. Such a listener would need to perform $event->getTarget() instanceof ZF\Rest\RestController check and return early if it evaluates to false
2. or identifiers are replaced by tags, so a ZF\Rest\RestController would need to add its FQCN as a tag to a eventTagCollection managed by the Zend\Controller\AbstractController and this eventTagCollection would then be passed to EVM::trigger('mvc.dispatch', $target, $args, $this->eventTagCollection) by the Zend\Controller\AbstractController.

Disadvantage of 1:

All listeners need to be triggered (performance problem with regards to the lazy listeners feature)

Advantage and Disadvantage of 2:

SEM is replaced with a global EVM, so the trigger interface is unified and therefor simpler but behind the scenes it is just as fragile as before and it requires migration effort.

IMHO

A global EVM with a

• unified trigger interface,
• lazy listeners,
• attaching listeners via configuration,
• attach to namespaced events with the ability to filter it even more down by using tags

... would be my favorite too.
But I can't weight it against keeping BC, because currently I don't know what ZF 3.0 really means. Is it just the component split? Will the mvc component be replaced by middleware in the long run? How will MVC + modules + EVM + ServiceManager + middleware work together?
Answers to these questions would help, because if zend-mvc is refactored anyway why not refactor zend-eventmanager too?

[weierophinney]

I'm sorry but I'm still not able to understand the use case (sorry...). I still don't understand which value the identifiers bring. You are giving me an example from Apigility, but once again if your event is called "mvc.dispatch", and that all controllers attach to it using a global event manager (which is what I suggest), this will have the exact same result, without the introduced complexity.

Let me explain the example a bit more.

Let's say I have the following:

$shared->attach('ZF\Rest\RestController', 'dispatch', $listener);

In your proposed refactor, this would need to become:

$events->attach('mvc.dispatch', $listener);

And they are not at all equivalent.

In the current incarnation, any controller can trigger the dispatch event; however, only instances of the EM that have the ZF\Rest\RestController identifier will trigger the listener specified above. That's how identifiers work; they are used by the EM to determine which listeners in the shared manager to invoke. Essentially, the identifier system is a filtering system for determining which listeners are relevant to the trigger context.

How would this work in your refactor? Right now, any object that triggers the mvc.dispatch event triggers every single listener attached to that event, and there is no way to opt-out/filter; event instances do not even have target awareness!

The above example is an example from the MVC, and it is widely used. We use it extensively in Apigility, I've seen it in use in the Zfc modules, and I see it used in a ton of modules listed on modules.zendframework.com. The technique allows doing lazy listeners without having access to either the controller instance or the EM instance it would compose. It also simultaneously prevents naming collisions (multiple objects can trigger events of the same name, without needing to worry about namespacing), while allowing groups of related objects to trigger the same event.

My point is that just because you don't see a need for it or understand it does not mean the feature is not important and/or integral to the system. In fact, you're likely relying on it without even realizing in many of the modules you already used, particularly anything in the MVC. This is why I'm adamant that the feature must be retained; it's a differentiator from other systems, and a feature that is fundamental to our MVC implementation.

[weierophinney]

But I can't weight it against keeping BC, because currently I don't know what ZF 3.0 really means. Is it just the component split? Will the mvc component be replaced by middleware in the long run? How will MVC + modules + EVM + ServiceManager + middleware work together?
Answers to these questions would help, because if zend-mvc is refactored anyway why not refactor zend-eventmanager too?

As noted on the ZF3 roadmap post, the following are the key components of ZF3:

• Component split (already done!)
• PSR-7 implementation and integration (implementation is done; integration is in progress)
• Addition of a middleware implementation (done) and a slim, limited microframework built on it (in progress)
• Performance/consistency improvements to the current MVC (which include proposed refactors to the SM and EM primarily, but also a few other components).

Essentially, the ZF2 MVC will be retained in a mostly BC way to ease migration. However, it will be PSR-7 middleware-aware to allow composing middleware from other projects and components. Additionally, we'll be providing middleware wrappers around ZF1 and ZF2 bootstraps to allow invoking them as middleware (which allows segregating different application aspects, and migrating from the ZF1 and ZF2 MVC to middleware-based solutions).

But, again, we are not proposing major changes to the current MVC. Many people are building successful, complex applications using it, and are happy with the solution; we want to continue enabling these solutions, without providing an onerous upgrade path.

[codeliner]

But, again, we are not proposing major changes to the current MVC. Many people are building successful, complex applications using it, and are happy with the solution; we want to continue enabling these solutions, without providing an onerous upgrade path.

@weierophinney Thanks! One of the most important information for my own project (which heavily depends on the ZF2 module system). I'm very happy with the module manager, I ❤️ the ServiceManager and I can live with your proposed new version of the SEM & EVM :-)

[bakura10]

@weierophinney , I'm just trying to understand the use case. Indeed, from the few talks I have given and the discussion I had, most people found this one confusign because indeed, they were ALL using the SEM only, giving therefore the feeling that if the common use case must be solved using the complex method, then the common use case should instead be solved using the simple method, and the complex method using the complex method. That's why I'm advocating for making the EVM shared by default, so that the expected, logical and common behaviour can be achieved without the SEM (that means making the main EVM shared).

Now, I could understand that the SEM could have an interest.

In the current incarnation, any controller can trigger the dispatch event; however, only instances of the EM that have the ZF\Rest\RestController identifier will trigger the listener specified above. That's how identifiers work; they are used by the EM to determine which listeners in the shared manager to invoke. Essentially, the identifier system is a filtering system for determining which listeners are relevant to the trigger context.

So why not using a new event like "rest_mvc.dispatch"? If events are properly namespaced, your use case can be better solved using namespaced event names. If you attach an event for the dispatch event JUST for EVM that has a specific listeners, then to my understanding, you should simply use a new event, like "apigility.mvc.dispatch". If you want to hook to the global mvc.dispatch, you can do so. This solve the use case much more elegantly, because attaching a listener from third party code no longer require the fragile logic of identifiers, but instead rely on simple event names, that can be documented more easily.

Once again, my main problem with identifier is the fragility. Currently, identifiers are set automatically using the class name. Moving the event manager that trigger the event from one class to another automatically modify the identifier.

For your specific use case, I think that tags would be be a better solution:

$evm->attach('dispatch', $callable, 'my_tag');

The tag seems to be a better alternative, because the tag is like an "identifier" but is indepdnant of the calling class.

[bakura10]

Once again, sorry if I seem rude on this, but no longer how hard I try most of your use case can be replicated using a simpler and more logical code (in my opinion).

For me, I was ALWAYS frustrated that attaching a listener to "dispatch" does not allow to listen to the event that are attached in the controller, and that for that, I need to instead retrieve the SEM, attach an event using a specific identifier (Zend\Mvc\DispatchableInterface or something like that). I mean, what's the logic behind this? Why do I need to go through this complex path? If you wanted to segregate those two events from the beginning, then they should have two different names (because indeed they are not the same event), like "mvc.dispatch", and "controller.dispatch". It makes the distinction clear, you use the same logic to attach a listener (retrieve the shared EVM and attach a listener).

Trust me, this is really a major pain, and SEM solves this problem in a very complex and unelegant way.

I can definitely see a use case for the SEM that ocramius tried to explain me long time ago with a shipping example, and for that specific case I can definitely understand the usage of SEM, but once again: complex problem can be solved with compelx tools like SEM, but not the oppposite.

[weierophinney]

@bakura10 Part of the problem here is that we're talking around two separate events entirely, but which happen to have the same name: "dispatch". This was an unfortunate decision on our part, and it's clearly muddying the discussion here.

Let's ignore the dispatch event triggered by Zend\Mvc\Application for now, as it's not relevant to the SEM discussion; that particular event is one you always tie directly to instead of via the SEM.

What's of interest here is the dispatch event triggered by controllers, as triggered in Zend\Mvc\Controller\AbstractController here. This is the one where things get interesting.

Going back to my original example, let's say I have MyController, which extends ZF\Rest\RestController, which in turn extends Zend\Mvc\Controller\AbstractRestfulController, which extends AbstractController. When the EM instance is injected, the following identifiers are injected in it:

• Zend\Stdlib\DispatchableInterface
• Zend\Mvc\Controller\AbstractController
• Zend\Mvc\Controller\AbstractRestfulController
• ZF\Rest\RestController
• MyController

In none of these is the dispatch() method overridden in such a way that the trigger() statement linked above is not executed; in other words, that single trigger() statement applies to each implementation in the hierarchy.

As a consumer, I could do the following:

• Attach to the identifer Zend\Stdlib\DispatchableInterface. If I do this, essentially any controller in the MVC that triggers a dispatch event will trigger this listener.
• Attach to the identifier Zend\Mvc\Controller\AbstractController. In this case, any subclass of that abstract class will trigger the listener — but not a generic, vanilla implementation of DispatchableInterface.
• Attach to the identifier Zend\Mvc\Controller\AbstractRestfulController. In this case, only subclasses of the AbstractRestfulController will be triggered.
• Attach to the identifier ZF\Rest\RestController. This further narrows things; a generic AbstractController or AbstractRestfulController implementation will not trigger my listener.
• Attach only to MyController, narrowing further the number of controllers which will trigger the listener to essentially only this one. (In the case of Apigility, this might apply to later versions of the service, however.)

What is interesting about this is that I can have a variety of listeners, attached at varying degrees of granularity. I might have one attached to any AbstractController that checks to see if a view model was created, and, if so, determine the view renderer to register based on it. Another might attach to AbstractRestfulController and try and populate body parameters by parsing the request body based on the Content-Type. Finally, for ZF\Rest\RestController implementations, I'd have one that determines the view model to create based on the Accept header. If the currently selected controller descends from ZF\Rest\RestController, all of the listeners will be fired; on the flip side, if the controller matched is a general action controller, only the first listener will trigger. This is the granularity to which I refer.

In none of the various steps of the inheritance tree do we need to change the trigger statement; the only thing that changes is the set of identifiers that I register with the EM instance (which can be achieved via either a property or overriding the setter for the EM instance; we use properties currently). Similarly, when registering listeners, I do not need to add the listener to a growing number of events to ensure it triggers for all the contexts in which it will be useful.

You indicate you can do all this simpler, but when I tried playing with your implementation, I never determined a way to accomplish this granularity without doing one or both of the following:

• Overriding the trigger statement to trigger a different event (or using a property to indicate the event to trigger).
• Overriding where I attach to attach to a growing list of contexts (e.g., ['dispatch', 'rest.dispatch', 'zf-rest.dispatch']).

In the latter case, this quickly became untenable, as it required general, framework-level listeners to know about application-specific contexts.

So, if you truly were able to achieve this stuff more simply, please, please, please share how you're doing it, because I cannot recreate it without using a shared manager at this point.

It's possible that a "tags" implementation would also work; similar to how we do identifiers, the tags could be defined as a property of the class, so the trigger statement could be generic. However, as noted elsewhere, this completely breaks the current usage of the component, as the signatures are completely different, and the shared manager goes away, making migration a nightmare. Every single place the SEM is used to attach listeners would need to change (this is likely scriptable), and every single place a trigger() is invoked would need to change (this is not, as there's no easy way to determine what the tags should be from the context where the trigger occurs).

[weierophinney]

Closing in favor of #4, which retains the differentiating functionality present in the v2 EM, while getting the performance benefits originally outlined in this pull request.