From 1d2b23eed7be670a0fcae59975261a99a251594e 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 63a2d37383c95..69ef91bf5b088 100644 --- a/apps/theming/lib/Controller/IconController.php +++ b/apps/theming/lib/Controller/IconController.php @@ -100,8 +100,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); @@ -142,14 +142,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 ef26eb0cc32e1..5a2e12c04f18b 100644 --- a/apps/theming/tests/Controller/IconControllerTest.php +++ b/apps/theming/tests/Controller/IconControllerTest.php @@ -125,6 +125,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') @@ -180,6 +198,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')