Summary

Fix exception of Pillow for SVGs

Review Request #10851 — Created Jan. 24, 2020 and submitted Aug. 12, 2020, 3 p.m.

Information

Owner

misery

Repository

Djblets

Branch

release-1.0.x

Bugs

4687

Depends On

Commit

a93b0d8...

Reviewers

Groups

djblets

People

Description

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.

Testing Done

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

Issues

Description	From	Last Updated
Looks like the right solution for checking valid image file extension (based on Pillow's own logic) is to do: from …	chipx86	July 27, 2020, 8:20 a.m.
Can you add unit tests for this in djblets/util/tests/test_djblets_image_tags.py?	chipx86	July 27, 2020, 8:29 a.m.
E111 indentation is not a multiple of four	reviewbot	Jan. 24, 2020, 7:30 a.m.
This should probably be filename.lower().endswith('.svg')	david	Jan. 25, 2020, 2:15 a.m.
This should be updated to say that SVGs are returned directly without generating a new image in storage.	chipx86	July 27, 2020, 8:20 a.m.
This is happening probably too late. We're doing a lot of computation of sizes and filenames, we're checking for a …	chipx86	July 27, 2020, 8:20 a.m.
same problem with webp	misery	July 27, 2020, 8:20 a.m.

flake8 failed.

JSHint passed.

flake8

djblets/util/templatetags/djblets_images.py (Diff revision 1)
Show all issues
```
E111 indentation is not a multiple of four
```

Commit:

-	9303bee6a8e36dd33a9e3c374e4a57853a818f5d
+	a7fe460e2288ca87c02ec3f5ba1cc9af21b63f05

Diff:

Revision 2 (+3)

Show changes

djblets/util/templatetags/djblets_images.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/util/templatetags/djblets_images.py (Diff revision 2)
Show all issues
```
This should probably be filename.lower().endswith('.svg')
```

Commit:

-	a7fe460e2288ca87c02ec3f5ba1cc9af21b63f05
+	67970d04668e7a0cc1c207d2167938db00d9ec5e

Diff:

Revision 3 (+3)

Show changes

djblets/util/templatetags/djblets_images.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

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

misery July 27, 2020, 8:29 a.m.
```
don't know how
```
chipx86 July 27, 2020, 1:40 p.m.
```
No problem, I'll take care of it.
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

same problem with webp

chipx86 May 7, 2020, 12:11 a.m.

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.

Show all issues

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

Commit:

-	67970d04668e7a0cc1c207d2167938db00d9ec5e
+	a93b0d88eec1d07fd1de8a3aea123a4a2b3d2dbb

Diff:

Revision 4 (+10 -2)

Show changes

djblets/util/templatetags/djblets_images.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (d4da991)