EventManager refactor for version 3

[weierophinney]

This patch refactors zend-eventmanager with the following goals:

• Maintainability.
  • It removes methods where they have no clear purpose or are redundant.
  • It simplifies signatures where arguments have multiple possible types or meanings; if necessary, it creates new methods when multiple signatures are deemed useful.
• Performance.
  • Queues are inlined into the implementations whenever possible to promote performance.
  • Arrays are preferred over objects for queues due to the speed of array manipulation in PHP.
  • Features such as lazy listeners were added to help users further optimize their event manager usage.
  • A benchmarking suite using Athletic was written to help gauge the differences between versions and approaches.
• Usability.
  • Whenever possible, signatures were refactored to have a single set of valid arguments, and a single execution path, providing reliability and easing the ability to learn usage.
  • Documentation was written alongside the implementation to aid in identifying difficult and redundant usage areas.

Alongside these goals, we also tried to minimize migration for end users. To this end, migration document was written, and a 2.6.0 release is planned in order to deprecate methods as well as add implementation methods to the EventManager that will aid in helping users prepare their code for version 3.

Listeners are executed exactly as they were in version 2, using the same priorities and precedence between local explicit and wildcard listeners, and shared and shared wildcard listeners. Tests were added to describe and document these rules.

Change synopsis

Removed functionality

• GlobalEventManager and StaticEventManager are removed (with prejudice!).
• ProvidesEvents, which was previously deprecated, is removed.
• EventManagerInterface::setSharedManager() is removed. Shared managers are now expected to be injected during instantiation.
• EventManagerInterface::getEvents() and getListeners() are removed; they had now purpose within the implementation.
• EventManagerInterface::setEventClass() was renamed to setEventPrototype(), which now expects an EventInterface instance. That instance will be cloned whenever a new event is created.
• EventManagerInterface::attachAggregate() and detachAggregate() are removed. Users should use the attach() and detach() methods of the aggregates themselves.
• SharedEventAggregateAwareInterface and SharedListenerAggregateInterface are removed. This was an undocumented and largely unused feature.
• SharedEventManagerAwareInterface is removed. A new interface, SharedEventsCapableInterface defines the getSharedManager() method from the interface, and EventManagerInterface extends that new interface.
• SharedEventManagerInterface::getEvents() is removed, as it had no purpose in the implementation.

Changed signatures

• EventManager::__construct() now accepts an optional SharedEventManagerInterface instance as the first argument, and an optional array of identifiers as the second. As identifiers have no meaning without a shared manager present, they are secondary to providing the shared manager.
• EventManagerInterface::trigger() changes its signature to trigger($eventName, $target = null, $argv = []); each argument has exactly one possible meaning; the $eventName can only be a string event name. The fourth $callback argument is removed.
• EventManagerInterface::triggerUntil() changes its signature to triggerUntil(callable $callback, $eventName, $target = null, $argv = null). Each argument has exactly one meaning.
• EventManagerInterface adds two new methods for triggering provided EventInterface arguments: triggerEvent(EventInterface $event) and triggerEventUntil(callable $callback, EventInterface $event).
• EventManagerInterface::attach() and detach() change their signatures to attach($eventName, callable $listener, $priority = 1) and detach(callable $listener, $eventName = null), respectively. Note that $eventName can now only be a string event name, not an array or Traversable.
• EventManagerInterface::setIdentifiers() and addIdentifiers() change their signatures to each only accept an array of identifiers.
• SharedEventManagerInterface::getListeners() changes signature to getListeners(array $identifiers, $eventName) and now guarantees return of an array. Note that the second argument is now required.
• SharedEventManagerInterface::attach() changes signature to attach($identifier, $eventName, callable $listener, $priority = 1). The $identifier and $eventName must be strings.
• SharedEventManagerInterface::detach() changes signature to detach(callable $listener, $identifier = null, $eventName = null); $identifier and $eventName must be strings if passed.
• ListenerAggregateInterface::attach() adds an optional $priority = 1 argument. This was used already in v2, but not dictated by the interface.
• FilterInterface::attach() and detach() have changed signature to attach(callable $callback) and detach(callable $ilter), respectively.
• FilterIterator::insert() has been modified to raise an exception if the value provided is not a callable.
• ResponseCollection::setStopped() no longer implements a fluent interface.

