Merge pull request #23827 from rails/new_implicit_render
Lock down new `ImplicitRender` behavior for 5.0 RC
This commit is contained in:
commit
6b3176170a
|
@ -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.
|
||||
|
|
|
@ -1,5 +1,5 @@
|
|||
module ActionController
|
||||
module BasicImplicitRender
|
||||
module BasicImplicitRender # :nodoc:
|
||||
def send_action(method, *args)
|
||||
super.tap { default_render unless performed? }
|
||||
end
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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 }
|
||||
assert_response :no_content
|
||||
end
|
||||
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
|
||||
|
||||
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
|
||||
get :variant_without_implicit_template_rendering, xhr: true
|
||||
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_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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -0,0 +1 @@
|
|||
mobile
|
1
actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb
vendored
Normal file
1
actionpack/test/fixtures/implicit_render_test/empty_action_with_template.html.erb
vendored
Normal file
|
@ -0,0 +1 @@
|
|||
<h1>Empty action rendered this implicitly.</h1>
|
|
@ -0,0 +1 @@
|
|||
zomg
|
|
@ -136,6 +136,11 @@ module ActionView
|
|||
end
|
||||
alias :template_exists? :exists?
|
||||
|
||||
def any?(name, prefixes = [], partial = false)
|
||||
@view_paths.exists?(*args_for_any(name, prefixes, partial))
|
||||
end
|
||||
alias :any_templates? :any?
|
||||
|
||||
# Adds fallbacks to the view paths. Useful in cases when you are rendering
|
||||
# a :file.
|
||||
def with_fallbacks
|
||||
|
@ -172,6 +177,32 @@ module ActionView
|
|||
[user_details, details_key]
|
||||
end
|
||||
|
||||
def args_for_any(name, prefixes, partial) # :nodoc:
|
||||
name, prefixes = normalize_name(name, prefixes)
|
||||
details, details_key = detail_args_for_any
|
||||
[name, prefixes, partial || false, details, details_key]
|
||||
end
|
||||
|
||||
def detail_args_for_any # :nodoc:
|
||||
@detail_args_for_any ||= begin
|
||||
details = {}
|
||||
|
||||
registered_details.each do |k|
|
||||
if k == :variants
|
||||
details[k] = :any
|
||||
else
|
||||
details[k] = Accessors::DEFAULT_PROCS[k].call
|
||||
end
|
||||
end
|
||||
|
||||
if @cache
|
||||
[details, DetailsKey.get(details)]
|
||||
else
|
||||
[details, nil]
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# Support legacy foo.erb names even though we now ignore .erb
|
||||
# as well as incorrectly putting part of the path in the template
|
||||
# name instead of the prefix.
|
||||
|
|
|
@ -15,7 +15,7 @@ module ActionView
|
|||
# that new object is called in turn. This abstracts the setup and rendering
|
||||
# into a separate classes for partials and templates.
|
||||
class AbstractRenderer #:nodoc:
|
||||
delegate :find_template, :find_file, :template_exists?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context
|
||||
delegate :find_template, :find_file, :template_exists?, :any_templates?, :with_fallbacks, :with_layout_format, :formats, :to => :@lookup_context
|
||||
|
||||
def initialize(lookup_context)
|
||||
@lookup_context = lookup_context
|
||||
|
|
|
@ -222,7 +222,7 @@ module ActionView
|
|||
end
|
||||
|
||||
def find_template_paths(query)
|
||||
Dir[query].reject do |filename|
|
||||
Dir[query].uniq.reject do |filename|
|
||||
File.directory?(filename) ||
|
||||
# deals with case-insensitive file systems.
|
||||
!File.fnmatch(query, filename, File::FNM_EXTGLOB)
|
||||
|
@ -340,7 +340,11 @@ module ActionView
|
|||
query = escape_entry(File.join(@path, path))
|
||||
|
||||
exts = EXTENSIONS.map do |ext, prefix|
|
||||
"{#{details[ext].compact.uniq.map { |e| "#{prefix}#{e}," }.join}}"
|
||||
if ext == :variants && details[ext] == :any
|
||||
"{#{prefix}*,}"
|
||||
else
|
||||
"{#{details[ext].compact.uniq.map { |e| "#{prefix}#{e}," }.join}}"
|
||||
end
|
||||
end.join
|
||||
|
||||
query + exts
|
||||
|
|
|
@ -10,7 +10,7 @@ module ActionView
|
|||
self._view_paths.freeze
|
||||
end
|
||||
|
||||
delegate :template_exists?, :view_paths, :formats, :formats=,
|
||||
delegate :template_exists?, :any_templates?, :view_paths, :formats, :formats=,
|
||||
:locale, :locale=, :to => :lookup_context
|
||||
|
||||
module ClassMethods
|
||||
|
|
|
@ -256,6 +256,12 @@ Please refer to the [Changelog][action-pack] for detailed changes.
|
|||
* Rails will only generate "weak", instead of strong ETags.
|
||||
([Pull Request](https://github.com/rails/rails/pull/17573))
|
||||
|
||||
* Controller actions without an explicit `render` call and with no
|
||||
corresponding templates will render `head :no_conten` implicitly
|
||||
instead of raising an error.
|
||||
(Pull Request [1](https://github.com/rails/rails/pull/19377),
|
||||
[2](https://github.com/rails/rails/pull/23827))
|
||||
|
||||
Action View
|
||||
-------------
|
||||
|
||||
|
|
Loading…
Reference in New Issue