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.

CommentFileSizeAuthor
#12 696150-gd-remote.patch2.57 KBmfb
#11 696150-gd-remote.patch2.67 KBmfb
#5 696150-gd-tempnam-5.patch1.99 KBpwolanin

Comments

aaron’s picture

aaron commented

subscribe

pwolanin’s picture

pwolanin commented

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

c960657’s picture

c960657 commented

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?

pwolanin’s picture

pwolanin commented

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

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 696150-gd-tempnam-5.patch1.99 KB

totally untested...

aaron’s picture

aaron commented

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.

eMPee584’s picture

eMPee584 commented

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

c960657’s picture

c960657 commented

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

pwolanin’s picture

pwolanin commented

#5: 696150-gd-tempnam-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 696150-gd-tempnam-5.patch, failed testing.

mfb’s picture

mfb
they or he
commented
Status: Needs work » Needs review
StatusFileSize
new 696150-gd-remote.patch2.67 KB

refactored to look for STREAM_WRAPPERS_LOCAL

mfb’s picture

mfb
they or he
commented
StatusFileSize
new 696150-gd-remote.patch2.57 KB

While I'm here, remove the unused $extension param from the docs. extension is part of the image object.

pwolanin’s picture

pwolanin commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.