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: Apply image format when saving file in ImagickDriver #268

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

nicoverbruggen
Copy link
Contributor

After upgrading to the latest version of spatie/image we noticed some issues when converting and cropping PDF files with spatie/media-library. We specify that for each PDF a square JPG thumbnail must be generated, but these turned out to be corrupt.

(I initially believed this was a bug in the spatie/media-library package but I was wrong. The destination files simply weren't JPGs, but PDFs instead. This was weird, as we definitely specify the output format.)

Upon further investigation, it seems like the GD and the Imagick driver handle setting the desired output format slightly differently.

What I discovered was that certain functions like resizeCanvas and overlay in ImagickDriver actually re-assign the image property, thus "losing" the format if it was set prior to calling those transformations (using ->format('jpg'), for example).

You can see the reassignment here. Initially, prior to reassigning $image the format may have been set, and it can be lost afterwards.

A few observations:

  • I verified that for our use case $image->getFormat() goes from JPG to (an empty string) after reassignment, which means that the original object's extension will be (incorrectly/unexpectedly) used when actually persisting the image instance.
  • This is also why this is an issue in combination with spatie/media-library, since there a temp copy of the original file is created, which is then transformed and saved (as the same format because the extension is now used as a fallback).
  • If the input and output format are the same, you were unlikely to hit this edge case.

This problem can be avoided by ensuring setFormat() on the Imagick image(s) is only called prior to saving the image, at which point the format will always be correct when saving the file.

To do this, the format is stored as a property on the driver instance and is reset when the image is saved. (The GD driver already does it the same way, so I just made a few adjustments to bring this driver in line with the GD driver.)

In order to ensure all tests passed locally, I also had to make sure to check that 'JPEG' is set as the format if the extension or format is specified as 'JFIF', so I added that to the save method as well.

Let me know if I need to make any further adjustments :)

Cheers!

Certain functions like `resizeCanvas`, `overlay` actually re-assign
the `image` property, thus "losing" the format if it was set prior to
calling those transformations.

This can be avoided by ensuring `setFormat()` on the Imagick image(s)
is only called prior to saving the image, at which point the format
will always be correct when saving the file.

To do this, the format is stored as a property on the driver object.
(The GD driver already does it the same way, so I just made a few
adjustments to bring this driver in line with the GD driver.)
@freekmurze freekmurze merged commit 14f93e4 into spatie:main Jul 26, 2024
3 checks passed
@freekmurze
Copy link
Member

Great work, thanks for the deep investigation and the fix!

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