make logic for fetching handler lists more consistent

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{

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);
 	}
 
 	/**

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));
 	}
 }

tests/phpunit/event/HandlerListManagerTest.php

@@ -0,0 +1,81 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+use PHPUnit\Framework\TestCase;
+
+class HandlerListManagerTest extends TestCase{
+
+	/** @var \Closure */
+	private $isValidFunc;
+	/** @var \Closure */
+	private $resolveParentFunc;
+
+
+	public function setUp(){
+		/** @see HandlerListManager::isValidClass() */
+		$this->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());
+		}
+	}
+}

tests/phpunit/event/TestAbstractAllowHandleEvent.php

@@ -0,0 +1,31 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+/**
+ * @allowHandle
+ */
+abstract class TestAbstractAllowHandleEvent extends Event{
+
+}

tests/phpunit/event/TestAbstractEvent.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+abstract class TestAbstractEvent extends Event{
+
+}

tests/phpunit/event/TestConcreteEvent.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+class TestConcreteEvent extends Event{
+
+}

tests/phpunit/event/TestConcreteExtendsAbstractEvent.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+class TestConcreteExtendsAbstractEvent extends TestAbstractEvent{
+
+}

tests/phpunit/event/TestConcreteExtendsAllowHandleEvent.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+class TestConcreteExtendsAllowHandleEvent extends TestAbstractAllowHandleEvent{
+
+}

tests/phpunit/event/TestConcreteExtendsConcreteEvent.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\event;
+
+class TestConcreteExtendsConcreteEvent extends TestConcreteEvent{
+
+}