From d9e2b04af1acce659067ab6d585755cf2b2a83c4 Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Wed, 23 Sep 2026 21:47:06 +0200 Subject: [PATCH] feat(theming): generate favicons as SVG without imagick The favicon endpoint now returns an SVG that embeds the app icon on the themed background, instead of rendering an ICO file with imagick. This removes the imagick requirement for favicons and allows uploading SVG favicons without imagick. Touch icons need to be PNG for Safari "Add to Dock" and the iOS home screen, so they are still rendered with imagick when available and fall back to the default touch icon otherwise. An uploaded favicon is served as-is by both endpoints. The manifest declares the type of the served icons, and the social media preview image of shares is only replaced by the touch icon if it is a raster image. The imagick setup check now only reports when touch icons cannot be themed or an SVG logo cannot be converted to PNG for emails. Closes #36607 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Simon L. --- .../theming/lib/Controller/IconController.php | 110 +++++---- .../lib/Controller/ThemingController.php | 6 +- apps/theming/lib/IconBuilder.php | 77 ++++--- apps/theming/lib/ImageManager.php | 8 +- .../lib/SetupChecks/PhpImagickModule.php | 32 ++- apps/theming/lib/ThemingDefaults.php | 15 +- apps/theming/openapi.json | 20 +- .../tests/Controller/IconControllerTest.php | 214 +++++++++--------- .../Controller/ThemingControllerTest.php | 23 +- apps/theming/tests/IconBuilderTest.php | 140 ++++++------ apps/theming/tests/ImageManagerTest.php | 1 + .../SetupChecks/PhpImagickModuleTest.php | 65 ++++++ apps/theming/tests/ThemingDefaultsTest.php | 31 +++ openapi.json | 20 +- 14 files changed, 433 insertions(+), 329 deletions(-) create mode 100644 apps/theming/tests/SetupChecks/PhpImagickModuleTest.php diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php index 69ef91bf5b088..d19046b277a0c 100644 --- a/apps/theming/lib/Controller/IconController.php +++ b/apps/theming/lib/Controller/IconController.php @@ -18,10 +18,12 @@ use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataDisplayResponse; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\Http\Response; use OCP\Files\NotFoundException; +use OCP\Files\SimpleFS\ISimpleFile; use OCP\IConfig; use OCP\IRequest; @@ -75,10 +77,10 @@ public function getThemedIcon(string $app, string $image): Response { } /** - * Return a 32x32 favicon as png + * Return a favicon as svg * * @param string $app ID of the app - * @return DataDisplayResponse|FileDisplayResponse|NotFoundResponse + * @return FileDisplayResponse|NotFoundResponse * @throws \Exception * * 200: Favicon returned @@ -92,46 +94,32 @@ public function getFavicon(string $app = 'core'): Response { $app = 'core'; } - $response = null; - $iconFile = null; - // retrieve instance favicon - try { - $iconFile = $this->imageManager->getImage('favicon', false); - $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); - } catch (NotFoundException $e) { + $customFavicon = $this->getCustomFaviconResponse(); + if ($customFavicon !== null) { + return $customFavicon; } - // retrieve or generate app specific favicon, but only if no custom favicon was uploaded - if ($iconFile === null && ($this->imageManager->canConvert('PNG') || $this->imageManager->canConvert('SVG')) && $this->imageManager->canConvert('ICO')) { - $color = $this->themingDefaults->getColorPrimary(); - try { - $iconFile = $this->imageManager->getCachedImage('favIcon-' . $app . $color); - } catch (NotFoundException $exception) { - $icon = $this->iconBuilder->getFavicon($app); - if ($icon === false || $icon === '') { - return new NotFoundResponse(); - } - $iconFile = $this->imageManager->setCachedImage('favIcon-' . $app . $color, $icon); + + $cacheKey = 'favIconSvg-' . $app . $this->themingDefaults->getColorPrimary(); + try { + $iconFile = $this->imageManager->getCachedImage($cacheKey); + } catch (NotFoundException $exception) { + $icon = $this->iconBuilder->getFavicon($app); + if ($icon === false || $icon === '') { + return new NotFoundResponse(); } - $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); - } - // fallback to core favicon - if ($response === null) { - $fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon.png'; - $response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']); + $iconFile = $this->imageManager->setCachedImage($cacheKey, $icon); } - $response->cacheFor(86400); - return $response; + return $this->createIconResponse($iconFile, 'image/svg+xml'); } /** * Return a 512x512 icon for touch devices * * @param string $app ID of the app - * @return DataDisplayResponse|FileDisplayResponse|NotFoundResponse + * @return DataDisplayResponse|FileDisplayResponse * @throws \Exception * * 200: Touch icon returned - * 404: Touch icon not found */ #[PublicPage] #[NoCSRFRequired] @@ -141,33 +129,57 @@ public function getTouchIcon(string $app = 'core'): Response { $app = 'core'; } - $response = null; - $iconFile = null; - // retrieve instance favicon - try { - $iconFile = $this->imageManager->getImage('favicon'); - $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => $iconFile->getMimeType()]); - } catch (NotFoundException $e) { + $customFavicon = $this->getCustomFaviconResponse(); + if ($customFavicon !== null) { + return $customFavicon; } - // retrieve or generate app specific touch icon, but only if no custom favicon was uploaded - if ($iconFile === null && $this->imageManager->canConvert('PNG')) { - $color = $this->themingDefaults->getColorPrimary(); + + // touch icons need to be png, which can only be rendered with imagick + if ($this->imageManager->canConvert('PNG')) { + $iconFile = null; + $cacheKey = 'touchIcon-' . $app . $this->themingDefaults->getColorPrimary(); try { - $iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app . $color); + $iconFile = $this->imageManager->getCachedImage($cacheKey); } catch (NotFoundException $exception) { $icon = $this->iconBuilder->getTouchIcon($app); - if ($icon === false || $icon === '') { - return new NotFoundResponse(); + if ($icon !== false && $icon !== '') { + $iconFile = $this->imageManager->setCachedImage($cacheKey, $icon); } - $iconFile = $this->imageManager->setCachedImage('touchIcon-' . $app . $color, $icon); } - $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/png']); + if ($iconFile !== null) { + return $this->createIconResponse($iconFile, 'image/png'); + } } - // fallback to core touch icon - if ($response === null) { - $fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon-touch.png'; - $response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']); + + $fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon-touch.png'; + $response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']); + $response->cacheFor(86400); + return $response; + } + + /** + * An uploaded favicon is used for all apps and served as-is + * + * @return FileDisplayResponse|null + */ + private function getCustomFaviconResponse(): ?FileDisplayResponse { + try { + $iconFile = $this->imageManager->getImage('favicon'); + } catch (NotFoundException $e) { + return null; } + return $this->createIconResponse($iconFile, $this->imageManager->getImageMime('favicon')); + } + + /** + * @return FileDisplayResponse + */ + private function createIconResponse(ISimpleFile $iconFile, string $mime): FileDisplayResponse { + $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => $mime]); + // the generated svg embeds the app icon as data uri + $csp = new EmptyContentSecurityPolicy(); + $csp->addAllowedImageDomain('data:'); + $response->setContentSecurityPolicy($csp); $response->cacheFor(86400); return $response; } diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 76c1129366701..078dc4307e5d2 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -484,6 +484,8 @@ public function getManifest(string $app): JSONResponse { * @var string $description * @var string $shortName */ + // the icon endpoints serve an uploaded favicon as-is + $customFaviconType = $this->imageManager->hasImage('favicon') ? $this->imageManager->getImageMime('favicon') : null; $responseJS = [ 'name' => $name, 'short_name' => $shortName, @@ -496,13 +498,13 @@ public function getManifest(string $app): JSONResponse { [ 'src' => $this->urlGenerator->linkToRoute('theming.Icon.getTouchIcon', ['app' => $app]) . '?v=' . $cacheBusterValue, - 'type' => 'image/png', + 'type' => $customFaviconType ?? 'image/png', 'sizes' => '512x512' ], [ 'src' => $this->urlGenerator->linkToRoute('theming.Icon.getFavicon', ['app' => $app]) . '?v=' . $cacheBusterValue, - 'type' => 'image/svg+xml', + 'type' => $customFaviconType ?? 'image/svg+xml', 'sizes' => '16x16' ] ], diff --git a/apps/theming/lib/IconBuilder.php b/apps/theming/lib/IconBuilder.php index 92018456a21a9..f99715f14c9c2 100644 --- a/apps/theming/lib/IconBuilder.php +++ b/apps/theming/lib/IconBuilder.php @@ -28,47 +28,56 @@ public function __construct( } /** - * @param $app string app name - * @return string|false image blob + * Render app icon on themed background color as SVG + * fallback to logo + * + * @param string $app app name + * @return string|false content of the svg file */ - public function getFavicon($app) { - if (!$this->imageManager->canConvert('PNG')) { + public function getFavicon(string $app): string|false { + $appIcon = $this->util->getAppIcon($app); + if ($appIcon instanceof ISimpleFile) { + $appIconContent = $appIcon->getContent(); + } elseif (!file_exists($appIcon)) { return false; + } else { + $appIconContent = file_get_contents($appIcon); } - try { - $icon = $this->renderAppIcon($app, 128); - if ($icon === false) { - return false; - } - $icon->setImageFormat('PNG32'); - $favicon = new Imagick(); - $favicon->setFormat('ICO'); - - $clone = clone $icon; - $clone->scaleImage(16, 0); - $favicon->addImage($clone); - - $clone = clone $icon; - $clone->scaleImage(32, 0); - $favicon->addImage($clone); - - $clone = clone $icon; - $clone->scaleImage(64, 0); - $favicon->addImage($clone); + if ($appIconContent === false || $appIconContent === '') { + return false; + } - $clone = clone $icon; - $clone->scaleImage(128, 0); - $favicon->addImage($clone); + // the custom logo is stored without file extension, so the mime type is detected from the content + $mime = (new \finfo(FILEINFO_MIME_TYPE))->buffer($appIconContent); + if (!str_starts_with($mime, 'image/') || $mime === 'image/svg') { + if (!str_contains($appIconContent, 'themingDefaults->getColorPrimary(); - $data = $favicon->getImagesBlob(); - $favicon->destroy(); - $icon->destroy(); - $clone->destroy(); - return $data; - } catch (\ImagickException $e) { - return false; + /** + * invert app icons for bright primary colors + * the default nextcloud logo and custom logos will not be inverted + */ + $filter = ''; + $filterAttribute = ''; + if ($this->util->isBrightColor($color) + && !$appIcon instanceof ISimpleFile + && $app !== 'core' + ) { + $filter = ''; + $filterAttribute = ' filter="url(#invert)"'; } + + return '' + . $filter + . '' + . '' + . ''; } /** diff --git a/apps/theming/lib/ImageManager.php b/apps/theming/lib/ImageManager.php index 75dca1d096d66..dc1a6126d76dc 100644 --- a/apps/theming/lib/ImageManager.php +++ b/apps/theming/lib/ImageManager.php @@ -329,18 +329,12 @@ private function shouldOptimizeBackgroundImage(string $mimeType, int $contentSiz /** * Returns a list of supported mime types for image uploads. - * "favicon" images are only allowed to be SVG when imagemagick with SVG support is available. * * @param string $key The image key, e.g. "favicon" * @return string[] */ public function getSupportedUploadImageFormats(string $key): array { - $supportedFormats = ['image/jpeg', 'image/png', 'image/gif', 'image/webp']; - - if ($key !== 'favicon' || $this->canConvert('SVG') === true) { - $supportedFormats[] = 'image/svg+xml'; - $supportedFormats[] = 'image/svg'; - } + $supportedFormats = ['image/jpeg', 'image/png', 'image/gif', 'image/webp', 'image/svg+xml', 'image/svg']; if ($key === 'favicon') { $supportedFormats[] = 'image/x-icon'; diff --git a/apps/theming/lib/SetupChecks/PhpImagickModule.php b/apps/theming/lib/SetupChecks/PhpImagickModule.php index 266e728d4c08d..9c4ff4d32e4c8 100644 --- a/apps/theming/lib/SetupChecks/PhpImagickModule.php +++ b/apps/theming/lib/SetupChecks/PhpImagickModule.php @@ -9,6 +9,7 @@ namespace OCA\Theming\SetupChecks; +use OCA\Theming\ImageManager; use OCP\IL10N; use OCP\IURLGenerator; use OCP\SetupCheck\ISetupCheck; @@ -18,6 +19,7 @@ class PhpImagickModule implements ISetupCheck { public function __construct( private IL10N $l10n, private IURLGenerator $urlGenerator, + private ImageManager $imageManager, ) { } @@ -33,18 +35,26 @@ public function getCategory(): string { #[\Override] public function run(): SetupResult { - if (!extension_loaded('imagick')) { - return SetupResult::info( - $this->l10n->t('The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.'), - $this->urlGenerator->linkToDocs('admin-php-modules') - ); - } elseif (count(\Imagick::queryFormats('SVG')) === 0) { - return SetupResult::info( - $this->l10n->t('The PHP module "imagick" in this instance has no SVG support. For better compatibility it is recommended to install it.'), - $this->urlGenerator->linkToDocs('admin-php-modules') - ); - } else { + if ($this->imageManager->canConvert('SVG') && $this->imageManager->canConvert('PNG')) { return SetupResult::success(); } + + $issues = []; + // an uploaded favicon is used as touch icon as-is + if (!$this->imageManager->hasImage('favicon')) { + $issues[] = $this->l10n->t('Icons for the home screen of mobile devices and for "Add to Dock" in Safari cannot be themed and show the default icon instead. Upload a PNG favicon or install the module with SVG support to avoid this.'); + } + $logoMime = $this->imageManager->getImageMime('logo'); + if ($logoMime === 'image/svg+xml' || $logoMime === 'image/svg') { + $issues[] = $this->l10n->t('The custom logo was uploaded as SVG and cannot be converted to PNG, so it will be missing in emails for many mail clients (e.g. Gmail and Outlook) that do not display SVG images. Upload the logo as PNG or install the module with SVG support to avoid this.'); + } + if ($issues === []) { + return SetupResult::success(); + } + + return SetupResult::info( + $this->l10n->t('The PHP module "imagick" is not enabled or has no SVG support.') . ' ' . implode(' ', $issues), + $this->urlGenerator->linkToDocs('admin-php-modules') + ); } } diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php index 9b44fb6ae1492..1fc85d736607a 100644 --- a/apps/theming/lib/ThemingDefaults.php +++ b/apps/theming/lib/ThemingDefaults.php @@ -407,10 +407,10 @@ public function replaceImagePath($app, $image) { } $route = false; - if ($image === 'favicon.ico' && ($this->imageManager->canConvert('ICO') || $this->getCustomFavicon() !== null)) { + if ($image === 'favicon.ico') { $route = $this->urlGenerator->linkToRoute('theming.Icon.getFavicon', ['app' => $app]); } - if (($image === 'favicon-touch.png' || $image === 'favicon-fb.png') && ($this->imageManager->canConvert('PNG') || $this->getCustomFavicon() !== null)) { + if ($image === 'favicon-touch.png' || ($image === 'favicon-fb.png' && $this->useTouchIconForSocialPreview())) { $route = $this->urlGenerator->linkToRoute('theming.Icon.getTouchIcon', ['app' => $app]); } if ($image === 'manifest.json') { @@ -434,6 +434,17 @@ public function replaceImagePath($app, $image) { return false; } + /** + * Social media previews only support raster images, so the touch icon replaces + * the default preview image only if it is an uploaded raster favicon or a themed png + */ + private function useTouchIconForSocialPreview(): bool { + if ($this->getCustomFavicon() === null) { + return $this->imageManager->canConvert('PNG'); + } + return in_array($this->imageManager->getImageMime('favicon'), ['image/png', 'image/jpeg', 'image/gif'], true); + } + protected function getCustomFavicon(): ?ISimpleFile { try { return $this->imageManager->getImage('favicon'); diff --git a/apps/theming/openapi.json b/apps/theming/openapi.json index 24297b2a7e91b..d87263fb99df9 100644 --- a/apps/theming/openapi.json +++ b/apps/theming/openapi.json @@ -460,7 +460,7 @@ "/index.php/apps/theming/favicon/{app}": { "get": { "operationId": "icon-get-favicon", - "summary": "Return a 32x32 favicon as png", + "summary": "Return a favicon as svg", "tags": [ "icon" ], @@ -489,13 +489,7 @@ "200": { "description": "Favicon returned", "content": { - "image/png": { - "schema": { - "type": "string", - "format": "binary" - } - }, - "image/x-icon": { + "*/*": { "schema": { "type": "string", "format": "binary" @@ -572,16 +566,6 @@ } } }, - "404": { - "description": "Touch icon not found", - "content": { - "text/html": { - "schema": { - "type": "string" - } - } - } - }, "500": { "description": "", "content": { diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php index 5a2e12c04f18b..1f8567a55ab22 100644 --- a/apps/theming/tests/Controller/IconControllerTest.php +++ b/apps/theming/tests/Controller/IconControllerTest.php @@ -17,7 +17,9 @@ use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataDisplayResponse; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\AppFramework\Http\FileDisplayResponse; +use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\File; use OCP\Files\NotFoundException; @@ -89,149 +91,157 @@ public function testGetThemedIcon(): void { $this->assertEquals($expected, $this->iconController->getThemedIcon('core', 'filetypes/folder.svg')); } + private function iconResponse(SimpleFile $file, string $mime): FileDisplayResponse { + $response = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => $mime]); + $csp = new EmptyContentSecurityPolicy(); + $csp->addAllowedImageDomain('data:'); + $response->setContentSecurityPolicy($csp); + $response->cacheFor(86400); + return $response; + } + public function testGetFaviconThemed(): void { - if (!extension_loaded('imagick')) { - $this->markTestSkipped('Imagemagick is required for dynamic icon generation.'); - } - $checkImagick = new \Imagick(); - if (count($checkImagick->queryFormats('SVG')) < 1) { - $this->markTestSkipped('No SVG provider present.'); - } $file = $this->iconFileMock('filename', 'filecontent'); - $this->imageManager->expects($this->once()) - ->method('getImage', false) - ->with('favicon') - ->willThrowException(new NotFoundException()); - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->willReturnMap([ - ['SVG', true], - ['PNG', true], - ['ICO', true], - ]); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); + $this->imageManager->expects($this->never())->method('canConvert'); $this->imageManager->expects($this->once()) ->method('getCachedImage') + ->with('favIconSvg-core#0082c9') ->willThrowException(new NotFoundException()); $this->iconBuilder->expects($this->once()) ->method('getFavicon') ->with('core') - ->willReturn('filecontent'); + ->willReturn(''); $this->imageManager->expects($this->once()) ->method('setCachedImage') + ->with('favIconSvg-core#0082c9', '') ->willReturn($file); - $expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getFavicon()); + $this->assertEquals($this->iconResponse($file, 'image/svg+xml'), $this->iconController->getFavicon()); } - public function testGetFaviconUploaded(): void { - // a custom favicon was uploaded, so it must be served as-is and the - // app-specific generation path must not overwrite it - $file = $this->iconFileMock('favicon.ico', 'filecontent'); + public function testGetFaviconCached(): void { + $file = $this->iconFileMock('filename', 'filecontent'); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); + $this->appManager->method('isEnabledForUser')->with('files')->willReturn(true); $this->imageManager->expects($this->once()) - ->method('getImage') - ->with('favicon', false) + ->method('getCachedImage') + ->with('favIconSvg-files#0082c9') ->willReturn($file); - $this->imageManager->expects($this->never()) - ->method('getCachedImage'); - $this->iconBuilder->expects($this->never()) - ->method('getFavicon'); - - $expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getFavicon()); + $this->iconBuilder->expects($this->never())->method('getFavicon'); + $this->imageManager->expects($this->never())->method('setCachedImage'); + + $this->assertEquals($this->iconResponse($file, 'image/svg+xml'), $this->iconController->getFavicon('files')); } - public function testGetFaviconDefault(): void { + public function testGetFaviconDisabledApp(): void { + $file = $this->iconFileMock('filename', 'filecontent'); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); + $this->appManager->method('isEnabledForUser')->with('disabledapp')->willReturn(false); $this->imageManager->expects($this->once()) - ->method('getImage') - ->with('favicon', false) - ->willThrowException(new NotFoundException()); - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->willReturnMap([ - ['SVG', false], - ['PNG', false], - ['ICO', false], - ]); - $fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon.png'; - $this->fileAccessHelper->expects($this->once()) - ->method('file_get_contents') - ->with($fallbackLogo) - ->willReturn(file_get_contents($fallbackLogo)); - $expected = new DataDisplayResponse(file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getFavicon()); + ->method('getCachedImage') + ->with('favIconSvg-core#0082c9') + ->willReturn($file); + + $this->assertEquals($this->iconResponse($file, 'image/svg+xml'), $this->iconController->getFavicon('disabledapp')); + } + + public function testGetFaviconFail(): void { + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->imageManager->method('getCachedImage')->willThrowException(new NotFoundException()); + $this->iconBuilder->method('getFavicon')->willReturn(false); + $this->imageManager->expects($this->never())->method('setCachedImage'); + + $this->assertEquals(new NotFoundResponse(), $this->iconController->getFavicon()); + } + + public static function dataIconEndpoints(): array { + return [ + ['getFavicon'], + ['getTouchIcon'], + ]; } - public function testGetTouchIconDefault(): void { - if (!extension_loaded('imagick')) { - $this->markTestSkipped('Imagemagick is required for dynamic icon generation.'); - } - $checkImagick = new \Imagick(); - if (count($checkImagick->queryFormats('SVG')) < 1) { - $this->markTestSkipped('No SVG provider present.'); - } + #[\PHPUnit\Framework\Attributes\DataProvider('dataIconEndpoints')] + public function testGetIconUploaded(string $method): void { + // a custom favicon was uploaded, so it must be served as-is and the + // app-specific generation path must not overwrite it + $file = $this->iconFileMock('favicon', 'filecontent'); + $this->imageManager->method('getImage')->with('favicon')->willReturn($file); + $this->imageManager->method('getImageMime')->with('favicon')->willReturn('image/png'); + $this->imageManager->expects($this->never())->method('getCachedImage'); + $this->iconBuilder->expects($this->never())->method('getFavicon'); + $this->iconBuilder->expects($this->never())->method('getTouchIcon'); + + $this->assertEquals($this->iconResponse($file, 'image/png'), $this->iconController->$method()); + } + public function testGetTouchIconThemed(): void { + $file = $this->iconFileMock('filename', 'filecontent'); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->imageManager->method('canConvert')->with('PNG')->willReturn(true); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); $this->imageManager->expects($this->once()) - ->method('getImage') + ->method('getCachedImage') + ->with('touchIcon-core#0082c9') ->willThrowException(new NotFoundException()); - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->with('PNG') - ->willReturn(true); $this->iconBuilder->expects($this->once()) ->method('getTouchIcon') ->with('core') - ->willReturn('filecontent'); - $file = $this->iconFileMock('filename', 'filecontent'); - $this->imageManager->expects($this->once()) - ->method('getCachedImage') - ->willThrowException(new NotFoundException()); + ->willReturn('pngcontent'); $this->imageManager->expects($this->once()) ->method('setCachedImage') + ->with('touchIcon-core#0082c9', 'pngcontent') ->willReturn($file); - $expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/png']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getTouchIcon()); + $this->assertEquals($this->iconResponse($file, 'image/png'), $this->iconController->getTouchIcon()); } - public function testGetTouchIconUploaded(): void { - // a custom favicon was uploaded, so it must be served as-is and the - // app-specific generation path must not overwrite it - $file = $this->iconFileMock('favicon.png', 'filecontent'); + public function testGetTouchIconCached(): void { + $file = $this->iconFileMock('filename', 'filecontent'); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->imageManager->method('canConvert')->with('PNG')->willReturn(true); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); + $this->appManager->method('isEnabledForUser')->with('files')->willReturn(true); $this->imageManager->expects($this->once()) - ->method('getImage') - ->with('favicon') + ->method('getCachedImage') + ->with('touchIcon-files#0082c9') ->willReturn($file); - $this->imageManager->expects($this->never()) - ->method('getCachedImage'); - $this->iconBuilder->expects($this->never()) - ->method('getTouchIcon'); - - $expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image type']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getTouchIcon()); + $this->iconBuilder->expects($this->never())->method('getTouchIcon'); + + $this->assertEquals($this->iconResponse($file, 'image/png'), $this->iconController->getTouchIcon('files')); + } + + public function testGetTouchIconWithoutImagick(): void { + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->imageManager->method('canConvert')->with('PNG')->willReturn(false); + $this->iconBuilder->expects($this->never())->method('getTouchIcon'); + $this->iconBuilder->expects($this->never())->method('getFavicon'); + + $this->assertEquals($this->fallbackTouchIconResponse(), $this->iconController->getTouchIcon()); } public function testGetTouchIconFail(): void { - $this->imageManager->expects($this->once()) - ->method('getImage') - ->with('favicon') - ->willThrowException(new NotFoundException()); - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->with('PNG') - ->willReturn(false); + $this->imageManager->method('getImage')->with('favicon')->willThrowException(new NotFoundException()); + $this->imageManager->method('canConvert')->with('PNG')->willReturn(true); + $this->imageManager->method('getCachedImage')->willThrowException(new NotFoundException()); + $this->iconBuilder->method('getTouchIcon')->willReturn(false); + $this->imageManager->expects($this->never())->method('setCachedImage'); + + $this->assertEquals($this->fallbackTouchIconResponse(), $this->iconController->getTouchIcon()); + } + + private function fallbackTouchIconResponse(): DataDisplayResponse { $fallbackLogo = \OC::$SERVERROOT . '/core/img/favicon-touch.png'; $this->fileAccessHelper->expects($this->once()) ->method('file_get_contents') ->with($fallbackLogo) - ->willReturn(file_get_contents($fallbackLogo)); - $expected = new DataDisplayResponse(file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']); - $expected->cacheFor(86400); - $this->assertEquals($expected, $this->iconController->getTouchIcon()); + ->willReturn('fallbackcontent'); + $response = new DataDisplayResponse('fallbackcontent', Http::STATUS_OK, ['Content-Type' => 'image/png']); + $response->cacheFor(86400); + return $response; } } diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 0c4b81ba727e8..2f51b4382f52b 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -246,14 +246,10 @@ public function testUploadInvalidUploadKey(): void { } /** - * Checks that trying to upload an SVG favicon without imagemagick - * results in an unsupported media type response. + * Checks that an image rejected by the image manager + * results in an unprocessable entity response. */ - public function testUploadSVGFaviconWithoutImagemagick(): void { - $this->imageManager - ->method('shouldReplaceIcons') - ->willReturn(false); - + public function testUploadUnsupportedImage(): void { $this->request ->expects($this->once()) ->method('getParam') @@ -725,13 +721,16 @@ public function testGetLoginBackground(): void { public static function dataGetManifest(): array { return [ - [true], - [false], + [true, false, 'image/png', 'image/svg+xml'], + [false, false, 'image/png', 'image/svg+xml'], + [true, true, 'image/gif', 'image/gif'], ]; } #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataGetManifest')] - public function testGetManifest(bool $standalone): void { + public function testGetManifest(bool $standalone, bool $customFavicon, string $touchIconType, string $faviconType): void { + $this->imageManager->method('hasImage')->with('favicon')->willReturn($customFavicon); + $this->imageManager->method('getImageMime')->with('favicon')->willReturn('image/gif'); $this->appConfig ->expects($this->once()) ->method('getAppValueInt') @@ -764,12 +763,12 @@ public function testGetManifest(bool $standalone): void { => [ [ 'src' => 'touchicon?v=0', - 'type' => 'image/png', + 'type' => $touchIconType, 'sizes' => '512x512' ], [ 'src' => 'favicon?v=0', - 'type' => 'image/svg+xml', + 'type' => $faviconType, 'sizes' => '16x16' ] ], diff --git a/apps/theming/tests/IconBuilderTest.php b/apps/theming/tests/IconBuilderTest.php index 3ffaf7248e5a9..e1e5890f79427 100644 --- a/apps/theming/tests/IconBuilderTest.php +++ b/apps/theming/tests/IconBuilderTest.php @@ -14,6 +14,7 @@ use OCA\Theming\ThemingDefaults; use OCA\Theming\Util; use OCP\App\IAppManager; +use OCP\Files\SimpleFS\ISimpleFile; use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -191,72 +192,82 @@ public function testGetTouchIconPng(string $app, string $color, string $file): v $expectedIcon->destroy(); } - #[\PHPUnit\Framework\Attributes\DataProvider('dataRenderAppIconSvg')] - public function testGetFavIconSvg(string $app, string $color, string $file): void { - $this->checkImagick('SVG'); - // mock required methods - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->willReturnMap([ - ['ICO', true], - ['SVG', true], - ['PNG', true] - ]); + public static function dataGetFavicon(): array { + return [ + ['settings', 'settings.svg', 'image/svg+xml', '#0082c9', false, false], + ['settings', 'settings.png', 'image/png', '#0082c9', false, false], + ['settings', 'settings.svg', 'image/svg+xml', '#ffffff', true, true], + ['core', 'logo.svg', 'image/svg+xml', '#ffffff', true, false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataGetFavicon')] + public function testGetFavicon(string $app, string $file, string $mime, string $color, bool $isBright, bool $inverted): void { + $path = __DIR__ . '/data/' . $file; + $this->themingDefaults->method('getColorPrimary')->willReturn($color); + $this->util->method('isBrightColor')->with($color)->willReturn($isBright); $this->util->expects($this->once()) ->method('getAppIcon') - ->with($app, true) - ->willReturn(__DIR__ . '/data/' . $file); - $this->themingDefaults->expects($this->any()) - ->method('getColorPrimary') - ->willReturn($color); - // generate expected output from source file - $expectedIcon = $this->generateTestFavIcon($file, 'SVG', $color); - // run test - $result = $this->iconBuilder->getFavicon($app); - $this->assertIsString($result, 'Favicon generation should return a ICO blob'); - $this->assertEquals($expectedIcon->getImagesBlob(), $result, 'Generated favicon differs from expected'); - $expectedIcon->destroy(); + ->with($app) + ->willReturn($path); + + $svg = simplexml_load_string($this->iconBuilder->getFavicon($app)); + + $this->assertNotFalse($svg, 'Generated favicon is no valid XML'); + $this->assertEquals($color, (string)$svg->rect['fill']); + $this->assertEquals('data:' . $mime . ';base64,' . base64_encode(file_get_contents($path)), (string)$svg->image['href']); + $this->assertEquals($inverted, isset($svg->filter)); + $this->assertEquals($inverted ? 'url(#invert)' : '', (string)$svg->image['filter']); } - #[\PHPUnit\Framework\Attributes\DataProvider('dataRenderAppIconPng')] - public function testGetFaviconPng(string $app, string $color, string $file): void { - $this->checkImagick('PNG'); - // mock required methods - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->willReturnMap([ - ['ICO', true], - ['SVG', false], - ['PNG', true] - ]); + public function testGetFaviconCustomLogoNotInverted(): void { + $logo = $this->createMock(ISimpleFile::class); + $logo->method('getContent')->willReturn(file_get_contents(__DIR__ . '/data/logo.png')); + $logo->method('getMimeType')->willReturn('application/octet-stream'); + $this->themingDefaults->method('getColorPrimary')->willReturn('#ffffff'); + $this->util->method('isBrightColor')->willReturn(true); $this->util->expects($this->once()) ->method('getAppIcon') - ->with($app, false) - ->willReturn(__DIR__ . '/data/' . $file); - $this->themingDefaults->expects($this->any()) - ->method('getColorPrimary') - ->willReturn($color); - // generate expected output from source file - $expectedIcon = $this->generateTestFavIcon($file, 'PNG', $color); - // run test - $result = $this->iconBuilder->getFavicon($app); - $this->assertIsString($result, 'Favicon generation should return a ICO blob'); - $this->assertEquals($expectedIcon->getImagesBlob(), $result, 'Generated favicon differs from expected'); - $expectedIcon->destroy(); + ->with('settings') + ->willReturn($logo); + + $svg = simplexml_load_string($this->iconBuilder->getFavicon('settings')); + + $this->assertFalse(isset($svg->filter)); + $this->assertStringStartsWith('data:image/png;base64,', (string)$svg->image['href']); + } + + public static function dataGetFaviconCustomLogoMime(): array { + return [ + ['', 'image/svg+xml'], + ["\n", 'image/svg+xml'], + ['', 'image/svg+xml'], + ['no image', false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataGetFaviconCustomLogoMime')] + public function testGetFaviconCustomLogoMime(string $content, string|false $mime): void { + $logo = $this->createMock(ISimpleFile::class); + $logo->method('getContent')->willReturn($content); + $this->themingDefaults->method('getColorPrimary')->willReturn('#0082c9'); + $this->util->method('getAppIcon')->willReturn($logo); + + $result = $this->iconBuilder->getFavicon('settings'); + + if ($mime === false) { + $this->assertFalse($result); + } else { + $svg = simplexml_load_string($result); + $this->assertEquals('data:' . $mime . ';base64,' . base64_encode($content), (string)$svg->image['href']); + } } public function testGetFaviconNotFound(): void { - $this->checkImagick('ICO'); - $util = $this->createMock(Util::class); - $iconBuilder = new IconBuilder($this->themingDefaults, $util, $this->imageManager); - $this->imageManager->expects($this->any()) - ->method('canConvert') - ->willReturn(true); - $util->expects($this->once()) + $this->util->expects($this->once()) ->method('getAppIcon') ->willReturn('notexistingfile'); - $result = $iconBuilder->getFavicon('noapp'); - $this->assertFalse($result, 'Favicon generation should fail for missing file'); + $this->assertFalse($this->iconBuilder->getFavicon('noapp')); } public function testGetTouchIconNotFound(): void { @@ -338,23 +349,4 @@ private function generateTestIcon(string $file, string $format, int $size, strin $appIconFile->destroy(); return $finalIconFile; } - - /** - * Helper to generate expected favicon from source file for tests. - */ - private function generateTestFavIcon(string $file, string $format, string $color): \Imagick { - $baseIcon = $this->generateTestIcon($file, $format, 128, $color); - $baseIcon->setImageFormat('PNG32'); - - $testIcon = new \Imagick(); - $testIcon->setFormat('ICO'); - foreach ([16, 32, 64, 128] as $size) { - $clone = clone $baseIcon; - $clone->scaleImage($size, 0); - $testIcon->addImage($clone); - $clone->destroy(); - } - $baseIcon->destroy(); - return $testIcon; - } } diff --git a/apps/theming/tests/ImageManagerTest.php b/apps/theming/tests/ImageManagerTest.php index 69092e4ce4e9b..198de9b839f6e 100644 --- a/apps/theming/tests/ImageManagerTest.php +++ b/apps/theming/tests/ImageManagerTest.php @@ -357,6 +357,7 @@ public static function dataUpdateImage(): array { ['background', __DIR__ . '/../../../tests/data/testimage-large.jpg', true, true], ['background', __DIR__ . '/../../../tests/data/testimage-wide.png', true, true], ['logo', __DIR__ . '/../../../tests/data/testimagelarge.svg', true, false], + ['favicon', __DIR__ . '/../../../tests/data/testimagelarge.svg', true, false], ]; } diff --git a/apps/theming/tests/SetupChecks/PhpImagickModuleTest.php b/apps/theming/tests/SetupChecks/PhpImagickModuleTest.php new file mode 100644 index 0000000000000..fc0bbe51f2bd8 --- /dev/null +++ b/apps/theming/tests/SetupChecks/PhpImagickModuleTest.php @@ -0,0 +1,65 @@ +l10n = $this->createMock(IL10N::class); + $this->l10n->method('t')->willReturnArgument(0); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->imageManager = $this->createMock(ImageManager::class); + $this->check = new PhpImagickModule($this->l10n, $this->urlGenerator, $this->imageManager); + } + + public static function dataRun(): array { + return [ + 'imagick with svg logo' => [true, true, false, 'image/svg+xml', SetupResult::SUCCESS, false, false], + 'no imagick' => [false, false, false, '', SetupResult::INFO, true, false], + 'imagick without svg support' => [false, true, false, '', SetupResult::INFO, true, false], + 'imagick without png support' => [true, false, false, '', SetupResult::INFO, true, false], + 'no imagick with favicon' => [false, false, true, '', SetupResult::SUCCESS, false, false], + 'no imagick with favicon and png logo' => [false, false, true, 'image/png', SetupResult::SUCCESS, false, false], + 'no imagick with favicon and svg logo' => [false, false, true, 'image/svg+xml', SetupResult::INFO, false, true], + 'no imagick with svg logo' => [false, false, false, 'image/svg', SetupResult::INFO, true, true], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataRun')] + public function testRun(bool $canConvertSvg, bool $canConvertPng, bool $hasFavicon, string $logoMime, string $severity, bool $touchIconIssue, bool $logoIssue): void { + $this->imageManager->method('canConvert')->willReturnMap([ + ['SVG', $canConvertSvg], + ['PNG', $canConvertPng], + ]); + $this->imageManager->method('hasImage')->with('favicon')->willReturn($hasFavicon); + $this->imageManager->method('getImageMime')->with('logo')->willReturn($logoMime); + + $result = $this->check->run(); + + $this->assertEquals($severity, $result->getSeverity()); + $description = $result->getDescription() ?? ''; + $this->assertEquals($touchIconIssue, str_contains($description, 'Add to Dock')); + $this->assertEquals($logoIssue, str_contains($description, 'emails')); + } +} diff --git a/apps/theming/tests/ThemingDefaultsTest.php b/apps/theming/tests/ThemingDefaultsTest.php index 4646c0ced80a4..756343a75d3e1 100644 --- a/apps/theming/tests/ThemingDefaultsTest.php +++ b/apps/theming/tests/ThemingDefaultsTest.php @@ -803,6 +803,37 @@ public static function dataReplaceImagePath(): array { ]; } + public static function dataReplaceImagePathFacebookIcon(): array { + return [ + 'no custom favicon with imagick' => ['', true, true], + 'no custom favicon without imagick' => ['', false, false], + 'png favicon' => ['image/png', false, true], + 'jpeg favicon' => ['image/jpeg', false, true], + 'gif favicon' => ['image/gif', false, true], + 'svg favicon' => ['image/svg+xml', true, false], + 'ico favicon' => ['image/x-icon', true, false], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataReplaceImagePathFacebookIcon')] + public function testReplaceImagePathFacebookIcon(string $faviconMime, bool $canConvertPng, bool $replaced): void { + $this->imageManager->method('getImageMime') + ->with('favicon') + ->willReturn($faviconMime); + if ($faviconMime === '') { + $this->imageManager->method('getImage') + ->with('favicon') + ->willThrowException(new NotFoundException()); + } + $this->imageManager->method('canConvert')->with('PNG')->willReturn($canConvertPng); + $this->urlGenerator->method('linkToRoute') + ->with('theming.Icon.getTouchIcon', ['app' => 'core']) + ->willReturn('themingRoute'); + $this->util->method('getCacheBuster')->willReturn('1234abcd'); + + $this->assertEquals($replaced ? 'themingRoute?v=1234abcd' : false, $this->template->replaceImagePath('core', 'favicon-fb.png')); + } + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataReplaceImagePath')] public function testReplaceImagePath(string $app, string $image, string|bool $result = 'themingRoute?v=1234abcd'): void { $this->cache->expects($this->any()) diff --git a/openapi.json b/openapi.json index eea7f6a7906ad..eac73e11dd4bd 100644 --- a/openapi.json +++ b/openapi.json @@ -41398,7 +41398,7 @@ "/index.php/apps/theming/favicon/{app}": { "get": { "operationId": "theming-icon-get-favicon", - "summary": "Return a 32x32 favicon as png", + "summary": "Return a favicon as svg", "tags": [ "theming/icon" ], @@ -41427,13 +41427,7 @@ "200": { "description": "Favicon returned", "content": { - "image/png": { - "schema": { - "type": "string", - "format": "binary" - } - }, - "image/x-icon": { + "*/*": { "schema": { "type": "string", "format": "binary" @@ -41510,16 +41504,6 @@ } } }, - "404": { - "description": "Touch icon not found", - "content": { - "text/html": { - "schema": { - "type": "string" - } - } - } - }, "500": { "description": "", "content": {