image_gd_save() does not support remote stream wrappers

Posted by c960657 on January 26, 2010 at 9:04pm

7 followers

Issue Summary

The GD functions imagegif(), imagepng(), and imagejpeg() do not support remote stream wrappers (e.g. imagegif($im, "s3://foo/bar.gif") fails).

image_gd_save() contains a comment related to stream wrappers and uses $wrapper->realpath() to get the local filepath:

  // Convert URI to a normal path because PHP apparently has some gaps in stream wrapper support.
  if ($wrapper = file_stream_wrapper_get_instance_by_uri($destination)) {
    $destination = $wrapper->realpath();
  }

If $wrapper->realpath() returns a path that is not a filesystem path (is that allowed? The documentation for DrupalStreamWrapperInterface::realpath() isn't very explicit about the return value), the call to imagexxx() will fail.

The function needs to use a temporary path a suggested in one of the early patches for #227232: Support PHP stream wrappers, e.g. the one in comment #103.

Comments

#1

Posted by aaron on January 26, 2010 at 9:25pm

#2

Posted by pwolanin on January 27, 2010 at 9:51pm

So - the suggested approach is to save to a temporary local file and then move/copy to a remote destination?

#3

Posted by c960657 on January 27, 2010 at 10:02pm

Yes. Perhaps we can use some of the new stream wrapper flags that were just introduced in #685074: (Tests needed) Some stream wrappers need to be hidden or read-only.

I'm not sure I like how $wrapper->realpath() currently returns a path without wrapper prefix. This makes it difficult to normalize a path while preserving the wrapper prefix (e.g. "public://foo/bar/../baz.txt" to "public://foo/baz.txt"). Perhaps we should make the current getLocalPath() part of the DrupalLocalStreamWrapper interface and use that instead?

#4

Posted by pwolanin on January 27, 2010 at 10:25pm

Hmm, well we can use the local property I think.

#5

Posted by pwolanin on January 27, 2010 at 11:32pm

Status:

active

» needs review

totally untested...

Attachment	Size	Status	Test result	Operations
696150-gd-tempnam-5.patch	1.99 KB	Idle	FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 696150-gd-tempnam-5.patch.	View details

#6

Posted by aaron on January 28, 2010 at 12:30am

shouldn't you use !isset($parents['DrupalLocalStreamWrapper']) rather than empty($parents['DrupalLocalStreamWrapper']) so it doesn't throw a warning if that's not set?

looks good otherwise.

#7

Posted by eMPee584 on January 28, 2010 at 3:39am

@aaron: the empty() check includes isset(), i.e. it doesn't throw an error if fed an uninitialized variable, so it is usually the most appropriate choice if some needed data is present...

#8

Posted by c960657 on January 28, 2010 at 10:15pm

This would require alternate wrapper implementations to be subclass DrupalLocalStreamWrapper (it is an abstract class, not an interface). Would it be better to either introduce an interface or look for STREAM_WRAPPERS_LOCAL? Stream wrappers is a generic PHP feature, so a wrapper may be written for other uses than just in Drupal.

BTW whatever approach for detecting local files chosen here should also be applied to the top of archiver_get_archiver() (the logic currently in use there is flawed).