Minor fixes and cleanup

Signed-off-by: Julius Härtl <jus@bitgrid.net>

apps/theming/lib/Controller/IconController.php

@@ -125,7 +125,7 @@ class IconController extends Controller {
 			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		} catch (NotFoundException $e) {
 		}
-		if ($iconFile === null && $this->themingDefaults->shouldReplaceIcons()) {
+		if ($iconFile === null && $this->imageManager->shouldReplaceIcons()) {
 			try {
 				$iconFile = $this->imageManager->getCachedImage('favIcon-' . $app);
 			} catch (NotFoundException $exception) {
@@ -161,7 +161,7 @@ class IconController extends Controller {
 			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		} catch (NotFoundException $e) {
 		}
-		if ($this->themingDefaults->shouldReplaceIcons()) {
+		if ($this->imageManager->shouldReplaceIcons()) {
 			try {
 				$iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app);
 			} catch (NotFoundException $exception) {

apps/theming/lib/IconBuilder.php

@@ -35,19 +35,24 @@ class IconBuilder {
 	private $themingDefaults;
 	/** @var Util */
 	private $util;
+	/** @var ImageManager */
+	private $imageManager;
 
 	/**
 	 * IconBuilder constructor.
 	 *
 	 * @param ThemingDefaults $themingDefaults
 	 * @param Util $util
+	 * @param ImageManager $imageManager
 	 */
 	public function __construct(
 		ThemingDefaults $themingDefaults,
-		Util $util
+		Util $util,
+		ImageManager $imageManager
 	) {
 		$this->themingDefaults = $themingDefaults;
 		$this->util = $util;
+		$this->imageManager = $imageManager;
 	}
 
 	/**
@@ -55,7 +60,7 @@ class IconBuilder {
 	 * @return string|false image blob
 	 */
 	public function getFavicon($app) {
-		if (!$this->themingDefaults->shouldReplaceIcons()) {
+		if (!$this->imageManager->shouldReplaceIcons()) {
 			return false;
 		}
 		try {

apps/theming/lib/ImageManager.php

@@ -181,6 +181,7 @@ class ImageManager {
 	}
 
 	public function delete(string $key) {
+		/* ignore exceptions, since we don't want to fail hard if something goes wrong during cleanup */
 		try {
 			$file = $this->appData->getFolder('images')->getFile($key);
 			$file->delete();

apps/theming/lib/Settings/Admin.php

@@ -81,7 +81,7 @@ class Admin implements ISettings {
 			'slogan'          => $this->themingDefaults->getSlogan(),
 			'color'           => $this->themingDefaults->getColorPrimary(),
 			'uploadLogoRoute' => $this->urlGenerator->linkToRoute('theming.Theming.uploadImage'),
-			'canThemeIcons'   => $this->themingDefaults->shouldReplaceIcons(),
+			'canThemeIcons'   => $this->imageManager->shouldReplaceIcons(),
 			'iconDocs'        => $this->urlGenerator->linkToDocs('admin-theming-icons'),
 			'images'		  => $this->imageManager->getCustomImages(),
 			'imprintUrl'      => $this->themingDefaults->getImprintUrl(),

apps/theming/lib/ThemingDefaults.php

@@ -317,10 +317,10 @@ class ThemingDefaults extends \OC_Defaults {
 			$customFavicon = null;
 		}
 
-		if ($image === 'favicon.ico' && ($customFavicon !== null || $this->shouldReplaceIcons())) {
+		if ($image === 'favicon.ico' && ($customFavicon !== null || $this->imageManager->shouldReplaceIcons())) {
 			return $this->urlGenerator->linkToRoute('theming.Icon.getFavicon', ['app' => $app]) . '?v=' . $cacheBusterValue;
 		}
-		if ($image === 'favicon-touch.png' && ($customFavicon !== null || $this->shouldReplaceIcons())) {
+		if ($image === 'favicon-touch.png' && ($customFavicon !== null || $this->imageManager->shouldReplaceIcons())) {
 			return $this->urlGenerator->linkToRoute('theming.Icon.getTouchIcon', ['app' => $app]) . '?v=' . $cacheBusterValue;
 		}
 		if ($image === 'manifest.json') {
@@ -335,18 +335,6 @@ class ThemingDefaults extends \OC_Defaults {
 		return false;
 	}
 	
-	/**
-	 * Check if Imagemagick is enabled and if SVG is supported
-	 * otherwise we can't render custom icons
-	 *
-	 * TODO: move to usage of image manager
-	 *
-	 * @return bool
-	 */
-	public function shouldReplaceIcons() {
-		return $this->imageManager->shouldReplaceIcons();
-	}
-
 	/**
 	 * Increases the cache buster key
 	 */

apps/theming/tests/Controller/IconControllerTest.php

@@ -118,7 +118,7 @@ class IconControllerTest extends TestCase {
 			->method('getImage')
 			->with('favicon')
 			->will($this->throwException(new NotFoundException()));
-		$this->themingDefaults->expects($this->any())
+		$this->imageManager->expects($this->any())
 			->method('shouldReplaceIcons')
 			->willReturn(true);
 		$this->imageManager->expects($this->once())
@@ -142,7 +142,7 @@ class IconControllerTest extends TestCase {
 			->method('getImage')
 			->with('favicon')
 			->will($this->throwException(new NotFoundException()));
-		$this->themingDefaults->expects($this->any())
+		$this->imageManager->expects($this->any())
 			->method('shouldReplaceIcons')
 			->willReturn(false);
 		$fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon.png';
@@ -163,10 +163,13 @@ class IconControllerTest extends TestCase {
 		if (count($checkImagick->queryFormats('SVG')) < 1) {
 			$this->markTestSkipped('No SVG provider present.');
 		}
-		$this->themingDefaults->expects($this->any())
+
+		$this->imageManager->expects($this->once())
+			->method('getImage')
+			->will($this->throwException(new NotFoundException()));
+		$this->imageManager->expects($this->any())
 			->method('shouldReplaceIcons')
 			->willReturn(true);
-
 		$this->iconBuilder->expects($this->once())
 			->method('getTouchIcon')
 			->with('core')
@@ -189,7 +192,7 @@ class IconControllerTest extends TestCase {
 			->method('getImage')
 			->with('favicon')
 			->will($this->throwException(new NotFoundException()));
-		$this->themingDefaults->expects($this->any())
+		$this->imageManager->expects($this->any())
 			->method('shouldReplaceIcons')
 			->willReturn(false);
 		$fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon-touch.png';

apps/theming/tests/Controller/ThemingControllerTest.php

@@ -688,7 +688,7 @@ class ThemingControllerTest extends TestCase {
 			->method('getImage')
 			->willReturn($file);
 		$this->config
-			->expects($this->once())
+			->expects($this->any())
 			->method('getAppValue')
 			->with('theming', 'logoMime', '')
 			->willReturn('text/svg');
@@ -716,7 +716,7 @@ class ThemingControllerTest extends TestCase {
 			->willReturn($file);
 
 		$this->config
-			->expects($this->once())
+			->expects($this->any())
 			->method('getAppValue')
 			->with('theming', 'backgroundMime', '')
 			->willReturn('image/png');

apps/theming/tests/IconBuilderTest.php

@@ -27,6 +27,7 @@ namespace OCA\Theming\Tests;
 
 use OC\Files\AppData\AppData;
 use OCA\Theming\IconBuilder;
+use OCA\Theming\ImageManager;
 use OCA\Theming\ThemingDefaults;
 use OCA\Theming\Util;
 use OCP\App\IAppManager;
@@ -45,6 +46,8 @@ class IconBuilderTest extends TestCase {
 	protected $themingDefaults;
 	/** @var Util */
 	protected $util;
+	/** @var ImageManager */
+	protected $imageManager;
 	/** @var IconBuilder */
 	protected $iconBuilder;
 	/** @var IAppManager */
@@ -53,13 +56,13 @@ class IconBuilderTest extends TestCase {
 	protected function setUp() {
 		parent::setUp();
 
-		$this->config = $this->getMockBuilder(IConfig::class)->getMock();
+		$this->config = $this->createMock(IConfig::class);
 		$this->appData = $this->createMock(AppData::class);
-		$this->themingDefaults = $this->getMockBuilder('OCA\Theming\ThemingDefaults')
-			->disableOriginalConstructor()->getMock();
-		$this->appManager = $this->getMockBuilder('OCP\App\IAppManager')->getMock();
+		$this->themingDefaults = $this->createMock(ThemingDefaults::class);
+		$this->appManager = $this->createMock(IAppManager::class);
+		$this->imageManager = $this->createMock(ImageManager::class);
 		$this->util = new Util($this->config, $this->appManager, $this->appData);
-		$this->iconBuilder = new IconBuilder($this->themingDefaults, $this->util);
+		$this->iconBuilder = new IconBuilder($this->themingDefaults, $this->util, $this->imageManager);
 	}
 
 	private function checkImagick() {
@@ -152,7 +155,7 @@ class IconBuilderTest extends TestCase {
 	 */
 	public function testGetFavicon($app, $color, $file) {
 		$this->checkImagick();
-		$this->themingDefaults->expects($this->once())
+		$this->imageManager->expects($this->once())
 			->method('shouldReplaceIcons')
 			->willReturn(true);
 		$this->themingDefaults->expects($this->once())
@@ -183,8 +186,8 @@ class IconBuilderTest extends TestCase {
 		$this->checkImagick();
 		$this->expectException(Warning::class);
 		$util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
-		$iconBuilder = new IconBuilder($this->themingDefaults, $util);
-		$this->themingDefaults->expects($this->once())
+		$iconBuilder = new IconBuilder($this->themingDefaults, $util, $this->imageManager);
+		$this->imageManager->expects($this->once())
 			->method('shouldReplaceIcons')
 			->willReturn(true);
 		$util->expects($this->once())
@@ -197,7 +200,7 @@ class IconBuilderTest extends TestCase {
 		$this->checkImagick();
 		$this->expectException(Warning::class);
 		$util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
-		$iconBuilder = new IconBuilder($this->themingDefaults, $util);
+		$iconBuilder = new IconBuilder($this->themingDefaults, $util, $this->imageManager);
 		$util->expects($this->once())
 			->method('getAppIcon')
 			->willReturn('notexistingfile');
@@ -208,7 +211,7 @@ class IconBuilderTest extends TestCase {
 		$this->checkImagick();
 		$this->expectException(Warning::class);
 		$util = $this->getMockBuilder(Util::class)->disableOriginalConstructor()->getMock();
-		$iconBuilder = new IconBuilder($this->themingDefaults, $util);
+		$iconBuilder = new IconBuilder($this->themingDefaults, $util, $this->imageManager);
 		$util->expects($this->once())
 			->method('getAppImage')
 			->willReturn('notexistingfile');

apps/theming/tests/ImageManagerTest.php

@@ -66,10 +66,10 @@ class ImageManagerTest extends TestCase {
 			$this->markTestSkipped('Imagemagick is required for dynamic icon generation.');
 		}
 		$checkImagick = new \Imagick();
-		if (count($checkImagick->queryFormats('SVG')) < 1) {
+		if (empty($checkImagick->queryFormats('SVG'))) {
 			$this->markTestSkipped('No SVG provider present.');
 		}
-		if (count($checkImagick->queryFormats('PNG')) < 1) {
+		if (empty($checkImagick->queryFormats('PNG'))) {
 			$this->markTestSkipped('No PNG provider present.');
 		}
 	}