joomla / joomla-framework

[READ-ONLY] This repo is no longer in active development. Please see https://github.com/joomla-framework for the individual Framework packages.
http://framework.joomla.org
GNU General Public License v2.0
189 stars 140 forks source link

[Event] Improved the package #324

Closed florianv closed 10 years ago

florianv commented 10 years ago

I improved the Dispatcher so instead of registering only objects or closures, it now supports any PHP callable. It removes the need of the filter thing @eddieajau added, because now you register a callable per event. Also it makes it easier for people to understand what the addListener method actually does.

It introduces this backward compatibility issue :

The function in the Dispatcher :

public function addListener($listener, array $events = array())

becomes :

public function addListener($listener, $eventName, $priority = Priority::NORMAL)

and the setFilter method has been removed.

eddieajau commented 10 years ago

There are a lot of backward compatibility issues to resolve, but I'm not sure I like the changes. There is a lot of documentation that you are removing that I find useful.

florianv commented 10 years ago

The implementation is simpler and allows to inject any callable as listener, while previously only objects where supported. So it's simpler (that's why the useless documentation has been deleted) and has more features.

florianv commented 10 years ago

Let's wait 2.0

eddieajau commented 10 years ago

2.0 could be 2 months or 2 years away - nobody knows. I think there are ways for you to make some changes within 1.x. Am I reading it correctly that you removing the ability to register an object?

florianv commented 10 years ago

Yes instead people would have to register each callable separately.

$listener = new Listener;

$dispatcher->addListener(array($listener, 'myMethod'), 'onSomething')

It also allows to have a method name different from the event name.

eddieajau commented 10 years ago

Hrm. Looking at it I really do prefer the way addListener is at the moment. However, what you could do is add you new method signature as:

public function addEventListener($listener, $eventName, $priority = Priority::NORMAL)
// or possibly, to keep the order consistent with the method name
public function addEventListener($eventName, $listener, $priority = Priority::NORMAL)

In hindsight though, I think the setListenerFilter was a mistake. We should just do something like:

$myListener = new myListener;

$dispatcher->addListener($myListener, $myListener->getEvents());

So, I'm happy to deprecate that functionality if you want.

florianv commented 10 years ago

I think a method signature like :

public function addCallableListener($listener, $eventName, $priority = Priority::NORMAL)

is better according to the proposed method name here https://github.com/joomla/joomla-framework/pull/325#issuecomment-31160121