Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Webp full support #711

Merged
merged 7 commits into from
Jul 8, 2019
Merged

Webp full support #711

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7556450
Webp fully support
May 9, 2019
4d7bd09
Fallback for 5.3 and configurable temp dir
May 10, 2019
45c50a7
Fix code style
lashus May 10, 2019
8516277
Further codestyle improvement w/tests
lashus May 10, 2019
1214807
Fix codestyle using cs fixer
lashus May 10, 2019
193f333
Merge branch develop into pr-711
mlocati Jul 8, 2019
bbff90d
Simplify Gd\Imagine::isWebp()
mlocati Jul 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions src/Gd/Imagine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,14 @@ public function open($path)
$path = $loader->getPath();

$data = $loader->getData();
$resource = $this->withoutExceptionHandlers(function () use (&$data) {
return @imagecreatefromstring($data);
});

if($this->isWebp($data)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imagecreatefromwebp has been introduced in PHP 5.4, but Imagine supports PHP 5.3.2. so what about

Suggested change
if($this->isWebp($data)) {
if(function_exists('imagecreatefromwebp') && $this->isWebp($data)) {

$resource = $this->loadWebp($data);
} else {
$resource = $this->withoutExceptionHandlers(function () use (&$data) {
return @imagecreatefromstring($data);
});
}

if (!is_resource($resource)) {
throw new RuntimeException(sprintf('Unable to open image %s', $path));
Expand Down Expand Up @@ -208,9 +213,14 @@ private function requireGdVersion($version)
*/
private function doLoad($string, MetadataBag $metadata)
{
$resource = $this->withoutExceptionHandlers(function () use (&$string) {
return @imagecreatefromstring($string);
});

if($this->isWebp($string)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imagecreatefromwebp has been introduced in PHP 5.4, but Imagine supports PHP 5.3.2. so what about

Suggested change
if($this->isWebp($string)) {
if(function_exists('imagecreatefromwebp') && $this->isWebp($data)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed the new commit with your suggestion in it :)

$resource = $this->loadWebp($string);
} else {
$resource = $this->withoutExceptionHandlers(function () use (&$string) {
return @imagecreatefromstring($string);
});
}

if (!is_resource($resource)) {
throw new RuntimeException('An image could not be created from the given input');
Expand Down Expand Up @@ -239,4 +249,19 @@ private function withoutExceptionHandlers($callback)

return $result;
}

private function isWebp(string $data)
{
return strncmp( substr( $data, 8, 7 ), "WEBPVP8", 7 ) === 0;
}

private function loadWebp(string $data)
{
$tmpfile = tempnam(sys_get_temp_dir(), 'imaginewebp_');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that:

  1. The temporary directory should be configurable
  2. We should delete the temporary file once we've done reading it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will fix that in next commit after we resolve the second issue.

file_put_contents($tmpfile, $data);
$resource = $this->withoutExceptionHandlers(function () use ($tmpfile) {
return @imagecreatefromwebp($tmpfile);
});
return $resource;
}
}
Binary file added tests/fixtures/google.webp
View file
Open in desktop
Binary file not shown.
17 changes: 17 additions & 0 deletions tests/tests/Gd/ImagineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ protected function setUp()
}
}

public function testShouldOpenAWebpImage()
{
$source = IMAGINE_TEST_FIXTURESFOLDER . '/google.webp';
$factory = $this->getImagine();
$image = $factory->open($source);
$size = $image->getSize();

$this->assertInstanceOf('Imagine\Image\ImageInterface', $image);
$this->assertEquals(550, $size->getWidth());
$this->assertEquals(368, $size->getHeight());

$metadata = $image->metadata();

$this->assertEquals($source, $metadata['uri']);
$this->assertEquals(realpath($source), $metadata['filepath']);
}

protected function getImagine()
{
return new Imagine();
Expand Down