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.

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

Replace 5 with $quality - Else quality is always 5 #10

Closed
jenstornell opened this issue May 7, 2019 · 7 comments
Closed

Replace 5 with $quality - Else quality is always 5 #10

jenstornell opened this issue May 7, 2019 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@jenstornell
Copy link
Collaborator

jenstornell commented May 7, 2019

It should probably be $quality instead of 5.

function convert($from, $to, $quality = -1) {
  $converter = new ImageConverter();
  return $converter->convert($from, $to, 5);
}
@peter279k peter279k added the enhancement New feature or request label May 8, 2019
@peter279k
Copy link
Member

It's not weird.

  • According to the imagejpeg function reference, the default image quality value is -1.
  • According to the imagepng function reference, the default image quality value is -1.
  • The imagegif and imagebmp functions don't have the quality value.

Could you explain more details about why you think this default quality is weird?

@peter279k peter279k self-assigned this May 8, 2019
@jenstornell
Copy link
Collaborator Author

@peter279k I was in a hurry when adding this issue. It's not the built in PHP default value that is weird, it's my convert function default value that is weird, or wrong is a better word.

When sending a $quality value to it, it will be ignored, because it's never sent to the class method. It will always end up as 5.

I just need to move the $quality argument and replace 5 with it and it should be fine.

@peter279k
Copy link
Member

@jenstornell, thank you for your explanation.

I know that and I think we have to consider/note that some image formats in PHP GD extension will not have the quality argument.

@peter279k
Copy link
Member

And the function you create should change into following code snippets:

function convert($from, $to, $quality = -1) {
  $converter = new ImageConverter();
  return $converter->convert($from, $to, $quality);
}

@jenstornell
Copy link
Collaborator Author

@peter279k Yes. I tested sending a quality argument to a method that did not support it and it will just be ignored, so I think it will be fine out of the box. I think a note about it in the docs will do (maybe that's what you ment?). Also the quality argument differ between formats. Some have 1-100 and others 1-9 etc. Docs would be good not forcing users to go to php.net.

Yes, I know how the function will look like. I did not have the time to implement it yesterday, that's why I added an issue instead.

@peter279k
Copy link
Member

@jenstornell, thank you for your comment.

Perhaps we should have the quality validation before passing the $quality to specific image manipulation functions?

@jenstornell jenstornell changed the title Quality default argument is weird Replace 5 with $quality - Else quality is always 5 May 8, 2019
jenstornell added a commit that referenced this issue May 11, 2019
@jenstornell
Copy link
Collaborator Author

✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants