Fix PluginManager annotation

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

apps/dav/lib/AppInfo/PluginManager.php

@@ -34,6 +34,8 @@ use OCA\DAV\CalDAV\Integration\ICalendarProvider;
 use OCA\DAV\CardDAV\Integration\IAddressBookProvider;
 use OCP\App\IAppManager;
 use OCP\AppFramework\QueryException;
+use Sabre\DAV\Collection;
+use Sabre\DAV\ServerPlugin;
 use function array_map;
 use function class_exists;
 use function is_array;
@@ -57,30 +59,33 @@ class PluginManager {
 	/**
 	 * App plugins
 	 *
-	 * @var array
+	 * @var ServerPlugin[]
 	 */
-	private $plugins = null;
+	private $plugins = [];
 
 	/**
 	 * App collections
 	 *
-	 * @var array
+	 * @var Collection[]
 	 */
-	private $collections = null;
+	private $collections = [];
 
 	/**
 	 * Address book plugins
 	 *
-	 * @var IAddressBookProvider[]|null
+	 * @var IAddressBookProvider[]
 	 */
-	private $addressBookPlugins = null;
+	private $addressBookPlugins = [];
 
 	/**
 	 * Calendar plugins
 	 *
-	 * @var array
+	 * @var ICalendarProvider[]
 	 */
-	private $calendarPlugins = null;
+	private $calendarPlugins = [];
+
+	/** @var bool */
+	private $populated = false;
 
 	/**
 	 * Contstruct a PluginManager
@@ -96,12 +101,10 @@ class PluginManager {
 	/**
 	 * Returns an array of app-registered plugins
 	 *
-	 * @return array
+	 * @return ServerPlugin[]
 	 */
 	public function getAppPlugins() {
-		if (null === $this->plugins) {
+		$this->populate();
-			$this->populate();
-		}
 		return $this->plugins;
 	}
 
@@ -111,9 +114,7 @@ class PluginManager {
 	 * @return array
 	 */
 	public function getAppCollections() {
-		if (null === $this->collections) {
+		$this->populate();
-			$this->populate();
-		}
 		return $this->collections;
 	}
 
@@ -121,46 +122,62 @@ class PluginManager {
 	 * @return IAddressBookProvider[]
 	 */
 	public function getAddressBookPlugins(): array {
-		if ($this->addressBookPlugins === null) {
+		$this->populate();
-			$this->populate();
-		}
 		return $this->addressBookPlugins;
 	}
 
 	/**
 	 * Returns an array of app-registered calendar plugins
 	 *
-	 * @return array
+	 * @return ICalendarProvider[]
 	 */
 	public function getCalendarPlugins():array {
-		if (null === $this->calendarPlugins) {
+		$this->populate();
-			$this->populate();
-		}
 		return $this->calendarPlugins;
 	}
 
 	/**
 	 * Retrieve plugin and collection list and populate attributes
 	 */
-	private function populate() {
+	private function populate(): void {
-		$this->plugins = [];
+		if ($this->populated) {
-		$this->addressBookPlugins = [];
+			return;
-		$this->calendarPlugins = [];
+		}
-		$this->collections = [];
+		$this->populated = true;
+
 		foreach ($this->appManager->getInstalledApps() as $app) {
 			// load plugins and collections from info.xml
 			$info = $this->appManager->getAppInfo($app);
 			if (!isset($info['types']) || !in_array('dav', $info['types'], true)) {
 				continue;
 			}
-			$this->loadSabrePluginsFromInfoXml($this->extractPluginList($info));
+			$plugins = $this->loadSabrePluginsFromInfoXml($this->extractPluginList($info));
-			$this->loadSabreCollectionsFromInfoXml($this->extractCollectionList($info));
+			foreach ($plugins as $plugin) {
-			$this->loadSabreAddressBookPluginsFromInfoXml($this->extractAddressBookPluginList($info));
+				$this->plugins[] = $plugin;
-			$this->loadSabreCalendarPluginsFromInfoXml($this->extractCalendarPluginList($info));
+			}
+
+			$collections = $this->loadSabreCollectionsFromInfoXml($this->extractCollectionList($info));
+			foreach ($collections as $collection) {
+				$this->collections[] = $collection;
+			}
+
+			$addresbookPlugins = $this->loadSabreAddressBookPluginsFromInfoXml($this->extractAddressBookPluginList($info));
+			foreach ($addresbookPlugins as $addresbookPlugin) {
+				$this->addressBookPlugins[] = $addresbookPlugin;
+			}
+
+			$calendarPlugins = $this->loadSabreCalendarPluginsFromInfoXml($this->extractCalendarPluginList($info));
+			foreach ($calendarPlugins as $calendarPlugin) {
+				$this->calendarPlugins[] = $calendarPlugin;
+			}
 		}
 	}
 
-	private function extractPluginList(array $array) {
+	/**
+	 * @param array $array
+	 * @return string[]
+	 */
+	private function extractPluginList(array $array): array {
 		if (isset($array['sabre']) && is_array($array['sabre'])) {
 			if (isset($array['sabre']['plugins']) && is_array($array['sabre']['plugins'])) {
 				if (isset($array['sabre']['plugins']['plugin'])) {
@@ -175,7 +192,11 @@ class PluginManager {
 		return [];
 	}
 
-	private function extractCollectionList(array $array) {
+	/**
+	 * @param array $array
+	 * @return string[]
+	 */
+	private function extractCollectionList(array $array): array {
 		if (isset($array['sabre']) && is_array($array['sabre'])) {
 			if (isset($array['sabre']['collections']) && is_array($array['sabre']['collections'])) {
 				if (isset($array['sabre']['collections']['collection'])) {
@@ -192,7 +213,6 @@ class PluginManager {
 
 	/**
 	 * @param array $array
-	 *
 	 * @return string[]
 	 */
 	private function extractAddressBookPluginList(array $array): array {
@@ -213,7 +233,11 @@ class PluginManager {
 		return $items;
 	}
 
-	private function extractCalendarPluginList(array $array):array {
+	/**
+	 * @param array $array
+	 * @return string[]
+	 */
+	private function extractCalendarPluginList(array $array): array {
 		if (isset($array['sabre']) && is_array($array['sabre'])) {
 			if (isset($array['sabre']['calendar-plugins']) && is_array($array['sabre']['calendar-plugins'])) {
 				if (isset($array['sabre']['calendar-plugins']['plugin'])) {
@@ -228,35 +252,7 @@ class PluginManager {
 		return [];
 	}
 
-	private function loadSabrePluginsFromInfoXml(array $plugins) {
+	private function createClass(string $className): object {
-		foreach ($plugins as $plugin) {
-			try {
-				$this->plugins[] = $this->container->query($plugin);
-			} catch (QueryException $e) {
-				if (class_exists($plugin)) {
-					$this->plugins[] = new $plugin();
-				} else {
-					throw new \Exception("Sabre plugin class '$plugin' is unknown and could not be loaded");
-				}
-			}
-		}
-	}
-
-	private function loadSabreCollectionsFromInfoXml(array $collections) {
-		foreach ($collections as $collection) {
-			try {
-				$this->collections[] = $this->container->query($collection);
-			} catch (QueryException $e) {
-				if (class_exists($collection)) {
-					$this->collections[] = new $collection();
-				} else {
-					throw new \Exception("Sabre collection class '$collection' is unknown and could not be loaded");
-				}
-			}
-		}
-	}
-
-	private function createPluginInstance(string $className) {
 		try {
 			return $this->container->query($className);
 		} catch (QueryException $e) {
@@ -265,42 +261,64 @@ class PluginManager {
 			}
 		}
 
-		throw new \Exception("Sabre plugin class '$className' is unknown and could not be loaded");
+		throw new \Exception('Could not load ' . $className, 0, $e);
+	}
+
+
+	/**
+	 * @param string[] $classes
+	 * @return ServerPlugin[]
+	 * @throws \Exception
+	 */
+	private function loadSabrePluginsFromInfoXml(array $classes): array {
+		return array_map(function (string $className): ServerPlugin {
+			$instance = $this->createClass($className);
+			if (!($instance instanceof ServerPlugin)) {
+				throw new \Exception('Sabre server plugin ' . $className . ' does not implement the ' . ServerPlugin::class . ' interface');
+			}
+			return $instance;
+		}, $classes);
 	}
 
 	/**
-	 * @param string[] $plugin
+	 * @param string[] $classes
+	 * @return Collection[]
 	 */
-	private function loadSabreAddressBookPluginsFromInfoXml(array $plugins): void {
+	private function loadSabreCollectionsFromInfoXml(array $classes): array {
-		$providers = array_map(function (string $className): IAddressBookProvider {
+		return array_map(function (string $className): Collection {
-			$instance = $this->createPluginInstance($className);
+			$instance = $this->createClass($className);
-			if (!($instance instanceof IAddressBookProvider)) {
+			if (!($instance instanceof Collection)) {
-				throw new \Exception("Sabre address book plugin class '$className' does not implement the \OCA\DAV\CardDAV\Integration\IAddressBookProvider interface");
+				throw new \Exception('Sabre collection plugin ' . $className . ' does not implement the ' . Collection::class . ' interface');
 			}
 			return $instance;
-		}, $plugins);
+		}, $classes);
-		foreach ($providers as $provider) {
-			$this->addressBookPlugins[] = $provider;
-		}
 	}
 
-	private function loadSabreCalendarPluginsFromInfoXml(array $calendarPlugins):void {
+	/**
-		foreach ($calendarPlugins as $calendarPlugin) {
+	 * @param string[] $classes
-			try {
+	 * @return IAddressBookProvider[]
-				$instantiatedCalendarPlugin = $this->container->query($calendarPlugin);
+	 */
-			} catch (QueryException $e) {
+	private function loadSabreAddressBookPluginsFromInfoXml(array $classes): array {
-				if (class_exists($calendarPlugin)) {
+		return array_map(function (string $className): IAddressBookProvider {
-					$instantiatedCalendarPlugin = new $calendarPlugin();
+			$instance = $this->createClass($className);
-				} else {
+			if (!($instance instanceof IAddressBookProvider)) {
-					throw new \Exception("Sabre calendar-plugin class '$calendarPlugin' is unknown and could not be loaded");
+				throw new \Exception('Sabre address book plugin class ' . $className . ' does not implement the ' . IAddressBookProvider::class . ' interface');
-				}
 			}
+			return $instance;
+		}, $classes);
+	}
 
-			if (!($instantiatedCalendarPlugin instanceof ICalendarProvider)) {
+	/**
-				throw new \Exception("Sabre calendar-plugin class '$calendarPlugin' does not implement ICalendarProvider interface");
+	 * @param string[] $classes
+	 * @return ICalendarProvider[]
+	 */
+	private function loadSabreCalendarPluginsFromInfoXml(array $classes): array {
+		return array_map(function (string $className): ICalendarProvider {
+			$instance = $this->createClass($className);
+			if (!($instance instanceof ICalendarProvider)) {
+				throw new \Exception('Sabre calendar plugin class ' . $className . ' does not implement the ' . ICalendarProvider::class . ' interface');
 			}
-
+			return $instance;
-			$this->calendarPlugins[] = $instantiatedCalendarPlugin;
+		}, $classes);
-		}
 	}
 }

apps/dav/tests/unit/AppInfo/PluginManagerTest.php

@@ -31,6 +31,8 @@ use OC\App\AppManager;
 use OC\ServerContainer;
 use OCA\DAV\AppInfo\PluginManager;
 use OCA\DAV\CalDAV\Integration\ICalendarProvider;
+use Sabre\DAV\Collection;
+use Sabre\DAV\ServerPlugin;
 use Test\TestCase;
 
 /**
@@ -97,23 +99,31 @@ class PluginManagerTest extends TestCase {
 		$calendarPlugin2 = $this->createMock(ICalendarProvider::class);
 		$calendarPlugin3 = $this->createMock(ICalendarProvider::class);
 
+		$dummyPlugin1 = $this->createMock(ServerPlugin::class);
+		$dummyPlugin2 = $this->createMock(ServerPlugin::class);
+		$dummy2Plugin1 = $this->createMock(ServerPlugin::class);
+
+		$dummyCollection1 = $this->createMock(Collection::class);
+		$dummyCollection2 = $this->createMock(Collection::class);
+		$dummy2Collection1 = $this->createMock(Collection::class);
+
 		$server->method('query')
 			->willReturnMap([
-				['\OCA\DAV\ADavApp\PluginOne', true, 'dummyplugin1'],
+				['\OCA\DAV\ADavApp\PluginOne', true, $dummyPlugin1],
-				['\OCA\DAV\ADavApp\PluginTwo', true, 'dummyplugin2'],
+				['\OCA\DAV\ADavApp\PluginTwo', true, $dummyPlugin2],
 				['\OCA\DAV\ADavApp\CalendarPluginOne', true, $calendarPlugin1],
 				['\OCA\DAV\ADavApp\CalendarPluginTwo', true, $calendarPlugin2],
-				['\OCA\DAV\ADavApp\CollectionOne', true, 'dummycollection1'],
+				['\OCA\DAV\ADavApp\CollectionOne', true, $dummyCollection1],
-				['\OCA\DAV\ADavApp\CollectionTwo', true, 'dummycollection2'],
+				['\OCA\DAV\ADavApp\CollectionTwo', true, $dummyCollection2],
-				['\OCA\DAV\ADavApp2\PluginOne', true, 'dummy2plugin1'],
+				['\OCA\DAV\ADavApp2\PluginOne', true, $dummy2Plugin1],
 				['\OCA\DAV\ADavApp2\CalendarPluginOne', true, $calendarPlugin3],
-				['\OCA\DAV\ADavApp2\CollectionOne', true, 'dummy2collection1'],
+				['\OCA\DAV\ADavApp2\CollectionOne', true, $dummy2Collection1],
 			]);
 
 		$expectedPlugins = [
-			'dummyplugin1',
+			$dummyPlugin1,
-			'dummyplugin2',
+			$dummyPlugin2,
-			'dummy2plugin1',
+			$dummy2Plugin1,
 		];
 		$expectedCalendarPlugins = [
 			$calendarPlugin1,
@@ -121,9 +131,9 @@ class PluginManagerTest extends TestCase {
 			$calendarPlugin3,
 		];
 		$expectedCollections = [
-			'dummycollection1',
+			$dummyCollection1,
-			'dummycollection2',
+			$dummyCollection2,
-			'dummy2collection1',
+			$dummy2Collection1,
 		];
 
 		$this->assertEquals($expectedPlugins, $pluginManager->getAppPlugins());