Merge pull request #3884 from sikachu/master-fix_layout

Allow layout fallback when using `layout` method

actionpack/CHANGELOG.md

@@ -1,5 +1,15 @@
 ## Rails 3.2.0 (unreleased) ##
 
+*   Rails will now use your default layout (such as "layouts/application") when you specify a layout with `:only` and `:except` condition, and those conditions fail. *Prem Sichanugrist*
+
+    For example, consider this snippet:
+
+        class CarsController
+          layout 'single_car', :only => :show
+        end
+
+    Rails will use 'layouts/single_car' when a request comes in `:show` action, and use 'layouts/application' (or 'layouts/cars', if exists) when a request comes in for any other actions.
+
 *   form_for with +:as+ option uses "#{action}_#{as}" as css class and id:
 
     Before:

actionpack/lib/abstract_controller/layouts.rb

@@ -244,42 +244,51 @@ def _implied_layout_name
       def _write_layout_method
         remove_possible_method(:_layout)
 
-        case defined?(@_layout) ? @_layout : nil
-        when String
-          self.class_eval %{def _layout; #{@_layout.inspect} end}, __FILE__, __LINE__
-        when Symbol
-          self.class_eval <<-ruby_eval, __FILE__, __LINE__ + 1
-            def _layout
+        prefixes = _implied_layout_name =~ /\blayouts/ ? [] : ["layouts"]
+        layout_definition = case defined?(@_layout) ? @_layout : nil
+          when String
+            @_layout.inspect
+          when Symbol
+            <<-RUBY
               #{@_layout}.tap do |layout|
                 unless layout.is_a?(String) || !layout
                   raise ArgumentError, "Your layout method :#{@_layout} returned \#{layout}. It " \
                     "should have returned a String, false, or nil"
                 end
               end
-            end
-          ruby_eval
-        when Proc
-          define_method :_layout_from_proc, &@_layout
-          self.class_eval %{def _layout; _layout_from_proc(self) end}, __FILE__, __LINE__
-        when false
-          self.class_eval %{def _layout; end}, __FILE__, __LINE__
-        when true
-          raise ArgumentError, "Layouts must be specified as a String, Symbol, false, or nil"
-        when nil
-          if name
-            _prefixes = _implied_layout_name =~ /\blayouts/ ? [] : ["layouts"]
-
-            self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
-              def _layout
-                if template_exists?("#{_implied_layout_name}", #{_prefixes.inspect})
+            RUBY
+          when Proc
+            define_method :_layout_from_proc, &@_layout
+            "_layout_from_proc(self)"
+          when false
+            nil
+          when true
+            raise ArgumentError, "Layouts must be specified as a String, Symbol, false, or nil"
+          when nil
+            if name
+              <<-RUBY
+                if template_exists?("#{_implied_layout_name}", #{prefixes.inspect})
                   "#{_implied_layout_name}"
                 else
                   super
                 end
-              end
-            RUBY
+              RUBY
+            end
           end
-        end
+
+        self.class_eval <<-RUBY, __FILE__, __LINE__ + 1
+          def _layout
+            if action_has_layout?
+              #{layout_definition}
+            elsif self.class.name
+              if template_exists?("#{_implied_layout_name}", #{prefixes.inspect})
+                  "#{_implied_layout_name}"
+              else
+                super
+              end
+            end
+          end
+        RUBY
         self.class_eval { private :_layout }
       end
     end
@@ -337,7 +346,7 @@ def _layout_for_option(name)
     # * <tt>template</tt> - The template object for the default layout (or nil)
     def _default_layout(require_layout = false)
       begin
-        layout_name = _layout if action_has_layout?
+        layout_name = _layout
       rescue NameError => e
         raise e, "Could not render layout: #{e.message}"
       end

actionpack/lib/action_controller/test_case.rb

@@ -86,6 +86,21 @@ def assert_template(options = {}, message = nil)
           end
         end
       when Hash
+        if expected_layout = options[:layout]
+          msg = build_message(message,
+                  "expecting layout <?> but action rendered <?>",
+                  expected_layout, @layouts.keys)
+
+          case expected_layout
+          when String
+            assert(@layouts.keys.include?(expected_layout), msg)
+          when Regexp
+            assert(@layouts.keys.any? {|l| l =~ expected_layout }, msg)
+          when nil
+            assert(@layouts.empty?, msg)
+          end
+        end
+
         if expected_partial = options[:partial]
           if expected_locals = options[:locals]
             actual_locals = @locals[expected_partial.to_s.sub(/^_/,'')]
@@ -98,19 +113,6 @@ def assert_template(options = {}, message = nil)
                     "expecting ? to be rendered ? time(s) but rendered ? time(s)",
                      expected_partial, expected_count, actual_count)
             assert(actual_count == expected_count.to_i, msg)
-          elsif options.key?(:layout)
-            msg = build_message(message,
-                    "expecting layout <?> but action rendered <?>",
-                    expected_layout, @layouts.keys)
-
-            case layout = options[:layout]
-            when String
-              assert(@layouts.include?(expected_layout), msg)
-            when Regexp
-              assert(@layouts.any? {|l| l =~ layout }, msg)
-            when nil
-              assert(@layouts.empty?, msg)
-            end
           else
             msg = build_message(message,
                     "expecting partial <?> but action rendered <?>",

actionpack/test/abstract/layouts_test.rb

@@ -141,6 +141,30 @@ def index
       end
     end
 
+    class WithOnlyConditional < WithStringImpliedChild
+      layout "overwrite", :only => :show
+
+      def index
+        render :template => ActionView::Template::Text.new("Hello index!")
+      end
+
+      def show
+        render :template => ActionView::Template::Text.new("Hello show!")
+      end
+    end
+
+    class WithExceptConditional < WithStringImpliedChild
+      layout "overwrite", :except => :show
+
+      def index
+        render :template => ActionView::Template::Text.new("Hello index!")
+      end
+
+      def show
+        render :template => ActionView::Template::Text.new("Hello show!")
+      end
+    end
+
     class TestBase < ActiveSupport::TestCase
       test "when no layout is specified, and no default is available, render without a layout" do
         controller = Blank.new
@@ -260,6 +284,30 @@ class ::BadFailLayout < AbstractControllerTests::Layouts::Base
           end
         end
       end
+
+      test "when specify an :only option which match current action name" do
+        controller = WithOnlyConditional.new
+        controller.process(:show)
+        assert_equal "Overwrite Hello show!", controller.response_body
+      end
+
+      test "when specify an :only option which does not match current action name" do
+        controller = WithOnlyConditional.new
+        controller.process(:index)
+        assert_equal "With Implied Hello index!", controller.response_body
+      end
+
+      test "when specify an :except option which match current action name" do
+        controller = WithExceptConditional.new
+        controller.process(:show)
+        assert_equal "With Implied Hello show!", controller.response_body
+      end
+
+      test "when specify an :except option which does not match current action name" do
+        controller = WithExceptConditional.new
+        controller.process(:index)
+        assert_equal "Overwrite Hello index!", controller.response_body
+      end
     end
   end
-end
+end

actionpack/test/controller/action_pack_assertions_test.rb

@@ -71,6 +71,10 @@ def render_text_with_custom_content_type
     render :text => "Hello!", :content_type => Mime::RSS
   end
 
+  def render_with_layout
+    render "test/hello_world", :layout => "layouts/standard"
+  end
+
   def session_stuffing
     session['xmas'] = 'turkey'
     render :text => "ho ho ho"
@@ -471,6 +475,18 @@ def test_fails_with_incorrect_symbol
     end
   end
 
+  def test_fails_with_wrong_layout
+    get :render_with_layout
+    assert_raise(ActiveSupport::TestCase::Assertion) do
+      assert_template :layout => "application"
+    end
+  end
+
+  def test_passes_with_correct_layout
+    get :render_with_layout
+    assert_template :layout => "layouts/standard"
+  end
+
   def test_assert_template_reset_between_requests
     get :hello_world
     assert_template 'test/hello_world'

actionpack/test/controller/layout_test.rb

@@ -167,7 +167,7 @@ def test_layout_is_not_set_when_none_rendered
   def test_layout_is_picked_from_the_controller_instances_view_path
     @controller = PrependsViewPathController.new
     get :hello
-    assert_template :layout => /layouts\/alt\.\w+/
+    assert_template :layout => /layouts\/alt/
   end
 
   def test_absolute_pathed_layout