New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression: restore extraction of data-URI images from source for builders whose output formats do not support them natively. #12344
base: master
Are you sure you want to change the base?
Conversation
…n the builder's file format does not support remotely-hosted image references.
…copy_images test cases.
…` and ``supported_data_uri_images`` builder attributes.
@@ -72,6 +72,7 @@ def test_copy_images(app, status, warning): | |||
images_dir = Path(app.outdir) / '_images' | |||
images = {image.name for image in images_dir.rglob('*')} | |||
assert images == { | |||
# 'ba30773957c3fe046897111afd65a80b81cad089.png', # html: image from data:image/png URI in source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep; it's intended as a hint that the same image exists as in the other test_copy_images
unit tests, but that it isn't written to file because HTML (and similarly ePub) can natively include img src="data:..."
elements.
Probably not required, but if some future change affects the logic, it's possible that all of the relevant test cases would need to be updated, and in that case the cross-reference provided by the comment might be useful.
I'll remove the shoutiness from the pull request description; that was written at a moment where I'd emerged from confusion about what the |
Feature or Bugfix
Purpose
data:
URIs even when the builder has thesupported_remote_images
flag set toFalse
(the default).Ambiguity
I think the change could seem counter-intuitive. If I saw this changeset I might ask: if remote images are disabled, then why should we parse and extract an image from a URI on an image node?
The way I've resolved that question while developing it is:
These data-URI images can be considered 'local' to the source -- they're found in the input
reStructuredText
content, so we don't need to check the builder'ssupported_remote_images
attribute at all during theDataURIExtractor.match
method; these are not remote images.Detail
Expected behaviour description
DataURIExtractor
:data:
URIs natively, leave the URI reference on the image unmodified, and allow the builder to process it normally.data:
, and the contents are declared as having a supported MIME type, then decode the contents and write them to a hash-based filename with the corresponding filetype extension suffixed.Relates
data:
URIs #12331.