Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix fragment caching regression from 03d01ec #2150

Closed
wants to merge 5 commits into from

6 participants

@kommen

Alias << as append= for ActiveSupport::SafeBuffer to fix "undefined method `append='" error with fragment cache helper on cache miss introduced with 03d01ec.

Since 03d01ec fragment_for sets theoutput_buffer to a SafeBuffer. ERB expects append= to be defined on the output_buffer.
This means subsequent ERB tags after a call to the fragment cache helper and a cache miss would cause an "undefined method `append='" error.

Includes updated testcase.
This should be cherry-picked into master as well.

@kommen kommen Alias << as append= for ActiveSupport::SafeBuffer to fix "undefined m…
…ethod `append='" error with fragment cache helper on cache miss introduced with 03d01ec.

Since 03d01ec fragment_for sets the output_buffer to a SafeBuffer. ERB expects append= to be defined on the output_buffer.
This means subsequent ERB tags would cause an "undefined method `append='" error.
9f7e775
@mk
mk commented

+1

@kommen

Of course, this could also be fixed by again using an OutputBuffer instead of a SafeBuffer. Any thoughts?

@kommen

Seems to be related to #2080

@josevalim
Owner
@josevalim
Owner
@kommen

@josevalim, #2080 is currently for 3-0-stable, and seems to be in conflict to 03d01ec, which is in 3-1-stable, so I guess incorporating my changes only makes sense if the changes from #2080 make it into master and 3-1-stable as well.

kommen and others added some commits
@kommen kommen Instead of adding append= to SafeBuffer, use the same class for the n…
…ew output_buffer which was used for the old one.
55beb17
@lhahne lhahne Added tests for the output_buffer returned by CacheHelper
The output_buffer returned by CacheHelper should be html_safe if the original buffer is html_safe.
b2a2035
@lhahne lhahne made sure that the possible new output_buffer created by CacheHelper …
…is of the same type as the original
bd6206b
@kommen kommen Remove tests to check if the old output_buffer is still html_safe. It…
… is replaced with an html_safe one anyway, if the old one was safe.
ccdbf49
@kommen

I've cherry-picked @lhahne's commits from #2080 to my branch and merged them with the changes from 03d01ec.

@fxn
Owner

@kommen could you please squash the commits somehow? Feel free to open a new pr if needed.

@kommen

@fxn: I squashed the commits and opened pull request #2219.

@spastorino
Owner

Closed in PR 2219

@spastorino spastorino closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 19, 2011
  1. @kommen

    Alias << as append= for ActiveSupport::SafeBuffer to fix "undefined m…

    kommen authored
    …ethod `append='" error with fragment cache helper on cache miss introduced with 03d01ec.
    
    Since 03d01ec fragment_for sets the output_buffer to a SafeBuffer. ERB expects append= to be defined on the output_buffer.
    This means subsequent ERB tags would cause an "undefined method `append='" error.
  2. @kommen

    Instead of adding append= to SafeBuffer, use the same class for the n…

    kommen authored
    …ew output_buffer which was used for the old one.
  3. @lhahne @kommen

    Added tests for the output_buffer returned by CacheHelper

    lhahne authored kommen committed
    The output_buffer returned by CacheHelper should be html_safe if the original buffer is html_safe.
  4. @lhahne @kommen

    made sure that the possible new output_buffer created by CacheHelper …

    lhahne authored kommen committed
    …is of the same type as the original
  5. @kommen

    Remove tests to check if the old output_buffer is still html_safe. It…

    kommen authored
    … is replaced with an html_safe one anyway, if the old one was safe.
This page is out of date. Refresh to see the latest.
View
2  actionpack/lib/action_view/helpers/cache_helper.rb
@@ -54,7 +54,7 @@ def fragment_for(name = {}, options = nil, &block) #:nodoc:
output_safe = output_buffer.html_safe?
fragment = output_buffer.slice!(pos..-1)
if output_safe
- self.output_buffer = output_buffer.html_safe
+ self.output_buffer = output_buffer.class.new(output_buffer)
end
controller.write_fragment(name, fragment, options)
end
View
49 actionpack/test/controller/caching_test.rb
@@ -742,6 +742,7 @@ def test_fragment_caching
expected_body = <<-CACHED
Hello
This bit's fragment cached
+Ciao
CACHED
assert_equal expected_body, @response.body
@@ -783,3 +784,51 @@ def test_xml_formatted_fragment_caching
assert_equal " <p>Builder</p>\n", @store.read('views/test.host/functional_caching/formatted_fragment_cached')
end
end
+
+class CacheHelperOutputBufferTest < ActionController::TestCase
+
+ class MockController
+ def read_fragment(name, options)
+ return false
+ end
+
+ def write_fragment(name, fragment, options)
+ fragment
+ end
+ end
+
+ def setup
+ super
+ end
+
+ def test_output_buffer
+ output_buffer = ActionView::OutputBuffer.new
+ controller = MockController.new
+ cache_helper = Object.new
+ cache_helper.extend(ActionView::Helpers::CacheHelper)
+ cache_helper.expects(:controller).returns(controller).at_least(0)
+ cache_helper.expects(:output_buffer).returns(output_buffer).at_least(0)
+ # if the output_buffer is changed, the new one should be html_safe and of the same type
+ cache_helper.expects(:output_buffer=).with(responds_with(:html_safe?, true)).with(instance_of(output_buffer.class)).at_least(0)
+
+ assert_nothing_raised do
+ cache_helper.send :fragment_for, 'Test fragment name', 'Test fragment', &Proc.new{ nil }
+ end
+ end
+
+ def test_safe_buffer
+ output_buffer = ActiveSupport::SafeBuffer.new
+ controller = MockController.new
+ cache_helper = Object.new
+ cache_helper.extend(ActionView::Helpers::CacheHelper)
+ cache_helper.expects(:controller).returns(controller).at_least(0)
+ cache_helper.expects(:output_buffer).returns(output_buffer).at_least(0)
+ # if the output_buffer is changed, the new one should be html_safe and of the same type
+ cache_helper.expects(:output_buffer=).with(responds_with(:html_safe?, true)).with(instance_of(output_buffer.class)).at_least(0)
+
+ assert_nothing_raised do
+ cache_helper.send :fragment_for, 'Test fragment name', 'Test fragment', &Proc.new{ nil }
+ end
+ end
+
+end
View
1  actionpack/test/fixtures/functional_caching/fragment_cached.html.erb
@@ -1,2 +1,3 @@
Hello
<%= cache do %>This bit's fragment cached<% end %>
+<%= 'Ciao' %>
Something went wrong with that request. Please try again.