[3.0] New Event system #153

Closed
wants to merge 5 commits into
from

Projects

None yet

5 participants

@guilhermeblanco
Member

No description provided.

@schmittjoh schmittjoh and 1 other commented on an outdated diff Jun 15, 2012
lib/Doctrine/Common/Event/Event.php
+ return $this->propagationStopped;
+ }
+
+ /**
+ * If an event is cancelable, the preventDefault method is used to signify that the event is to
+ * be canceled, meaning any default action normally taken by the implementation as a result of
+ * the event will not occur. If, during any stage of event flow, the preventDefault method is
+ * called the event is canceled. Any default action associated with the event will not occur.
+ * Calling this method for a non-cancelable event has no effect. Once preventDefault has been
+ * called it will remain in effect throughout the remainder of the event's propagation. This
+ * method may be used during any stage of event flow.
+ *
+ */
+ public function preventDefault()
+ {
+ if ($this->cancellable) {
@schmittjoh
schmittjoh Jun 15, 2012 Member

Should we throw an exception if not? Something seems to be wrong in that case in the calling code, no?

@guilhermeblanco
guilhermeblanco Jun 15, 2012 Member

Yes. This is one of the things I was considering originally.
The point here is whether we should do that or not. If it's not cancellable, then it should still execute the default behavior, not block it. Sometimes I think we should just ignore, others we should shout at user.
The way that CallableEventListener is implemented, it automatically triggers preventDefault if user defined to return a value there. This may heavily affect this behavior if we throw Exception. I don't know....

@stof stof commented on an outdated diff Jun 16, 2012
lib/Doctrine/Common/Event/EventSubscriber.php
+ */
+abstract class EventSubscriber implements EventSubscriberInterface
+{
+ /**
+ * @see EventSubscriberInterface::addEventListeners
+ *
+ * @api
+ */
+ abstract public function addEventListeners(EventTarget $target);
+
+ /**
+ * @see EventSubscriberInterface::removeEventListeners
+ *
+ * @api
+ */
+ abstract public function removeEventListeners(EventTarget $target);
@stof
stof Jun 16, 2012 Member

why defining abstract methods for the method of the interface ? This is useless as the interface already enforces them (making the whole class useless btw)

@stof stof commented on the diff Jun 16, 2012
lib/Doctrine/Common/Event/EventTarget.php
+
+namespace Doctrine\Common\Event;
+
+/**
+ * Allow binding specific on its subclasses instances. Allow registration and
+ * removal of {@link EventListener}s on an EventTarget and dispatch events to
+ * that EventTarget.
+ *
+ * @api
+ *
+ * @abstract
+ * @link www.doctrine-project.org
+ * @since 2.3
+ * @author Guilherme Blanco <guilhermeblanco@hotmail.com>
+ */
+abstract class EventTarget
@stof
stof Jun 16, 2012 Member

why making it abstract and then extending it with an empty class ?
Btw, you should probably add an interface

@stof
stof Jun 16, 2012 Member

ok, there is an interface, but you should implement it :)

@stof stof commented on an outdated diff Jun 16, 2012
lib/Doctrine/Common/Event/CallableEventListener.php
+ * @param mixed $target
+ */
+ public function setTarget($target)
+ {
+ $this->target = $target;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function handleEvent(Event $event)
+ {
+ $event->setCurrentTarget($this->getTarget());
+
+ // Automatically prevent default if return is used in callable
+ if (call_user_func($this->handler, $event)) {
@stof
stof Jun 16, 2012 Member

wouldn't it be better to prevent the default when the return value is false instead of doing it when true is returned ? The interface of your event system looks like the DOM event system so it would be logical to use the same way to prevent the default too.

@stof stof commented on an outdated diff Jun 19, 2012
lib/Doctrine/Common/Event/CallableEventListener.php
+ * @param mixed $target
+ */
+ public function setTarget($target)
+ {
+ $this->target = $target;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function handleEvent(Event $event)
+ {
+ $event->setCurrentTarget($this->getTarget());
+
+ // Automatically prevent default if return is used in callable
+ if ( ! call_user_func($this->handler, $event) && $event->isCancellable()) {
@stof
stof Jun 19, 2012 Member

I would use false === to cancel only if the user returned false. currently, you cancel also on null, meaning the default behavior for an handler is cancelling

@guilhermeblanco guilhermeblanco Started tests on new Event system. Added generic support for lists (p…
…lanning for cached support). General optimizations adnd changes. Simplified API.
c440291
@Ocramius Ocramius commented on the diff Jul 10, 2012
lib/Doctrine/Common/Event/EventListenerList.php
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function size($type)
+ {
+ return count($this->listenerList[$type]);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function get($type)
+ {
+ if (isset($this->sortedListenerList[$type]) && ! $this->sortedListenerList[$type]) {
@Ocramius
Ocramius Jul 10, 2012 Member

What about using an SplPriorityQueue instead of this?

@Ocramius Ocramius commented on the diff Jul 10, 2012
lib/Doctrine/Common/Event/EventListenerList.php
+ * @var array
+ */
+ protected $listenerList = array();
+
+ /**
+ * @var array
+ */
+ private $sortedListenerList = array();
+
+ /**
+ * {@inheritdoc}
+ */
+ public function add(EventListenerInterface $listener)
+ {
+ $type = $listener->getType();
+ $hash = spl_object_hash($listener);
@Ocramius
Ocramius Jul 10, 2012 Member

Is it by design that you cannot add the same listener twice?

@henrikbjorn henrikbjorn commented on the diff Jul 12, 2012
lib/Doctrine/Common/Event/Event.php
+ *
+ * @var boolean Flag holding if event is assigned to stop propagating to other EventListeners.
+ */
+ protected $propagationStopped = false;
+
+ /**
+ * Constructor.
+ *
+ * @param string $type Event type
+ * @param boolean $cancellable Specifies whether or not the event's default action can be prevented.
+ */
+ public function __construct($type, $cancellable = false)
+ {
+ $this->type = $type;
+ $this->timeStamp = microtime(true);
+ $this->cancelable = $cancellable;
@Ocramius
Member

Closing: after 3 years, I think we might first look at how the ecosystem may have evolved before going and building our own (again)

@Ocramius Ocramius closed this Mar 25, 2015
@Ocramius Ocramius self-assigned this Mar 25, 2015
@Ocramius Ocramius deleted the event branch Mar 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment