Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

content-legth should be eq 0 when render nothing: true #14350

Closed
wants to merge 1 commit into from

6 participants

@arthurnn
Collaborator

Problem

When doing render nothing: true, it should render a body == ""

This how Rails behaves since 2009, because of a Safari bug.

As far as I can see the history, seems like this problem was introduced by this commit 53596d0 . Before that change if you only pass a nothing: true body would be nil.

[fixes #14336]

review @rafaelfranca @carlosantoniodasilva @sikachu

I am not sure about the implementation. looks dirty, but I couldnt think a better way. I am opened for suggestions.

If it is all good, I can add a CHANGELOG.

actionpack/lib/action_controller/metal/rendering.rb
@@ -67,9 +67,10 @@ 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] = " "
- end
+ body = options[:body]
+ body = nil if nothing = (options.delete(:nothing) == true)
+ body = " " if _any_render_format_is_nil?(options)
+ options[:body] = body if body || nothing

How about this?

if options.delete(:nothing)
  options[:body] = nil
elsif _any_render_format_is_nil?(options)
  options[:body] = " "
end
@arthurnn Collaborator

that wont work for the case:

render nothing: true, body: nil

which should add a body " "

And why you'd do such a thing? Do we have tests that cover that scenario?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sikachu
Collaborator

render :nothing => true adds a space since it was a workaround for bug in Safari. It has been that way since 2009. See cb0f8fd and 807df4f for the implementation. Unless Safari doesn't have that bug anymore, we could change it to return an empty string.

But that'd be for 5.0, not 4.2.

/cc @dhh and @jeremy, do you guys have any input on this?

@dhh
Owner
@arthurnn
Collaborator

Updated PR with @carlosantoniodasilva suggestion.
Indeed this is not a regression(I changed the PR description so people wont get confused).

Agree with @sikachu @dhh , This is an old behaviour. will be good, if we could fix it. Otherwise I am ok to close. For the time being I will leave it open until we are sure we cannot fix this behaviour.

@robin850 robin850 added the actionpack label
@jeremy
Owner

Moving over to #14883, verifies whether the old behavior is no longer required.

@jeremy jeremy closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 actionpack/lib/action_controller/metal/rendering.rb
@@ -67,7 +67,9 @@ def _normalize_options(options) #:nodoc:
options[:html] = ERB::Util.html_escape(options[:html])
end
- if options.delete(:nothing) || _any_render_format_is_nil?(options)
+ if options.delete(:nothing)
+ options[:body] = nil
+ elsif _any_render_format_is_nil?(options)
options[:body] = " "
end
View
6 actionpack/test/controller/action_pack_assertions_test.rb
@@ -260,6 +260,12 @@ def test_assert_redirected_to_top_level_named_route_with_same_controller_name_in
end
end
+ def test_nothing_content_length
+ @controller = Admin::InnerModuleController.new
+ get :index
+ assert_equal 0, response.body.length
+ end
+
def test_assert_redirect_failure_message_with_protocol_relative_url
begin
process :redirect_external_protocol_relative
View
3  actionview/test/actionpack/controller/render_test.rb
@@ -1022,7 +1022,7 @@ def test_layout_overriding_layout
def test_rendering_nothing_on_layout
get :rendering_nothing_on_layout
- assert_equal " ", @response.body
+ assert_equal "", @response.body
end
def test_render_to_string_doesnt_break_assigns
@@ -1332,4 +1332,3 @@ def test_render_call_to_partial_with_layout_in_main_layout_and_within_content_fo
assert_equal "Before (Anthony)\nInside from partial (Anthony)\nAfter\nBefore (David)\nInside from partial (David)\nAfter\nBefore (Ramm)\nInside from partial (Ramm)\nAfter", @response.body
end
end
-
Something went wrong with that request. Please try again.