Support for ListenerAggregates in SharedEventManager #2819

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@stefankleff
Contributor

stefankleff commented Oct 22, 2012

No description provided.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Oct 30, 2012

Member

This is problematic as coded currently, as ListenerAggregateInterface is expecting an instance of EventManagerInterface. While internally each context is using an EMI, I would expect an aggregate attaching to the SEM to receive the SEM as the argument to attach() and detach() -- which would allow it to then attach to shared events.

For that to happen, I think we need a SharedListenerAggregateInterface, and that it should typehint off of SharedEventManagerInterface in its attach() and detach() methods.

Member

weierophinney commented Oct 30, 2012

This is problematic as coded currently, as ListenerAggregateInterface is expecting an instance of EventManagerInterface. While internally each context is using an EMI, I would expect an aggregate attaching to the SEM to receive the SEM as the argument to attach() and detach() -- which would allow it to then attach to shared events.

For that to happen, I think we need a SharedListenerAggregateInterface, and that it should typehint off of SharedEventManagerInterface in its attach() and detach() methods.

@ghost ghost assigned weierophinney Oct 30, 2012

@stefankleff

This comment has been minimized.

Show comment Hide comment
@stefankleff

stefankleff Oct 30, 2012

Contributor

I agree that it is problematic - But I disagree on your point "I would expect an aggregate attaching to the SEM to receive the SEM as the argument". I think that the listener should not be aware of that.
From a practical point of view this is not desirable: In that case you need to know which kind of EventManager your Listener should be used with. You cannot write just a "common" Listener and let the application decide which type of EventManager is used later on.

A possible solution would be, as you proposed, different interfaces for shared listeners. Then a "common" listener needs to implement the ListenerAggregateInterface as well as the SharedListenerAggregateInterface. And you need to rename the attach method to avoid naming conflicts (shown in the code example). Otherwise it is possible that SLAI extends LAI and add a second parameter to attach(), which requires to get rid of the explicit typing in LAI.

This would look like the following (SEMI::attach() is explicit for demo):


interface SharedListenerAggregateInterface {
  public function attachShared(SharedEventManagerInterface $sharedEvents, $identifiers);
}

interface SharedEventManagerInterface {
  public function attach($id, $event, SharedListenerAggregateInterface $listener);
}

class CommonListener implements ListenerAggregateInterface, SharedListenerAggregateInterface {
    public function attach(EventManagerInterface $events) {
        $events->attach('foo', array($this, 'onFoo'));
    }

    public function attachShared(SharedEventManagerInterface $sharedEvents, $identifiers) {
         $sharedEvents->attach('foo', $identifiers, array($this, 'onFoo'));
    }
}

The other Solution which came to my mind was to somehow unify the attach/detach capabilities of EM and SEM and make it "ListenerAttachable". If you imagine a Listener implementing your proposed SharedListenerAggrergateInterface, I think you would agree, that the Listener should not be aware of the identifiers it is attached to. (like it is shown above). This should be in the responsibility of the application which attaches the Listener to the EM. So this parameter is just passed from SEM, to the listener implementing SLAI, back to the same SEM. So why not omit the identifier parameter in SLAI? What if we somehow store the identifiers in the SEM and just let the listener itself attach to an event? This was luckily possible with the current implementation of SEM, since it uses EMs internally. And the concrete EM could even be used for the listener, so we can use the default LAI::attach method signature. A different interface isn't needed anymore.

To sum it up: I really think that the Listener should not be aware of the EventManager implementation. But the problem is that the SharedEventManager isn't an EventManager implemenation. I solved this by using the internal EventManagers of the SharedEventManager.This is from an OOP point of view not an optimal solution, but I think using a SharedAggeragteListenerInterface isn't optimal neither. Furthermore it introduces additional complexity while creating listeners.

Contributor

stefankleff commented Oct 30, 2012

I agree that it is problematic - But I disagree on your point "I would expect an aggregate attaching to the SEM to receive the SEM as the argument". I think that the listener should not be aware of that.
From a practical point of view this is not desirable: In that case you need to know which kind of EventManager your Listener should be used with. You cannot write just a "common" Listener and let the application decide which type of EventManager is used later on.

