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.

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
 

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
 

actionpack/lib/action_view/helpers/cache_helper.rb

@@ -32,7 +32,7 @@ module ActionView
       #      <i>Topics listed alphabetically</i>
       #    <% end %>
       def cache(name = {}, options = nil, &block)
-        controller.fragment_for(output_buffer, name, options, &block)
+        controller.fragment_for(name, options, &block)
       end
     end
   end

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 <tt>yield</tt>.
-      # 
+      #
       # ==== 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 %>
       #     <li><%= link_to 'Login', :action => 'login' %></li>
       #   <% 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

actionpack/lib/action_view/helpers/deprecated_block_helpers.rb

@@ -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

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 =~ /(.*)=$/

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

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

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 "  <p>Builder</p>\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

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 %>

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 -%>

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 -%>
+<%= render :layout => 'layouts/column' do -%>content<% end -%>

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 %>
+<%= render(:layout => "layout_for_partial", :locals => { :name => "David" }) do %>Inside from block<% end %>

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 "<div>Hello world</div>", render_content("content_tag :div", "Hello world")
+      maybe_deprecated { assert_equal "<div>Hello world</div>", render_content("content_tag :div", "Hello world") }
     end
 
     test "percent equals works for javascript_tag" do
       expected_output = "<script type=\"text/javascript\">\n//<![CDATA[\nalert('Hello')\n//]]>\n</script>"
-      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 = "<script id=\"the_js_tag\" type=\"text/javascript\">\n//<![CDATA[\nalert('Hello')\n//]]>\n</script>"
-      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 = "<form action=\"foo\" method=\"post\">hello</form>"
-      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 = "<fieldset><legend>foo</legend>hello</fieldset>"
-      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

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>title</title>\n\n\n<div id="column">column</div>\n<div id="content">content</div>\n),
+        @view.render(:file => "test/deprecated_nested_layout.erb", :layout => "layouts/yield")
+    end
+  end
+
   def test_render_with_nested_layout
     assert_equal %(<title>title</title>\n\n\n<div id="column">column</div>\n<div id="content">content</div>\n),
       @view.render(:file => "test/nested_layout.erb", :layout => "layouts/yield")