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

Fix to ImageMagickHandler #3104

Closed

Conversation

colethorsen
Copy link
Contributor

@colethorsen colethorsen commented Jun 15, 2020

-Saving an image back to the same location with a lower quality would result in a failure attempting to copy.
-Saving an image to a new spot with a lower quality would result in the image not having a new quality set.

Each pull request should address a single issue and have a meaningful title.

Description
Explain what you have changed, and why.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

-Saving an image back to the same location with a lower quality would result in a failure attempting to copy.
-Saving an image to a new spot with a lower quality would result in the image not having a new quality set.
@colethorsen colethorsen force-pushed the feature/imagemagick branch from 4c7a2ea to ffa64c8 Compare June 15, 2020 14:04
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Thanks for stepping in and making your first PR to this project.

Just so you know - we probably won't merge anything that involves ImageMagick handler until we prepare some tests for it.

{
// we're moving it to the same spot so don't do anything.
if($target === null)
Copy link
Member

Choose a reason for hiding this comment

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

I can't see the scenario when this condition would be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would fail for instance if someone set it to save to the same location and set a quality to 100. I'd think it would be easier for this to just leave it alone, than to fail trying to copy it to the same spot

Copy link
Member

Choose a reason for hiding this comment

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

I understand but look at line 289. $target will never be a null value.

system/Images/Handlers/ImageMagickHandler.php Outdated Show resolved Hide resolved
@michalsn
Copy link
Member

Fixed via #3268

@michalsn michalsn closed this Jul 11, 2020
@colethorsen colethorsen deleted the feature/imagemagick branch June 21, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants