Problem/Motivation
Before GDToolkit::save() writes files (e.g. newly generated image styles) using GD Image functions, it calls realpath() to convert stream protocols to local filenames. This is done for compatibility with old php-gd2 versions.
This realpath() call does essentially nothing for public://, private:// and temporary:// schemes. It's also non-buggy for remote stream protocols/wrappers, because these are written to temporary:// first. But for any custom local stream wrappers, it introduces potential bugs.
See below for bug I encountered - but also: generally, there is (AFAICT) no requirement for a stream wrapper to implement realpath(). So GDToolkit::save()'s reliance on it returning an actual path is arguably a bug.
It seems that it can be safely removed for PHP8. (Already for PHP7, probably.)
Details about the 'stream conversion' changes
GDToolkit::save($destination) - where $destination is the destination path to be written. e.g. public://styles/public/thumbnail/photo.jpg - has the following added:
1)
$destination = $this->fileSystem->realpath($destination); This code was added in
#517814: File API Stream Wrapper Conversion →
, in 7.0-alpha1, November 2009.
The issue text mentions "Issues caused by the image cache commits over the weekend", which were resolved first in the 180k patch in comment #2, with the comment
// Convert URI to a normal path because PHP apparently has some gaps in stream wrapper support.
2)
Code to work around lack of support in remote stream wrappers in image*() save functions, by first generating the image to temporary:// and then move()ing it to the remote location:
#696150: image_gd_save() does not support remote stream wrappers →
, indroduced in 7.0-beta1, September 2010.
My take on this
I'm going to take point 2 / #696150 at its word and assume that there may still be some remote stream wrappers which GD save functions have an issue with. I've just included it here for completeness -- feel free to disagree or request testing of that assumption.
It's point 1 that I have an issue with:
- I don't believe it's necessary anymore in PHP8 (and probably PHP7). In my test using a custom stream wrapper,
imagejpeg(IMAGE, "custom://OUTPUTFILE.jpg") works without issues. Unfortunately it's just an assumption that this works for all GD save functions for all modern PHPs: I can't find docs/changelogs explicitly saying there was an issue with php-gd2 and/or it's solved now.
- Also, see 'bug' above.
My specific issue / how I noticed this
I needed to work with image styles + a custom 'encrypted://' stream wrapper, which identifies as 'local' type, and which stores each file encrypted on a local disk.
This must not call realpath() before saving - otherwise the image will not pass through the stream wrapper's code on writing, so it will be saved un-encrypted. So the wrapper's realpath() returns FALSE, Making GDToolkit::save() fail.
This also should not do an intermediate write to temporary://; we don't want unencrypted files to be written anywhere.
Steps to reproduce
Well... it requires a custom stream wrapper.
Proposed resolution
Alternative 1:
Just remove the realpath() call, because we are sure it doesn't do anything anymore; all image*() write functions work well with at least all scheme://s whose getType() is local. (And if not: that's their bug to fix, not ours.)
Alternative 2:
If we don't trust the above: do we want a configuration option? I see a few issues with that:
- I don't know how testable the different values are
- Unless we do multiple options instead of a checkbox: we will be intermingling the code for above points 1 and 2, keeping the realpath() call as default (which I personally don't really like).
...but I've taken a stab at it, to give an idea. Again: all sites that write 'regular' files (to public:// or private://) will see no change because calling realpath() just makes no difference. But
- the 'on/write direct' case potentially will not work for remote sream wrappers' schemes, and therefore should not be default
- which means the default case will, non-predictably, not work for some custom-local stream wrappers.
Alternative 3 (added later, not changed into patch/branch yet): add if()
$real_destination = $this->fileSystem->realpath($destination);
if ($real_destination) {
$destination = $real_destination;
}
...Keeping the realpath call here seems theoretically 'problematic' if it's not necessary; it prevents the write operation from going through the stream wrapper. (But... wrappers who want to force that, will return FALSE from realpath()?)
Remaining tasks
First: anyone, chime in with your opinion about just removing the realpath() call, or 'alternative 3'. (I don't think that needs tests; does it?)
If that is not OK: then we need to think further about
- what then? Alternative 2? Or some other configuration that does not tie above points 1 + 2 together?
- Does that need tests?
Review one of the patches. (I'm uploading them as patch files, not doing branches, because it will likely take ages for this issue to be handled anyway.) [Edit - oh hey, I found a parent issue to raise awareness...]
User interface changes
Maybe
API changes
No
Data model changes
No
Release notes snippet
TBD