New functionality

• LazyListener allows wrapping:
  • fetching a listener service from a container-interop container, and
  • invoking a designated listener method with the provided event.
• LazyEventListener extends LazyListener, and provides metadata for discovering the intended event name and priority at which to attach the lazy listener; these are consumed by:
• LazyListenerAggregate, which, provided a list of LazyEventListeners and/or definitions to use to create them, acts as an aggregate for attaching a number of such listeners at once.

Benchmarks

Benchmarks are listed in a gist. In particular:

• benchmark of version 2
• benchmark of final version of this implementation

The highlight is the MultipleEventMultipleLocalAndSharedListener benchmark, which demonstrates a real-world usage scenario in zend-mvc with many local and shared listeners, and many events being triggered. This benchmark jumped from 435 operations per second in version 2 to 1265 operations per second in this implementation — a change of ~300%! Considering this represents 10–50X the number of events triggered in a reasonably sized zend-mvc application, performance will be in the sub-millisecond range.

[weierophinney]

Ping @akrabat @Ocramius @bakura10 @codeliner - this is an alternate approach that retains BC, but improves performance radically.

[bakura10]

I will have a look at the code but does this PR contains the lazy listener feature of my refactor? This is actually what I consider the most important feature of my refactor and is the key for intensive use of Evm without having to revert to complex solutions.

[weierophinney]

Not yet, but I'd like to get that feature in; I think it would simplify
listener attachment a ton.
On Jul 2, 2015 5:07 PM, "Michaël Gallego" notifications@github.com wrote:

I will have a look at the code but does this PR contains the lazy listener
feature of my refactor? This is actually what I consider the most important
feature of my refactor and is the key for intensive use of Evm without
having to revert to complex solutions.

—
Reply to this email directly or view it on GitHub
#4 (comment)
.

[gianarb]

178 additions and 386 deletions.
This PR starts well 👍

[bakura10]

A lot more could be removed imho:

