Skip to content
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.

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

Adds more context to the svg compatibility warning #31742

Closed
wants to merge 1 commit into from

Conversation

pierreozoux
Copy link
Member

Refers to this lengthy discussion:
nextcloud/docker#1414

@@ -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

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

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.

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

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).

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.

/**
* 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants