Problem/Motivation

Currently, it's not possible to use SVGs with image styles, because the image style system only works with raster graphics. The core Image module is unaware of the distinction, and may attempt to process SVGs anyway, which can result in errors and other user-facing ugliness. This also makes it impossible to display vector and raster images side-by-side in a single image field with an image style applied, since it will not be able to display all the images in the field consistently.

Proposed Resolution

The system should not try to render images whose format is unsupported by the image style in use. This wouldn't affect images displayed without a style (i.e., the "original image" formatter for image fields).

Remaining Tasks

Review and commit the patch.

API Changes

None.

UI Changes

None.

Security Implications

None. This patch does not introduce any way to upload or store SVGs, or any other hazardous image format. It is only to display images that are 100% trusted and legitimate -- for instance, SVGs generated by the system itself.

CommentFileSizeAuthor
#59 interdiff-2652138-47-59.txt611 bytesphenaproxima
#59 2652138-59.patch8.6 KBphenaproxima
#47 interdiff-2652138-43-47.txt1.13 KBphenaproxima
#47 2652138-47.patch8.6 KBphenaproxima
#43 interdiff-2652138-41-43.txt1.55 KBphenaproxima
#43 2652138-43.patch8.14 KBphenaproxima
#41 2652138-41.patch7.8 KBphenaproxima
#40 interdiff-2652138-36-39.txt933 bytesphenaproxima
#39 2652138.patch8.13 KBGravypower
#35 interdiff-2652138-28-36.txt3.09 KBphenaproxima
#35 2652138-36.patch7.76 KBphenaproxima
#28 interdiff_20-28.txt9.4 KBmondrake
#28 2652138-28.patch5.93 KBmondrake
#20 interdiff-2652138-19-20.txt3.54 KBphenaproxima
#20 2652138-20.patch8.44 KBphenaproxima
#19 interdiff-2652138-15-19.txt3.65 KBphenaproxima
#19 2652138-19.patch4.76 KBphenaproxima
#15 2652138-15.patch3.37 KBphenaproxima
#13 2652138-13.patch3.21 KBr.nabiullin
#12 2652138-12.patch3.13 KBphenaproxima
#9 2652138-9.patch2.56 KBphenaproxima
image_formatter_svg_support.patch1.12 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
chx’s picture

Title: ImageFormatter should support displaying SVGs without image styles » ImageFormatter does not support SVGs
Version: 8.1.x-dev » 8.0.x-dev
Category: Feature request » Bug report
Status: Needs review » Needs work
Issue tags: -Needs security review +Needs tests
chx’s picture

Also, plz file a parallel D7 issue as well.

Wim Leers’s picture

+1 for principle

chx’s picture

Re hardcoding: yeah, ugly, but there are no other browser supported vector formats and given how long it took for SVG to finally work (with IE dead and buried) I can't imagine another vector format (what for?) rising and gaining widespread browser support. So: this is fine. We can always do more.

However, for the sake of extensibility please move it into a method so people can easily code around it.

phenaproxima’s picture

Issue tags: +svg
phenaproxima’s picture

FileSize
2.56 KB

Here's another approach, which moves the style selection logic into its own method as @chx requested in #6. No need for an interdiff, since the code is totally different.

mondrake’s picture

you cannot pipe an SVG image through the image style system, since the image style system works only on raster images

