Improve caching

Signed-off-by: Julius Haertl <jus@bitgrid.net>

5 files changed, 69 insertions, 31 deletions

diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php
index eb65ae54f1a..887dba8a68a 100644
--- a/apps/theming/lib/Controller/IconController.php
+++ b/apps/theming/lib/Controller/IconController.php
@@ -96,7 +96,10 @@ class IconController extends Controller {
 		}
 		$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
 		$response->cacheFor(86400);
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
+		$expires = new \DateTime();
+		$expires->setTimestamp($this->timeFactory->getTime());
+		$expires->add(new \DateInterval('PT24H'));
+		$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
 		$response->addHeader('Pragma', 'cache');
 		return $response;
 	}
@@ -111,19 +114,24 @@ class IconController extends Controller {
 	 * @return FileDisplayResponse|DataDisplayResponse
 	 */
 	public function getFavicon($app = "core") {
-		$iconFile = $this->imageManager->getCachedImage('favIcon-' . $app);
-		if($iconFile === null && $this->themingDefaults->shouldReplaceIcons()) {
-			$icon = $this->iconBuilder->getFavicon($app);
-			$iconFile = $this->imageManager->setCachedImage('favIcon-' . $app, $icon);
-		}
 		if ($this->themingDefaults->shouldReplaceIcons()) {
+			$iconFile = $this->imageManager->getCachedImage('favIcon-' . $app);
+			if($iconFile === null) {
+				$icon = $this->iconBuilder->getFavicon($app);
+				$iconFile = $this->imageManager->setCachedImage('favIcon-' . $app, $icon);
+			}
 			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
+			$response->cacheFor(86400);
+			$expires = new \DateTime();
+			$expires->setTimestamp($this->timeFactory->getTime());
+			$expires->add(new \DateInterval('PT24H'));
+			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+			$response->addHeader('Pragma', 'cache');
 		} else {
 			$response = new DataDisplayResponse(null, Http::STATUS_NOT_FOUND);
+			$response->cacheFor(0);
+			$response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		}
-		$response->cacheFor(86400);
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$response->addHeader('Pragma', 'cache');
 		return $response;
 	}
 
@@ -137,19 +145,24 @@ class IconController extends Controller {
 	 * @return FileDisplayResponse|DataDisplayResponse
 	 */
 	public function getTouchIcon($app = "core") {
-		$iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app);
-		if ($iconFile === null && $this->themingDefaults->shouldReplaceIcons()) {
-			$icon = $this->iconBuilder->getTouchIcon($app);
-			$iconFile = $this->imageManager->setCachedImage('touchIcon-' . $app, $icon);
-		}
 		if ($this->themingDefaults->shouldReplaceIcons()) {
+			$iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app);
+			if ($iconFile === null) {
+				$icon = $this->iconBuilder->getTouchIcon($app);
+				$iconFile = $this->imageManager->setCachedImage('touchIcon-' . $app, $icon);
+			}
 			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']);
+			$response->cacheFor(86400);
+			$expires = new \DateTime();
+			$expires->setTimestamp($this->timeFactory->getTime());
+			$expires->add(new \DateInterval('PT24H'));
+			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
+			$response->addHeader('Pragma', 'cache');
 		} else {
 			$response = new DataDisplayResponse(null, Http::STATUS_NOT_FOUND);
+			$response->cacheFor(0);
+			$response->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		}
-		$response->cacheFor(86400);
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$response->addHeader('Pragma', 'cache');
 		return $response;
 	}
 }
\ No newline at end of file
diff --git a/apps/theming/lib/IconBuilder.php b/apps/theming/lib/IconBuilder.php
index 10ba3cacc43..fac8cad430b 100644
--- a/apps/theming/lib/IconBuilder.php
+++ b/apps/theming/lib/IconBuilder.php
@@ -94,6 +94,8 @@ class IconBuilder {
 		if($mime === "image/svg+xml" || substr($appIconContent, 0, 4) === "<svg") {
 			if(substr($appIconContent, 0, 5) !== "<?xml") {
 				$svg = "<?xml version=\"1.0\"?>".$appIconContent;
+			} else {
+				$svg = $appIconContent;
 			}
 			$tmp = new Imagick();
 			$tmp->readImageBlob($svg);
@@ -147,4 +149,4 @@ class IconBuilder {
 		return $svg;
 	}
 
-}
\ No newline at end of file
+}
diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php
index b7968d0073f..e10870685a5 100644
--- a/apps/theming/lib/ThemingDefaults.php
+++ b/apps/theming/lib/ThemingDefaults.php
@@ -23,6 +23,7 @@
 namespace OCA\Theming;
 
 
