Permalink
Browse files

Removed single space padding from empty response body.

`render nothing: true` or rendering a `nil` body no longer add a single
space to the response body.

The old behavior was added as a workaround for a bug in an early version of
Safari, where the HTTP headers are not returned correctly if the response
body has a 0-length. This is been fixed since and the workaround is no
longer necessary.

Use `render body: ' '` if the old behavior is desired.
  • Loading branch information...
1 parent 6e76031 commit 013c74d645a5842ce35857e4f14af7fc9961c54d @chancancode chancancode committed Jul 10, 2014
View
@@ -1,3 +1,17 @@
+* `render nothing: true` or rendering a `nil` body no longer add a single
+ space to the response body.
+
+ The old behavior was added as a workaround for a bug in an early version of
+ Safari, where the HTTP headers are not returned correctly if the response
+ body has a 0-length. This is been fixed since and the workaround is no
+ longer necessary.
+
+ Use `render body: ' '` if the old behavior is desired.
+
+ See #14883 for details.
+
+ *Godfrey Chan*
+
* Prepend a JS comment to JSONP callbacks. Addresses CVE-2014-4671
("Rosetta Flash")
@@ -67,8 +67,8 @@ def _normalize_options(options) #:nodoc:
options[:html] = ERB::Util.html_escape(options[:html])
end
- if options.delete(:nothing) || _any_render_format_is_nil?(options)
- options[:body] = " "
+ if options.delete(:nothing)
+ options[:body] = nil
end
if options[:status]
@@ -86,10 +86,6 @@ def _normalize_text(options)
end
end
- def _any_render_format_is_nil?(options)
- RENDER_FORMATS_IN_PRIORITY.any? { |format| options.key?(format) && options[format].nil? }
- end
-
# Process controller specific options, as status, content-type and location.
def _process_options(options) #:nodoc:
status, content_type, location = options.values_at(:status, :content_type, :location)
@@ -111,17 +111,17 @@ class RenderBodyTest < Rack::TestCase
assert_status 404
end
- test "rendering body with nil returns an empty body padded for Safari" do
+ test "rendering body with nil returns an empty body" do
get "/render_body/with_layout/with_nil"
- assert_body " "
+ assert_body ""
assert_status 200
end
- test "Rendering body with nil and custom status code returns an empty body padded for Safari and the status" do
+ test "Rendering body with nil and custom status code returns an empty body and the status" do
get "/render_body/with_layout/with_nil_and_status"
- assert_body " "
+ assert_body ""
assert_status 403
end
@@ -114,17 +114,17 @@ class RenderHtmlTest < Rack::TestCase
assert_status 404
end
- test "rendering text with nil returns an empty body padded for Safari" do
+ test "rendering text with nil returns an empty body" do
get "/render_html/with_layout/with_nil"
- assert_body " "
+ assert_body ""
assert_status 200
end
- test "Rendering text with nil and custom status code returns an empty body padded for Safari and the status" do
+ test "Rendering text with nil and custom status code returns an empty body and the status" do
get "/render_html/with_layout/with_nil_and_status"
- assert_body " "
+ assert_body ""
assert_status 403
end
@@ -106,17 +106,17 @@ class RenderPlainTest < Rack::TestCase
assert_status 404
end
- test "rendering text with nil returns an empty body padded for Safari" do
+ test "rendering text with nil returns an empty body" do
get "/render_plain/with_layout/with_nil"
- assert_body " "
+ assert_body ""
assert_status 200
end
- test "Rendering text with nil and custom status code returns an empty body padded for Safari and the status" do
+ test "Rendering text with nil and custom status code returns an empty body and the status" do
get "/render_plain/with_layout/with_nil_and_status"
- assert_body " "
+ assert_body ""
assert_status 403
end
@@ -106,17 +106,17 @@ class RenderTextTest < Rack::TestCase
assert_status 404
end
- test "rendering text with nil returns an empty body padded for Safari" do
+ test "rendering text with nil returns an empty body" do
get "/render_text/with_layout/with_nil"
- assert_body " "
+ assert_body ""
assert_status 200
end
- test "Rendering text with nil and custom status code returns an empty body padded for Safari and the status" do
+ test "Rendering text with nil and custom status code returns an empty body and the status" do
get "/render_text/with_layout/with_nil_and_status"
- assert_body " "
+ assert_body ""
assert_status 403
end

5 comments on commit 013c74d

Contributor

egilburg replied Jul 11, 2014

render nothing: true, layout: 'layouts/something' used to render the layout without template content, but now it renders nothing at all. Now we need to do render 'layouts/something', layout: false to get same behavior back, but it seems hacky as it treats the layout as a template.

Is this change intended?

Member

chancancode replied Jul 11, 2014

@egilburg hm, that's definitely not intended. the scope of this change is just to remove the legacy single space padding. Do you want to submit a patch with test cases to cover the regression? 😄

Member

chancancode replied Jul 11, 2014

@egilburg I reverted the commit on my machine and I couldn't reproduce the behaviour you mentioned. This new test cases passes (without this commit):

diff --git a/actionview/test/actionpack/controller/render_test.rb b/actionview/test/actionpack/controller/render_test.rb
index ab7b961..2caa1c4 100644
--- a/actionview/test/actionpack/controller/render_test.rb
+++ b/actionview/test/actionpack/controller/render_test.rb
@@ -363,6 +363,10 @@ class TestController < ApplicationController
     render :nothing => true
   end

+  def rendering_nothing_with_layout
+    render :nothing => true, :layout => "standard"
+  end
+
   def render_to_string_with_assigns
     @before = "i'm before the render"
     render_to_string :text => "foo"
@@ -1030,6 +1034,11 @@ class RenderTest < ActionController::TestCase
     assert_equal " ", @response.body
   end

+  def test_rendering_nothing_with_layout
+    get :rendering_nothing_with_layout
+    assert_equal " ", @response.body
+  end
+
   def test_render_to_string_doesnt_break_assigns
     get :render_to_string_with_assigns
     assert_equal "i'm before the render", assigns(:before)

Do you have a link to another test case or documentation about the behaviour you mentioned?

Contributor

dmitry replied Jul 11, 2014

Is there are any statistics, how many safari browsers still have an issue (in the world) and can be broken by this commit?

Member

chancancode replied Jul 11, 2014

So small that no one actually keeps track of it anymore :)

Our best guess is that this is either fixed in Safari 1.x or in 2.0. I asked for someone to let me try reproducing the bug if they still own one of those computers, but it appears that no one have access to them anymore. See #14883

Please sign in to comment.