This is only true if we consider core's GD image toolkit. With ImageMagick, it's perfectly legit to pass an SVG in input and apply an image style to it, as ImageMagick internally converts the image to raster. The only consideration is that ImageMagick is not good at outputting vector images (see http://www.imagemagick.org/Usage/formats/#vector), so image styles should have explicit image effects to convert them to a raster format.

claudiu.cristea’s picture

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -189,16 +190,12 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +      $image_style = $this->getImageStyle($file);
    
    @@ -222,16 +219,36 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +        return $this->imageStyleStorage->load($image_style);
    

    If there are 4 .png files and one .svg, the we'll load the image style 4 times? No, you should get the image style outside foreach() or statically cache it. Also $cache_tags should be computed only once but should be empty if all no item will use an image style. Needs optimisation.

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -222,16 +219,36 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +   * @return \Drupal\image\ImageStyleInterface|NULL
    

    s/NULL/null

  3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -222,16 +219,36 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +    if ($file->getMimeType() != 'image/svg+xml') {
    

    !==

  4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
    @@ -222,16 +219,36 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +      $image_style = $this->getSetting('image_style');
    +      if ($image_style) {
    

    Maybe these might be merged?

    if ($image_style = $this->getSetting(...))
    

    Also I would change the name to $image_style_name or .._id because we usually use $image_style for the object.

Also there's need for tests.

phenaproxima’s picture

FileSize
3.13 KB

OK -- did some refactoring to optimize. Still no tests, but if this patch looks good I will write some :)

r.nabiullin’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
3.21 KB

rerolled patch from comment #12, become outdated
a bit later, i will add tests

claudiu.cristea’s picture

Category: Bug report » Feature request
phenaproxima’s picture

Rerolled and rewritten. I'd like to get this into 8.2, since it'll help get a few cool features into the Media Entity suite of modules.

No interdiff because it's substantially different from the patch in #13.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +D8Media, +Media Initiative
Related issues: +#2760797: MIME type guessing fails for remote stream wrapper URIs

Comment in #10 is not addressed. Hardcoding the MIME type check against image/svg+xml will forcefully prevent image styling also for toolkits that can process SVG images (I know about the security concerns related to letting users upload SVG files, but that's a separate discussion in #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)).

BTW, beware of #2760797: MIME type guessing fails for remote stream wrapper URIs.

Alternative suggestion: what about loading from the image.factory the image file at $file->getFileUri(), and check its validity via ::isValid(). The underlying toolkit will tell us if it can apply the style to the image or not. IMHO it will be more generic and avoid hardcoding.

mondrake’s picture

Or, if we want to avoid the file I/O connected with getting the image from the factory, we could check if the file extension falls into one of those returned by ImageFactory::getSupportedExtensions. But for GD toolkit we would need #2477381: GDToolkit::getSupportedExtensions returns incomplete list before.

mondrake’s picture

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
4.76 KB
3.65 KB

Added the ImageFactory-based check from #16. I'd rather have done something a little less I/O heavy, but I'd rather not postpone this issue on #2477381: GDToolkit::getSupportedExtensions returns incomplete list.

phenaproxima’s picture

Added tests!

mherchel’s picture

Status: Needs review » Reviewed & tested by the community

+1 Great work!

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

Actually, #16 defeats the concept of processing the image style formatting without opening an Image object that was implemented through ImageStyle::transformDimensions which is called in template_preprocess_image_style(). AFAICS the entire image style processing is split into HTML formatting, which occurs through #themes 'image_formatter' and 'image_style' within the request, and the actual delivery of the derivative image which occurs in separate requests. Sorry for not considering that before. #17 could do, though.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -61,10 +70,11 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
    * @param \Drupal\Core\Session\AccountInterface $current_user
    *   The current user.
    */
-  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, EntityStorageInterface $image_style_storage) {
+  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, AccountInterface $current_user, EntityStorageInterface $image_style_storage, ImageFactory $image_factory) {
     parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings);
     $this->currentUser = $current_user;
     $this->imageStyleStorage = $image_style_storage;
+    $this->imageFactory = $image_factory;
   }
 

Not sure we can change the constructor signature here, is it a BC break? Alternatively we can have a getter calling \Drupal::service('image.factory')

phenaproxima’s picture

Actually, #16 defeats the concept of processing the image style formatting without opening an Image object that was implemented through ImageStyle::transformDimensions which is called in template_preprocess_image_style(). AFAICS the entire image style processing is split into HTML formatting, which occurs through #themes 'image_formatter' and 'image_style' within the request, and the actual delivery of the derivative image which occurs in separate requests. Sorry for not considering that before. #17 could do, though.

