diff options
28 files changed, 321 insertions, 230 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index da96aef98b..6b73b29ace 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,13 +1,33 @@ -## Rails 5.0.0.beta3 (February 24, 2016) ## +* 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: -* Update session to have indifferent access across multiple requests. + 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). - session[:deep][:hash] = "Magic" + 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. - session[:deep][:hash] == "Magic" - session[:deep]["hash"] == "Magic" + 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. - *Tom Prats* + 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) ## * Add application/gzip as a default mime type. 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..6b540d42c7 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 respond " \ + "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/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index 6548ce326b..700317614f 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -176,7 +176,7 @@ module ActionController def initialize(session = {}) super(nil, nil) @id = SecureRandom.hex(16) - @data = session.with_indifferent_access + @data = stringify_keys(session) @loaded = true end diff --git a/actionpack/lib/action_dispatch/request/session.rb b/actionpack/lib/action_dispatch/request/session.rb index 38d0da3e67..42890225fa 100644 --- a/actionpack/lib/action_dispatch/request/session.rb +++ b/actionpack/lib/action_dispatch/request/session.rb @@ -9,7 +9,7 @@ module ActionDispatch # Singleton object used to determine if an optional param wasn't specified Unspecified = Object.new - + # Creates a session hash, merging the properties of the previous session if any def self.create(store, req, default_options) session_was = find req @@ -61,7 +61,7 @@ module ActionDispatch def initialize(by, req) @by = by @req = req - @delegate = {}.with_indifferent_access + @delegate = {} @loaded = false @exists = nil # we haven't checked yet end @@ -88,13 +88,13 @@ module ActionDispatch # nil if the given key is not found in the session. def [](key) load_for_read! - @delegate[key] + @delegate[key.to_s] end # Returns true if the session has the given key or false. def has_key?(key) load_for_read! - @delegate.key?(key) + @delegate.key?(key.to_s) end alias :key? :has_key? alias :include? :has_key? @@ -112,7 +112,7 @@ module ActionDispatch # Writes given value to given key of the session. def []=(key, value) load_for_write! - @delegate[key] = value + @delegate[key.to_s] = value end # Clears the session. @@ -139,13 +139,13 @@ module ActionDispatch # # => {"session_id"=>"e29b9ea315edf98aad94cc78c34cc9b2", "foo" => "bar"} def update(hash) load_for_write! - @delegate.update hash + @delegate.update stringify_keys(hash) end # Deletes given key from the session. def delete(key) load_for_write! - @delegate.delete key + @delegate.delete key.to_s end # Returns value of the given key from the session, or raises +KeyError+ @@ -165,9 +165,9 @@ module ActionDispatch def fetch(key, default=Unspecified, &block) load_for_read! if default == Unspecified - @delegate.fetch(key, &block) + @delegate.fetch(key.to_s, &block) else - @delegate.fetch(key, default, &block) + @delegate.fetch(key.to_s, default, &block) end end @@ -211,9 +211,15 @@ module ActionDispatch def load! id, session = @by.load_session @req options[:id] = id - @delegate.replace(session) + @delegate.replace(stringify_keys(session)) @loaded = true end + + def stringify_keys(other) + other.each_with_object({}) { |(key, value), hash| + hash[key.to_s] = value + } + end 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/dispatch/request/session_test.rb b/actionpack/test/dispatch/request/session_test.rb index 3433d82791..7dcbcc5c21 100644 --- a/actionpack/test/dispatch/request/session_test.rb +++ b/actionpack/test/dispatch/request/session_test.rb @@ -105,16 +105,6 @@ module ActionDispatch end end - def test_indifferent_access - s = Session.create(store, req, {}) - - s[:one] = { test: "deep" } - s[:two] = { "test" => "deep" } - - assert_equal 'deep', s[:one]["test"] - assert_equal 'deep', s[:two][:test] - end - private def store Class.new { diff --git a/actionpack/test/dispatch/session/abstract_store_test.rb b/actionpack/test/dispatch/session/abstract_store_test.rb index c9ce5cad42..d38d1bbce6 100644 --- a/actionpack/test/dispatch/session/abstract_store_test.rb +++ b/actionpack/test/dispatch/session/abstract_store_test.rb @@ -46,22 +46,6 @@ module ActionDispatch assert_equal session.to_hash, session1.to_hash end - def test_previous_session_has_indifferent_access - env = {} - as = MemoryStore.new app - as.call(env) - - assert @env - session = Request::Session.find ActionDispatch::Request.new @env - session[:foo] = { bar: "baz" } - - as.call(@env) - session = Request::Session.find ActionDispatch::Request.new @env - - assert_equal session[:foo][:bar], "baz" - assert_equal session[:foo]["bar"], "baz" - end - private def app(&block) @env = nil diff --git a/actionpack/test/dispatch/session/cache_store_test.rb b/actionpack/test/dispatch/session/cache_store_test.rb index b911392cf1..dbb996973d 100644 --- a/actionpack/test/dispatch/session/cache_store_test.rb +++ b/actionpack/test/dispatch/session/cache_store_test.rb @@ -12,11 +12,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest head :ok end - def set_deep_session_value - session[:foo] = { bar: "baz" } - head :ok - end - def set_serialized_session_value session[:foo] = SessionAutoloadTest::Foo.new head :ok @@ -26,14 +21,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "#{request.session.id}" end @@ -173,22 +160,6 @@ class CacheStoreTest < ActionDispatch::IntegrationTest end end - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - assert_response :success - assert cookies['_session_id'] - - get '/get_deep_session_value_with_symbol' - assert_response :success - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_response :success - assert_equal 'foo: { "bar" => "baz" }', response.body - end - end - private def with_test_route_set with_routing do |set| diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb index 71402b021a..f07e215e3a 100644 --- a/actionpack/test/dispatch/session/cookie_store_test.rb +++ b/actionpack/test/dispatch/session/cookie_store_test.rb @@ -24,23 +24,10 @@ class CookieStoreTest < ActionDispatch::IntegrationTest render plain: Rack::Utils.escape(Verifier.generate(session.to_hash)) end - def set_deep_session_value - session[:foo] = { bar: "baz" } - render plain: Rack::Utils.escape(Verifier.generate(session.to_hash)) - end - def get_session_value render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "id: #{request.session.id}" end @@ -94,15 +81,6 @@ class CookieStoreTest < ActionDispatch::IntegrationTest end end - def test_session_indifferent_access - with_test_route_set do - cookies[SessionKey] = SignedBar - get '/get_session_value' - assert_response :success - assert_equal 'foo: "bar"', response.body - end - end - def test_getting_session_id with_test_route_set do cookies[SessionKey] = SignedBar @@ -354,18 +332,6 @@ class CookieStoreTest < ActionDispatch::IntegrationTest end end - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - - get '/get_deep_session_value_with_symbol' - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_equal 'foo: { "bar" => "baz" }', response.body - end - end - private # Overwrite get to send SessionSecret in env hash diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb index 2e6b42856f..3fed9bad4f 100644 --- a/actionpack/test/dispatch/session/mem_cache_store_test.rb +++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb @@ -13,11 +13,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest head :ok end - def set_deep_session_value - session[:foo] = { bar: "baz" } - head :ok - end - def set_serialized_session_value session[:foo] = SessionAutoloadTest::Foo.new head :ok @@ -27,14 +22,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest render plain: "foo: #{session[:foo].inspect}" end - def get_deep_session_value_with_symbol - render plain: "foo: { bar: #{session[:foo][:bar].inspect} }" - end - - def get_deep_session_value_with_string - render plain: "foo: { \"bar\" => #{session[:foo]["bar"].inspect} }" - end - def get_session_id render plain: "#{request.session.id}" end @@ -192,24 +179,6 @@ class MemCacheStoreTest < ActionDispatch::IntegrationTest rescue Dalli::RingError => ex skip ex.message, ex.backtrace end - - def test_previous_session_has_indifferent_access - with_test_route_set do - get '/set_deep_session_value' - assert_response :success - assert cookies['_session_id'] - - get '/get_deep_session_value_with_symbol' - assert_response :success - assert_equal 'foo: { bar: "baz" }', response.body - - get '/get_deep_session_value_with_string' - assert_response :success - assert_equal 'foo: { "bar" => "baz" }', response.body - end - rescue Dalli::RingError => ex - skip ex.message, ex.backtrace - end rescue LoadError, RuntimeError, Dalli::DalliError $stderr.puts "Skipping MemCacheStoreTest tests. Start memcached and try again." end diff --git a/actionpack/test/dispatch/session/test_session_test.rb b/actionpack/test/dispatch/session/test_session_test.rb index 332c2ae3c8..3e61d123e3 100644 --- a/actionpack/test/dispatch/session/test_session_test.rb +++ b/actionpack/test/dispatch/session/test_session_test.rb @@ -60,11 +60,4 @@ class ActionController::TestSessionTest < ActiveSupport::TestCase session = ActionController::TestSession.new(one: '1') assert_equal(2, session.fetch('2') { |key| key.to_i }) end - - def test_fetch_returns_indifferent_access - session = ActionController::TestSession.new(three: { two: '1' }) - three = session.fetch(:three) - assert_equal('1', three[:two]) - assert_equal('1', three["two"]) - end end 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/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 diff --git a/actionview/lib/action_view/lookup_context.rb b/actionview/lib/action_view/lookup_context.rb index 4163e69a72..626c4b8f5e 100644 --- a/actionview/lib/action_view/lookup_context.rb +++ b/actionview/lib/action_view/lookup_context.rb @@ -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. diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index 23e672a95f..1dddf53df0 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -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 diff --git a/actionview/lib/action_view/template/resolver.rb b/actionview/lib/action_view/template/resolver.rb index 8a675cd521..b6de0b03bf 100644 --- a/actionview/lib/action_view/template/resolver.rb +++ b/actionview/lib/action_view/template/resolver.rb @@ -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 diff --git a/actionview/lib/action_view/view_paths.rb b/actionview/lib/action_view/view_paths.rb index 37722013ce..b46fe06b01 100644 --- a/actionview/lib/action_view/view_paths.rb +++ b/actionview/lib/action_view/view_paths.rb @@ -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 diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index 13053beb78..4a80cda0b8 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -18,7 +18,7 @@ module ActiveRecord relation = build_relation(finder_class, table, attribute, value) if record.persisted? if finder_class.primary_key - relation = relation.where.not(finder_class.primary_key => record.id_was) + relation = relation.where.not(finder_class.primary_key => record.id_was || record.id) else raise UnknownPrimaryKey.new(finder_class, "Can not validate uniqueness for persisted record without primary key.") end diff --git a/activerecord/lib/rails/generators/active_record/model/model_generator.rb b/activerecord/lib/rails/generators/active_record/model/model_generator.rb index 179a4dc91e..f191eff5bf 100644 --- a/activerecord/lib/rails/generators/active_record/model/model_generator.rb +++ b/activerecord/lib/rails/generators/active_record/model/model_generator.rb @@ -41,8 +41,8 @@ module ActiveRecord # FIXME: Change this file to a symlink once RubyGems 2.5.0 is required. def generate_application_record - if self.behavior == :invoke && !File.exist?('app/models/application_record.rb') - template 'application_record.rb', 'app/models/application_record.rb' + if self.behavior == :invoke && !application_record_exist? + template 'application_record.rb', application_record_file_name end end @@ -51,18 +51,22 @@ module ActiveRecord options[:parent] || determine_default_parent_class end - def determine_default_parent_class - application_record = nil + def application_record_exist? + file_exist = nil + in_root { file_exist = File.exist?(application_record_file_name) } + file_exist + end - in_root do - application_record = if mountable_engine? - File.exist?("app/models/#{namespaced_path}/application_record.rb") - else - File.exist?('app/models/application_record.rb') - end + def application_record_file_name + @application_record_file_name ||= if mountable_engine? + "app/models/#{namespaced_path}/application_record.rb" + else + 'app/models/application_record.rb' end + end - if application_record + def determine_default_parent_class + if application_record_exist? "ApplicationRecord" else "ActiveRecord::Base" diff --git a/activerecord/lib/rails/generators/active_record/model/templates/application_record.rb b/activerecord/lib/rails/generators/active_record/model/templates/application_record.rb index 10a4cba84d..60050e0bf8 100644 --- a/activerecord/lib/rails/generators/active_record/model/templates/application_record.rb +++ b/activerecord/lib/rails/generators/active_record/model/templates/application_record.rb @@ -1,3 +1,5 @@ +<% module_namespacing do -%> class ApplicationRecord < ActiveRecord::Base self.abstract_class = true end +<% end -%> diff --git a/activerecord/test/cases/validations/uniqueness_validation_test.rb b/activerecord/test/cases/validations/uniqueness_validation_test.rb index 8abb6c9844..4c14d93c66 100644 --- a/activerecord/test/cases/validations/uniqueness_validation_test.rb +++ b/activerecord/test/cases/validations/uniqueness_validation_test.rb @@ -53,6 +53,14 @@ class CoolTopic < Topic validates_uniqueness_of :id end +class TopicWithAfterCreate < Topic + after_create :set_author + + def set_author + update_attributes!(:author_name => "#{title} #{id}") + end +end + class UniquenessValidationTest < ActiveRecord::TestCase INT_MAX_VALUE = 2147483647 @@ -469,6 +477,16 @@ class UniquenessValidationTest < ActiveRecord::TestCase assert t.save, "Should still save t as unique" end + def test_validate_uniqueness_with_after_create_performing_save + TopicWithAfterCreate.validates_uniqueness_of(:title) + topic = TopicWithAfterCreate.create!(:title => "Title1") + assert topic.author_name.start_with?("Title1") + + topic2 = TopicWithAfterCreate.new(:title => "Title1") + refute topic2.valid? + assert_equal(["has already been taken"], topic2.errors[:title]) + end + def test_validate_uniqueness_uuid skip unless current_adapter?(:PostgreSQLAdapter) item = UuidItem.create!(uuid: SecureRandom.uuid, title: 'item1') diff --git a/guides/source/5_0_release_notes.md b/guides/source/5_0_release_notes.md index 91bca16356..dc631e5cb9 100644 --- a/guides/source/5_0_release_notes.md +++ b/guides/source/5_0_release_notes.md @@ -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_content` 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 ------------- diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 3f522386d3..b83d25c683 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -98,13 +98,15 @@ application. Accepts a valid week day symbol (e.g. `:monday`). * `config.exceptions_app` sets the exceptions application invoked by the ShowException middleware when an exception happens. Defaults to `ActionDispatch::PublicExceptions.new(Rails.public_path)`. +* `config.debug_exception_response_format` sets the format used in responses when errors occur in development mode. + * `config.file_watcher` is the class used to detect file updates in the file system when `config.reload_classes_only_on_change` is true. Rails ships with `ActiveSupport::FileUpdateChecker`, the default, and `ActiveSupport::EventedFileUpdateChecker` (this one depends on the [listen](https://github.com/guard/listen) gem). Custom classes must conform to the `ActiveSupport::FileUpdateChecker` API. * `config.filter_parameters` used for filtering out the parameters that you don't want shown in the logs, such as passwords or credit card numbers. New applications filter out passwords by adding the following `config.filter_parameters+=[:password]` in `config/initializers/filter_parameter_logging.rb`. -* `config.force_ssl` forces all requests to be served over HTTPS by using the `ActionDispatch::SSL` middleware. This can be configured by setting `config.ssl_options` - see the [ActionDispatch::SSL documentation](http://edgeapi.rubyonrails.org/classes/ActionDispatch/SSL.html) for details. +* `config.force_ssl` forces all requests to be served over HTTPS by using the `ActionDispatch::SSL` middleware, and sets `config.action_mailer.default_url_options` to be `{ protocol: 'https' }`. This can be configured by setting `config.ssl_options` - see the [ActionDispatch::SSL documentation](http://edgeapi.rubyonrails.org/classes/ActionDispatch/SSL.html) for details. * `config.log_formatter` defines the formatter of the Rails logger. This option defaults to an instance of `ActiveSupport::Logger::SimpleFormatter` for all modes except production, where it defaults to `Logger::Formatter`. diff --git a/guides/source/getting_started.md b/guides/source/getting_started.md index 8eb3b6190f..4431512eda 100644 --- a/guides/source/getting_started.md +++ b/guides/source/getting_started.md @@ -298,26 +298,30 @@ Open the file `config/routes.rb` in your editor. Rails.application.routes.draw do get 'welcome/index' - # The priority is based upon order of creation: - # first created -> highest priority. - # See how all your routes lay out with "bin/rails routes". - # - # You can have the root of your site routed with "root" - # root 'welcome#index' - # - # ... + # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html + + # Serve websocket cable requests in-process + # mount ActionCable.server => '/cable' +end ``` This is your application's _routing file_ which holds entries in a special [DSL (domain-specific language)](http://en.wikipedia.org/wiki/Domain-specific_language) that tells Rails how to connect incoming requests to -controllers and actions. This file contains many sample routes on commented -lines, and one of them actually shows you how to connect the root of your site -to a specific controller and action. Find the line beginning with `root` and -uncomment it. It should look something like the following: +controllers and actions. +Edit this file by adding the line of code `root 'welcome#index'`. +It should look something like the following: ```ruby -root 'welcome#index' +Rails.application.routes.draw do + get 'welcome/index' + + # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html + + # Serve websocket cable requests in-process + # mount ActionCable.server => '/cable' + root 'welcome#index' +end ``` `root 'welcome#index'` tells Rails to map requests to the root of the @@ -348,7 +352,7 @@ operations are referred to as _CRUD_ operations. Rails provides a `resources` method which can be used to declare a standard REST resource. You need to add the _article resource_ to the -`config/routes.rb` as follows: +`config/routes.rb` so the file will look as follows: ```ruby Rails.application.routes.draw do @@ -625,7 +629,7 @@ end The `render` method here is taking a very simple hash with a key of `:plain` and value of `params[:article].inspect`. The `params` method is the object which represents the parameters (or fields) coming in from the form. The `params` -method returns an `ActiveSupport::HashWithIndifferentAccess` object, which +method returns an `ActionController::Parameters` object, which allows you to access the keys of the hash using either strings or symbols. In this situation, the only parameters that matter are the ones from the form. @@ -635,7 +639,7 @@ If you re-submit the form one more time you'll now no longer get the missing template error. Instead, you'll see something that looks like the following: ```ruby -{"title"=>"First article!", "text"=>"This is my first article."} +<ActionController::Parameters {"title"=>"First Article!", "text"=>"This is my first article."} permitted: false> ``` This action is now displaying the parameters for the article that are coming in diff --git a/railties/test/generators/plugin_generator_test.rb b/railties/test/generators/plugin_generator_test.rb index d7d27e6b2e..cf3ed8405d 100644 --- a/railties/test/generators/plugin_generator_test.rb +++ b/railties/test/generators/plugin_generator_test.rb @@ -642,6 +642,20 @@ class PluginGeneratorTest < Rails::Generators::TestCase assert_file "app/models/bukkits/article.rb", /class Article < ApplicationRecord/ end + def test_generate_application_record_when_does_not_exist_in_mountable_engine + run_generator [destination_root, '--mountable'] + FileUtils.rm "#{destination_root}/app/models/bukkits/application_record.rb" + capture(:stdout) do + `#{destination_root}/bin/rails g model article` + end + + assert_file "#{destination_root}/app/models/bukkits/application_record.rb" do |record| + assert_match(/module Bukkits/, record) + assert_match(/class ApplicationRecord < ActiveRecord::Base/, record) + assert_match(/self.abstract_class = true/, record) + end + end + def test_after_bundle_callback path = 'http://example.org/rails_template' template = %{ after_bundle { run 'echo ran after_bundle' } } |