Lock down new `ImplicitRender` behavior for 5.0 RC

1. Conceptually revert #20276

   The feature was implemented for the `responders` gem. In the end,
   they did not need that feature, and have found a better fix (see
   plataformatec/responders#131).

   `ImplicitRender` is the place where Rails specifies our default
   policies for the case where the user did not explicitly tell us
   what to render, essentially describing a set of heuristics. If
   the gem (or the user) knows exactly what they want, they could
   just perform the correct `render` to avoid falling through to
   here, as `responders` did (the user called `respond_with`).

   Reverting the patch allows us to avoid exploding the complexity
   and defining “the fallback for a fallback” policies.

2. `respond_to` and templates are considered exhaustive enumerations

   If the user specified a list of formats/variants in a `respond_to`
   block, anything that is not explicitly included should result
   in an `UnknownFormat` error (which is then caught upstream to
   mean “406 Not Acceptable” by default). This is already how it
   works before this commit.

   Same goes for templates – if the user defined a set of templates
   (usually in the file system), that set is now considered exhaustive,
   which means that “missing” templates are considered `UnknownFormat`
   errors (406).

3. To keep API endpoints simple, the implicit render behavior for
   actions with no templates defined at all (regardless of formats,
   locales, variants, etc) are defaulted to “204 No Content”. This
   is a strictly narrower version of the feature landed in #19036 and
   #19377.

4. To avoid confusion when interacting in the browser, these actions
   will raise an `UnknownFormat` error for “interactive” requests
   instead. (The precise definition of “interactive” requests might
   change – the spirit here is to give helpful messages and avoid
   confusions.)

Closes #20666, #23062, #23077, #23564

[Godfrey Chan, Jon Moss, Kasper Timm Hansen, Mike Clark, Matthew Draper]

9 files changed, 190 insertions, 53 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index da96aef98b..5b762a8f17 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,32 @@
+*   Update default rendering policies when the controller action did
+    not explicitly indicate a response.
+
+    For API controllers, the implicit render always renders "204 No Content"
+    and does not account for any templates.
+
+    For other controllers, the following conditions are checked:
+
+    First, if a template exists for the controller action, it is rendered.
+    This template lookup takes into account the action name, locales, format,
+    variant, template handlers, etc. (see +render+ for details).
+
+    Second, if other templates exist for the controller action but is not in
+    the right format (or variant, etc.), an <tt>ActionController::UnknownFormat</tt>
+    is raised. The list of available templates is assumed to be a complete
+    enumeration of all the possible formats (or variants, etc.); that is,
+    having only HTML and JSON templates indicate that the controller action is
+    not meant to handle XML requests.
+
+    Third, if the current request is an "interactive" browser request (the user
+    navigated here by entering the URL in the address bar, submiting a form,
+    clicking on a link, etc. as opposed to an XHR or non-browser API request),
+    <tt>ActionView::UnknownFormat</tt> is raised to display a helpful error
+    message.
+
+    Finally, it falls back to the same "204 No Content" behavior as API controllers.
+
+    *Godfrey Chan*, *Jon Moss*, *Kasper Timm Hansen*, *Mike Clark*, *Matthew Draper*
+
 ## Rails 5.0.0.beta3 (February 24, 2016) ##
 
 *   Update session to have indifferent access across multiple requests.
diff --git a/actionpack/lib/action_controller/metal/basic_implicit_render.rb b/actionpack/lib/action_controller/metal/basic_implicit_render.rb
index 6c6f8381ff..cef65a362c 100644
--- a/actionpack/lib/action_controller/metal/basic_implicit_render.rb
+++ b/actionpack/lib/action_controller/metal/basic_implicit_render.rb
@@ -1,5 +1,5 @@
 module ActionController
-  module BasicImplicitRender
+  module BasicImplicitRender # :nodoc:
     def send_action(method, *args)
       super.tap { default_render unless performed? }
     end
diff --git a/actionpack/lib/action_controller/metal/implicit_render.rb b/actionpack/lib/action_controller/metal/implicit_render.rb
index 17fcc2fa02..1f0179a287 100644
--- a/actionpack/lib/action_controller/metal/implicit_render.rb
+++ b/actionpack/lib/action_controller/metal/implicit_render.rb
@@ -1,29 +1,80 @@
+require 'active_support/core_ext/string/strip'
+
 module ActionController
+  # Handles implicit rendering for a controller action when it did not
+  # explicitly indicate an appropiate response via methods such as +render+,
+  # +respond_to+, +redirect+ or +head+.
+  #
+  # For API controllers, the implicit render always renders "204 No Content"
+  # and does not account for any templates.
+  #
+  # For other controllers, the following conditions are checked:
+  #
+  # First, if a template exists for the controller action, it is rendered.
+  # This template lookup takes into account the action name, locales, format,
+  # variant, template handlers, etc. (see +render+ for details).
+  #
+  # Second, if other templates exist for the controller action but is not in
+  # the right format (or variant, etc.), an <tt>ActionController::UnknownFormat</tt>
+  # is raised. The list of available templates is assumed to be a complete
+  # enumeration of all the possible formats (or variants, etc.); that is,
+  # having only HTML and JSON templates indicate that the controller action is
+  # not meant to handle XML requests.
+  #
+  # Third, if the current request is an "interactive" browser request (the user
+  # navigated here by entering the URL in the address bar, submiting a form,
+  # clicking on a link, etc. as opposed to an XHR or non-browser API request),
+  # <tt>ActionView::UnknownFormat</tt> is raised to display a helpful error
+  # message.
+  #
+  # Finally, it falls back to the same "204 No Content" behavior as API controllers.
   module ImplicitRender
 
+    # :stopdoc:
     include BasicImplicitRender
 
-    # Renders the template corresponding to the controller action, if it exists.
-    # The action name, format, and variant are all taken into account.
-    # For example, the "new" action with an HTML format and variant "phone" 
-    # would try to render the <tt>new.html+phone.erb</tt> template.
-    #
-    # If no template is found <tt>ActionController::BasicImplicitRender</tt>'s implementation is called, unless
-    # a block is passed. In that case, it will override the super implementation.
-    #
-    #   default_render do
-    #     head 404 # No template was found
-    #   end
     def default_render(*args)
       if template_exists?(action_name.to_s, _prefixes, variants: request.variant)
         render(*args)
-      else
-        if block_given?
-          yield(*args)
-        else
-          logger.info "No template found for #{self.class.name}\##{action_name}, rendering head :no_content" if logger
-          super
+      elsif any_templates?(action_name.to_s, _prefixes)
+        message = "#{self.class.name}\##{action_name} does not know how to repond " \
+          "to this request. There are other templates available for this controller " \
+          "action but none of them were suitable for this request.\n\n" \
+          "This usually happens when the client requested an unsupported format " \
+          "(e.g. requesting HTML content from a JSON endpoint or vice versa), but " \
+          "it might also be failing due to other constraints, such as locales or" \
+          "variants.\n"
+
+        if request.formats.any?
+          message << "\nRequested format(s): #{request.formats.join(", ")}"
         end
+
+        if request.variant.any?
+          message << "\nRequested variant(s): #{request.variant.join(", ")}"
+        end
+
+        raise ActionController::UnknownFormat, message
+      elsif interactive_browser_request?
+        message = "You did not define any templates for #{self.class.name}\##{action_name}. " \
+          "This is not necessarily a problem (e.g. you might be building an API endpoint " \
+          "that does not require any templates), and the controller would usually respond " \
+          "with `head :no_content` for your convenience.\n\n" \
+          "However, you appear to have navigated here from an interactive browser request – " \
+          "such as by navigating to this URL directly, clicking on a link or submitting a form. " \
+          "Rendering a `head :no_content` in this case could have resulted in unexpected UI " \
+          "behavior in the browser.\n\n" \
+          "If you expected the `head :no_content` response, you do not need to take any " \
+          "actions – requests coming from an XHR (AJAX) request or other non-browser clients " \
+          "will receive the \"204 No Content\" response as expected.\n\n" \
+          "If you did not expect this behavior, you can resolve this error by adding a " \
+          "template for this controller action (usually `#{action_name}.html.erb`) or " \
+          "otherwise indicate the appropriate response in the action using `render`, " \
+          "`redirect_to`, `head`, etc.\n"
+
+        raise ActionController::UnknownFormat, message
+      else
+        logger.info "No template found for #{self.class.name}\##{action_name}, rendering head :no_content" if logger
+        super
       end
     end
 
@@ -32,5 +83,11 @@ module ActionController
         "default_render"
       end
     end
+
+    private
+
+      def interactive_browser_request?
+        request.format == Mime[:html] && !request.xhr?
+      end
   end
 end
diff --git a/actionpack/test/controller/mime/respond_to_test.rb b/actionpack/test/controller/mime/respond_to_test.rb
index 76e2d3ff43..d0c7b2e06a 100644
--- a/actionpack/test/controller/mime/respond_to_test.rb
+++ b/actionpack/test/controller/mime/respond_to_test.rb
@@ -160,7 +160,14 @@ class RespondToController < ActionController::Base
     end
   end
 
-  def variant_with_implicit_rendering
+  def variant_with_implicit_template_rendering
+    # This has exactly one variant template defined in the file system (+mobile.html.erb),
+    # which raises the regular MissingTemplate error for other variants.
+  end
+
+  def variant_without_implicit_template_rendering
+    # This differs from the above in that it does not have any templates defined in the file
+    # system, which triggers the ImplicitRender (204 No Content) behavior.
   end
 
   def variant_with_format_and_custom_render
@@ -272,6 +279,8 @@ class RespondToController < ActionController::Base
 end
 
 class RespondToControllerTest < ActionController::TestCase
+  NO_CONTENT_WARNING = "No template found for RespondToController#variant_without_implicit_template_rendering, rendering head :no_content"
+
   def setup
     super
     @request.host = "www.example.com"
@@ -616,30 +625,69 @@ class RespondToControllerTest < ActionController::TestCase
   end
 
   def test_invalid_variant
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_with_implicit_template_rendering, params: { v: :invalid }
+    end
+  end
+
+  def test_variant_not_set_regular_unknown_format
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_with_implicit_template_rendering
+    end
+  end
+
+  def test_variant_with_implicit_template_rendering
+    get :variant_with_implicit_template_rendering, params: { v: :mobile }
+    assert_equal "text/html", @response.content_type
+    assert_equal "mobile", @response.body
+  end
+
+  def test_variant_without_implicit_rendering_from_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_without_implicit_template_rendering, params: { v: :does_not_matter }
+    end
+  end
+
+  def test_variant_variant_not_set_and_without_implicit_rendering_from_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :variant_without_implicit_template_rendering
+    end
+  end
+
+  def test_variant_without_implicit_rendering_from_xhr
     logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
     old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
 
-    get :variant_with_implicit_rendering, params: { v: :invalid }
+    get :variant_without_implicit_template_rendering, xhr: true, params: { v: :does_not_matter }
     assert_response :no_content
-    assert_equal 1, logger.logged(:info).select{ |s| s =~ /No template found/ }.size, "Implicit head :no_content not logged"
+
+    assert_equal 1, logger.logged(:info).select{ |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
   ensure
     ActionController::Base.logger = old_logger
   end
 
-  def test_variant_not_set_regular_template_missing
-    get :variant_with_implicit_rendering
+  def test_variant_without_implicit_rendering_from_api
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
+
+    get :variant_without_implicit_template_rendering, format: 'json', params: { v: :does_not_matter }
     assert_response :no_content
+
+    assert_equal 1, logger.logged(:info).select{ |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
+  ensure
+    ActionController::Base.logger = old_logger
   end
 
-  def test_variant_with_implicit_rendering
-    get :variant_with_implicit_rendering, params: { v: :implicit }
+  def test_variant_variant_not_set_and_without_implicit_rendering_from_xhr
+    logger = ActiveSupport::LogSubscriber::TestHelper::MockLogger.new
+    old_logger, ActionController::Base.logger = ActionController::Base.logger, logger
+
+    get :variant_without_implicit_template_rendering, xhr: true
     assert_response :no_content
-  end
 
-  def test_variant_with_implicit_template_rendering
-    get :variant_with_implicit_rendering, params: { v: :mobile }
-    assert_equal "text/html", @response.content_type
-    assert_equal "mobile", @response.body
+    assert_equal 1, logger.logged(:info).select { |s| s == NO_CONTENT_WARNING }.size, "Implicit head :no_content not logged"
+  ensure
+    ActionController::Base.logger = old_logger
   end
 
   def test_variant_with_format_and_custom_render
@@ -778,24 +826,3 @@ class RespondToControllerTest < ActionController::TestCase
     assert_equal "phone", @response.body
   end
 end
-
-class RespondToWithBlockOnDefaultRenderController < ActionController::Base
-  def show
-    default_render do
-      render body: 'default_render yielded'
-    end
-  end
-end
-
-class RespondToWithBlockOnDefaultRenderControllerTest < ActionController::TestCase
-  def setup
-    super
-    @request.host = "www.example.com"
-  end
-
-  def test_default_render_uses_block_when_no_template_exists
-    get :show
-    assert_equal "default_render yielded", @response.body
-    assert_equal "text/plain", @response.content_type
-  end
-end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 60c6518c62..83d7405e4d 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -26,6 +26,9 @@ end
 class ImplicitRenderTestController < ActionController::Base
   def empty_action
   end
+
+  def empty_action_with_template
+  end
 end
 
 class TestController < ActionController::Base
@@ -537,10 +540,28 @@ end
 class ImplicitRenderTest < ActionController::TestCase
   tests ImplicitRenderTestController
 
-  def test_implicit_no_content_response
-    get :empty_action
+  def test_implicit_no_content_response_as_browser
+    assert_raises(ActionController::UnknownFormat) do
+      get :empty_action
+    end
+  end
+
+  def test_implicit_no_content_response_as_xhr
+    get :empty_action, xhr: true
     assert_response :no_content
   end
+
+  def test_implicit_success_response_with_right_format
+    get :empty_action_with_template
+    assert_equal "<h1>Empty action rendered this implicitly.</h1>\n", @response.body
+    assert_response :success
+  end
+
+  def test_implicit_unknown_format_response
+    assert_raises(ActionController::UnknownFormat) do
+      get :empty_action_with_template, format: 'json'
+    end
+  end
 end
 
 class HeadRenderTest < ActionController::TestCase
diff --git a/actionpack/test/fixtures/implicit_render_test/empty_action_with_mobile_variant.html+mobile.erb b/actionpack/test/fixtures/implicit_render_test/empty_action_with_mobile_variant.html+mobile.erb
new file mode 100644
index 0000000000..ded99ba52d
--- /dev/null
+++ b/actionpack/test/fixtures/implicit_render_test/empty_action_with_mobile_variant.html+mobile.erb
@@ -0,0 +1 @@
+mobile
diff --git a/actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb b/actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb
new file mode 100644
index 0000000000..dd294f8cf6
--- /dev/null
+++ b/actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb
@@ -0,0 +1 @@
+<h1>Empty action rendered this implicitly.</h1>
diff --git a/actionpack/test/fixtures/implicit_render_test/some_action_2.html+zomg.erb b/actionpack/test/fixtures/implicit_render_test/some_action_2.html+zomg.erb
new file mode 100644
index 0000000000..7d8187321c
--- /dev/null
+++ b/actionpack/test/fixtures/implicit_render_test/some_action_2.html+zomg.erb
@@ -0,0 +1 @@
+zomg
diff --git a/actionpack/test/fixtures/respond_to/variant_with_implicit_rendering.html+mobile.erb b/actionpack/test/fixtures/respond_to/variant_with_implicit_template_rendering.html+mobile.erb
index 317801ad30..317801ad30 100644
--- a/actionpack/test/fixtures/respond_to/variant_with_implicit_rendering.html+mobile.erb
+++ b/actionpack/test/fixtures/respond_to/variant_with_implicit_template_rendering.html+mobile.erb