Support SVG in Image Library

[yorkshire-pudding]

Description of the need

At present, if you have uploaded SVG files to use on site, these show as though the file can't be found:

As a content author I want to be able to preview SVG images when I go to add an image to a content item through the "Select from Image Library dialog" So that I can check I'm inserting the right image

As a content author I want to see light coloured SVG images against a dark background and vice versa So that I can easily see what the SVG will look like

Proposed solution

When viewing the Select from Image Library dialog, SVG files are rendered against a light or dark background depending on the dominant colour, or user could have an option to switch background colour.

Alternatives that have been considered

If you go to manage files, you can click on view and see them displayed as an entity.

Is there a Drupal or Backdrop contributed module that accomplishes this? Not aware of any. The SVG Image Support module adds support for fields but doesn't have any impact here.

Additional information

Add any other information or screenshots that would help.

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes support for previewing SVG files when you are selecting an image from the image library to add to content. The background is adjustable so you can preview easily even if the image is the same colour as the page background.

[indigoxela]

The main problem with SVG: it's not an image format, but SVG's are documents. Possibly they even contain scripts - which is a security problem.

The second problem: the GD library can't handle SVG. GD is the default image handling library in Backdrop (and other php based CMS).

[indigoxela]

Out of curiosity I played around a bit with svg. All necessary support seems to be in core already. The culprit here is the way, the image library view handles things (hardcodes to thumbnail style directory, but there's no "image"). That one would need a complete rewrite - which is probably not backwards compatible.

My experiments:

1. Create a custom file type SVG with "image/svg+xml"
2. Configure the display for that file entity type
3. Allow the upload for the svg extension
4. Add such a file
5. Create a view of files (entities)
6. Use the "preview" style of the rendered entity

What's still not working: the file display mode needs some extra file template to add the necessary attributes, so I didn't work that out. My conclusion: in theory it's possible with only core stuff. In practice ... it's probably still a contrib candidate.

[yorkshire-pudding]

For the first problem, I'm going to take a look at https://www.drupal.org/project/svg_sanitizer

Thanks for looking. I've been using SVGs for logos without too much difficulty. I spotted this while testing something else on that view and thought it's worth looking at.

I can't see a problem with this being contrib as its probably not something that the majority will try to do. Added label

[schoenid]

Even in the IMCE preview, a SVG preview is not displayed. Seems to be the same problem. There you have also the possibility to create thumbnails. If you try it with a SVG, you will get the message: <filename>.svg is not an image.

Anyway it can be selected and inserted properly and will then be displayed on the websites page.

By the way: Thumbnails for small SVGs makes no sense, because of it's size the file itself should be used as thumbnail !!!

[yorkshire-pudding]

Just adding a thought here that @schoenid inspired. If as part of #5966 we allowed upload of SVGs, perhaps the solution to getting these in the image browser is to create a small thumbnail in a compatible format when the file is uploaded?

[docwilmot]

Quick POC based on https://www.drupal.org/project/svg_image.

[yorkshire-pudding]

Thanks @docwilmot . I gave this a quick test and it works well 😃 - the SVG shows in the image library. On the file view page I do get these warnings:

Warning: Undefined array key "svg_settings" in image_field_formatter_view() (line 830 of /app/docroot/core/modules/image/image.field.inc).

Warning: foreach() argument must be of type array|object, null given in image_field_formatter_view() (line 832 of /app/docroot/core/modules/image/image.field.inc).

These don't affect functionality though, but probably need handling.

[docwilmot]

Warning: Undefined array key "svg_settings" in image_field_formatter_view() (line 830 of /app/docroot/core/modules/image/image.field.inc).

Ooh our sandboxes run PHP 8.2.8! Didnt realise this. Will need to update my local.

[docwilmot]

Quick note that this PR doesnt just add SVG to the Image browser, but adds support to Image fields as well. So you can add svg to the supported extensions in the content type field settings, then you can upload SVG through image fields and also insert existing through fields, and also insert SVG in CKEditor.

Tests all pass except PHP 8, which is unsurprising given the error above, but should be fixable.

[docwilmot]

We discussed this in the Thursday weekly meetings and the concern of security came up (details today's recording). Noting here quickly to followup on that and for any comments.

[klonos]

@docwilmot thank you for working on this 🙏🏼 ❤️

Re the previous comment, rehashing what @yorkshire-pudding said in an earlier comment about having a look at https://www.drupal.org/project/svg_sanitizer. The module seems to be implementing a separate formatter, but I'm thinking that if this is to get in core, then we should take the safest option and sanitize things by default in our existing image formatters (but make it possible for contrib to override that or add an option in the UI for it). ...having said that, watching https://www.youtube.com/watch?v=Nb9W9-Ufq0g makes me think that the default options may be doing a bit too much stripping out of certain tags and attributes, which may result in broken svgs - and having a UI to customize which tags/attributes should be allowed doesn't seem like a thing that should be in core 🤔

I also noticed that the svg is being rendered using an <img> HTML tag and referencing the .svg file "externally" instead of adding the svg file "inline" (as an actual <svg> HTML tag): <img src="https://pr4539-6dcl7rmc9im9zkg3pcuzussnfvayhalt.tugboatqa.com/files/field/image/android.svg" alt="">

...I am not in favor for doing it one way or the other, but if done that way, the ability to manipulate the svg via CSS (or via JavaScript, or be able to animate it) is apparently lost (see the "A Note About SVG Security" section in https://www.codeenigma.com/blog/allowing-svg-images-be-used-media-drupal-9, point 4 in the issue summary in https://www.drupal.org/project/drupal/issues/2433761 etc.). We should discuss that and decide which option is best to go with, or if it should be a formatter setting, which is what the contrib modules are doing (also worth noting that all of them have the "safest" option as the default):

• https://www.drupal.org/project/svg_formatter
• https://www.drupal.org/project/svg_image_field
• https://www.drupal.org/project/svg_image

[klonos]

FTR, I should mention that out of the 3 contrib implementations in Drupal-land, I'm in favor of svg_image (which is what the PR is also based on 👍🏼 ), which makes the required changes to allow the existing image field to support svg files.

I don't like the approach that svg_image_field is taking to introduce a separate, dedicated field type specifically for svg files by "locking" the extensions that can be used with it, as I find it too limiting and the addition of yet another field to be complicating things.

I don't like the approach of svg_formatter either, because it "locks" the svg support to the file field type instead of the image field. I understand that technically svg files aren't (raster) images, but the end result or the perception by non-technical people seems to be that these are images or at least "graphics" or media assets - something that resembles or behaves as an image.

[yorkshire-pudding]

Thank you for sharing thoughts.

I've mostly used SVGs as file reference in image tag but I agree that it should be a choice. I haven't explored all the potential of SVGs, but I think if inline then you can use CSS to change individual sub components of the image (e.g. colour) which opens lots of cool possibilities.

makes me think that the default options may be doing a bit too much stripping out of certain tags and attributes, which may result in broken SVGs - and having a UI to customize which tags/attributes should be allowed doesn't seem like a thing that should be in core

I agree. One use case I have thought about quite a bit is the ability of SVGs to have individual components have a link, so you could click on the desk you want to book and it would take you to a booking form and likewise you could colour the regions to indicate availability. There are lots of possibilities so we should avoid locking down too far. I can't think of any genuine use cases for scripts but there might be as long as someone knows what they are doing.

[docwilmot]

Updated with Sanitizer support and optional display as <img tag. Considering whether it makes sense to have a separate upload SVGs permission.

[klonos]

Updated with Sanitizer support and optional display as <img tag.

❤️ 🙏🏼

Considering whether it makes sense to have a separate upload SVGs permission.

I would normally consider this (adding a separate permission for a very specific file type/extension) a bit excessive, however in this case, and given the very real security considerations/risks, I think that security-conscious organizations would definitely appreciate the possibility to restrict this to specific user roles (think only the marketing department for instance). The added security/restrictions would help reassure that we are giving people the means to take all precautions possible.

[yorkshire-pudding]

I agree that a separate permission would make sense here. I'll try and do some further testing.

[docwilmot]

Added and implemented upload svg images permission. Users without that permission can still use and insert SVG images from the library.

I'll need to add tests next (since I cant think of anything else outstanding left to add), but will pause until its clear this is OK as is.

[docwilmot]

Reminder if anyone got time to test or review this please.

[indigoxela]

Reminder if anyone got time to test or review this please.

I planned to chime in earlier, but... :shrug:

@docwilmot great work so far! :+1: Works like a charm for image fields.

However, while testing, I ran into UX WTF's:

1. Selecting via editor dialog works, but the editor doesn't display the svg in content. After saving, it's there.
2. Uploading via editor dialog doesn't work, yet (no way to configure extensions for the editor upload)
3. Uploading via File form on /file/add doesn't work, yet (workaround: edit file.settings.json to add extension)

[indigoxela]

Re the SVG not displayed in the editor content: ouch, that's gonna be tricky.

It's the lack of dimensions. If I add dimensions manually (in source mode, width/height), the SVG gets displayed properly.

The problem is, that CKEditor doesn't automatically add dimensions for SVGs like it does for regular images, and the Backdrop dialog doesn't set any value on insert (other issue in the queue).

A-HA, some more findings: whether an SVG get's displayed in the editor depends a lot on the SVG itself. A simple SVG (no layers, dimensions set in px,...) works. So files might or might not be visible in the editor, depending on with which software they got created, it seems.

[indigoxela]

The editor again: drag-and-drop upload of SVG also doesn't work (these extensions are hardcoded in js/plugins/backdropimage/plugin.js in function clipboardIntegration).

Another finding: extraction of svg dimensions (attribute width and height) doesn't seem to work. All get the default dimensions.

[docwilmot]

Excellent, thanks for the review. Will get on to fixing those ASAP

[indigoxela]

I'm trying to think over the extra permission per extension (.svg in this case), and honestly, I'm not convinced it's a great idea.

It complicates things - in code and for admins.

UX first: If admins choose to add the extensions, they also have to think of (first of all, have to know about) also setting the permission accordingly. Admins usually are privileged, so they see the ability to upload svg after setting the extension in the field, but then editors complain, that they can't upload svg and the admin has a perfect puzzle. ("Huh? I see it, what's going on?") Having to configure one feature in two places, to get it working is something I'd like to avoid. And that's only for image fields. There are several ways to upload images

• image fields
• /file/add, where the extensions for the validator are controlled by file.settings.json (no UI for that, File module)
• editor dialog, extensions for the validator are controlled by image_get_supported_extensions(), so depend on the image toolkit (also no UI for extensions)
• editor drag-dop / paste upload, where the extension validation relies on file_validate_is_image/ image_get_supported_extensions (no UI, no way to influence that even by a hook)
• IMCE has its own extension handling (via admin UI)
• hero image upload (image_get_supported_extensions)
• user image upload (file_validate_is_image)
• logo upload (extensions hardcoded in system_site_information_settings_validate)
• contrib for sure has some more, I'm thinking of, for example, the Image Block module, which already supports SVG

All of these would be required to add permission validation for the 'upload svg images' perm. Do we really want that? Especially, as some are contrib, which means, we put the burden on them without providing a standardized API for the extension list to both, display available extensions (dynamically) in the form and validate accordingly.

Another consideration: we add a complete (and not even small) library to core to sanitize SVGs - and then obviously we don't trust it, to do a proper job? I mean: if it's good enough, why can't we simply rely on it and skip the additional permission per extension? If it's not good enough and its output isn't trustworthy, why do we include it, anyway?

And I really don't want to derail this issue. I think, handling SVG as images, consistently throughout core, consistently for contrib, seems like a nice thing to advertise with 1.27. :wink:

[indigoxela]

Small complement re contrib projects having to implement checks for upload svg images: keep in mind, that they'd also have to check for the core version, because they can't rely on Backdrop being the latest version. Yet another complication.

[indigoxela]

One concern re the current library location in 'core/modules/image/libraries' and the image_autoload_info() function:

That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required, so we should consider, that it might be off.

[docwilmot]

Moving from the PR comment:

@indigoxela said: I do have some questions re the current implementation:

Currently the library location is 'core/modules/image/libraries', and hook_autoload_info() is also in the image module. That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required. Should we consider that?

Besides that I wonder, if hook_autoload_info is actually necessary in this case. And I also wonder, why there's an abstraction layer for the sanitizer class. (Is someone supposed to come up with an alternative sanitizer class?) And then, still all (sub-)classes of this sanitizer would get loaded on every page call (because of hook_autoload_info). Even if it wouldn't ever be needed, for instance if no field allows svg's.

[indigoxela]

Looking at my recent comments ... hm, I did derail the issue a bit, I'm afraid. :disappointed:

What if we separated out some topics to their own issues (we already have a meta issue), and in a first step concentrate on the great enhancement to the image formatter?

What I imagine:

• Image formatter gets handled here (display SVG as image, fields only, fix image browser display glitch, ability to pick in image fields)
• Consider image upload handling separately (Image module), add sanitizer on upload (possibly in the Files module)
• Consider editor integration details (dialog / drag-and-drop upload)
• Consider core images not covered by image fields

@docwilmot what do you think, would that help? @yorkshire-pudding what's your opinion on splitting topics out? @klonos (and other people involved in this thread) would that be OK for you?

[yorkshire-pudding]

@indigoxela - derailing issues late in the day is normally a @klonos thing to do 😉

I suppose splitting it out reflects that the original issue was about supporting SVGs in the image browser so I'm fine with that.

I personally don't like having images added within text fields using the editor; I prefer my images to be in distinct image fields so the editor is a nice to have.

You make some valid points about UX which may be simpler to be split out but I'm not familiar with the code base to know if that will be simpler or not.

[indigoxela]

I prefer my images to be in distinct image fields so the editor is a nice to have.

Good point, the first step should already include the ability to pick svg from image browser in image fields (already possible, but (without patch) currently causes EntityStorageException). :+1: Uploading's currently (without patch) possible via /file/add, if one adds "svg" to file.settings.json (which is feasible via config manager).

(And, hey, I derailed issues before, I'd consider myself a pro re that. :laughing:)

[docwilmot]

Selecting via editor dialog works, but the editor doesn't display the svg in content. After saving, it's there.

Can you send me a copy of an SVG that doesnt show? All mine work fine.

I'm trying to think over the extra permission per extension (.svg in this case), and honestly, I'm not convinced it's a great idea.

You make some very good points. I'll leave the code in for now, but I'm in agreement that it probably isn't worth it.

That means, that the functionality to sanitize SVG is only available, if the image (field) module is enabled. That's for sure usually the case, but the module's not required, so we should consider, that it might be off.

I think I also might agree with this and had initially recommended putting the library in core/includes, but it was suggested to put it where it is now. But I'm not sure if there is a use case for an SVG sanitizer if image module is off though?

And then, still all (sub-)classes of this sanitizer would get loaded on every page call (because of hook_autoload_info).

My understanding of that hook is it registers the classes and they are only (lazy) loaded when an instance of the class is created. (It uses spl_autoload_register() in the background). Not an expert, though.

And I also wonder, why there's an abstraction layer for the sanitizer class.

There were comments somewhere as to all the things this sanitizer does might not be always desirable. I figured if we allow users to specify a sanitizer class, then they can extend this (or any other sanitizer really if the future brings better ones) and use any of the class methods (it has a reasonable list of options, setAllowedTags() etc). Otherwise there is no reasonable way to influence what this sanitizer does.

Thanks again for all the input.