Implementing and re-utilizing an abstract aggregate listener #3876

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Member

Ocramius commented Feb 22, 2013

As of zendframework/zf2#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

+use Zend\EventManager\StaticEventManager;
+use Zend\Stdlib\CallbackHandler;
+
+/**
@Ocramius

Ocramius Feb 22, 2013

Member

Is this stuff still needed?

@Maks3w

Maks3w Feb 23, 2013

Member

only @group

Member

Maks3w commented Feb 23, 2013

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)

Member

Ocramius commented Feb 23, 2013

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

Member

Maks3w commented Feb 23, 2013

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

Member

Ocramius commented Feb 23, 2013

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

Member

marc-mabe commented Feb 23, 2013

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

Member

Ocramius commented Feb 23, 2013

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

Member

marc-mabe commented Feb 25, 2013

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

Ocramius commented Feb 25, 2013

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

Member

marc-mabe commented Feb 25, 2013

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

+ public function testDetach()
+ {
+ $eventManager = $this->getMock('Zend\\EventManager\\EventManagerInterface');
+ $unrelatedEventManager = $this->getMock('Zend\\EventManager\\EventManagerInterface');
@Ocramius

Ocramius Feb 25, 2013

Member

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

Owner

weierophinney commented Mar 11, 2013

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.

@ghost ghost assigned weierophinney Mar 11, 2013

Contributor

danizord commented Mar 22, 2013

@Ocramius Since AbstractListenerAggregate implements ListenerAggregateInterface it should have:

abstract public function attach(EventManagerInterface $events);

right?

Member

Ocramius commented Mar 22, 2013

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 notifications@github.com 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/zendframework/zf2/pull/3876#issuecomment-15276540
.

+ /**
+ * @var \Zend\Stdlib\CallbackHandler[]
+ */
+ protected $callbacks = array();
@weierophinney

weierophinney Mar 25, 2013

Owner

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

+ /**
+ * @var \Zend\Stdlib\CallbackHandler[]
+ */
+ protected $callbacks = array();
@weierophinney

weierophinney Mar 25, 2013

Owner

Same comment here.

weierophinney added a commit that referenced this pull request Mar 25, 2013

Merge pull request #3876 from Ocramius/feature/abstract-aggregate-lis…
…tener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit that referenced this pull request Mar 25, 2013

[#3876] CS fixes
- EOF ending

weierophinney added a commit that referenced this pull request Mar 25, 2013

Owner

weierophinney commented Mar 25, 2013

Merged!

@Ocramius Ocramius deleted the Ocramius:feature/abstract-aggregate-listener branch Apr 19, 2013

weierophinney added a commit to zendframework/zend-eventmanager that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3876 from Ocramius/fea…
…ture/abstract-aggregate-listener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit to zendframework/zend-eventmanager that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-eventmanager that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3876 from Ocramius/fea…
…ture/abstract-aggregate-listener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3876 from Ocramius/fea…
…ture/abstract-aggregate-listener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3876 from Ocramius/fea…
…ture/abstract-aggregate-listener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit to zendframework/zend-cache that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#3876 from Ocramius/fea…
…ture/abstract-aggregate-listener

Implementing and re-utilizing an abstract aggregate listener

weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment