Implementing and re-utilizing an abstract aggregate listener

[Ocramius]

As of #3874, I abstracted the duplicate logic of the aggregate listener into an abstract class.

This abstract aggregate listener only defines the logic for detach. Developing something related to attach would increase the method calls within the framework by quite a bit, so I preferred leaving that part of the implementation to the end-user.

This PR introduces minor BCs that should be discussed first:

1. return value of attach (now void)
2. return value of detach (now void)
3. rename of protected property to listeners to callbacks (maybe callbackHandlers? let me know...)
4. Zend\Cache\Storage\Plugin\AbstractPlugin now inheriting an additional abstract method (this breaks eventual plugins extending it)
5. aggregate listeners in Zend\Cache don't throw exceptions anymore if the listener is attached multiple times
6. aggregate listeners in Zend\Cache don't throw exceptions anymore if the listener is detached and was never attached

Ping @marc-mabe

[Ocramius]

Is this stuff still needed?

[Maks3w]

only @group

[Maks3w]

I agree with to have a common superclass for *Listener classes. But I don't see the same for other classes that mainly aren't listeners (Plugin, Strategy, etc)

[Ocramius]

@Maks3w all changed classes were previously implementing the interface: no kittens were harmed in this PR :)

Actually: nvm... let's see what @marc-mabe has to say on the change on the abstract plugin for the cache...

[Maks3w]

Abstract class is not a replacement of the interface. If it is then the interface can be removed. I see the interfaces as features of a class and hierarchy as the natural classification

[Ocramius]

In fact I am not replacing the interface. The interface stays there and I'm fine with it: the repeated code goes away. I am adding one class in the inheritance, not pushing any contract out of it :)

[marc-mabe]

@Ocramius: the abstract class is a good way removing duplicate code but why you removed grouping callback handles by EventManager instances? An instance of a storage plugin than will no longer be re-usable over different adapters and why the LogicException has gone away?

[Ocramius]

@marc-mabe the fact is that (and that's a personal thought) following is perfectly valid:

<?php

$listener = new AggregateStuff();
$evm->attach($listener);
$evm->attach($listener); // second time (simply triggers the listener 2 times...)

// etc

$evm->detach($listener); 
$evm->detach($listener); // even if it was already detached - code is simply noop

I removed grouping because detach is rarely called, so the performance impact on attach has to take priority (again: my thoughts). Can you explain the last bit?

An instance of a storage plugin than will no longer be re-usable over different adapters and why the LogicException has gone away?

[marc-mabe]

@Ocramius For your example attaching a listener twice to be called twice is ok but on detaching it only one of the listener should be detached if it was attached twice.

Because grouping the following should work:

$excaptionHandler = new Zend\Cache\Storage\Plugin\ExceptionHandler();
$apcCache->addPlugin($excaptionHandler);
$memCache->addPlugin($excaptionHandler);
// exceptions should be catched by both caches
$apcCache->removePlugin($excaptionHandler);
// exceptions should be catched only by $memCache

[Ocramius]

@marc-mabe $apcCache->removePlugin() wouldn't remove the plugin from $memCache as long as the backing EventManager is a different instance

[marc-mabe]

@Ocramius If that's the case i missed something and all is valid 👍

[Ocramius]

@marc-mabe this is the example where I show that unrelated event managers don't cause event listeners to be detached (FYI)

[weierophinney]

Only one comment and request, @Ocramius : also create a PHP 5.4 trait that mimics what AbstractListenerAggregate does. That will allow extending from a different class, while still re-using logic, when under 5.4. :)

Scheduling for 2.2.0.

[danizord]

@Ocramius Since AbstractListenerAggregate implements ListenerAggregateInterface it should have:

abstract public function attach(EventManagerInterface $events);

right?

[Ocramius]

It's inherited from the interface, no need to re-define it

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 22 March 2013 02:31, Daniel Gimenes [email protected] wrote:

@Ocramius https://github.com/Ocramius Since AbstractListenerAggregateimplements
ListenerAggregateInterface it should have:

abstract public function attach(EventManagerInterface $events);

right?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3876#issuecomment-15276540
.

[weierophinney]

I'd call this "listeners", not "callbacks". I can change that on merge, though.

[weierophinney]

Merged!