From 9de83050d3a4b260d4aeb5d09ec4eb64f913ba64 Mon Sep 17 00:00:00 2001 From: Carlhuda Date: Mon, 15 Mar 2010 11:58:05 -0700 Subject: [PATCH] Add deprecation notices for <% %>. * The approach is to compile <% %> into a method call that checks whether the value returned from a block is a String. If it is, it concats to the buffer and prints a deprecation warning. * <%= %> uses exactly the same logic to compile the template, which first checks to see whether it's compiling a block. * This should have no impact on other uses of block in templates. For instance, in <% [1,2,3].each do |i| %><%= i %><% end %>, the call to each returns an Array, not a String, so the result is not concatenated * In two cases (#capture and #cache), a String can be returned that should *never* be concatenated. We have temporarily created a String subclass called NonConcattingString which behaves (and is serialized) identically to String, but is not concatenated by the code that handles deprecated <% %> block helpers. Once we remove support for <% %> block helpers, we can remove NonConcattingString. --- .../action_controller/caching/fragments.rb | 16 ++++-- actionpack/lib/action_view/base.rb | 3 ++ .../lib/action_view/helpers/cache_helper.rb | 2 +- .../lib/action_view/helpers/capture_helper.rb | 28 +++++----- .../helpers/deprecated_block_helpers.rb | 52 ------------------- .../action_view/helpers/prototype_helper.rb | 4 ++ .../lib/action_view/render/rendering.rb | 3 +- .../lib/action_view/template/handlers/erb.rb | 19 ++++++- actionpack/test/controller/caching_test.rb | 15 +----- .../fixtures/layouts/block_with_layout.erb | 4 +- .../test/deprecated_nested_layout.erb | 3 ++ .../test/fixtures/test/nested_layout.erb | 2 +- .../test/using_layout_around_block.html.erb | 2 +- .../test/template/erb/tag_helper_test.rb | 24 ++++++--- actionpack/test/template/render_test.rb | 8 +++ 15 files changed, 88 insertions(+), 97 deletions(-) delete mode 100644 actionpack/lib/action_view/helpers/deprecated_block_helpers.rb create mode 100644 actionpack/test/fixtures/test/deprecated_nested_layout.erb diff --git a/actionpack/lib/action_controller/caching/fragments.rb b/actionpack/lib/action_controller/caching/fragments.rb index 89787727bd..a07fe2b255 100644 --- a/actionpack/lib/action_controller/caching/fragments.rb +++ b/actionpack/lib/action_controller/caching/fragments.rb @@ -34,17 +34,23 @@ module ActionController #:nodoc: ActiveSupport::Cache.expand_cache_key(key.is_a?(Hash) ? url_for(key).split("://").last : key, :views) end - def fragment_for(buffer, name = {}, options = nil, &block) #:nodoc: + def fragment_for(name = {}, options = nil, &block) #:nodoc: if perform_caching if fragment_exist?(name, options) - buffer.safe_concat(read_fragment(name, options)) + read_fragment(name, options) else + # VIEW TODO: Make #capture usable outside of ERB + # This dance is needed because Builder can't use capture + buffer = view_context.output_buffer pos = buffer.length - block.call - write_fragment(name, buffer[pos..-1], options) + yield + fragment = buffer[pos..-1] + write_fragment(name, fragment, options) + ActionView::NonConcattingString.new(fragment) end else - block.call + ret = yield + ActiveSupport::SafeBuffer.new(ret) if ret.is_a?(String) end end diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index feaf45c333..3920d8593f 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -3,6 +3,9 @@ require 'active_support/core_ext/module/delegation' require 'active_support/core_ext/class/attribute' module ActionView #:nodoc: + class NonConcattingString < ActiveSupport::SafeBuffer + end + class ActionViewError < StandardError #:nodoc: end diff --git a/actionpack/lib/action_view/helpers/cache_helper.rb b/actionpack/lib/action_view/helpers/cache_helper.rb index d5cc14b29a..3729d7daa8 100644 --- a/actionpack/lib/action_view/helpers/cache_helper.rb +++ b/actionpack/lib/action_view/helpers/cache_helper.rb @@ -32,7 +32,7 @@ module ActionView # Topics listed alphabetically # <% end %> def cache(name = {}, options = nil, &block) - controller.fragment_for(output_buffer, name, options, &block) + controller.fragment_for(name, options, &block) end end end diff --git a/actionpack/lib/action_view/helpers/capture_helper.rb b/actionpack/lib/action_view/helpers/capture_helper.rb index 03c7ba5a87..b64dc1533f 100644 --- a/actionpack/lib/action_view/helpers/capture_helper.rb +++ b/actionpack/lib/action_view/helpers/capture_helper.rb @@ -2,22 +2,22 @@ module ActionView module Helpers # CaptureHelper exposes methods to let you extract generated markup which # can be used in other parts of a template or layout file. - # It provides a method to capture blocks into variables through capture and + # It provides a method to capture blocks into variables through capture and # a way to capture a block of markup for use in a layout through content_for. module CaptureHelper - # The capture method allows you to extract part of a template into a - # variable. You can then use this variable anywhere in your templates or layout. - # + # The capture method allows you to extract part of a template into a + # variable. You can then use this variable anywhere in your templates or layout. + # # ==== Examples # The capture method can be used in ERb templates... - # + # # <% @greeting = capture do %> # Welcome to my shiny new web page! The date and time is # <%= Time.now %> # <% end %> # # ...and Builder (RXML) templates. - # + # # @timestamp = capture do # "The current timestamp is #{Time.now}." # end @@ -33,15 +33,17 @@ module ActionView def capture(*args) value = nil buffer = with_output_buffer { value = yield *args } - buffer.presence || value + if string = buffer.presence || value and string.is_a?(String) + NonConcattingString.new(string) + end end # Calling content_for stores a block of markup in an identifier for later use. # You can make subsequent calls to the stored content in other templates or the layout # by passing the identifier as an argument to yield. - # + # # ==== Examples - # + # # <% content_for :not_authorized do %> # alert('You are not authorized to do that!') # <% end %> @@ -92,7 +94,7 @@ module ActionView # <% end %> # # <%# Add some other content, or use a different template: %> - # + # # <% content_for :navigation do %> #
  • <%= link_to 'Login', :action => 'login' %>
    • # <% end %> @@ -109,13 +111,13 @@ module ActionView # for elements that will be fragment cached. def content_for(name, content = nil, &block) content = capture(&block) if block_given? - return @_content_for[name] << content if content - @_content_for[name] + @_content_for[name] << content if content + @_content_for[name] unless content end # content_for? simply checks whether any content has been captured yet using content_for # Useful to render parts of your layout differently based on what is in your views. - # + # # ==== Examples # # Perhaps you will use different css in you layout if no content_for :right_column diff --git a/actionpack/lib/action_view/helpers/deprecated_block_helpers.rb b/actionpack/lib/action_view/helpers/deprecated_block_helpers.rb deleted file mode 100644 index 3d0657e873..0000000000 --- a/actionpack/lib/action_view/helpers/deprecated_block_helpers.rb +++ /dev/null @@ -1,52 +0,0 @@ -module ActionView - module Helpers - module DeprecatedBlockHelpers - extend ActiveSupport::Concern - - include ActionView::Helpers::TagHelper - include ActionView::Helpers::TextHelper - include ActionView::Helpers::JavaScriptHelper - include ActionView::Helpers::FormHelper - - def content_tag(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - def javascript_tag(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - def form_for(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - def form_tag(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - def fields_for(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - def field_set_tag(*, &block) - block_called_from_erb?(block) ? safe_concat(super) : super - end - - BLOCK_CALLED_FROM_ERB = 'defined? __in_erb_template' - - if RUBY_VERSION < '1.9.0' - # Check whether we're called from an erb template. - # We'd return a string in any other case, but erb <%= ... %> - # can't take an <% end %> later on, so we have to use <% ... %> - # and implicitly concat. - def block_called_from_erb?(block) - block && eval(BLOCK_CALLED_FROM_ERB, block) - end - else - def block_called_from_erb?(block) - block && eval(BLOCK_CALLED_FROM_ERB, block.binding) - end - end - end - end -end \ No newline at end of file diff --git a/actionpack/lib/action_view/helpers/prototype_helper.rb b/actionpack/lib/action_view/helpers/prototype_helper.rb index e58fdf81fb..e46ca53275 100644 --- a/actionpack/lib/action_view/helpers/prototype_helper.rb +++ b/actionpack/lib/action_view/helpers/prototype_helper.rb @@ -689,6 +689,10 @@ module ActionView @generator << root if root end + def is_a?(klass) + klass == JavaScriptProxy + end + private def method_missing(method, *arguments, &block) if method.to_s =~ /(.*)=$/ diff --git a/actionpack/lib/action_view/render/rendering.rb b/actionpack/lib/action_view/render/rendering.rb index 310efe40e2..d8dfd5077b 100644 --- a/actionpack/lib/action_view/render/rendering.rb +++ b/actionpack/lib/action_view/render/rendering.rb @@ -16,8 +16,7 @@ module ActionView case options when Hash if block_given? - content = _render_partial(options.merge(:partial => options[:layout]), &block) - safe_concat(content) + _render_partial(options.merge(:partial => options[:layout]), &block) elsif options.key?(:partial) _render_partial(options) else diff --git a/actionpack/lib/action_view/template/handlers/erb.rb b/actionpack/lib/action_view/template/handlers/erb.rb index ac5902cc0e..705c2bf82e 100644 --- a/actionpack/lib/action_view/template/handlers/erb.rb +++ b/actionpack/lib/action_view/template/handlers/erb.rb @@ -8,6 +8,13 @@ module ActionView super(value.to_s) end alias :append= :<< + + def append_if_string=(value) + if value.is_a?(String) && !value.is_a?(NonConcattingString) + ActiveSupport::Deprecation.warn("<% %> style block helpers are deprecated. Please use <%= %>", caller) + self << value + end + end end module Template::Handlers @@ -21,14 +28,24 @@ module ActionView src << "@output_buffer.safe_concat('" << escape_text(text) << "');" end + BLOCK_EXPR = /(do|\{)(\s*\|[^|]*\|)?\s*\Z/ + def add_expr_literal(src, code) - if code =~ /(do|\{)(\s*\|[^|]*\|)?\s*\Z/ + if code =~ BLOCK_EXPR src << '@output_buffer.append= ' << code else src << '@output_buffer.append= (' << code << ');' end end + def add_stmt(src, code) + if code =~ BLOCK_EXPR + src << '@output_buffer.append_if_string= ' << code + else + super + end + end + def add_expr_escaped(src, code) src << '@output_buffer.append= ' << escaped_expr(code) << ';' end diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index a792752ef4..432433f976 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -617,7 +617,7 @@ class FragmentCachingTest < ActionController::TestCase fragment_computed = false buffer = 'generated till now -> '.html_safe - @controller.fragment_for(buffer, 'expensive') { fragment_computed = true } + buffer << @controller.fragment_for('expensive') { fragment_computed = true } assert fragment_computed assert_equal 'generated till now -> ', buffer @@ -628,7 +628,7 @@ class FragmentCachingTest < ActionController::TestCase fragment_computed = false buffer = 'generated till now -> '.html_safe - @controller.fragment_for(buffer, 'expensive') { fragment_computed = true } + buffer << @controller.fragment_for('expensive') { fragment_computed = true } assert !fragment_computed assert_equal 'generated till now -> fragment content', buffer @@ -742,15 +742,4 @@ CACHED assert_equal "

    Builder

    \n", @store.read('views/test.host/functional_caching/formatted_fragment_cached') end - - def test_js_formatted_fragment_caching - get :formatted_fragment_cached, :format => "js" - assert_response :success - expected_body = %(title = "Hey";\n$("element_1").visualEffect("highlight");\n) + - %($("element_2").visualEffect("highlight");\nfooter = "Bye";) - assert_equal expected_body, @response.body - - assert_equal ['$("element_1").visualEffect("highlight");', '$("element_2").visualEffect("highlight");'], - @store.read('views/test.host/functional_caching/formatted_fragment_cached') - end end diff --git a/actionpack/test/fixtures/layouts/block_with_layout.erb b/actionpack/test/fixtures/layouts/block_with_layout.erb index f25b41271d..73ac833e52 100644 --- a/actionpack/test/fixtures/layouts/block_with_layout.erb +++ b/actionpack/test/fixtures/layouts/block_with_layout.erb @@ -1,3 +1,3 @@ -<% render(:layout => "layout_for_partial", :locals => { :name => "Anthony" }) do %>Inside from first block in layout<% "Return value should be discarded" %><% end %> +<%= render(:layout => "layout_for_partial", :locals => { :name => "Anthony" }) do %>Inside from first block in layout<% "Return value should be discarded" %><% end %> <%= yield %> -<% render(:layout => "layout_for_partial", :locals => { :name => "Ramm" }) do %>Inside from second block in layout<% end %> +<%= render(:layout => "layout_for_partial", :locals => { :name => "Ramm" }) do %>Inside from second block in layout<% end %> diff --git a/actionpack/test/fixtures/test/deprecated_nested_layout.erb b/actionpack/test/fixtures/test/deprecated_nested_layout.erb new file mode 100644 index 0000000000..7b6dcbb6c7 --- /dev/null +++ b/actionpack/test/fixtures/test/deprecated_nested_layout.erb @@ -0,0 +1,3 @@ +<% content_for :title, "title" -%> +<% content_for :column do -%>column<% end -%> +<% render :layout => 'layouts/column' do -%>content<% end -%> \ No newline at end of file diff --git a/actionpack/test/fixtures/test/nested_layout.erb b/actionpack/test/fixtures/test/nested_layout.erb index 7b6dcbb6c7..6078f74b4c 100644 --- a/actionpack/test/fixtures/test/nested_layout.erb +++ b/actionpack/test/fixtures/test/nested_layout.erb @@ -1,3 +1,3 @@ <% content_for :title, "title" -%> <% content_for :column do -%>column<% end -%> -<% render :layout => 'layouts/column' do -%>content<% end -%> \ No newline at end of file +<%= render :layout => 'layouts/column' do -%>content<% end -%> \ No newline at end of file diff --git a/actionpack/test/fixtures/test/using_layout_around_block.html.erb b/actionpack/test/fixtures/test/using_layout_around_block.html.erb index a93fa02c9c..3d6661df9a 100644 --- a/actionpack/test/fixtures/test/using_layout_around_block.html.erb +++ b/actionpack/test/fixtures/test/using_layout_around_block.html.erb @@ -1 +1 @@ -<% render(:layout => "layout_for_partial", :locals => { :name => "David" }) do %>Inside from block<% end %> \ No newline at end of file +<%= render(:layout => "layout_for_partial", :locals => { :name => "David" }) do %>Inside from block<% end %> \ No newline at end of file diff --git a/actionpack/test/template/erb/tag_helper_test.rb b/actionpack/test/template/erb/tag_helper_test.rb index cc96a43901..b5b7ae87de 100644 --- a/actionpack/test/template/erb/tag_helper_test.rb +++ b/actionpack/test/template/erb/tag_helper_test.rb @@ -20,7 +20,7 @@ module ERBTest end class DeprecatedViewContext < ViewContext - include ActionView::Helpers::DeprecatedBlockHelpers + # include ActionView::Helpers::DeprecatedBlockHelpers end module SharedTagHelpers @@ -31,28 +31,36 @@ module ERBTest ActionView::Template::Handlers::Erubis.new(template).evaluate(context.new) end + def maybe_deprecated + if @deprecated + assert_deprecated { yield } + else + yield + end + end + test "percent equals works for content_tag and does not require parenthesis on method call" do - assert_equal "
    Hello world
    ", render_content("content_tag :div", "Hello world") + maybe_deprecated { assert_equal "
    Hello world
    ", render_content("content_tag :div", "Hello world") } end test "percent equals works for javascript_tag" do expected_output = "" - assert_equal expected_output, render_content("javascript_tag", "alert('Hello')") + maybe_deprecated { assert_equal expected_output, render_content("javascript_tag", "alert('Hello')") } end test "percent equals works for javascript_tag with options" do expected_output = "" - assert_equal expected_output, render_content("javascript_tag(:id => 'the_js_tag')", "alert('Hello')") + maybe_deprecated { assert_equal expected_output, render_content("javascript_tag(:id => 'the_js_tag')", "alert('Hello')") } end test "percent equals works with form tags" do expected_output = "
    hello
    " - assert_equal expected_output, render_content("form_tag('foo')", "<%= 'hello' %>") + maybe_deprecated { assert_equal expected_output, render_content("form_tag('foo')", "<%= 'hello' %>") } end test "percent equals works with fieldset tags" do expected_output = "
    foohello
    " - assert_equal expected_output, render_content("field_set_tag('foo')", "<%= 'hello' %>") + maybe_deprecated { assert_equal expected_output, render_content("field_set_tag('foo')", "<%= 'hello' %>") } end end @@ -77,6 +85,10 @@ module ERBTest "<% __in_erb_template=true %><% #{str} do %>#{rest}<% end %>" end + def setup + @deprecated = true + end + include SharedTagHelpers end end \ No newline at end of file diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index cea8ab1bce..e66202e8fe 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -228,6 +228,14 @@ module RenderTestCases @view.render(:file => "test/hello_world.erb", :layout => "layouts/yield") end + # TODO: Move to deprecated_tests.rb + def test_render_with_nested_layout_deprecated + assert_deprecated do + assert_equal %(title\n\n\n
    column
    \n
    content
    \n), + @view.render(:file => "test/deprecated_nested_layout.erb", :layout => "layouts/yield") + end + end + def test_render_with_nested_layout assert_equal %(title\n\n\n
    column
    \n
    content
    \n), @view.render(:file => "test/nested_layout.erb", :layout => "layouts/yield")