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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, data-uris.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review

data-uris.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, data-uris.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review

data-uris.patch queued for re-testing.

jbrown’s picture

data-uris.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, data-uris.patch, failed testing.

jbrown’s picture

Status: Needs work » Needs review
FileSize
8.61 KB
jbrown’s picture

FileSize
8.54 KB

D7 patch.

Eric_A’s picture

+  /**
+   * Test nested list rendering.
+   */
+  function testDataUri() {
+    $expected = '<img typeof="foaf:Image" 

I suggest "Test data URI in image tag." ;-)

 /**
+ * Unit tests for theme_image().
+ */
+class ThemeImageUnitTest extends DrupalWebTestCase {
+  public static function getInfo() {

Using the testing profile would be nice. Testing theme_image() does not depend on block, node, or rdf, to name a few.

jbrown’s picture

FileSize
8.63 KB

Thanks!

Both changes made.

jbrown’s picture

Title: theme_image() doesn't accept data URIs » Support data URIs
Component: image system » base system
FileSize
9.32 KB
9.42 KB

Added 'data' scheme to $allowed_protocols in drupal_strip_dangerous_protocols().

jbrown’s picture

#11: data-uris.patch queued for re-testing.

Dew’s picture

data-uris.patch queued for re-testing.

jbrown’s picture

Issue tags: +Needs security review

Is it secure to add 'data' scheme to $allowed_protocols ?

Mr. NoNaMe’s picture

Issue tags: -Needs security review

#11: data-uris.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, data-uris-d7.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

#7: data-uris.patch queued for re-testing.

bennos’s picture

Issue tags: +Needs security review

#7: data-uris.patch queued for re-testing.

Dave Reid’s picture

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

Marking #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.

Dave Reid’s picture

Component: base system » file system
Assigned: Unassigned » Dave Reid
Category: bug » feature

Going to pick this up to re-roll for latest 8.x. Fixing metadata.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
6.44 KB

Updated and re-rolled for current 8.x. Let's see what fails.

Status: Needs review » Needs work

The last submitted patch, 1342504-data-uri-support.patch, failed testing.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
6.52 KB

Try again.

Lars Toomre’s picture

Can you add type hints to the docblocks that are being changed in this issue? Thanks.

The last submitted patch, 1342504-data-uri-support.patch, failed testing.

webavant’s picture

Status: Needs work » Needs review

data-uris.patch queued for re-testing.

webavant’s picture

Issue summary: View changes

improve grammar

Status: Needs review » Needs work

The last submitted patch, 23: 1342504-data-uri-support.patch, failed testing.

Jelle_S’s picture

Issue summary: View changes
Issue tags: +Needs reroll
Jelle_S’s picture

Assigned: Dave Reid » Unassigned
Category: Feature request » Task
Status: Needs work » Needs review
FileSize
7.44 KB

The 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:

  if ($style_name == RESPONSIVE_IMAGE_EMPTY_IMAGE) {
    // The smallest data URI for a 1px square transparent GIF image.
    return 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
  }
  return entity_load('image_style', $style_name)->buildUrl($path);

BUT, since we now use data URI's in Core, it should really be supported...

Status: Needs review » Needs work

The last submitted patch, 30: 1342504-30-data-uri-support.patch, failed testing.

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
7.45 KB
attiks’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs security review, -Needs reroll
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Some small things, sorry.

  1. +++ b/core/includes/file.inc
    @@ -323,10 +327,12 @@ function file_stream_wrapper_valid_scheme($scheme) {
    +  // If nothing was replaced, the uri doesn't have a valid scheme.
    

    s/uri/URI/

  2. +++ b/core/includes/file.inc
    @@ -439,11 +445,11 @@ function file_stream_wrapper_get_instance_by_scheme($scheme) {
    + *   The URI to a file for which we need an external URL, the path to a
    + *   shipped file or a data 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.

  3. +++ b/core/includes/file.inc
    @@ -476,9 +482,9 @@ function file_create_url($uri) {
    +    // implement getExternalUrl() for the HTTP and data wrappers.
    

    s/wrappers/schemes/?

Jelle_S’s picture

Status: Needs work » Needs review
FileSize
7.34 KB
1.31 KB

New patch. Addresses remarks from #34.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

+++ b/core/includes/file.inc
@@ -312,10 +316,10 @@ function file_stream_wrapper_valid_scheme($scheme) {
+ *   A stream, referenced as "scheme://target" or "data:target".

data: is just another scheme, the flaw is actually that Drupal assumed :// before. I think this is sufficiently clear though.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 5e938c1 and pushed to 8.0.x. Thanks!

  • alexpott committed 5e938c1 on 8.0.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    
smussbach’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.68 KB

Backport 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?

Status: Needs review » Needs work

The last submitted patch, 39: 1342504-39-data-uri-support-D7.patch, failed testing.

smussbach’s picture

Had some problems with the patchformat. Next try, probably not the last one

smussbach’s picture

Status: Needs work » Needs review

Sorry for spamming, newbie in patching and drupal issue queue. Didn't notice that bot has changed status.

  • alexpott committed 5e938c1 on 8.1.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    
zwerg’s picture

Is 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?

Perignon’s picture

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

Perignon’s picture

@zwerg, What error is still there?

zwerg’s picture

@Perignon data URIs are still not supported and Storage API continues showing this message.

Perignon’s picture

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

  • alexpott committed 5e938c1 on 8.3.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    

  • alexpott committed 5e938c1 on 8.3.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    

  • alexpott committed 5e938c1 on 8.4.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    

  • alexpott committed 5e938c1 on 8.4.x
    Issue #1342504 by jbrown, Jelle_S, Dave Reid: Support data URIs.
    
jollysolutions’s picture

Status: Needs review » Reviewed & tested by the community

patch #41 works for me

jollysolutions’s picture

How do we get this committed, please?

pobster’s picture

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