+use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IURLGenerator;
@@ -38,6 +39,8 @@ class ThemingDefaults extends \OC_Defaults {
 	private $urlGenerator;
 	/** @var IRootFolder */
 	private $rootFolder;
+	/** @var ICacheFactory */
+	private $cacheFactory;
 	/** @var string */
 	private $name;
 	/** @var string */
@@ -60,13 +63,15 @@ class ThemingDefaults extends \OC_Defaults {
 								IL10N $l,
 								IURLGenerator $urlGenerator,
 								\OC_Defaults $defaults,
-								IRootFolder $rootFolder
+								IRootFolder $rootFolder,
+								ICacheFactory $cacheFactory
 	) {
 		parent::__construct();
 		$this->config = $config;
 		$this->l = $l;
 		$this->urlGenerator = $urlGenerator;
 		$this->rootFolder = $rootFolder;
+		$this->cacheFactory = $cacheFactory;
 
 		$this->name = $defaults->getName();
 		$this->url = $defaults->getBaseUrl();
@@ -151,14 +156,20 @@ class ThemingDefaults extends \OC_Defaults {
 	 * @return bool
 	 */
 	public function shouldReplaceIcons() {
+		$cache = $this->cacheFactory->create('theming');
+		if($cache->hasKey('shouldReplaceIcons')) {
+			return (bool)$cache->get('shouldReplaceIcons');
+		}
+		$value = false;
 		if(extension_loaded('imagick')) {
 			$checkImagick = new \Imagick();
 			if (count($checkImagick->queryFormats('SVG')) >= 1) {
-				return true;
+				$value = true;
 			}
 			$checkImagick->clear();
 		}
-		return false;
+		$cache->set('shouldReplaceIcons', $value);
+		return $value;
 	}
 
 	/**
diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php
index 82a937c3b91..98fa4f1a243 100644
--- a/apps/theming/tests/Controller/IconControllerTest.php
+++ b/apps/theming/tests/Controller/IconControllerTest.php
@@ -103,7 +103,10 @@ class IconControllerTest extends TestCase {
 			->willReturn($file);
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
 		$expected->cacheFor(86400);
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
+		$expires = new \DateTime();
+		$expires->setTimestamp($this->timeFactory->getTime());
+		$expires->add(new \DateInterval('PT24H'));
+		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
 		$expected->addHeader('Pragma', 'cache');
 		@$this->assertEquals($expected, $this->iconController->getThemedIcon('core', 'filetypes/folder.svg'));
 
@@ -133,7 +136,10 @@ class IconControllerTest extends TestCase {
 
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		$expected->cacheFor(86400);
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
+		$expires = new \DateTime();
+		$expires->setTimestamp($this->timeFactory->getTime());
+		$expires->add(new \DateInterval('PT24H'));
+		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
 		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getFavicon());
 	}
@@ -143,9 +149,8 @@ class IconControllerTest extends TestCase {
 			->method('shouldReplaceIcons')
 			->willReturn(false);
 		$expected = new DataDisplayResponse(null, Http::STATUS_NOT_FOUND);
-		$expected->cacheFor(86400);
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$expected->addHeader('Pragma', 'cache');
+		$expected->cacheFor(0);
+		$expected->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		$this->assertEquals($expected, $this->iconController->getFavicon());
 	}
 
@@ -172,7 +177,10 @@ class IconControllerTest extends TestCase {
 
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/png']);
 		$expected->cacheFor(86400);
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
+		$expires = new \DateTime();
+		$expires->setTimestamp($this->timeFactory->getTime());
+		$expires->add(new \DateInterval('PT24H'));
+		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
 		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getTouchIcon());
 	}
@@ -182,9 +190,8 @@ class IconControllerTest extends TestCase {
 			->method('shouldReplaceIcons')
 			->willReturn(false);
 		$expected = new DataDisplayResponse(null, Http::STATUS_NOT_FOUND);
-		$expected->cacheFor(86400);
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$expected->addHeader('Pragma', 'cache');
+		$expected->cacheFor(0);
+		$expected->setLastModified(new \DateTime('now', new \DateTimeZone('GMT')));
 		$this->assertEquals($expected, $this->iconController->getTouchIcon());
 	}
 
diff --git a/apps/theming/tests/ThemingDefaultsTest.php b/apps/theming/tests/ThemingDefaultsTest.php
index 204c96d86d5..cd3a90e760a 100644
--- a/apps/theming/tests/ThemingDefaultsTest.php
+++ b/apps/theming/tests/ThemingDefaultsTest.php
@@ -24,6 +24,7 @@
 namespace OCA\Theming\Tests;
 
 use OCA\Theming\ThemingDefaults;
+use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IURLGenerator;
@@ -43,6 +44,8 @@ class ThemingDefaultsTest extends TestCase {
 	private $template;
 	/** @var IRootFolder */
 	private $rootFolder;
+	/** @var ICacheFactory */
+	private $cacheFactory;
 
 	public function setUp() {
 		parent::setUp();
@@ -52,6 +55,7 @@ class ThemingDefaultsTest extends TestCase {
 		$this->rootFolder = $this->getMockBuilder(IRootFolder::class)
 			->disableOriginalConstructor()
 			->getMock();
+		$this->cacheFactory = $this->getMockBuilder(ICacheFactory::class)->getMock();
 		$this->defaults = $this->getMockBuilder(\OC_Defaults::class)
 			->disableOriginalConstructor()
 			->getMock();
@@ -76,7 +80,8 @@ class ThemingDefaultsTest extends TestCase {
 			$this->l10n,
 			$this->urlGenerator,
 			$this->defaults,
-			$this->rootFolder
+			$this->rootFolder,
+			$this->cacheFactory
 		);
 	}