fix(theming): fix broken custom images introduced by #58224#60198
Open
miaulalala wants to merge 3 commits intomasterfrom
Open
fix(theming): fix broken custom images introduced by #58224#60198miaulalala wants to merge 3 commits intomasterfrom
miaulalala wants to merge 3 commits intomasterfrom
Conversation
miaulalala
requested review from
leftybournes,
nfebe,
provokateurin and
sorbaugh
and removed request for
a team
Three bugs were introduced by the backport of #55132 (PR #58224): 1. ImageManager::getImage() incorrectly tried to convert any uploaded raster image (PNG, JPEG) to SVG when imagick SVG support was available. Imagick can read SVGs but reliably fails writing raster→SVG, causing an ImagickException that silently fell through to bug 2. 2. ThemingController::getImage() changed Content-Type from the config- stored MIME to $file->getMimeType(). The original stored file has no extension (named "logo", not "logo.png"), so detectPath() returned application/octet-stream, causing browsers to not render the image. 3. ImageManager::delete() cleaned up logo and logo.png but not logo.svg, leaving stale SVG conversions that would be served after image updates. Fixes #60146 AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com>
Replace two deprecated IConfig::getAppValue() calls in ThemingController with IAppConfig::getAppValueString(): - getImage(): MIME type lookup for the original extensionless stored file - getManifest(): cachebuster value lookup Signed-off-by: Anna Larch <anna@nextcloud.com> AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
force-pushed
the
fix/noid/theming-broken-images-32-0-9
branch
from
d12c01c to
036d52a
Compare
The deprecated IConfig::getAppValue() calls in ThemingController have been replaced with IAppConfig::getAppValueString(), so the baseline suppression entry is no longer needed. Signed-off-by: Anna Larch <anna@nextcloud.com> AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #60146 — custom theming images (logo, background, favicon, header logo) broken since 32.0.9.
Three bugs were introduced by the backport of #55132 (PR #58224):
ImageManager::getImage()tried to convert any uploaded image (PNG, JPEG) to SVG when imagick had SVG support. Imagick reliably fails writing raster→SVG, throwing anImagickExceptionthat silently fell through to the next bug.ThemingController::getImage()switched from the config-stored MIME type to$file->getMimeType(). The original stored file has no extension (namedlogo, notlogo.png), sodetectPath()returnedapplication/octet-stream, causing browsers to not render it as an image.ImageManager::delete()removedlogoandlogo.pngbut notlogo.svg, so any cached SVG conversion persisted after a new image was uploaded.The workaround of removing
libmagickcore-6.q16-6-extra(disabling imagick SVG support) worked because it forced$useSvg=false, bypassing the broken SVG path and ending up in the PNG conversion path — which uses a properly-namedlogo.pngfile with a correct MIME type.Fix
getImage()now only converts SVG originals to PNG when$useSvg=false(the original intended behaviour).ThemingController::getImage()uses$file->getMimeType()for converted files (which have a.png/.svgextension), and falls back to the config-stored MIME type for the original extensionless file.ImageManager::delete()now also deletes the.svgcached variant.Test plan
/settings/admin/theming— logo should appear correctlyNOCOVERAGE=1 ./autotest.sh sqlite apps/theming/tests/ImageManagerTest.phpandNOCOVERAGE=1 ./autotest.sh sqlite apps/theming/tests/Controller/ThemingControllerTest.php🤖 Generated with Claude Code