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

Fix to ImageMagickHandler #3104

Closed
wants to merge 2 commits into from
Closed

Fix to ImageMagickHandler #3104

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa64c8
Fix to ImageMagickHandler
colethorsen Jun 15, 2020
544106e
Fix for existing resource
colethorsen Jun 16, 2020
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
21 changes: 16 additions & 5 deletions system/Images/Handlers/ImageMagickHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,23 +288,34 @@ public function save(string $target = null, int $quality = 90): bool
{
$target = empty($target) ? $this->image() : $target;

// If no new resource has been created, then we're
// simply copy the existing one.
if (empty($this->resource))
// If no new resource has been created, and we
// aren't adjusting the quality then we're simply
// copy the existing one.
if (empty($this->resource) && $quality == 100)
{
// 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.

{
return true;
}

$name = basename($target);
$path = pathinfo($target, PATHINFO_DIRNAME);

return $this->image()->copy($path, $name);
}

$source = ! empty($this->resource) ? $this->resource : $this->image()->getPathname();

// Copy the file through ImageMagick so that it has
// a chance to convert file format.
$action = '"' . $this->resource . '" "' . $target . '"';
$action = '"' . $source . '" "' . $target . '"';

$result = $this->process($action, $quality);

unlink($this->resource);
if(!empty($this->resource)) {
unlink($this->resource);
}

return true;
}
Expand Down