• $eventClass (what's the point of it? assuming Zend\EventManager\Event by default is enough, and remove one confusing method and property... and if they need to override it, they can do it using a custom event, which is better).
• the trigger method is still awfully complex. What about this:

 public function trigger($event, $target = null, $argv = array(), callable $callback = null)
    {
        if ($event instanceof EventInterface) {
            $e        = $event;
            $event    = $e->getName();
            $callback = $target;
        } else {
            $e = new Event();
            $e->setName($event);
            $e->setTarget($target);
            $e->setParams($argv);
        }

        // Initial value of stop propagation flag should be false
        $e->stopPropagation(false);
        return $this->triggerListeners($e, $callback);
    }

• prepareArgs: is this really needed? I think we should encourage people to create their event object instead of relying on this.

In overall I'm not against keeping the SEM, but for me the problem is that currently, the use case for SEM is what most people want but it is complicated to use. If SEM stays here for most complex usage, why not, but the most simple use case should be the default one. That's why I think EVM SHOULD be a shared service, to simply allow attaching without having to revert to the complexity of SEM.

[codeliner]

@weierophinney 👍 for the performance improvments. That + BC is a strong argument for keeping the SEM.

But I see problems with removing EVM::addIdentifiers because

class MyClass extends AbstractEventTriggeringClass
{
    public function setEventManager(EventManagerInterface $evm) {
        parent::setEventManager($evm);
        $evm->addIdentifiers([MyClass::class]);
    }
}

... is no longer possible and adds even more complexity to the game because now MyClass needs to be aware of the identifiers used in AbstractEventTriggeringClass and all the way down.

AND

Any shared listeners attached after the instance is created will not be executed. This means attachment of shared listeners should be done early, preferably during application bootstrap, but really anytime before an instance is fetched from the service manager.

That sounds like a debugging nightmare! How do you want to document it so that EVERY developer is aware of it?
I'm not completely against it but the question is for whom the zend framework is made for? A small group of experts who always know what they are doing or a larger group of PHP developers who are fighting against deadlines, inaccurate requirements, ... every day and therefor need tools / a framework / components which just work!

[weierophinney]

@codeliner Thanks for the feedback! I've updated to re-add addIdentifiers(), which necessitated deferring the attachment of shared listeners to the first trigger() operation. This actually solves a lingering doubt I had with the original refactor, as aggregating shared listeners early leads to some potential order-of-operations issues (which you confirmed in your comment). This also simplified the logic for aggregating wildcard events — and posed no changes to the initial performance improvements I observed!

(However, if you attempt to add identifiers or a shared manager after the first trigger operation an exception is now raised. I consider those situations to be edge cases in the current implementation, and feel raising the exception is a reasonable change at this time.)

It also addresses your second point. It's rare (in fact, I cannot think of any time it happens in the current MVC) that either identifiers or additional shared listeners will be added after you have triggered the first event. However, it's not uncommon that you'll still be setting up both after you initialize the EM instance. As such, the changes in the latest commit address those issues.

[weierophinney]

$eventClass (what's the point of it? assuming Zend\EventManager\Event by default is enough, and remove one confusing method and property... and if they need to override it, they can do it using a custom event, which is better).

The point of it is to allow a default event implementation per instance; as an example, a DB component could have a DbEvent that has additional methods and/or defaults. However, that could also be accomplished by sub-classing the EM.

the trigger method is still awfully complex.

Your recommendation looks reasonable, but before I incorporate it, I'm going to survey how trigger() is called in a number of projects to make sure this wouldn't be removing use cases that are more widely used.

prepareArgs: is this really needed? I think we should encourage people to create their event object instead of relying on this.

The idea behind prepareArgs() is to create an object containing the event arguments, which allows updating the individual parameters by reference. It's actually used in a fair number of places, but it may be something that can be accomplished better in other ways.

[akrabat]

I like the approach. Will try and play with the code on Sunday / early next week.

[codeliner]

@weierophinney 👍

However, if you attempt to add identifiers or a shared manager after the first trigger operation an exception is now raised. I consider those situations to be edge cases in the current implementation, and feel raising the exception is a reasonable change at this time.

Agreed. At least as long as you use PHP's shared nothing "feature". Running an app in a node.js like event loop (f.e. with reactphp) is a different story. In this case I can imagine that you may want to detach a listener (which is not possible with this refacor!) and add another one -> hot plug in.
But that's future. Don't know if you plan to support event loops with zend-diactoros. After moving some ideas from node to PHP this would be the next logical step for me.

[samsonasik]

$argv = array() can be $argv = []

[weierophinney]

I'll run it through the new CS rules when complete; right now, I'm mainly
testing some concepts for refactoring.
On Jul 3, 2015 4:50 PM, "Abdul Malik Ikhsan" notifications@github.com
wrote:

In src/EventManager.php
#4 (comment)
:

@@ -174,8 +164,10 @@ public function addIdentifiers($identifiers)
* @return ResponseCollection All listener return values
* @throws Exception\InvalidCallbackException
*/

• public function trigger($event, $target = null, $argv = [], $callback = null)
• public function trigger($event, $target = null, $argv = array(), callable $callback = null)

$argv = array() can be $argv = []

—
Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-eventmanager/pull/4/files#r33881585
.

[weierophinney]

In this case I can imagine that you may want to detach a listener (which is not possible with this refacor!) and add another one -> hot plug in.
But that's future.

This would not be hard to accomplish at this point. The logic would be far easier than we have currently, as you only need the callable.

Don't know if you plan to support event loops with zend-diactoros. After moving some ideas from node to PHP this would be the next logical step for me.

Diactoros doesn't need them, really; its primary focus is implementation of the HTTP message interfaces. (The Server implementation in Diactoros is essentially a nod at the front-controller pattern currently.) I'd put any idea of an event loop in a different package, to be honest. But it is definitely something we want to target eventually.

[weierophinney]

Odd failure on HHVM:

• https://travis-ci.org/zendframework/zend-eventmanager/jobs/80916792#L214

However, tests passed (segfault was within PHPUnit after test completion).

At this point, we have excellent test coverage. Creating a few TODO items, and then we can start reviewing for merge.

[weierophinney]

I took a look at the "lazy listener" functionality as proposed by @bakura10, and I'm not convinced by the implementation. It essentially requires:

• A static class method, attachAggregate()
• In which it will attach using pseudo-callback notation: [self::class, 'onWhatever']
• and the EventManager will determine it's non-callable at trigger() and instantiate the listener class.

I have problems with this for a few reasons:

• I'd rather we hint on callable for the listeners. This makes the code more robust, as we can push validation of the argument to PHP itself, instead of having to validate it.
• It leads to complex logic when iterating listeners; we have to determine if it's callable, and, if not, try and create something that is out of the provided listener. Then, for performance, we also need to cache it.
• Finally, and the biggest nail in the coffin as far as I'm concerned, we lose a primary benefit of aggregate listeners: they allowed us to have objects with dependencies that we could attach as listeners. For example, in Apigility, we have a number of listeners with dependencies on things like the Authentication component, a database, etc.; these are defined as services, and attached as aggregates to the EventManager, giving us stateful listeners. While this can be accomplished without aggregates, it seems ridiculous to change aggregates to only allow static listeners.

However, I think we already can achieve lazy listeners, due to something I've already alluded to: listeners must be callables. This allows creating lazy listeners via closures!

$events->attach('foo', function ($e) use ($container) {
    return $container->get('MyCustomListener')->onWhatever($e);
});

Essentially, if we want lazy listeners, we can wrap the logic in closures, pushing the performance hit to when the listener is actually triggered. With PHP 7, it would be relatively simple to achieve lazy aggregates as well via anonymous classes.

As such, I'm ticking off the box "lazy services", and will move to the other items on the list.

[weierophinney]

@codeliner — I've looked into what it would take for the detach() logic, and it's very complex, as it turns out. It would require us to store the priority for each listener internally alongside the listener so that we can rebuild the priority queue after locating and removing the given listener.

I have implemented EventManager::clearListeners($event) and SharedEventManager::clearListeners($identifier, $event = null), however, and my inclination is that if you do need to worry about removing listeners on-demand, you could do so by fetching the listeners (using getListeners($event)), clearing listeners, removing members from the list, and re-injecting in the order in which they were returnd to you. It's ugly, but works:

// We'll assume `$callback` is a listener we want to remove:
$listeners = $events->getListeners('event-of-interest');
$events->clearListeners('event-of-interest');
foreach ($listeners as $listener) {
    if ($listener === $callback) {
        continue;
    }
    $events->attach('event-of-interest', $listener);
}

It will work:

• During first trigger(), we attach shared listeners directly to the EM instance; this means that getListeners() will return the shared listeners as well after that first trigger().
• After first trigger(), we never re-attach shared listeners.
• Clearing listeners will thus remove both local and shared listeners on that event.
• However, you can always add local listeners.

So, the above will work. Where it gets dicey is priority.

You'll be iterating them in priority order, so re-attaching them preserves the original order. The problem is if you attach other listeners later, using priority: at this time, they'll all be added with priority 1; we will have no record of the original priority. As such, any priority you specify later when attaching listeners will be relative to the default of 1.

And that's why I don't want to re-add this facility; it requires we store the priority alongside the listener, inflating storage, making getting at the listeners harder (we're essentially back to the CallbackHandler solution), and making the internals overall far more complex, leading us back to performance issues.

tl;dr: Essentially, if you want a solution that has both priority and the ability to detach, you lose performance.

[bakura10]

@weierophinney , actually the hit is when you register aggregate listener. Aggregate listener is the one that do the attach, but the dependencies are part of the aggregate listener. This is where you have the performance it.

This would mean wrapping the aggregate listener, but if you do that, the aggregate listener do not have the chance to register its listeners.

[weierophinney]

@bakura10 Attaching has very little measurable performance impact, though (particularly with the revisions in this PR); where are you arriving at the idea that it incurs a performance hit?

[bakura10]

The expensive part is creating all the listener aggregates, which all needs to pull many dependencies from service manager. This is what incurs a performance hit, not the attaching itself :).

[bakura10]

When you do that:

$aggregate = $sm->get(UserListenerAggregate::class);
$aggregate->attach();

This is the "get" that is expensive. But becuase you need to create the lsitener aggregate to bind events, you have no other choice. That's what my PR tried to tackled with the static stuff :). Being able to attach events WITHOUT the expesnive part of creating the aggregate

[weierophinney]

@bakura10 —

When you do that:

$aggregate = $sm->get(UserListenerAggregate::class);
$aggregate->attach();

This is the "get" that is expensive. But becuase you need to create the lsitener aggregate to bind events, you have no other choice.

Understood. The static approach doesn't fix that, however; if you use that approach, of course it will be faster, because you're not fetching anything from the service manager! And that makes it less usable, because you can no longer have aggregate listeners that have dependencies.

Which brings me back to my earlier suggestion: if the aggregate is only attaching to a single event, wrapping the logic of pulling it from the container will work now; just bypass the attachAggregate() call entirely, and attach the closure directly to the EM.

For true aggregates that attach many listeners, you would need a different approach. But that can be done in userland as well, with a little finesse:

class ThisIsAnAggregate implements ListenerAggregateInterface
{
    public static $attachTo = [
        [ 'event' => 'event1', 'method' => 'onEvent1', 'priority' => 1000 ],
        [ 'event' => 'event2', 'method' => 'onEvent2', 'priority' => -100 ],
        [ 'event' => 'event3', 'method' => 'onEvent3', 'priority' => 1 ],
    ];

    // define a constructor with dependencies ...

    public function attach(EventManagerInterface $events, $priority = 1)
    {
        foreach (self::$attachTo as $spec) {
            $events->attach($spec['event'], [ $this, $spec['method'] ], $spec['priority']);
        }
    }

    // define the event listener methods ...
}

// In your application:
foreach (ThisIsAnAggregate::$attachTo as $spec) {
    $events->attach($spec['event'], function ($e) use ($services) {
        return $services->get(ThisIsAnAggregate::class)->{$spec['method']}($e);
    }, $spec['priority']);
}

It makes use of statics, just like what you were doing, but doesn't require any changes to any existing interfaces, either in v2 or this proposal. By using a closure, we can put anything we want into the logic, including any environmental aspects we may want to use to decide whether or not we really need to pull the service.

I suppose we could do a new attachLazy() method, which would accept a class name and optionally a list of context variables to bind to closures.

• The class would need to implement a new interface, which
• Defines a static getAttachSpecifications() method. It would accept the context variables to which to bind closures, and would return an array of:
• EventListenerSpecification instances. These would have methods for retrieving the event, callback, and priority.

and then attachLazy() would attach each of the returned specifications.

Or we could just document the solution I indicated above somewhere. 😄 Thoughts, @bakura10 ?

[bakura10]

Hhhmmm yeah that sounds like a nice workaround. It would be nice if we could have some syntactic sugar around this, but it should work.

[bakura10]

How the FastPriorityQueue compared to my simpler, dependency-less implementation here: #2?

I've been able to do that with pure array and much simpler code. My code being much much smaller, maybe you could start from this base, and start re-adding features you need (actually, only the sharedEventManager is really missing from my impl, and a few methods you may want for compatibility).

[Maks3w]

I would like to see removed all those setters on the EventInterface.

I think trigger() can typehint EventInterface without problems.

$eventManager->trigger($name, $target, $params, $callback);
$eventManager->trigger(new Event($name, $target, $params), $callback);

1. Still being simple trigger events without specialized classes.
2. Remove all that magic stuff about modify the object.
3. Remove those setters in the interface.

The only thing its no possible reuse an event with different event names (Is this really a need?)

$eventManager->attach('event.foo', function(EventInterface $eventFoo) {
   $eventManager->trigger('event.BAZ', $eventFoo);
});