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.

Sign up for GitHub

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

Jump to bottom

Use ActiveSupport::SafeBuffer when flushing content_for #20046

Merged
merged 1 commit into from
Jan 16, 2016
Merged

Use ActiveSupport::SafeBuffer when flushing content_for #20046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions actionview/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
* Create a new `ActiveSupport::SafeBuffer` instance when `content_for` is flushed.

Fixes #19890

*Yoong Kang Lim*

* Do not put partial name to `local_assigns` when rendering without
an object or a collection.

Expand Down
2 changes: 1 addition & 1 deletion actionview/lib/action_view/flows.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def get(key)

# Called by each renderer object to set the layout contents.
def set(key, value)
@content[key] = value
@content[key] = ActiveSupport::SafeBuffer.new(value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit to not knowing much about this code, so forgive me if the following questions are missing something...

It seems from the comment associated with this method that it might impact a lot more than content_for. Would the following method, append, which says it's called by content_for be more appropriate?

Also, would this make all content provided to content_for html safe? How do we know this is safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the intention would be to make all values in the content hash be instances of ActiveSupport::SafeBuffer, as implied by the constructor, which initializes all values of the content hash to be ActiveSupport::SafeBuffer instances by default:

def initialize
  @content = Hash.new { |h,k| h[k] = ActiveSupport::SafeBuffer.new }
end

Given this, it seems to me that the append method expects a new value to be appended to be an existing ActiveSupport::SafeBuffer object. A String object (not html_safe) appended to an ActiveSupport::SafeBuffer object will still result in an ActiveSupport::SafeBuffer object.

The set method as it currently stands is inconsistent with both the constructor and the append method. I believe this to be unintended, as it could wipe out the initial ActiveSupport::SafeBuffer object created in the constructor with a String.

end

# Called by content_for
Expand Down
13 changes: 13 additions & 0 deletions actionview/test/template/capture_helper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,19 @@ def test_content_for_question_mark
assert ! content_for?(:something_else)
end

def test_content_for_should_be_html_safe_after_flush_empty
assert ! content_for?(:title)
content_for :title do
content_tag(:p, 'title')
end
assert content_for(:title).html_safe?
content_for :title, "", flush: true
content_for(:title) do
content_tag(:p, 'title')
end
assert content_for(:title).html_safe?
end

def test_provide
assert !content_for?(:title)
provide :title, "hi"
Expand Down