Use ActiveSupport::SafeBuffer when flushing content_for

[yoongkang]

This addresses issue #19890.

Previously, when content_for is flushed, the content
was replaced directly by a new value in
ActionView::OutputFlow#set. The problem is this new
value passed to the method may not be an instance of
ActiveSupport::SafeBuffer.

This change forces the value to be set to a new
instance of ActiveSupport::SafeBuffer.

[derekprior]

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?

[yoongkang]

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.

[yoongkang]

Is there any other feedback for this? Not trying to be pushy (I understand the team is busy), but just thought it'd be a fairly straightforward patch.

[yoongkang]

Rebased. Btw can the ActiveSupport label be added at least? Thanks. :)

[maclover7]

Can you please rebase?

[rafaelfranca]

Backported in dd750a3