unknown file extensions return binary mime-type. no file extension returns fallback mime-type #366

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@montague

see discussion here #316 for background.

@chneukirchen

This comment has been minimized.

Show comment
Hide comment
@chneukirchen

chneukirchen Mar 16, 2012

Member

Does not make sense to me because the fallback can be overriden.

Member

chneukirchen commented Mar 16, 2012

Does not make sense to me because the fallback can be overriden.

@montague

This comment has been minimized.

Show comment
Hide comment
@montague

montague Mar 16, 2012

sorry, should have included that this pull request was in response to #316

my understanding was that "application/octet-stream" was being sent to the recipient as a catch-all for unknown file formats, although http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1 seems to imply that unknown should be sent without a content-type and allow the recipient to guess. when all else fails, the data should be treated as type "application/octet-stream" by the recipient.

reading the aforementioned conversation (#316), it seemed to me that the course of action was going to be removing mime-types set explicitly to "application/octet-stream", since they would be handled by the default. overriding the default is certainly possible, but i figured anyone doing so would have a good reason for it.

definitely a chance i misinterpreted that conversation. if so, could you clarify? i'm happy to fix and resubmit.

sorry, should have included that this pull request was in response to #316

my understanding was that "application/octet-stream" was being sent to the recipient as a catch-all for unknown file formats, although http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1 seems to imply that unknown should be sent without a content-type and allow the recipient to guess. when all else fails, the data should be treated as type "application/octet-stream" by the recipient.

reading the aforementioned conversation (#316), it seemed to me that the course of action was going to be removing mime-types set explicitly to "application/octet-stream", since they would be handled by the default. overriding the default is certainly possible, but i figured anyone doing so would have a good reason for it.

definitely a chance i misinterpreted that conversation. if so, could you clarify? i'm happy to fix and resubmit.

@chneukirchen

This comment has been minimized.

Show comment
Hide comment
@chneukirchen

chneukirchen Mar 16, 2012

Member

See my reasoning in #316.

Member

chneukirchen commented Mar 16, 2012

See my reasoning in #316.

@montague

This comment has been minimized.

Show comment
Hide comment
@montague

montague Mar 16, 2012

take two. changed the behavior of Rack::Mime#mime_type to reflect comments here: #316 (comment)

take two. changed the behavior of Rack::Mime#mime_type to reflect comments here: #316 (comment)

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 17, 2012

Member

I really don't understand the specification:

should ignore explicitly set fallback for unknown file extensions

That seems very wrong to me, the whole point of the fallback, is to fall back on. The idea of not setting a mime type when unknown is fair, and I'd be happy to consider that in some other places. It should be noted however, that the reason for defaulting to application/octet-stream is to get file download style behavior in browsers.

Member

raggi commented Mar 17, 2012

I really don't understand the specification:

should ignore explicitly set fallback for unknown file extensions

That seems very wrong to me, the whole point of the fallback, is to fall back on. The idea of not setting a mime type when unknown is fair, and I'd be happy to consider that in some other places. It should be noted however, that the reason for defaulting to application/octet-stream is to get file download style behavior in browsers.

@montague

This comment has been minimized.

Show comment
Hide comment
@montague

montague Mar 17, 2012

so i wrote this patch based on my understanding of the chneukirchen's comments here: #316 (comment)

my understanding was that fallback was intended to be used for a file w/o an extension, as opposed to an unknown extension. if the original intention in defaulting to 'application/octet-stream' was for file download, then not specifying a mime-type for unknown extensions does make more sense. the spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1) also mentions that it's up to the recipient to determine how to treat an unknown type, saying only 'the recipient SHOULD treat it as type "application/octet-stream".'

in that case, i think the spec still makes sense--the fallback isn't used if the extension is unknown. it's only used if there is no extension. my thinking is that i'll update the code to return an empty string for unknown mime-types instead of the binary type.

thoughts? it seems to me that sending back a particular mime-type for unknown extensions is preventing the recipient from deciding how to handle it. my thinking is that if we don't know what something is, we tell the recipient the same.

anyway, i appreciate you taking the time to look

so i wrote this patch based on my understanding of the chneukirchen's comments here: #316 (comment)

my understanding was that fallback was intended to be used for a file w/o an extension, as opposed to an unknown extension. if the original intention in defaulting to 'application/octet-stream' was for file download, then not specifying a mime-type for unknown extensions does make more sense. the spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1) also mentions that it's up to the recipient to determine how to treat an unknown type, saying only 'the recipient SHOULD treat it as type "application/octet-stream".'

