http://en.wikipedia.org/wiki/Data_URI_scheme
Drupal doesn't consider data URIs to be valid as they take the form 'data:' instead of 'data://'.
file_uri_scheme() can't be changed to have ':' as the delimiter instead of '://' as that function also accepts relative paths and automagically determines that it is not a URI. Paths can have ':' in them, so this patch tests explicitly for URIs starting with 'data:'.
It is bad practise for functions to be determining what has been passed to them. Perhaps this can be fixed properly in a wider refactoring.
This patch also includes full tests for data URIs.
Comment | File | Size | Author |
---|---|---|---|
#55 | 1342504-55-data-uri-support-D7.patch | 6.93 KB | pobster |
#41 | 1342504-40-data-uri-support-D7.patch | 6.68 KB | smussbach |
#39 | 1342504-39-data-uri-support-D7.patch | 6.68 KB | smussbach |
#35 | interdiff.txt | 1.31 KB | Jelle_S |
#35 | 1342504-35-data-uri-support.patch | 7.34 KB | Jelle_S |
Comments
Comment #2
jbrown CreditAttribution: jbrown commenteddata-uris.patch queued for re-testing.
Comment #4
jbrown CreditAttribution: jbrown commenteddata-uris.patch queued for re-testing.
Comment #5
jbrown CreditAttribution: jbrown commenteddata-uris.patch queued for re-testing.
Comment #7
jbrown CreditAttribution: jbrown commentedComment #8
jbrown CreditAttribution: jbrown commentedD7 patch.
Comment #9
Eric_A CreditAttribution: Eric_A commentedI suggest "Test data URI in image tag." ;-)
Using the testing profile would be nice. Testing theme_image() does not depend on block, node, or rdf, to name a few.
Comment #10
jbrown CreditAttribution: jbrown commentedThanks!
Both changes made.
Comment #11
jbrown CreditAttribution: jbrown commentedAdded 'data' scheme to $allowed_protocols in drupal_strip_dangerous_protocols().
Comment #12
jbrown CreditAttribution: jbrown commented#11: data-uris.patch queued for re-testing.
Comment #13
Dew CreditAttribution: Dew commenteddata-uris.patch queued for re-testing.
Comment #14
jbrown CreditAttribution: jbrown commentedIs it secure to add 'data' scheme to $allowed_protocols ?
Comment #15
Mr. NoNaMe CreditAttribution: Mr. NoNaMe commented#11: data-uris.patch queued for re-testing.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commented#7: data-uris.patch queued for re-testing.
Comment #18
bennos CreditAttribution: bennos commented#7: data-uris.patch queued for re-testing.
Comment #19
Dave ReidMarking #1851472: Add support for data:// encoded URIs in file_create_url (native theme_image support) as a duplicate of this issue.
I think for now we should likely not add data: as an allowed protocol in drupal_strip_dangerous_protocols() unless we have good reason to. I believe supporting data from file.inc is good for now.
Comment #20
Dave ReidGoing to pick this up to re-roll for latest 8.x. Fixing metadata.
Comment #21
Dave ReidUpdated and re-rolled for current 8.x. Let's see what fails.
Comment #23
Dave ReidTry again.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedCan you add type hints to the docblocks that are being changed in this issue? Thanks.
Comment #26
webavant CreditAttribution: webavant commenteddata-uris.patch queued for re-testing.
Comment #26.0
webavant CreditAttribution: webavant commentedimprove grammar
Comment #29
Jelle_SComment #30
Jelle_SThe Responsive Image module already uses a data uri (for mapping to an empty image), but since it uses image styles it doesn't need file_create_url directly:
BUT, since we now use data URI's in Core, it should really be supported...
Comment #32
Jelle_SComment #33
attiks CreditAttribution: attiks commentedComment #34
Wim LeersSome small things, sorry.
s/uri/URI/
"path to a shipped file" is the special case here: that looks like
core/misc/druplicon.png
Everything else, including data URIs are encompassed in the first case.
Therefore, this change is unnecessary, and confusing.
s/wrappers/schemes/?
Comment #35
Jelle_SNew patch. Addresses remarks from #34.
Comment #36
Wim LeersThanks!
data:
is just another scheme, the flaw is actually that Drupal assumed://
before. I think this is sufficiently clear though.Comment #37
alexpottCommitted 5e938c1 and pushed to 8.0.x. Thanks!
Comment #39
smussbach CreditAttribution: smussbach commentedBackport to D7
Some remarks:
- The theming function for image expects 'path' not 'uri', right?
- I had to add 'typeof="foaf:Image"' to the expected result in testImage() and strip the newline at the end, is that correct?
Comment #41
smussbach CreditAttribution: smussbach commentedHad some problems with the patchformat. Next try, probably not the last one
Comment #42
smussbach CreditAttribution: smussbach commentedSorry for spamming, newbie in patching and drupal issue queue. Didn't notice that bot has changed status.
Comment #44
zwerg CreditAttribution: zwerg commentedIs there something new? After applying the patch (#41) and patching image.module (as described here) the error is still there:
Maybe it is possible to upload the patched/working files?
Comment #45
Perignon CreditAttribution: Perignon as a volunteer commented#41 applies cleanly to Drupal 7. Can we get this committed to D7? It has been applied to D8 already. This is an ongoing issue for Storage API module which could greatly benefit from data URIs in core.
Comment #46
Perignon CreditAttribution: Perignon as a volunteer commented@zwerg, What error is still there?
Comment #47
zwerg CreditAttribution: zwerg commented@Perignon data URIs are still not supported and Storage API continues showing this message.
Comment #48
Perignon CreditAttribution: Perignon as a volunteer commented@zwerg, That would be an issue to bring up within the Storage API issue queue. It doesn't affect the patch here. Even though the patch was started by JBrown.
Comment #53
jollysolutionspatch #41 works for me
Comment #54
jollysolutionsHow do we get this committed, please?
Comment #55
pobster CreditAttribution: pobster at ArcadeGeek LTD commentedRerolled for 7.84.
Hmmm ... wait for the re-test, that exception is nothing to do with the changes.. Yeah, it's fine now - some internal failure with testbot, now all good.