getnikola / nikola

A static website and blog generator
https://getnikola.com/
MIT License
2.6k stars 444 forks source link

Fix exception from gallery RSS with WebP images #3674

Closed tartley closed 1 year ago

tartley commented 1 year ago

Pull Request Checklist

Description

WebP images in a gallery cause nikola build to fail with an unhandled AttributeError exception, as described in #3671. This happens because the MIME type of WebP images is not known, and the resulting None value causes the RSS generation code to choke.

This PR fixes that, by augmenting Python's list of known mimetypes with a hardcoded WebP one, before we ask for the mimetype of each image in a gallery.

I don't think the content of the generated gallery RSS feed is tested yet. In the issue comments, I talk about adding a new test for that. But instead, I noticed that converting one of the images in the samplesite demo gallery into a WebP causes the site build to fail during the test suite, due to the above problem, causing many test failures. Adding this fix to the galleries.py file fixes all those tests again.

Also, running a manual build on a site containing webp galleries now works.

This addresses the above issue's checklist item "Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called.", but does not complete the issue.

tartley commented 1 year ago

It belatedly crosses my mind that perhaps, if the samplesite demo gallery is used throughout the automated tests, then perhaps its gallery ought to contain not just a placeholder WebP, but one example of every supported image type (jpg, webp, png, etc). This would help the automated tests to surface any problems in a highly-visible way (e.g. Presumably this would have exposed the WebP issue a long time ago? And including things like TIFF images here would be desirable, so we can assert they are also correctly handled.)

However, the samplesite is not just test data. It is also prominently user-visible in many places. These uses are better suited to a demo that shows things working (e.g. TIFF images here would be a bad idea, many users would not be able to see them, depending on which browser they are using.)

I realize I'm a newcomer to this project (although have been using it happily for years), so I feel foolish making disruptive suggestions, but:

I would consider making the samplesite a separate thing from the test data. One can be optimized to look good, the other to be fragile. Initially, I would simply copy the samplesite into nikola/tests/data or somesuch, then modify the test data gallery to contain one of each image type.

I'm happy to submit a PR. Thoughts welcome! :-)

Kwpolska commented 1 year ago

However, the samplesite is not just test data. It is also prominently user-visible in many places. These uses are better suited to a demo that shows things working (e.g. TIFF images here would be a bad idea, many users would not be able to see them, depending on which browser they are using.)

I would consider making the samplesite a separate thing from the test data. One can be optimized to look good, the other to be fragile. Initially, I would simply copy the samplesite into nikola/tests/data or somesuch, then modify the test data gallery to contain one of each image type.

I think this would add more complexity and maintenance burden than it’s worth. We might end up making a change that works with the test site, but breaks the demo site. Converting a sample image to PNG/GIF, and perhaps adding some simple SVG would be beneficial for both the demo site and tests.

tartley commented 1 year ago

Be aware I force pushed this branch at this point, as mentioned in one of the above comments.

Kwpolska commented 1 year ago

Force pushes are fine. Can you try placing the mimetype registration in a more global place, as suggested in the review comment?

tartley commented 1 year ago

Converting a sample image to PNG/GIF, and perhaps adding some simple SVG would be beneficial for both the demo site and tests.

I added this as a new checklist item to #3672

tartley commented 1 year ago

As far as I know, this PR is ready, and completes checklist item "Inject a hardcoded 'image/web' mimetype at runtime, before .guess_type() gets called" on #3671. Thank you for your attention. Sorry it's been so wordy for such a small change.

tartley commented 1 year ago

Let me know if the baseline test failure is something I can help with (the error says "maintainers only", so I'm assuming you want to be hands-on yourselves. Thanks!