Skip to content

Commit 5e42eb1

Browse files
committed
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. <szaimen@e.mail.de>
1 parent a0a8ee5 commit 5e42eb1

2 files changed

Lines changed: 41 additions & 4 deletions

File tree

apps/theming/lib/Controller/IconController.php

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ public function getFavicon(string $app = 'core'): Response {
100100
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
101101
} catch (NotFoundException $e) {
102102
}
103-
// retrieve or generate app specific favicon
104-
if (($this->imageManager->canConvert('PNG') || $this->imageManager->canConvert('SVG')) && $this->imageManager->canConvert('ICO')) {
103+
// retrieve or generate app specific favicon, but only if no custom favicon was uploaded
104+
if ($iconFile === null && ($this->imageManager->canConvert('PNG') || $this->imageManager->canConvert('SVG')) && $this->imageManager->canConvert('ICO')) {
105105
$color = $this->themingDefaults->getColorPrimary();
106106
try {
107107
$iconFile = $this->imageManager->getCachedImage('favIcon-' . $app . $color);
@@ -142,14 +142,15 @@ public function getTouchIcon(string $app = 'core'): Response {
142142
}
143143

144144
$response = null;
145+
$iconFile = null;
145146
// retrieve instance favicon
146147
try {
147148
$iconFile = $this->imageManager->getImage('favicon');
148149
$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => $iconFile->getMimeType()]);
149150
} catch (NotFoundException $e) {
150151
}
151-
// retrieve or generate app specific touch icon
152-
if ($this->imageManager->canConvert('PNG')) {
152+
// retrieve or generate app specific touch icon, but only if no custom favicon was uploaded
153+
if ($iconFile === null && $this->imageManager->canConvert('PNG')) {
153154
$color = $this->themingDefaults->getColorPrimary();
154155
try {
155156
$iconFile = $this->imageManager->getCachedImage('touchIcon-' . $app . $color);

apps/theming/tests/Controller/IconControllerTest.php

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,24 @@ public function testGetFaviconThemed(): void {
125125
$this->assertEquals($expected, $this->iconController->getFavicon());
126126
}
127127

128+
public function testGetFaviconUploaded(): void {
129+
// a custom favicon was uploaded, so it must be served as-is and the
130+
// app-specific generation path must not overwrite it
131+
$file = $this->iconFileMock('favicon.ico', 'filecontent');
132+
$this->imageManager->expects($this->once())
133+
->method('getImage')
134+
->with('favicon', false)
135+
->willReturn($file);
136+
$this->imageManager->expects($this->never())
137+
->method('getCachedImage');
138+
$this->iconBuilder->expects($this->never())
139+
->method('getFavicon');
140+
141+
$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
142+
$expected->cacheFor(86400);
143+
$this->assertEquals($expected, $this->iconController->getFavicon());
144+
}
145+
128146
public function testGetFaviconDefault(): void {
129147
$this->imageManager->expects($this->once())
130148
->method('getImage')
@@ -180,6 +198,24 @@ public function testGetTouchIconDefault(): void {
180198
$this->assertEquals($expected, $this->iconController->getTouchIcon());
181199
}
182200

201+
public function testGetTouchIconUploaded(): void {
202+
// a custom favicon was uploaded, so it must be served as-is and the
203+
// app-specific generation path must not overwrite it
204+
$file = $this->iconFileMock('favicon.png', 'filecontent');
205+
$this->imageManager->expects($this->once())
206+
->method('getImage')
207+
->with('favicon')
208+
->willReturn($file);
209+
$this->imageManager->expects($this->never())
210+
->method('getCachedImage');
211+
$this->iconBuilder->expects($this->never())
212+
->method('getTouchIcon');
213+
214+
$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image type']);
215+
$expected->cacheFor(86400);
216+
$this->assertEquals($expected, $this->iconController->getTouchIcon());
217+
}
218+
183219
public function testGetTouchIconFail(): void {
184220
$this->imageManager->expects($this->once())
185221
->method('getImage')

0 commit comments

Comments
 (0)