I don't quite understand how this is a problem. This patch is about making ImageFormatter smarter, but nothing about it will prevent people from attempting to apply image styles to SVGs in the theme layer; trying to prevent that would be way out of scope for this patch.

Not sure we can change the constructor signature here, is it a BC break? Alternatively we can have a getter calling \Drupal::service('image.factory')

Oooh, good catch. I will add an internal getter for image.factory, but I'll keep the constructor parameter as well and simply default it to NULL (in case anyone is weird enough to instantiate the formatter directly :)

mondrake’s picture

I don't quite understand how this is a problem.

Please see #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O. It's all about getting the final dimensions of a derivative image without accessing the image file. With just the simple call to $this->imageFactory->get($file->getFileUri()) we do access the image file at the very beginning of the process, so defeating the purpose of that issue.

phenaproxima’s picture

Status: Needs work » Postponed

I'd rather avoid the extra I/O as well (as I said in #19), but if the only way to do that is to get #2477381: GDToolkit::getSupportedExtensions returns incomplete list in first and postpone this issue on that one, then that is what must be done.

phenaproxima’s picture

Status: Postponed » Needs work

#2477381: GDToolkit::getSupportedExtensions returns incomplete list has been committed to 8.2.x, so this is now unblocked. Full steam ahead!

Gábor Hojtsy’s picture

Issue tags: -Media Initiative

Remove duplicate tag.

mondrake’s picture

I have an alternative proposal.

The effect of the patch in #20 will be that if an image file is not supported by the toolkit, the original image will still be rendered, but on its full dimensions. The image style theme, however, can still determine (in some cases - if an image effect sets the dimensions and/or if the original dimensions are stored in the filed) the derivative image dimensions, via ImageStyle::transformDimensions, which is toolkit agnostic.

We can work on a lower level and just let template_preprocess_image_style determine if the image can be derived or not. If yes, all OK; if no, still render the original image as in patch #20, but with the plus of adding width/height HTML attributes as determined by the image style if that is possible. Browsers will then do the sizing at runtime.

With the patch here, the ImageFormatter code is in fact not touched at all. The interdiff is only interesting for the test code differences, I guess.

andypost’s picture

