-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
SanitizedFile doesn't set content type when retrieving image from store. #1216
Conversation
Same issue here #27 |
up ? |
any idea if this is gonna be merged? |
@file = File.open(file_path('sponsored.doc')) | ||
@file.stub!(:content_type).and_return(MIME::Type.new('application/msword')) | ||
@sanitized_file = CarrierWave::SanitizedFile.new(@file) | ||
@sanitized_file.content_type.should == 'application/msword' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these four lines here? Seems like you're just duplicating the test immediately before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, useless.
It's an interesting feature and I think I'd need feedback from other repo collaborators before I could merge it. By making SanitizedFile calculate its own mime type, I think we're making the entire CarrierWave::MimeTypes module redundant. This could actually be great, but if we're going to do it, we should probably remove the now-unnecessary code... right? |
@@ -57,7 +57,7 @@ def generic_content_type? | |||
# false by default | |||
# | |||
def set_content_type(override=false) | |||
if override || file.content_type.blank? || generic_content_type? | |||
if override || file.instance_variable_get(:@content_type).blank? || generic_content_type? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, this was the line that tipped me off we might be duplicating our Carrierwave::MimeTypes module. I'd rather not merge this line, since all it does is make a single (now incorrect) test pass.
I agree with you, but I did not dare remove this module immediately. |
Event if this module may not be usefull anymore. Proxy read content type itself now.
@taavo I've made some updates according to your recommandations. But, as you said, I think CarrierWave::MimeTypes can be deprecated. |
@@ -244,7 +244,11 @@ def to_file | |||
# | |||
def content_type | |||
return @content_type if @content_type | |||
@file.content_type.to_s.chomp if @file.respond_to?(:content_type) and @file.content_type | |||
if @file.respond_to?(:content_type) and @file.content_type | |||
@file.content_type.to_s.chomp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to cache @content_type
in this case also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason at all.
Updated. |
@taavo do you think it will be merged ? |
Would you squash it down to one commit so we can keep track of it better? Given that I seem to be the only one available to provide an opinion, my inclination is to merge. Worst case scenario it gets reverted down the road if someone feels strongly, but I think the odds are on your side—it's a useful feature. Thanks for your work. |
If we're going this direction, I'd like to see |
@bensie 👍 |
s/tomorrow/tonight |
Closing in favor of #1245. |
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)
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)
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)
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)
Hi,
Using CarrierWave::MimeTypes, the content_type is only available directly after the upload phase.
After upload, calling
my_model.uploader.content_type
always returnnil
This PR seems to fix that.