A possible solution would be, as you proposed, different interfaces for shared listeners. Then a "common" listener needs to implement the ListenerAggregateInterface as well as the SharedListenerAggregateInterface. And you need to rename the attach method to avoid naming conflicts (shown in the code example). Otherwise it is possible that SLAI extends LAI and add a second parameter to attach(), which requires to get rid of the explicit typing in LAI.

This would look like the following (SEMI::attach() is explicit for demo):


interface SharedListenerAggregateInterface {
  public function attachShared(SharedEventManagerInterface $sharedEvents, $identifiers);
}

interface SharedEventManagerInterface {
  public function attach($id, $event, SharedListenerAggregateInterface $listener);
}

class CommonListener implements ListenerAggregateInterface, SharedListenerAggregateInterface {
    public function attach(EventManagerInterface $events) {
        $events->attach('foo', array($this, 'onFoo'));
    }

    public function attachShared(SharedEventManagerInterface $sharedEvents, $identifiers) {
         $sharedEvents->attach('foo', $identifiers, array($this, 'onFoo'));
    }
}

The other Solution which came to my mind was to somehow unify the attach/detach capabilities of EM and SEM and make it "ListenerAttachable". If you imagine a Listener implementing your proposed SharedListenerAggrergateInterface, I think you would agree, that the Listener should not be aware of the identifiers it is attached to. (like it is shown above). This should be in the responsibility of the application which attaches the Listener to the EM. So this parameter is just passed from SEM, to the listener implementing SLAI, back to the same SEM. So why not omit the identifier parameter in SLAI? What if we somehow store the identifiers in the SEM and just let the listener itself attach to an event? This was luckily possible with the current implementation of SEM, since it uses EMs internally. And the concrete EM could even be used for the listener, so we can use the default LAI::attach method signature. A different interface isn't needed anymore.

To sum it up: I really think that the Listener should not be aware of the EventManager implementation. But the problem is that the SharedEventManager isn't an EventManager implemenation. I solved this by using the internal EventManagers of the SharedEventManager.This is from an OOP point of view not an optimal solution, but I think using a SharedAggeragteListenerInterface isn't optimal neither. Furthermore it introduces additional complexity while creating listeners.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Nov 9, 2012

Member

@stefankleff I actually was not thinking of passing the identifiers to attachShared(); I'd only expect the $sharedEvents argument. That allows the aggregate to decide what identifiers each listener should attach to. The reason for needing the separate interface is clear, though -- the signatures of SharedEventManagerInterface and EventManagerInterface differ, and the former does not extend the latter.

Considering that the use cases for shared listeners are quite different -- the assumption is we don't have a access to a discrete EM instance when attaching as a shared listener -- I think this is still necessary.

Are you willing to go forward with this? If not, I'll create an alternate PR.

Member

weierophinney commented Nov 9, 2012

@stefankleff I actually was not thinking of passing the identifiers to attachShared(); I'd only expect the $sharedEvents argument. That allows the aggregate to decide what identifiers each listener should attach to. The reason for needing the separate interface is clear, though -- the signatures of SharedEventManagerInterface and EventManagerInterface differ, and the former does not extend the latter.

Considering that the use cases for shared listeners are quite different -- the assumption is we don't have a access to a discrete EM instance when attaching as a shared listener -- I think this is still necessary.

Are you willing to go forward with this? If not, I'll create an alternate PR.

@stefankleff

This comment has been minimized.

Show comment Hide comment
@stefankleff

stefankleff Nov 17, 2012

Contributor

Sorry I'm very busy atm. I can code this at the end of next week. If this is too late and if you have time to create the functionality on your own - go for it.

Contributor

stefankleff commented Nov 17, 2012

Sorry I'm very busy atm. I can code this at the end of next week. If this is too late and if you have time to create the functionality on your own - go for it.

@stefankleff

This comment has been minimized.

Show comment Hide comment
@stefankleff

stefankleff Nov 28, 2012

Contributor

I created a new PR to incorporate the feedback: zendframework#3084

Contributor

stefankleff commented Nov 28, 2012

I created a new PR to incorporate the feedback: zendframework#3084

akandels pushed a commit to akandels/zf2 that referenced this pull request Nov 15, 2013

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

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

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