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

Tiff support #2799

Merged
merged 3 commits into from
Feb 21, 2022
Merged

Tiff support #2799

merged 3 commits into from
Feb 21, 2022

Conversation

Synchro
Copy link
Contributor

@Synchro Synchro commented Feb 21, 2022

Further to the discussion in #2794, this PR adds TIFF support to Media Library. This is my second attempt at this PR, having made it against the wrong branch last time!

I'm not entirely happy with the implementation, and I think it would be much better to obtain the list of supported file extensions and MIME types from spatie/image rather than maintaining a separate set of them in here. However, it's workable for now.

This is the final PR in a chain starting in glide, going via spatie/image before ending up here, all because I needed to use TIFF in MLP! It's the first time I've had a PR chain this long!

@freekmurze
Copy link
Member

Very nice work! Thanks for all the PRs you made in this chain 👏

I think it would be much better to obtain the list of supported file extensions and MIME types from spatie/image rather than maintaining a separate set of them in here. However, it's workable for now.

Yeah, I agree, it could be nicer, but this list won't change too much in the near future.

@freekmurze freekmurze merged commit b68b488 into spatie:v9 Feb 21, 2022
@freekmurze
Copy link
Member

Could you create a similar PR on the main branch for v10 of the package?

@Synchro
Copy link
Contributor Author

Synchro commented Feb 22, 2022

Could you create a similar PR on the main branch for v10 of the package?

I can, however, I will need to do a similar shuffle in spatie/image to port the TIFF support to the 2.x branch first.

@Synchro
Copy link
Contributor Author

Synchro commented Feb 22, 2022

Actually I'm a bit confused. ML v10 depends on spatie/image 2.x, but spatie/image doesn't seem to have a v2 branch: the PR I did there was against main, but it was released as 1.11.0 yesterday; the latest 2.x release is from 2 months ago, but it was also released from main. Where should I make a PR to get these patches into 2.x?

@freekmurze
Copy link
Member

but spatie/image doesn't seem to have a v2 branch

The main branch is essentially v2. If see that we've made a wrong tag yesterday on spatie/image. We've removed 1.11.0 and replaced it with 2.2.2

To make v1 of spatie/image support TIFFs, send a PR to the v1 branch.

My apologies for the confusion.

@Synchro Synchro mentioned this pull request Feb 22, 2022
@Synchro Synchro deleted the tiff-support branch February 22, 2022 11:07
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