diff --git a/src/event/Event.php b/src/event/Event.php index 7f1cc4bb5..f5ecd17e9 100644 --- a/src/event/Event.php +++ b/src/event/Event.php @@ -56,7 +56,6 @@ abstract class Event{ } $handlerList = HandlerListManager::global()->getListFor(get_class($this)); - assert($handlerList !== null, "Called event should have a valid HandlerList"); ++self::$eventCallDepth; try{ diff --git a/src/event/HandlerListManager.php b/src/event/HandlerListManager.php index 0097d7a1c..a21e7a49c 100644 --- a/src/event/HandlerListManager.php +++ b/src/event/HandlerListManager.php @@ -58,6 +58,18 @@ class HandlerListManager{ } } + private static function isValidClass(\ReflectionClass $class) : bool{ + $tags = Utils::parseDocComment((string) $class->getDocComment()); + return !$class->isAbstract() || isset($tags["allowHandle"]); + } + + private static function resolveNearestHandleableParent(\ReflectionClass $class) : ?\ReflectionClass{ + for($parent = $class->getParentClass(); $parent !== false && !self::isValidClass($parent); $parent = $parent->getParentClass()){ + //NOOP + } + return $parent ?: null; + } + /** * Returns the HandlerList for listeners that explicitly handle this event. * @@ -65,30 +77,22 @@ class HandlerListManager{ * * @param string $event * - * @return null|HandlerList + * @return HandlerList * @throws \ReflectionException + * @throws \InvalidArgumentException */ - public function getListFor(string $event) : ?HandlerList{ + public function getListFor(string $event) : HandlerList{ if(isset($this->allLists[$event])){ return $this->allLists[$event]; } $class = new \ReflectionClass($event); - $tags = Utils::parseDocComment((string) $class->getDocComment()); - - if($class->isAbstract() && !isset($tags["allowHandle"])){ - return null; + if(!self::isValidClass($class)){ + throw new \InvalidArgumentException("Event must be non-abstract or have the @allowHandle annotation"); } - $super = $class; - $parentList = null; - while($parentList === null && ($super = $super->getParentClass()) !== false){ - // skip $noHandle events in the inheritance tree to go to the nearest ancestor - // while loop to allow skipping $noHandle events in the inheritance tree - $parentList = $this->getListFor($super->getName()); - } - - return $this->allLists[$event] = new HandlerList($event, $parentList); + $parent = self::resolveNearestHandleableParent($class); + return $this->allLists[$event] = new HandlerList($event, $parent !== null ? $this->getListFor($parent->getName()) : null); } /** diff --git a/src/plugin/PluginManager.php b/src/plugin/PluginManager.php index 76325d469..87714d628 100644 --- a/src/plugin/PluginManager.php +++ b/src/plugin/PluginManager.php @@ -568,19 +568,6 @@ class PluginManager{ $timings = new TimingsHandler("Plugin: " . $plugin->getDescription()->getFullName() . " Event: " . $handlerName . "(" . (new \ReflectionClass($event))->getShortName() . ")"); - $this->getEventListeners($event)->register(new RegisteredListener($handler, $priority, $plugin, $handleCancelled, $timings)); - } - - /** - * @param string $event - * - * @return HandlerList - */ - private function getEventListeners(string $event) : HandlerList{ - $list = HandlerListManager::global()->getListFor($event); - if($list === null){ - throw new PluginException("Abstract events not declaring @allowHandle cannot be handled (tried to register listener for $event)"); - } - return $list; + HandlerListManager::global()->getListFor($event)->register(new RegisteredListener($handler, $priority, $plugin, $handleCancelled, $timings)); } } diff --git a/tests/phpunit/event/HandlerListManagerTest.php b/tests/phpunit/event/HandlerListManagerTest.php new file mode 100644 index 000000000..a15456d9e --- /dev/null +++ b/tests/phpunit/event/HandlerListManagerTest.php @@ -0,0 +1,81 @@ +isValidFunc = (new \ReflectionMethod(HandlerListManager::class, 'isValidClass'))->getClosure(); + /** @see HandlerListManager::resolveNearestHandleableParent() */ + $this->resolveParentFunc = (new \ReflectionMethod(HandlerListManager::class, 'resolveNearestHandleableParent'))->getClosure(); + } + + public function isValidClassProvider() : \Generator{ + yield [new \ReflectionClass(Event::class), false, "event base should not be handleable"]; + yield [new \ReflectionClass(TestConcreteEvent::class), true, ""]; + yield [new \ReflectionClass(TestAbstractEvent::class), false, "abstract event cannot be handled"]; + yield [new \ReflectionClass(TestAbstractAllowHandleEvent::class), true, "abstract event declaring @allowHandle should be handleable"]; + } + + /** + * @dataProvider isValidClassProvider + * + * @param \ReflectionClass $class + * @param bool $isValid + * @param string $reason + */ + public function testIsValidClass(\ReflectionClass $class, bool $isValid, string $reason) : void{ + self::assertSame($isValid, ($this->isValidFunc)($class), $reason); + } + + public function resolveParentClassProvider() : \Generator{ + yield [new \ReflectionClass(TestConcreteExtendsAllowHandleEvent::class), new \ReflectionClass(TestAbstractAllowHandleEvent::class)]; + yield [new \ReflectionClass(TestConcreteEvent::class), null]; + yield [new \ReflectionClass(TestConcreteExtendsAbstractEvent::class), null]; + yield [new \ReflectionClass(TestConcreteExtendsConcreteEvent::class), new \ReflectionClass(TestConcreteEvent::class)]; + } + + /** + * @dataProvider resolveParentClassProvider + * + * @param \ReflectionClass $class + * @param \ReflectionClass|null $expect + */ + public function testResolveParentClass(\ReflectionClass $class, ?\ReflectionClass $expect) : void{ + if($expect === null){ + self::assertNull(($this->resolveParentFunc)($class)); + }else{ + self::assertSame(($this->resolveParentFunc)($class)->getName(), $expect->getName()); + } + } +} diff --git a/tests/phpunit/event/TestAbstractAllowHandleEvent.php b/tests/phpunit/event/TestAbstractAllowHandleEvent.php new file mode 100644 index 000000000..382660714 --- /dev/null +++ b/tests/phpunit/event/TestAbstractAllowHandleEvent.php @@ -0,0 +1,31 @@ +