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

Default content type for head is text/html #28937

Merged
merged 1 commit into from Jul 31, 2018

Conversation

@maclover7
Copy link
Member

@maclover7 maclover7 commented Apr 30, 2017

Summary

Otherwise an instance of Mime::NullType will be returned as the Content-Type header.

Other Information

Fixes #28927

Otherwise Mime::NullType will be returned as the `Content-Type` header.
@kaspth
Copy link
Member

@kaspth kaspth commented Apr 30, 2017

If format.any { head 200 } could be any format and we have no clue what, why should we assume text/html? Makes no sense to me.

Think it's just an oversight where Mime::NullType is not being a valid mime type instance substitute anymore.

What default content types could we make Mime::NullType stand in for — application/octet-stream, nothing at all? Or should head raise if there's no content type?

@maclover7
Copy link
Member Author

@maclover7 maclover7 commented May 1, 2017

@kaspth Was just throwing out text/html as an idea -- definitely thinking we should be returning a content type, just not sure which one :)

Does application/octet-stream sound ok to you?

@happycoloredbanana
Copy link

@happycoloredbanana happycoloredbanana commented May 2, 2017

Currently having an action method like:

def index
    head 404
end

returns text/html for unknown formats.

Wouldn't it make sense to have

def index
  respond_to do |format|
    format.any  { head 404 }
  end
end

also return the same content type since conceptually these two methods are the same?

@kaspth
Copy link
Member

@kaspth kaspth commented May 7, 2017

@maclover7 would you happen to know why head 404 defaults to text/html?

@happycoloredbanana all right, that makes sense then 👍

@maclover7
Copy link
Member Author

@maclover7 maclover7 commented May 7, 2017

@kaspth I believe it's because html is the first format listed here

@schneems
Copy link
Member

@schneems schneems commented Jul 31, 2018

What’s the hold up here? Seems fine.

@rafaelfranca rafaelfranca merged commit b66bf91 into rails:master Jul 31, 2018
2 checks passed
2 checks passed
@rails-bot
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants