Skip to content

Adds more context to the svg compatibility warning #31742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pierreozoux wants to merge 1 commit into from
Closed

Adds more context to the svg compatibility warning #31742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/js/setupchecks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@
messages.push({
msg: t(
'core',
'Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it.'
'Module php-imagick in this instance has no SVG support. For better compatibility it is recommended to install it, but by enabling this support you are adding a potential security risk.'
Copy link
Member

@tcitworld tcitworld Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this part without extra context and this message still being a recommendation is really not useful. :/

Also, by default, Nextcloud uses Imagick with SVG support only to generate some themed icons, which is a far lower exposure surface than the rendering previews for SVG files (which is disabled by default for a good reason).

It seems than nextcloud/docker#1414 is quite confused by all this.

Copy link

@TLATER TLATER Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting! Scrolling by because I was trying to add a similar message, I hadn't realized that imagemagick was used for separate purposes. Let's see if we can finally solve nextcloud/docker#1414 by adding imagemagick to the image instead then.

Copy link

@mschoettle mschoettle Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. If it is used to generate themed icons, is this happening all the time or only in certain cases (i.e., not mandatory)?

Copy link
Member

@tcitworld tcitworld Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for instance to generate avatars containing the user's initials, and fallbacks to the samething with the GD library if it fails (GD may produce ugly things).

server/lib/private/Avatar/PlaceholderAvatar.php

Lines 134 to 136 in 12ed5c9

if (!$data = $this->generateAvatarFromSvg($size)) {
$data = $this->generateAvatar($this->getDisplayName(), $size);
}

Pretty much the same goes with themed favicons and app icons when the base logo is an svg.

server/apps/theming/lib/IconBuilder.php

Lines 120 to 223 in 12ed5c9

/**
* Render app icon on themed background color
* fallback to logo
*
* @param $app string app name
* @param $size int size of the icon in px
* @return Imagick|false
*/
public function renderAppIcon($app, $size) {
$appIcon = $this->util->getAppIcon($app);
if ($appIcon === false) {
return false;
}
if ($appIcon instanceof ISimpleFile) {
$appIconContent = $appIcon->getContent();
$mime = $appIcon->getMimeType();
} else {
$appIconContent = file_get_contents($appIcon);
$mime = mime_content_type($appIcon);
}
if ($appIconContent === false || $appIconContent === "") {
return false;
}
$color = $this->themingDefaults->getColorPrimary();
// generate background image with rounded corners
$background = '<?xml version="1.0" encoding="UTF-8"?>' .
'<svg xmlns="http://www.w3.org/2000/svg" version="1.1" xmlns:cc="http://creativecommons.org/ns#" width="512" height="512" xmlns:xlink="http://www.w3.org/1999/xlink">' .
'<rect x="0" y="0" rx="100" ry="100" width="512" height="512" style="fill:' . $color . ';" />' .
'</svg>';
// resize svg magic as this seems broken in Imagemagick
if ($mime === "image/svg+xml" || substr($appIconContent, 0, 4) === "<svg") {
if (substr($appIconContent, 0, 5) !== "<?xml") {
$svg = "<?xml version=\"1.0\"?>".$appIconContent;
} else {
$svg = $appIconContent;
}
$tmp = new Imagick();
$tmp->readImageBlob($svg);
$x = $tmp->getImageWidth();
$y = $tmp->getImageHeight();
$res = $tmp->getImageResolution();
$tmp->destroy();
if ($x > $y) {
$max = $x;
} else {
$max = $y;
}
// convert svg to resized image
$appIconFile = new Imagick();
$resX = (int)(512 * $res['x'] / $max * 2.53);
$resY = (int)(512 * $res['y'] / $max * 2.53);
$appIconFile->setResolution($resX, $resY);
$appIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$appIconFile->readImageBlob($svg);
/**
* invert app icons for bright primary colors
* the default nextcloud logo will not be inverted to black
*/
if ($this->util->invertTextColor($color)
&& !$appIcon instanceof ISimpleFile
&& $app !== "core"
) {
$appIconFile->negateImage(false);
}
$appIconFile->scaleImage(512, 512, true);
} else {
$appIconFile = new Imagick();
$appIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$appIconFile->readImageBlob($appIconContent);
$appIconFile->scaleImage(512, 512, true);
}
// offset for icon positioning
$border_w = (int)($appIconFile->getImageWidth() * 0.05);
$border_h = (int)($appIconFile->getImageHeight() * 0.05);
$innerWidth = ($appIconFile->getImageWidth() - $border_w * 2);
$innerHeight = ($appIconFile->getImageHeight() - $border_h * 2);
$appIconFile->adaptiveResizeImage($innerWidth, $innerHeight);
// center icon
$offset_w = (int)(512 / 2 - $innerWidth / 2);
$offset_h = (int)(512 / 2 - $innerHeight / 2);
$finalIconFile = new Imagick();
$finalIconFile->setBackgroundColor(new ImagickPixel('transparent'));
$finalIconFile->readImageBlob($background);
$finalIconFile->setImageVirtualPixelMethod(Imagick::VIRTUALPIXELMETHOD_TRANSPARENT);
$finalIconFile->setImageArtifact('compose:args', "1,0,-0.5,0.5");
$finalIconFile->compositeImage($appIconFile, Imagick::COMPOSITE_ATOP, $offset_w, $offset_h);
$finalIconFile->setImageFormat('png24');
if (defined("Imagick::INTERPOLATE_BICUBIC") === true) {
$filter = Imagick::INTERPOLATE_BICUBIC;
} else {
$filter = Imagick::FILTER_LANCZOS;
}
$finalIconFile->resizeImage($size, $size, $filter, 1, false);
$appIconFile->destroy();
return $finalIconFile;
}

So it's really as the message says, just better compatibility.

),
type: OC.SetupChecks.MESSAGE_TYPE_INFO
})
Expand Down