Webp full support #711
Webp full support #711
Conversation
src/Gd/Imagine.php
Outdated
|
|
||
| private function loadWebp(string $data) | ||
| { | ||
| $tmpfile = tempnam(sys_get_temp_dir(), 'imaginewebp_'); |
There was a problem hiding this comment.
I think that:
- The temporary directory should be configurable
- We should delete the temporary file once we've done reading it
There was a problem hiding this comment.
I'm not sure if that's necessery. Why would we want to configure the temp directory for just loading the file?
Also if we want to delete the file we should probably do this somewhere else - we give the resource back and we cannot remove file that is currently opened as resources AFAIK. Perhaps on destructor of the class? To be honest - if it's in /tmp dir then filesystem will clear it eventually so it seemed like a "safe" choice :)
There was a problem hiding this comment.
I've seen crappy hosting providers where the directory returned from sys_get_temp_dir() was not writable...
About deleting the file: PHP loads the file content in memory, so the file can be safely deleted.
There was a problem hiding this comment.
Ok, will fix that in next commit after we resolve the second issue.
src/Gd/Imagine.php
Outdated
| return @imagecreatefromstring($data); | ||
| }); | ||
|
|
||
| if($this->isWebp($data)) { |
There was a problem hiding this comment.
imagecreatefromwebp has been introduced in PHP 5.4, but Imagine supports PHP 5.3.2. so what about
| if($this->isWebp($data)) { | |
| if(function_exists('imagecreatefromwebp') && $this->isWebp($data)) { |
src/Gd/Imagine.php
Outdated
| return @imagecreatefromstring($string); | ||
| }); | ||
|
|
||
| if($this->isWebp($string)) { |
There was a problem hiding this comment.
imagecreatefromwebp has been introduced in PHP 5.4, but Imagine supports PHP 5.3.2. so what about
| if($this->isWebp($string)) { | |
| if(function_exists('imagecreatefromwebp') && $this->isWebp($data)) { |
There was a problem hiding this comment.
Well, you have to choose whether you want to support version that is 10 years old and unsupported since last 5 years or to use the image standard from 2010, early adopted by facebook & google in 2013 :) I'm all up for the latter but I can work on my fork so it's up to you.
Do you think that function check is good enough? Shouldn't we post some information that with the newer version of PHP this will be working at least?
(Sorry for the other comment, wrong account).
There was a problem hiding this comment.
I'd just do the function exists check. We can add a note to the changelog / release notes that webp support requires PHP 5.4+.
There was a problem hiding this comment.
I've pushed the new commit with your suggestion in it :)
|
Tests are failing. |
I've fixed code styling and tests. Do you want me to rebase as well? The Appveyor tests are failing but not because of my code :( There are a lot of issues with imagick from what I can tell. |
|
I created an alternative pull request for this issue: #718 Instead of using a temp directory, it uses a data URI to load the WEBP image. Maybe this would be a better solution? We often had issues with using |
src/Gd/Imagine.php
Outdated
| */ | ||
| private function isWebp($data) | ||
| { | ||
| return 0 === strncmp(substr($data, 8, 7), 'WEBPVP8', 7); |
There was a problem hiding this comment.
Isn't this the same as substr($data, 8, 7) === 'WEBPVP8'?
# Conflicts: # src/Gd/Imagine.php
According to #501