-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Supporting webp format #21
base: master
Are you sure you want to change the base?
Conversation
$type = IMAGETYPE_WEBP; | ||
|
||
// Quality is different in webp than PNG | ||
$quality = 85; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. It's better not to set $quality
value here (webp
handling should be similar to the one for jpeg
, not gif
or png
).
With the PR-suggested change, end-users won't have control over the resulting webp
images quality, even passing $quality
to save
/render
functions. Note that $quality
is passed by reference to _save_function. So we get the value of 85 to be "hard-coded", instead of the expected behavior.
|
||
// Quality is different in webp than PNG | ||
$quality = 85; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check this line: my editor shows trailing spaces after the break;
.
|
||
// Quality is different in webp than PNG | ||
$quality = 85; | ||
break; | ||
default: | ||
throw new ErrorException(sprintf('Installed GD does not support %s images',$extension)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, not related to the PR directly: I really think this error message could be improved. Not handling webp
prior this PR, for instance, doesn't mean Installed GD does not support it. Hence the error message is misleading, and could be improved too, whilst we're looking at this part of code.
Well, I've wrapped up the above said into yugaego@ad4b181 and for now load it in my project with composer.json{
"require": {
"yurkinx/yii2-image": "dev-webp"
},
"repositories": [
{
"type": "vcs",
"url": "[email protected]:yugaego/yii2-image.git"
}
]
} The linked branch also adds WebP support to Imagick driver, since compiling For reference, to quick check whether WebP support is enabled on the server, do:
|
Allowing webp format for GD