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

Introduce `:plain`, `:html`, and `:body` render options. #14062

Merged
merged 10 commits into from Feb 18, 2014

Conversation

Projects
None yet
7 participants
@sikachu
Member

sikachu commented Feb 14, 2014

This is a continuation from #12374, with slight modification since the discussion went a bit off-topic.


Per discussion, render :text misdirect people to think that it would render content with text/plain MIME type. However, render :text actually sets the response body directly, and inherits the default response MIME type, which is text/html.

In order to reduce confusion, we're introducing 3 more render format to render:

render html: '<strong>HTML String</strong>'.html_safe # render with `text/html` MIME type.

render plain: 'plain text' # render with `text/plain` MIME type.

render body: 'raw body' # render raw content, does not set content type.

We want to phrase out the usage of render :text, to reduce the confusion in the future. There were some discussion about deprecate render :text, but we haven't come into a conclusion yet. So, we'll consider doing that before the next major release.

Fixes #12374

@sikachu sikachu added this to the 4.1.0 milestone Feb 14, 2014

@sikachu sikachu self-assigned this Feb 14, 2014

@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
@rafaelfranca

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md Outdated
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md Outdated
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
@carlosantoniodasilva

View changes

Show outdated Hide outdated actionpack/lib/action_controller/metal/rendering.rb Outdated
if options[:body]
self.no_content_type = true
end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 14, 2014

Member

