Fix exception of Pillow for SVGs

Review Request #10851 — Created Jan. 24, 2020 and updated

misery
Djblets
release-1.0.x
4687
a93b0d8...
djblets
If someone uploads a SVG as file attachment
Review Boards tries to generate a thumbnail.

Pillow do not support SVGs and throws an exception.

https://github.com/python-pillow/Pillow/issues/3509

 - djblets.util.templatetags.djblets_images - Error thumbnailing image file uploaded/files/2020/01/24/d6a12e49-05b7-42d9-bd6c-50985fd98f8e__check.svg and saving as uploaded/files/2020/01/24/d6a12e49-05b7-42d9-bd6c-50985fd98f8e__check_600.svg: cannot identify image file <cStringIO.StringI object at 0x7ff6a9b57b58>
Traceback (most recent call last):
  File "/opt/reviewboard/dist/lib/python2.7/site-packages/djblets/util/templatetags/djblets_images.py", line 126, in thumbnail
    image = Image.open(data)
  File "/opt/reviewboard/dist/lib/python2.7/site-packages/PIL/Image.py", line 2705, in open
    % (filename if filename else fp))
IOError: cannot identify image file <cStringIO.StringI object at 0x7ff6a9b57b58>

As SVG do not need thumbnail because Browsers can
scale that without a problem we can provide
the original file here.

Added an SVG file and saw that the thumbnail exists
instead of an error 404.

Description From Last Updated

Can you add unit tests for this in djblets/util/tests/test_djblets_image_tags.py?

chipx86chipx86

Looks like the right solution for checking valid image file extension (based on Pillow's own logic) is to do: from ...

chipx86chipx86

E111 indentation is not a multiple of four

reviewbotreviewbot

This should probably be filename.lower().endswith('.svg')

daviddavid

This should be updated to say that SVGs are returned directly without generating a new image in storage.

chipx86chipx86

This is happening probably too late. We're doing a lot of computation of sizes and filenames, we're checking for a ...

chipx86chipx86

same problem with webp

miserymisery
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

misery
david
  1. 
      
  2. This should probably be filename.lower().endswith('.svg')

  3. 
      
misery
chipx86
  1. 
      
  2. Can you add unit tests for this in djblets/util/tests/test_djblets_image_tags.py?

    1. don't know how

    2. No problem, I'll take care of it.

  3. djblets/util/templatetags/djblets_images.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     

    This should be updated to say that SVGs are returned directly without generating a new image in storage.

  4. djblets/util/templatetags/djblets_images.py (Diff revision 3)
     
     
     

    This is happening probably too late. We're doing a lot of computation of sizes and filenames, we're checking for a thumbnail variant from storage (which will never exist for the svg), and then we're doing this. I think we'd instead want to perform this check first (along with handling any failures/non-existent images).

  5. 
      
misery
  1. 
      
  2. djblets/util/templatetags/djblets_images.py (Diff revision 3)
     
     
     

    same problem with webp

    1. Looks like newer versions do support WebP. Though I have not tested it, I just posted a code snippet that should allow for the correct extensions for the installed version of Pillow.

  3. 
      
chipx86
  1. 
      
  2. Looks like the right solution for checking valid image file extension (based on Pillow's own logic) is to do:

    from PIL.Image import get_registered_extensions
    
    
    ext = os.path.splitext(filename)[1].lower()
    
    if ext in get_registered_extensions():
       ...
    
  3. 
      
misery
Review request changed

Commit:

-67970d04668e7a0cc1c207d2167938db00d9ec5e
+a93b0d88eec1d07fd1de8a3aea123a4a2b3d2dbb

Diff:

Revision 4 (+10 -2)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...