From 5b0ce806a3679b34ae3a6f8e6ae32a0b513a4ca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 5 Jun 2018 16:59:05 +0200 Subject: [PATCH] Minor fixes and cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../theming/lib/Controller/IconController.php | 4 ++-- apps/theming/lib/IconBuilder.php | 9 ++++++-- apps/theming/lib/ImageManager.php | 1 + apps/theming/lib/Settings/Admin.php | 2 +- apps/theming/lib/ThemingDefaults.php | 18 +++------------ .../tests/Controller/IconControllerTest.php | 13 +++++++---- .../Controller/ThemingControllerTest.php | 4 ++-- apps/theming/tests/IconBuilderTest.php | 23 +++++++++++-------- apps/theming/tests/ImageManagerTest.php | 4 ++-- 9 files changed, 39 insertions(+), 39 deletions(-) diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php index 13f385e0bd..eb01f47dd6 100644 --- a/apps/theming/lib/Controller/IconController.php +++ b/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) { diff --git a/apps/theming/lib/IconBuilder.php b/apps/theming/lib/IconBuilder.php index ad44dd7ed6..f85e2f9bff 100644 --- a/apps/theming/lib/IconBuilder.php +++ b/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 { diff --git a/apps/theming/lib/ImageManager.php b/apps/theming/lib/ImageManager.php index 77afbbe8a8..5d7c11b6e4 100644 --- a/apps/theming/lib/ImageManager.php +++ b/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(); diff --git a/apps/theming/lib/Settings/Admin.php b/apps/theming/lib/Settings/Admin.php index 6a95dd39d4..c8d2d56151 100644 --- a/apps/theming/lib/Settings/Admin.php +++ b/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(), diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php index a46676a43a..d29eb69873 100644 --- a/apps/theming/lib/ThemingDefaults.php +++ b/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') { @@ -334,19 +334,7 @@ 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 */ diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php index f509005d32..cc83bae32a 100644 --- a/apps/theming/tests/Controller/IconControllerTest.php +++ b/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'; diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 4a2b780076..aaed61016e 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/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'); diff --git a/apps/theming/tests/IconBuilderTest.php b/apps/theming/tests/IconBuilderTest.php index 1b9f204cd9..994e0e4a04 100644 --- a/apps/theming/tests/IconBuilderTest.php +++ b/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'); diff --git a/apps/theming/tests/ImageManagerTest.php b/apps/theming/tests/ImageManagerTest.php index 6912395268..6bfa5b330a 100644 --- a/apps/theming/tests/ImageManagerTest.php +++ b/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.'); } }