+++ b/core/modules/image/image.module
@@ -290,12 +290,22 @@ function template_preprocess_image_style(&$variables) {
+  // If the current image toolkit supports this file type, prepare the URI for
+  // the derivative image. If not, just use the original image resized to the
+  // dimensions specified by the style.
+  if (in_array(pathinfo($variables['uri'], PATHINFO_EXTENSION), \Drupal::service('image.factory')->getSupportedExtensions())) {
+    $uri = $style->buildUrl($variables['uri']);
+  }
+  else {
+    $uri = $variables['uri'];
+  }

Looks greaT!!!

andypost’s picture

Version: 8.1.x-dev » 8.3.x-dev

sad to miss a window

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#28: great insight, great write-up! Thanks a lot! :)

Just reviewed the patch. I have zero remarks! (That's very rare!) I only have nits, but they're so minor that I'll just omit them. The solution is elegant, the test coverage is comprehensive. What more could we want? :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

The solution on this patch gives me pause because there are many things an image style can do that are no related to width and size for example rotation and watermarks - this patch makes it look like it is working when it is not - which is a very confusing behaviour. Are we sure that this is the right approach?

phenaproxima’s picture

EDIT: Never mind, misunderstood the patch.

phenaproxima’s picture

Status: Needs review » Needs work

I'm in favor of committing the patch as-is, personally, but I don't have commit access :) So I discussed with @alexpott on IRC and we arrived at a compromise.

Let's add a new method to ImageStyleInterface called supports() or similar. This method will accept a URI (or a file entity, from which it can extract a URI) and return a boolean indicating if that URI is supported by the image style. The default implementation in ImageStyle will, if the URI ends in .svg, loop through its effects and return false if any of its effects are not instances of ResizeImageEffect, since that, as far as I know, is the only effect we currently have that can work with SVGs. If the URI is not an SVG, supports() will just return true.

This method will be called by template_preprocess_image_style(), and if it returns false, $variables['#image'] will just be unset (or otherwise cut off) so that the image will not be rendered at all. Anything, in my opinion, is better than displaying a 404 (or worse, 500) error and a "broken image" icon where the image should be. template_preprocess_image_style() will also write something to the error log so that the site builder will not be mystified as to what happened.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
7.76 KB
3.09 KB

First attempt to implement that approach. I changed a few things relative to my previous comment -- for one thing, I moved the ImageFactory::getSupportedExtensions() check into the new ImageStyle::supportsUri() method. And template_preprocess_image_style() will simply set $variables['images']['#access'] to FALSE if the style does not support the image, rather than unset $variables['image'], so as to give any further preprocess functions some wiggle room.

bkosborne’s picture

Slightly off topic here, but can't we not use an image field to store SVGs until this is resolved anyway? #1014816: Allow image fields to use any extensions the current image toolkit supports (instead of hard-coding jpg, png and gif only)

Looks like image field has a hard coded set of file extensions it can allow.

phenaproxima’s picture

Image fields can already store SVGs, but they cannot properly display them using the standard Image formatter. That's what this issue aims to resolve.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gravypower’s picture

Hello I believe there may be an issue with the patch,

Some of the images on my site have file extensions that are upper case, this causes the in_array check on line 351 of ImageStyle.php to fail. Can I suggest that the result from the pathinfo is converted to lowercase before the in_array call.

return in_array(
        Unicode::strtolower(pathinfo($uri, PATHINFO_EXTENSION)),
      // @todo Inject the image.factory service.
      \Drupal::service('image.factory')->getSupportedExtensions()
    );

patch attached.

phenaproxima’s picture

FileSize
933 bytes

@Gravypower, please post interdiffs when making patch changes; it helps everyone else review your good work :)

phenaproxima’s picture

Re-rolled for 8.4.x.

Wim Leers’s picture

Status: Needs review » Needs work