Why would the view mess up with the response content type? =(

@carlosantoniodasilva

carlosantoniodasilva Feb 14, 2014

Member

Why would the view mess up with the response content type? =(

This comment has been minimized.

@sikachu

sikachu Feb 18, 2014

Member

I have a weird feeling about this too, but I don't see another places where we can tell the response that it should skip setting the content type, as in other places we already lose context on which option has been passed to render.

@sikachu

sikachu Feb 18, 2014

Member

I have a weird feeling about this too, but I don't see another places where we can tell the response that it should skip setting the content type, as in other places we already lose context on which option has been passed to render.

This comment has been minimized.

@sikachu

sikachu Feb 18, 2014

Member

Any suggestion on where I should put this?

@sikachu

sikachu Feb 18, 2014

Member

Any suggestion on where I should put this?

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 18, 2014

Member

It just looks like render :body shouldn't be something Action View is aware of, it seems a controller responsibility only?

@carlosantoniodasilva

carlosantoniodasilva Feb 18, 2014

Member

It just looks like render :body shouldn't be something Action View is aware of, it seems a controller responsibility only?

This comment has been minimized.

@sikachu

sikachu Feb 18, 2014

Member

@carlosantoniodasilva my thought exactly! I was surprised as well that render :text is actually handled by AV and not AC when I started this patch.

However, unless you are using a minimal controller (Just inherits from AC::Metal and include rendering), AV is the one handling render :text. I think we can fix that in the future release, if we think that's not right.

@sikachu

sikachu Feb 18, 2014

Member

@carlosantoniodasilva my thought exactly! I was surprised as well that render :text is actually handled by AV and not AC when I started this patch.

However, unless you are using a minimal controller (Just inherits from AC::Metal and include rendering), AV is the one handling render :text. I think we can fix that in the future release, if we think that's not right.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Feb 18, 2014

Member

Yeah, awkward indeed. Anyway nothing comes to my mind right now so I guess that's ok.

@carlosantoniodasilva

carlosantoniodasilva Feb 18, 2014

Member

Yeah, awkward indeed. Anyway nothing comes to my mind right now so I guess that's ok.

@carlosantoniodasilva

View changes

Show outdated Hide outdated actionview/test/template/render_test.rb Outdated

@robin850 robin850 removed the attached PR label Feb 15, 2014

@robin850

View changes

Show outdated Hide outdated guides/source/layouts_and_rendering.md Outdated
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 18, 2014

Member

@sikachu, do you expect to be able to wrap this up today or should we delay it to 4.2.0?

Member

dhh commented Feb 18, 2014

@sikachu, do you expect to be able to wrap this up today or should we delay it to 4.2.0?

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Feb 18, 2014

Member

I can respond to reviews this morning! Sorry for the delay.

On Tue, Feb 18, 2014 at 5:10 AM, David Heinemeier Hansson
notifications@github.com wrote:

@sikachu, do you expect to be able to wrap this up today or should we delay it to 4.2.0?

Reply to this email directly or view it on GitHub:
#14062 (comment)

Member

sikachu commented Feb 18, 2014

I can respond to reviews this morning! Sorry for the delay.

On Tue, Feb 18, 2014 at 5:10 AM, David Heinemeier Hansson
notifications@github.com wrote:

@sikachu, do you expect to be able to wrap this up today or should we delay it to 4.2.0?

Reply to this email directly or view it on GitHub:
#14062 (comment)

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Feb 18, 2014

Member

I've addressed all of the comments so far. This should be ready to go.

Member

sikachu commented Feb 18, 2014

I've addressed all of the comments so far. This should be ready to go.

@rafaelfranca

View changes

Show outdated Hide outdated guides/source/layouts_and_rendering.md Outdated

sikachu added some commits Jan 31, 2014

Introduce `render :body` for render raw content
This is an option for sending a raw content back to browser. Note that
this rendering option will unset the default content type and does not
include "Content-Type" header back in the response.

You should only use this option if you are expecting the "Content-Type"
header to not be set. More information on "Content-Type" header can be
found on RFC 2616, section 7.2.1.

Please see #12374 for more detail.
Introduce `render :plain` for render plain text
This is as an option to render content with a content type of
`text/plain`. This is the preferred option if you are planning to render
a plain text content.

Please see #12374 for more detail.
Introduce `render :html` for render HTML string
This is an option for to HTML content with a content type of
`text/html`. This rendering option calls `ERB::Util.html_escape`
internally to escape unsafe HTML string, so you will have to mark your
string as html safe if you have any HTML tag in it.

Please see #12374 for more detail.
Fix a fragile test on `action_view/render`
This test were assuming that the list of render options will always be
the same. Fixing that so this doesn't break when we add/remove render
option in the future.
Update guides for new rendering options
* Introduces `:plain`, `:html`, `:body` render option.
* Update guide to use `render :plain` instead of `render :text`.

sikachu added some commits Feb 14, 2014

Add `#no_content_type` attribute to `AD::Response`
Setting this attribute to `true` will remove the content type header
from the request. This is use in `render :body` feature.

rafaelfranca added a commit that referenced this pull request Feb 18, 2014

Merge pull request #14062 from sikachu/ps-render-format
Introduce `:plain`, `:html`, and `:body` render options.

@rafaelfranca rafaelfranca merged commit acc0e63 into rails:master Feb 18, 2014

@sikachu sikachu deleted the sikachu:ps-render-format branch Feb 18, 2014

robertomiranda added a commit to robertomiranda/rails that referenced this pull request Feb 18, 2014

tenderlove added a commit that referenced this pull request Feb 20, 2014

Merge branch 'master' into jobs
* master: (1455 commits)
  change 'assert !' to 'assert_not' in guides [ci skip]
  Pointing to latest guides [ci skip]
  Methods silence_stream/quietly are not thread-safe [skip ci]
  [ci skip] Close the meta tag with '/>' instead of '>'
  Fix render plain docs example in AM::Base
  Update Docs in favor to use render plain instead of text option ref #14062
  Typo fix for unscope
  Use the reference for the mime type to get the format
  Preparing for 4.1.0.beta2 release
  Correctly escape PostgreSQL arrays.
  Escape format, negative_format and units options of number helpers
  Sync 4.1 release notes with changes since 7f648bc [ci skip]
  Update upgrading guide regarding `render :text`
  Add `#no_content_type` attribute to `AD::Response`
  Add missing CHANGELOG entry to Action View
  Update guides for new rendering options
  Cleanup `ActionController::Rendering`
  Fix a fragile test on `action_view/render`
  Introduce `render :html` for render HTML string
  Introduce `render :plain` for render plain text
  ...

Conflicts:
	actionmailer/lib/action_mailer/railtie.rb
	railties/lib/rails/application.rb
	railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt
	railties/lib/rails/generators/rails/app/templates/config/environments/test.rb.tt
@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Feb 28, 2014

Member

Why are we mutating the header hash here? It didn't used to do that.

If you're using a streaming response, this will break because the header hash is frozen (since it has already been sent to the client).

Also, what if someone specifically set a content type in their response? Do we actually want to delete it? For example:

class MainController < ApplicationController
  def index
    response.headers['Content-Type'] = 'application/json'
    render body: JSON.dump([1,2,3,4])
  end
end

I know we have a render json thing, but this is just an example. Are we sure unconditionally rm'ing the content type header is The Right Thing™?

/cc @jeremy

Why are we mutating the header hash here? It didn't used to do that.

If you're using a streaming response, this will break because the header hash is frozen (since it has already been sent to the client).

Also, what if someone specifically set a content type in their response? Do we actually want to delete it? For example:

class MainController < ApplicationController
  def index
    response.headers['Content-Type'] = 'application/json'
    render body: JSON.dump([1,2,3,4])
  end
end

I know we have a render json thing, but this is just an example. Are we sure unconditionally rm'ing the content type header is The Right Thing™?

/cc @jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Mar 1, 2014

Member

👍 we want to not set the default content type, but we don't want to erase the content type you provided—that's precisely why people would be using render body: ... 😁

Member

jeremy replied Mar 1, 2014

👍 we want to not set the default content type, but we don't want to erase the content type you provided—that's precisely why people would be using render body: ... 😁

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Mar 1, 2014

Member

Yep, this patch is going too far. Going to revert this then 😅.

See #14238 for more detail.

Member

sikachu replied Mar 1, 2014

Yep, this patch is going too far. Going to revert this then 😅.

See #14238 for more detail.

homu added a commit to rubygems/rubygems.org that referenced this pull request Dec 26, 2016

Auto merge of #1524 - sonalkr132:render_plain, r=segiddins
Use render plain: instead of text:

This is in preparation of rails 5 update. Chips away: e0a5d2e ( minus env deprecation).

`render plain: ` was introduced in rails 4.1. rails/rails#14062

Edouard-chin added a commit to Shopify/cacheable that referenced this pull request Jul 19, 2017

Update `actionpack` dependency to be `>= 4.1`:
- `render plain:` was introduced in Rails 4.1 rails/rails#14062
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment