-
Notifications
You must be signed in to change notification settings - Fork 379
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
Enable configuration of post processors using parameters (closes #720) #759
Conversation
07a8dd6
to
55af62e
Compare
please rebase |
Fix for invalid variable name Move symfony/process component to require section
55af62e
to
67307c6
Compare
Done, tests re-running now |
*/ | ||
public function __construct($jpegoptimBin = '/usr/bin/jpegoptim', $stripAll = true, $max = 100, $progressive = true) | ||
public function __construct($jpegoptimBin = '/usr/bin/jpegoptim', $stripAll = true, $max = null, $progressive = true) |
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.
is this change intentional?
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.
I was just looking at this myself during the rebase - it's originally from here 81ee48a#diff-9cd4d6a634315c7cf9b3483cb8b37db8R45 so I chose to keep it.
Setting --max disables lossless mode and will reduce the quality factor of jpegs (if greater than its current value) to the current setting - My assumption then is that it doesn't matter, as 100 is the max quality factor for the jpeg format anyway, so removing the flag shouldn't matter much.
I can run a few test cases using jpegoptim by hand to see if there's any difference, if that'd be preferred?
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.
yeah I guess defaulting to null
is better then
$pb->add('--strip=all'); | ||
} | ||
|
||
$pb->add($input = tempnam(sys_get_temp_dir(), 'imagine_optipng')); |
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.
Sorry for my late comment, but I have noticed that there is merge conflict. $input gets added twice.
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.
Oops, nice catch!
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.
can you do a follow up PR?
Closes #720