Wanted to RTBC, but…

  1. +++ b/core/modules/image/image.module
    @@ -292,10 +292,28 @@ function template_preprocess_image_style(&$variables) {
    +    ¶
    

    Trailing spaces.

  2. +++ b/core/modules/image/src/Entity/ImageStyle.php
    @@ -343,6 +344,19 @@ public function deleteImageEffect(ImageEffectInterface $effect) {
    +      // @todo Inject the image.factory service.
    

    Should be fixed before this can be RTBC.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
8.14 KB
1.55 KB

Fixed #42.1, but I left #42.2 as-is because ImageStyle is an entity and entities don't support dependency injection. In fact, the ImageStyle entity already calls \Drupal::service('image.factory') elsewhere -- see the createDerivative() method. (I've seen \Drupal in many other entity classes as well.) So I just removed the @todo comment instead.

Wim Leers’s picture

Status: Needs review » Needs work

#43.2: fine, but then let's instead add a protected function getImageFactory() method, which wraps the \Drupal::service() call, so you can at least still do unit testing.

phenaproxima’s picture

Sounds like a plan, but can you direct me to an existing example in a core entity class, so that I have something to pattern this after?

Wim Leers’s picture

For example: \Drupal\Core\Config\Entity\ConfigEntityBase::getTypedConfig() + \Drupal\image\Entity\ImageStyle::getImageEffectPluginManager().

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
1.13 KB

Fixed! Thanks, @Wim Leers!

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect :)

phenaproxima’s picture

Title: ImageFormatter does not support SVGs » Add SVG support to ImageFormatter
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs security review

Since SVGs are considered a potential security risk

It's not that they are a potential - it is that they are a security risk. If you give someone the ability to upload an SVG to your site then they can embed javascript on that site. I really think we need to be careful here. See https://stackoverflow.com/questions/5378559/including-javascript-in-svg

At the very least we should file a blocking issue that if someone adds svg to the allowed filetypes list they are made aware that this should not be granted to users you have no control over or trust of.

In #3 there was no security review.

alexpott’s picture

An alternative would be for Drupal to ship with a content security policy for SVG - something like what is discussed here https://github.com/github/markup/issues/289

phenaproxima’s picture

I had initially requested security review for this issue, but because it does not give anyone the ability to upload SVGs in any way, shape, or form, @chx removed the tag. The point of this issue is to allow the use of legitimate SVG that has been generated by the system. (For example, Lightning generates thumbnails for text-only tweets and saves them as SVG, since the format lends itself well to that use case.)

Not only that, but you already cannot grant SVG upload capability on image fields at the moment. If you add it as an allowed extension, the core image widget will reject it anyway. I filed an issue for that as well -- not to allow SVGs, but to inform the user that this is the case. #2867494: File and image widgets are misleading about the extensions they accept

alexpott’s picture

Status: Needs review » Needs work
Issue tags: -Needs security review +Needs issue summary update, +Needs change record

@phenaproxima good points - I agree with your assessment of the security implications of this patch. I'm removing the "needs security review" as a member of the security team. I'm removing it because the current patch does not enable SVG upload or directly support SVGs and thereby does not introduce new security concerns for core.

However I think that the issue title and summary need to be be updated to reflect how the final solution implemented here does not "Add SVG support to ImageFormatter". We also need a change record that explains the nature of the change and how to leverage it.

phenaproxima’s picture

Title: Add SVG support to ImageFormatter » Image styles do not play nicely with SVGs
Issue summary: View changes
Issue tags: -Needs issue summary update
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record
phenaproxima’s picture

+++ b/core/modules/image/image.module
@@ -292,10 +292,28 @@ function template_preprocess_image_style(&$variables) {
+    \Drupal::logger('image')->warning('Could not apply @style image style @uri because the style does not support it.', [

One thing I notice: It should say "Could not apply @style image style to @uri" (missing the 'to'). Could that be fixed on commit?

alexpott’s picture

@phenaproxima committers have a lot to do on commit - the more that is done before the better.

phenaproxima’s picture

Done. Leaving at RTBC because it was just a wording change.

alexpott’s picture

Updating issue credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 87c73d8 and pushed to 8.4.x. Thanks!

diff --git a/core/modules/image/src/ImageStyleInterface.php b/core/modules/image/src/ImageStyleInterface.php
index cf78b0d..7dec361 100644
--- a/core/modules/image/src/ImageStyleInterface.php
+++ b/core/modules/image/src/ImageStyleInterface.php
@@ -200,7 +200,7 @@ public function deleteImageEffect(ImageEffectInterface $effect);
    * @param string $uri
    *   The URI of the image.
    *
-   * @return boolean
+   * @return bool
    *   TRUE if the image is supported, FALSE otherwise.
    */
   public function supportsUri($uri);
diff --git a/core/modules/image/tests/src/Kernel/ImageFormatterTest.php b/core/modules/image/tests/src/Kernel/ImageFormatterTest.php
index c7c3d6f..e994f12 100644
--- a/core/modules/image/tests/src/Kernel/ImageFormatterTest.php
+++ b/core/modules/image/tests/src/Kernel/ImageFormatterTest.php
@@ -4,7 +4,6 @@
 
 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
-use Drupal\Core\Render\Element;
 use Drupal\entity_test\Entity\EntityTest;
 use Drupal\field\Entity\FieldConfig;
 use Drupal\field\Entity\FieldStorageConfig;

  • alexpott committed 87c73d8 on 8.4.x
    Issue #2652138 by phenaproxima, mondrake, nabiyllin, Gravypower,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.