Merge pull request #1663 from nextcloud/dont-reparse-info-xml

Dont reparse info xml + cache AppInfo XML

7 files changed, 71 insertions, 69 deletions

diff --git a/core/register_command.php b/core/register_command.php
index 23621e54f8d..a6da3cbd899 100644
--- a/core/register_command.php
+++ b/core/register_command.php
@@ -36,7 +36,7 @@
 $application->add(new \Stecman\Component\Symfony\Console\BashCompletion\CompletionCommand());
 $application->add(new OC\Core\Command\Status);
 $application->add(new OC\Core\Command\Check(\OC::$server->getConfig()));
-$infoParser = new \OC\App\InfoParser(\OC::$server->getURLGenerator());
+$infoParser = new \OC\App\InfoParser();
 $application->add(new OC\Core\Command\App\CheckCode($infoParser));
 $application->add(new OC\Core\Command\L10n\CreateJs());
 $application->add(new \OC\Core\Command\Integrity\SignApp(
diff --git a/lib/private/App/InfoParser.php b/lib/private/App/InfoParser.php
index e975ad6f096..fbeb932763e 100644
--- a/lib/private/App/InfoParser.php
+++ b/lib/private/App/InfoParser.php
@@ -1,6 +1,7 @@
 <?php
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
+ * @copyright Copyright (c) 2016, Lukas Reschke <lukas@statuscode.ch>
  *
  * @author Andreas Fischer <bantu@owncloud.com>
  * @author Christoph Wurst <christoph@owncloud.com>
@@ -26,18 +27,17 @@
 
 namespace OC\App;
 
-use OCP\IURLGenerator;
+use OCP\ICache;
 
 class InfoParser {
-
-	/** @var IURLGenerator */
-	private $urlGenerator;
+	/** @var \OCP\ICache|null */
+	private $cache;
 
 	/**
-	 * @param IURLGenerator $urlGenerator
+	 * @param ICache|null $cache
 	 */
-	public function __construct(IURLGenerator $urlGenerator) {
-		$this->urlGenerator = $urlGenerator;
+	public function __construct(ICache $cache = null) {
+		$this->cache = $cache;
 	}
 
 	/**
@@ -49,18 +49,28 @@ class InfoParser {
 			return null;
 		}
 
+		if(!is_null($this->cache)) {
+			$fileCacheKey = $file . filemtime($file);
+			if ($cachedValue = $this->cache->get($fileCacheKey)) {
+				return json_decode($cachedValue, true);
+			}
+		}
+
 		libxml_use_internal_errors(true);
 		$loadEntities = libxml_disable_entity_loader(false);
 		$xml = simplexml_load_file($file);
+
 		libxml_disable_entity_loader($loadEntities);
-		if ($xml == false) {
+		if ($xml === false) {
 			libxml_clear_errors();
 			return null;
 		}
 		$array = $this->xmlToArray($xml);
+
 		if (is_null($array)) {
 			return null;
 		}
+
 		if (!array_key_exists('info', $array)) {
 			$array['info'] = [];
 		}
@@ -98,17 +108,6 @@ class InfoParser {
 			$array['two-factor-providers'] = [];
 		}
 
-		if (array_key_exists('documentation', $array) && is_array($array['documentation'])) {
-			foreach ($array['documentation'] as $key => $url) {
-				// If it is not an absolute URL we assume it is a key
-				// i.e. admin-ldap will get converted to go.php?to=admin-ldap
-				if (!$this->isHTTPURL($url)) {
-					$url = $this->urlGenerator->linkToDocs($url);
-				}
-
-				$array['documentation'][$key] = $url;
-			}
-		}
 		if (array_key_exists('types', $array)) {
 			if (is_array($array['types'])) {
 				foreach ($array['types'] as $type => $v) {
@@ -139,6 +138,10 @@ class InfoParser {
 		if (isset($array['background-jobs']['job']) && is_array($array['background-jobs']['job'])) {
 			$array['background-jobs'] = $array['background-jobs']['job'];
 		}
+
+		if(!is_null($this->cache)) {
+			$this->cache->set($fileCacheKey, json_encode($array));
+		}
 		return $array;
 	}
 
@@ -193,8 +196,4 @@ class InfoParser {
 
 		return $array;
 	}
-
-	private function isHTTPURL($url) {
-		return stripos($url, 'https://') === 0 || stripos($url, 'http://') === 0;
-	}
 }
diff --git a/lib/private/AppFramework/App.php b/lib/private/AppFramework/App.php
index 427a850f396..e15e4a797ea 100644
--- a/lib/private/AppFramework/App.php
+++ b/lib/private/AppFramework/App.php
@@ -59,24 +59,11 @@ class App {
 			return $topNamespace . self::$nameSpaceCache[$appId];
 		}
 
-		// first try to parse the app's appinfo/info.xml <namespace> tag
-		$appPath = OC_App::getAppPath($appId);
-		if ($appPath !== false) {
-			$filePath = "$appPath/appinfo/info.xml";
-			if (is_file($filePath)) {
-				$loadEntities = libxml_disable_entity_loader(false);
-				$xml = @simplexml_load_file($filePath);
-				libxml_disable_entity_loader($loadEntities);
-				if ($xml) {
-					$result = $xml->xpath('/info/namespace');
-					if ($result && count($result) > 0) {
-						self::$nameSpaceCache[$appId] = trim((string) $result[0]);
-						// take first namespace result
-						return $topNamespace . self::$nameSpaceCache[$appId];
-					}
-				}
-			}
+		$appInfo = \OC_App::getAppInfo($appId);
+		if (isset($appInfo['namespace'])) {
+			return $topNamespace . trim($appInfo['namespace']);
 		}
+
 		// if the tag is not found, fall back to uppercasing the first letter
 		self::$nameSpaceCache[$appId] = ucfirst($appId);
 		return $topNamespace . self::$nameSpaceCache[$appId];
diff --git a/lib/private/legacy/app.php b/lib/private/legacy/app.php
index c3d2d1d6ad4..5e05884f5c0 100644
--- a/lib/private/legacy/app.php
+++ b/lib/private/legacy/app.php
@@ -47,6 +47,7 @@
  *
  */
 use OC\App\DependencyAnalyzer;
+use OC\App\InfoParser;
 use OC\App\Platform;
 use OC\Installer;
 use OC\OCSClient;
@@ -681,7 +682,7 @@ class OC_App {
 			$file = $appPath . '/appinfo/info.xml';
 		}
 
-		$parser = new \OC\App\InfoParser(\OC::$server->getURLGenerator());
+		$parser = new InfoParser(\OC::$server->getMemCacheFactory()->create('core.appinfo'));
 		$data = $parser->parse($file);
 
 		if (is_array($data)) {
@@ -847,6 +848,7 @@ class OC_App {
 		$blacklist = \OC::$server->getAppManager()->getAlwaysEnabledApps();
 		$appList = array();
 		$langCode = \OC::$server->getL10N('core')->getLanguageCode();
+		$urlGenerator = \OC::$server->getURLGenerator();
 
 		foreach ($installedApps as $app) {
 			if (array_search($app, $blacklist) === false) {
@@ -900,6 +902,19 @@ class OC_App {
 						}
 					}
 				}
+				// fix documentation
+				if (isset($info['documentation']) && is_array($info['documentation'])) {
+					foreach ($info['documentation'] as $key => $url) {
+						// If it is not an absolute URL we assume it is a key
+						// i.e. admin-ldap will get converted to go.php?to=admin-ldap
+						if (stripos($url, 'https://') !== 0 && stripos($url, 'http://') !== 0) {
+							$url = $urlGenerator->linkToDocs($url);
+						}
+
+						$info['documentation'][$key] = $url;
+					}
+				}
+
 				$info['version'] = OC_App::getAppVersion($app);
 				$appList[] = $info;
 			}
diff --git a/tests/data/app/expected-info.json b/tests/data/app/expected-info.json
index 6d8d85ab552..6ddd3ae8568 100644
--- a/tests/data/app/expected-info.json
+++ b/tests/data/app/expected-info.json
@@ -10,8 +10,8 @@
 	"requiremin": "4",
 	"shipped": "true",
 	"documentation": {
-		"user": "https://docs.example.com/server/go.php?to=user-encryption",
-		"admin": "https://docs.example.com/server/go.php?to=admin-encryption"
+		"user": "user-encryption",
+		"admin": "admin-encryption"
 	},
 	"rememberlogin": "false",
 	"types": ["filesystem"],
diff --git a/tests/lib/App/CodeChecker/InfoCheckerTest.php b/tests/lib/App/CodeChecker/InfoCheckerTest.php
index 1032e800be1..c16874fbd33 100644
--- a/tests/lib/App/CodeChecker/InfoCheckerTest.php
+++ b/tests/lib/App/CodeChecker/InfoCheckerTest.php
@@ -44,9 +44,7 @@ class InfoCheckerTest extends TestCase {
 
 	protected function setUp() {
 		parent::setUp();
-		$infoParser = new InfoParser(\OC::$server->getURLGenerator());
-
-		$this->infoChecker = new InfoChecker($infoParser);
+		$this->infoChecker = new InfoChecker(new InfoParser());
 	}
 
 	public function appInfoData() {
diff --git a/tests/lib/App/InfoParserTest.php b/tests/lib/App/InfoParserTest.php
index 7f52507bcf7..5a3847a71e8 100644
--- a/tests/lib/App/InfoParserTest.php
+++ b/tests/lib/App/InfoParserTest.php
@@ -1,5 +1,4 @@
 <?php
-
 /**
  * @author Thomas Müller
  * @copyright 2014 Thomas Müller deepdiver@owncloud.com
@@ -10,46 +9,50 @@
 namespace Test\App;
 
 use OC;
-use OCP\IURLGenerator;
+use OC\App\InfoParser;
 use Test\TestCase;
 
 class InfoParserTest extends TestCase {
+	/** @var OC\Cache\CappedMemoryCache */
+	private static $cache;
 
-	/** @var \OC\App\InfoParser */
-	private $parser;
+	public static function setUpBeforeClass() {
+		self::$cache = new OC\Cache\CappedMemoryCache();
+	}
 
-	public function setUp() {
-		$urlGenerator = $this->getMockBuilder('\OCP\IURLGenerator')
-			->disableOriginalConstructor()
-			->getMock();
 
-		/** @var IURLGenerator | \PHPUnit_Framework_MockObject_MockObject $urlGenerator */
-		$urlGenerator->expects($this->any())
-			->method('linkToDocs')
-			->will($this->returnCallback(function ($url) {
-				return "https://docs.example.com/server/go.php?to=$url";
-			}));
+	public function parserTest($expectedJson, $xmlFile, $cache = null) {
+		$parser = new InfoParser($cache);
 
-		$this->parser = new \OC\App\InfoParser($urlGenerator);
-	}
-
-	/**
-	 * @dataProvider providesInfoXml
-	 */
-	public function testParsingValidXml($expectedJson, $xmlFile) {
 		$expectedData = null;
 		if (!is_null($expectedJson)) {
 			$expectedData = json_decode(file_get_contents(OC::$SERVERROOT . "/tests/data/app/$expectedJson"), true);
 		}
-		$data = $this->parser->parse(OC::$SERVERROOT. "/tests/data/app/$xmlFile");
+		$data = $parser->parse(OC::$SERVERROOT. "/tests/data/app/$xmlFile");
 
 		$this->assertEquals($expectedData, $data);
 	}
 
+	/**
+	 * @dataProvider providesInfoXml
+	 */
+	public function testParsingValidXmlWithoutCache($expectedJson, $xmlFile) {
+		$this->parserTest($expectedJson, $xmlFile);
+	}
+
+	/**
+	 * @dataProvider providesInfoXml
+	 */
+	public function testParsingValidXmlWithCache($expectedJson, $xmlFile) {
+		$this->parserTest($expectedJson, $xmlFile, self::$cache);
+	}
+
 	function providesInfoXml() {
 		return array(
 			array('expected-info.json', 'valid-info.xml'),
 			array(null, 'invalid-info.xml'),
+			array('expected-info.json', 'valid-info.xml'),
+			array(null, 'invalid-info.xml'),
 		);
 	}
 }