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

Remove CarrierWave::MimeTypes processor module #1813

Merged

Conversation

mehlah
Copy link
Member

@mehlah mehlah commented Jan 1, 2016

Since #1245 (*), CarrierWave::MimeTypes module is redundant as mime-types gem is a runtime dependency now, and SanitizedFile gets it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

What do you think about removing MagicMimeTypes processor module too?
I can't see any valid reason to keep it around.
A related side note: CarrierWave rely on ruby-filemagic gem (MagicMime) to validates the content type of a file against a white/blacklist.
I think we should use mime-types (Mime::Types) for this as it's a runtime dependency, and it's consistent with what it's used by SanitizedFile to set a content type.
Otherwise, we could use MagicMime everywhere as a runtime dependency instead of Mime::Types, but this kills JRuby support.

(*) I discovered this when tried to fix specs marked as pending. I looked at commits history and PRs to understand what happened.
It started with #372 that added CarrierWave::MimeTypes processor (Jun 2011), then #1216 and #1245 that made mime-types a runtime dependency and deprecated CarrierWave::MimeTypes module (2013), and finally #1584 CarrierWave::MagicMimeTypes (2015)

@thomasfedb
Copy link
Contributor

I'm happy to remove this processor module. It's clearly deprecated, and offers very little.

@thomasfedb
Copy link
Contributor

I'd welcome a PR that removed the need for MagicMime if you can implement the same functionality with Mime::Types.

@mehlah
Copy link
Member Author

mehlah commented Jan 1, 2016

@thomasfedb thanks for your feedback. I'll work on removing MagicMime then 👍
I rebased this against master.

Since carrierwaveuploader#1245 (*), CarrierWave::MimeTypes module is redundant as
`mime-types` gem is a runtime dependency now, and `SanitizedFile` gets
it's content_type using it.
The module was deprecated since 0.10 and warned developers when used it.

(*) I discovered this when tried to fix specs marked as pending. I
looked at commits history and PRs to understand what happened.
It started with carrierwaveuploader#372 that added `CarrierWave::MimeTypes`  processor (Jun
2011), then carrierwaveuploader#1216 and carrierwaveuploader#1245 that made `mime-types` a runtime dependency
and deprecated `CarrierWave::MimeTypes` module (2013), and finally carrierwaveuploader#1584
`CarrierWave::MagicMimeTypes` (2015)
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature was relying only on ruby-filemagic
to read the content type of a file. It was optional and required mixing
a module to the uploader.

This commit remove this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 1, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
@mehlah
Copy link
Member Author

mehlah commented Jan 1, 2016

@thomasfedb I removed the need of MagicMime 💃
Work based on this branch:
mehlah/carrierwave@remove-mimetypes-module...mehlah:remove-magicmime
I'll PR that as soon as this got merged

mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
thomasfedb added a commit that referenced this pull request Jan 2, 2016
Remove depreciated CarrierWave::MimeTypes processor
@thomasfedb thomasfedb merged commit 4362fbf into carrierwaveuploader:master Jan 2, 2016
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
@mehlah mehlah deleted the remove-mimetypes-module branch January 2, 2016 03:47
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
mehlah pushed a commit to mehlah/carrierwave that referenced this pull request Jan 2, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
mynock pushed a commit to mynock/carrierwave that referenced this pull request Aug 9, 2016
mynock pushed a commit to mynock/carrierwave that referenced this pull request Aug 9, 2016
Before this, CarrierWave was relying on mime-types or ruby-filemagic
gems to get/set the content type of a file in an inconsistent way.

After making mime-types a runtime dependency for CW,
`CarrierWave::MimeTypes` has been dropped (carrierwaveuploader#1813 for details), and
`CarrierWave::MagicMimeTypes` as well.

White/blacklisting content types feature (introduced by carrierwaveuploader#1596)
was relying only on ruby-filemagic to read the content type of a file.
It was optional and required mixing a module to the uploader.

This commit removes this dependency by using the already available
content type (thanks to `Mime::Types`).
This let us drop any dependency on ruby-filemagic in CarrierWave.
It makes JRuby support better as ruby-filemagic is unsupported on
JRuby due to the C extension.
@ersinakinci
Copy link
Contributor

Just wanted to thank @mehlah for going through the extra effort of researching and documenting the history of CarrierWave::MimeTypes. It was a huge help in figuring out what was going wrong while upgrading CarrierWave. Kudos!

@mehlah
Copy link
Member Author

mehlah commented Mar 14, 2017

Thanks @earksiinni for the kind words ;)

bagedevimo added a commit to NZOI/nztrain that referenced this pull request Feb 27, 2023
According the README and changelog these are now optional - the
changelog is automatically inferred. It's hard to reference specific
commits, but this MR removed it [1].

[1] carrierwaveuploader/carrierwave#1813
tom93 added a commit to NZOI/nztrain that referenced this pull request Feb 27, 2023
The method has been removed, and according to the README and changelog
it is longer necessary to set the content type manually - it is
automatically inferred. It's hard to reference specific commits, but
this MR removed it: [1].

[1] carrierwaveuploader/carrierwave#1813

Co-authored-by: Tom Levy <tomlevy93@gmail.com>
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.

None yet

3 participants