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

Feat: Add image service #616

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

CamKem
Copy link
Collaborator

@CamKem CamKem commented Sep 16, 2024

This PR handles several issues related to image optimisation:

  • fix the ios orientation issue when EXIF tags are being stripped.
  • extracts the logic of optimisation to a service so it can be both tested & reused for post & avatar uploads.
  • removes intervention/image dep - currently only used for avatars.

Tests have been added for this new service to ensure that it is fully covered.

Closes #507

@nunomaduro
Copy link
Member

@MrPunyapal can you test this, and validate it?

@steven-fox
Copy link
Collaborator

@CamKem Genuine question: what are the pros/cons to using spatie/image for our image manipulation needs here? Obviously there's the trade off of adding dependencies (tho, Spatie is amongst the most reliable dependency vendors out there), but curious if you feel it'd be worth it for the functionality and nice api that package provides. I use it for projects, personally.

@CamKem
Copy link
Collaborator Author

CamKem commented Sep 19, 2024

@CamKem Genuine question: what are the pros/cons to using spatie/image for our image manipulation needs here? Obviously there's the trade off of adding dependencies (tho, Spatie is amongst the most reliable dependency vendors out there), but curious if you feel it'd be worth it for the functionality and nice api that package provides. I use it for projects, personally.

@tomloprod refactored our optimiser away from intervention/image due to the lack of ability to strip tags & Imagick is really the bare metal that is at the heart of image processing in PHP, so felt it was a good move rather than move to another dependency.

For me the biggest thing, especially with Pinkary is demonstrating what can be done without external deps, so I figured I'd continue in the process & remove the intervention dependency entirely.

Really there is nothing wrong with the Spatie package & if it were client work, I'd probably reach for it, but when I'm working on side projects & open source for fun I'd rather showcase the possibilities & abilities of the underlying technology.

Plus I feel that's been a core tenant of Nuno's direction from the beginning, to keep external deps to an absolute minimum.

@steven-fox
Copy link
Collaborator

@CamKem Gotcha and makes sense. I was interested in spatie/image for this case because, if I'm not mistaken, they don't use intervention/image either. It's mainly a convenience wrapper around Imagick/gd by default.

Totally cool with not loading in the dependency if we don't need all the extra functionality. 👍

That said, it may still be worth a look at the source of spatie/image to get a feel for their api and implementation. I think it's really well done.

Ok. Over and out. 🙂

@MrPunyapal
Copy link
Collaborator

works well

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.

Fix: abstract image optimisation & fix EXIF orientation
4 participants