in that case, i think the spec still makes sense--the fallback isn't used if the extension is unknown. it's only used if there is no extension. my thinking is that i'll update the code to return an empty string for unknown mime-types instead of the binary type.

thoughts? it seems to me that sending back a particular mime-type for unknown extensions is preventing the recipient from deciding how to handle it. my thinking is that if we don't know what something is, we tell the recipient the same.

anyway, i appreciate you taking the time to look

raggi added a commit that referenced this pull request Mar 17, 2012

Correct some of the mime type issues. References #316 and #366.
HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD). They also have text describing how clients should handle this header being missing.

@raggi raggi referenced this pull request Mar 17, 2012

Merged

Mime type corrections #367

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 17, 2012

Member

@montague what if we simply didn't set it, that seems more sensible?

That's a SPEC change though, and so is minimum rack 1.5.

Member

raggi commented Mar 17, 2012

@montague what if we simply didn't set it, that seems more sensible?

That's a SPEC change though, and so is minimum rack 1.5.

@montague

This comment has been minimized.

Show comment
Hide comment
@montague

montague Mar 18, 2012

sounds reasonable to me. it looks like you've jumped on this issue with #367, although https://github.com/rack/rack/blob/c72121aa0fcaebc5e38365f50ea1f9be52cc3a06/test/spec_mime.rb#L10-13 looks like we are still returning 'application/octet-stream' for unknown. was your intention to return no content-type? or should i adjust this pull request to have Rack::Mime#mime_type return nil for unknown extensions?

sounds reasonable to me. it looks like you've jumped on this issue with #367, although https://github.com/rack/rack/blob/c72121aa0fcaebc5e38365f50ea1f9be52cc3a06/test/spec_mime.rb#L10-13 looks like we are still returning 'application/octet-stream' for unknown. was your intention to return no content-type? or should i adjust this pull request to have Rack::Mime#mime_type return nil for unknown extensions?

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 18, 2012

Member

I'm considering making it return nil by default, and let apps select it.

Member

raggi commented Mar 18, 2012

I'm considering making it return nil by default, and let apps select it.

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 18, 2012

Member

This will be a 1.5 change, not a 1.4 patch level

Member

raggi commented Mar 18, 2012

This will be a 1.5 change, not a 1.4 patch level

@montague

This comment has been minimized.

Show comment
Hide comment
@montague

montague Mar 18, 2012

I'm considering making it return nil by default, and let apps select it.
i think that's the right way to go.

This will be a 1.5 change, not a 1.4 patch level
i take this to mean that #367 has the fixes necessary and you will handle getting them into the 1.5 release if they are accepted?

i want to make sure i don't drop the ball on this if there's more i can do on my end.

I'm considering making it return nil by default, and let apps select it.
i think that's the right way to go.

This will be a 1.5 change, not a 1.4 patch level
i take this to mean that #367 has the fixes necessary and you will handle getting them into the 1.5 release if they are accepted?

i want to make sure i don't drop the ball on this if there's more i can do on my end.

raggi added a commit that referenced this pull request Dec 30, 2012

Correct some of the mime type issues. References #316 and #366.
HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD). They also have text describing how clients should handle this header being missing.
@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Dec 30, 2012

Member

@montague I'm planning on merging #367, does that do everything you need? If not, feel free to reopen this one (I'm cleaning tickets, so pre-emptively closing this for now).

Thanks!

Member

raggi commented Dec 30, 2012

@montague I'm planning on merging #367, does that do everything you need? If not, feel free to reopen this one (I'm cleaning tickets, so pre-emptively closing this for now).

Thanks!

@raggi raggi closed this Dec 30, 2012

pjambet added a commit to harrystech/rack that referenced this pull request Feb 25, 2015

Correct some of the mime type issues. References #316 and #366.
HTTP 1.0 and 1.1 do not have MUST for Content-Type requirements, they have "should" (not SHOULD). They also have text describing how clients should handle this header being missing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment