From 7b0f83f2f04cf2f49a1e149acaf6294561bb635a Mon Sep 17 00:00:00 2001 From: "Simon L." Date: Fri, 12 Jun 2026 17:05:35 +0200 Subject: [PATCH] fix(theming): preserve uploaded favicon and touch icon PR #58224 dropped the `$iconFile === null` guard around the app-specific icon generation in getFavicon()/getTouchIcon(), so an uploaded custom favicon was always overwritten by the generated, context-colored icon whenever Imagick could produce an ICO/PNG. Restore the guard so the generation path only runs as a fallback when no custom favicon was uploaded, while keeping the improved Imagick capability detection from #58224. Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Simon L. --- .../theming/lib/Controller/IconController.php | 9 ++--- .../tests/Controller/IconControllerTest.php | 36 +++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php index ed4026944bd28..d4f1306e7eb8a 100644 --- a/apps/theming/lib/Controller/IconController.php +++ b/apps/theming/lib/Controller/IconController.php @@ -103,8 +103,8 @@ public function getFavicon(string $app = 'core'): Response { $response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']); } catch (NotFoundException $e) { } - // retrieve or generate app specific favicon - if (($this->imageManager->canConvert('PNG') || $this->imageManager->canConvert('SVG')) && $this->imageManager->canConvert('ICO')) { + // 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); @@ -145,14 +145,15 @@ public function getTouchIcon(string $app = 'core'): Response { } $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) { } - // retrieve or generate app specific touch icon - if ($this->imageManager->canConvert('PNG')) { + // 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(); try { $iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app . $color); diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php index e90aee07580e9..5d525fd9b859a 100644 --- a/apps/theming/tests/Controller/IconControllerTest.php +++ b/apps/theming/tests/Controller/IconControllerTest.php @@ -123,6 +123,24 @@ public function testGetFaviconThemed(): void { $this->assertEquals($expected, $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'); + $this->imageManager->expects($this->once()) + ->method('getImage') + ->with('favicon', false) + ->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()); + } + public function testGetFaviconDefault(): void { $this->imageManager->expects($this->once()) ->method('getImage') @@ -178,6 +196,24 @@ public function testGetTouchIconDefault(): void { $this->assertEquals($expected, $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'); + $this->imageManager->expects($this->once()) + ->method('getImage') + ->with('favicon') + ->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()); + } + public function testGetTouchIconFail(): void { $this->imageManager->expects($this->once()) ->method('getImage')