diff options
26 files changed, 303 insertions, 268 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 4a47c0fdf8..bb15edee63 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,8 @@ +* ActionDispatch::Response#new no longer applies default headers. If you want + default headers applied to the response object, then call + `ActionDispatch::Response.create`. This change only impacts people who are + directly constructing an `ActionDispatch::Response` object. + * Accessing mime types via constants like `Mime::HTML` is deprecated. Please change code like this: diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb index 0384740fef..3d72755f1d 100644 --- a/actionpack/lib/action_controller/metal.rb +++ b/actionpack/lib/action_controller/metal.rb @@ -135,7 +135,7 @@ module ActionController end def self.make_response!(request) - ActionDispatch::Response.new.tap do |res| + ActionDispatch::Response.create.tap do |res| res.request = request end end diff --git a/actionpack/lib/action_controller/metal/http_authentication.rb b/actionpack/lib/action_controller/metal/http_authentication.rb index 0ff5ceb728..fe470552b0 100644 --- a/actionpack/lib/action_controller/metal/http_authentication.rb +++ b/actionpack/lib/action_controller/metal/http_authentication.rb @@ -436,15 +436,17 @@ module ActionController end end - # Parses the token and options out of the token authorization header. If - # the header looks like this: + # Parses the token and options out of the token authorization header. + # The value for the Authorization header is expected to have the prefix + # <tt>"Token"</tt> or <tt>"Bearer"</tt>. If the header looks like this: # Authorization: Token token="abc", nonce="def" - # Then the returned token is "abc", and the options is {nonce: "def"} + # Then the returned token is <tt>"abc"</tt>, and the options are + # <tt>{nonce: "def"}</tt> # # request - ActionDispatch::Request instance with the current headers. # - # Returns an Array of [String, Hash] if a token is present. - # Returns nil if no token is found. + # Returns an +Array+ of <tt>[String, Hash]</tt> if a token is present. + # Returns +nil+ if no token is found. def token_and_options(request) authorization_request = request.authorization.to_s if authorization_request[TOKEN_REGEX] diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb index 667c7f87ca..c874165816 100644 --- a/actionpack/lib/action_controller/metal/live.rb +++ b/actionpack/lib/action_controller/metal/live.rb @@ -236,6 +236,10 @@ module ActionController end end + def initialize(status = 200, header = {}, body = []) + super(status, Header.new(self, header), body) + end + private def before_committed @@ -257,10 +261,6 @@ module ActionController buf end - def merge_default_headers(original, default) - Header.new self, super - end - def handle_conditional_get! super unless committed? end diff --git a/actionpack/lib/action_controller/metal/testing.rb b/actionpack/lib/action_controller/metal/testing.rb index 47d940f692..b2b3b4283f 100644 --- a/actionpack/lib/action_controller/metal/testing.rb +++ b/actionpack/lib/action_controller/metal/testing.rb @@ -2,12 +2,6 @@ module ActionController module Testing extend ActiveSupport::Concern - # TODO : Rewrite tests using controller.headers= to use Rack env - def headers=(new_headers) - @_response ||= ActionDispatch::Response.new - @_response.headers.replace(new_headers) - end - # Behavior specific to functional tests module Functional # :nodoc: def set_response!(request) diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb index 47b6b5d4b3..cf78688126 100644 --- a/actionpack/lib/action_controller/test_case.rb +++ b/actionpack/lib/action_controller/test_case.rb @@ -585,7 +585,7 @@ module ActionController end def build_response(klass) - klass.new + klass.create end included do diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index b1b70f7d61..cbeea9e267 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -103,13 +103,21 @@ module ActionDispatch # :nodoc: end end + def self.create(status = 200, header = {}, body = [], default_headers: self.default_headers) + header = merge_default_headers(header, default_headers) + new status, header, body + end + + def self.merge_default_headers(original, default) + default.respond_to?(:merge) ? default.merge(original) : original + end + # The underlying body, as a streamable object. attr_reader :stream - def initialize(status = 200, header = {}, body = [], default_headers: self.class.default_headers) + def initialize(status = 200, header = {}, body = []) super() - header = merge_default_headers(header, default_headers) @header = header self.body, self.status = body, status @@ -345,10 +353,6 @@ module ActionDispatch # :nodoc: def before_sending end - def merge_default_headers(original, default) - default.respond_to?(:merge) ? default.merge(original) : original - end - def build_buffer(response, body) Buffer.new response, body end diff --git a/actionpack/lib/action_dispatch/testing/test_response.rb b/actionpack/lib/action_dispatch/testing/test_response.rb index 6a31d6243f..4b79a90242 100644 --- a/actionpack/lib/action_dispatch/testing/test_response.rb +++ b/actionpack/lib/action_dispatch/testing/test_response.rb @@ -7,7 +7,7 @@ module ActionDispatch # See Response for more information on controller response objects. class TestResponse < Response def self.from_response(response) - new response.status, response.headers, response.body, default_headers: nil + new response.status, response.headers, response.body end # Was the response successful? diff --git a/actionpack/test/controller/live_stream_test.rb b/actionpack/test/controller/live_stream_test.rb index e9c19b7acf..4d1c23cbee 100644 --- a/actionpack/test/controller/live_stream_test.rb +++ b/actionpack/test/controller/live_stream_test.rb @@ -112,7 +112,7 @@ module ActionController class TestController < ActionController::Base include ActionController::Live - attr_accessor :latch, :tc + attr_accessor :latch, :tc, :error_latch def self.controller_path 'test' @@ -204,6 +204,12 @@ module ActionController end def overfill_buffer_and_die + logger = ActionController::Base.logger || Logger.new($stdout) + response.stream.on_error do + logger.warn 'Error while streaming' + error_latch.count_down + end + # Write until the buffer is full. It doesn't expose that # information directly, so we must hard-code its size: 10.times do @@ -256,20 +262,12 @@ module ActionController end def test_set_cookie - @controller = TestController.new get :set_cookie assert_equal({'hello' => 'world'}, @response.cookies) assert_equal "hello world", @response.body end - def test_set_response! - @controller.set_response!(@request) - assert_kind_of(Live::Response, @controller.response) - assert_equal @request, @controller.response.request - end - def test_write_to_stream - @controller = TestController.new get :basic_stream assert_equal "helloworld", @response.body assert_equal 'text/event-stream', @response.headers['Content-Type'] @@ -281,10 +279,9 @@ module ActionController @controller.latch = Concurrent::CountDownLatch.new parts = ['hello', 'world'] - @controller.request = @request - @controller.response = @response + get :blocking_stream - t = Thread.new(@response) { |resp| + t = Thread.new(response) { |resp| resp.await_commit resp.stream.each do |part| assert_equal parts.shift, part @@ -294,38 +291,28 @@ module ActionController end } - @controller.process :blocking_stream - assert t.join(3), 'timeout expired before the thread terminated' end def test_abort_with_full_buffer @controller.latch = Concurrent::CountDownLatch.new - - @request.parameters[:format] = 'plain' - @controller.request = @request - @controller.response = @response - - got_error = Concurrent::CountDownLatch.new - @response.stream.on_error do - ActionController::Base.logger.warn 'Error while streaming' - got_error.count_down - end - - t = Thread.new(@response) { |resp| - resp.await_commit - _, _, body = resp.to_a - body.each do - @controller.latch.wait - body.close - break - end - } + @controller.error_latch = Concurrent::CountDownLatch.new capture_log_output do |output| - @controller.process :overfill_buffer_and_die + get :overfill_buffer_and_die, :format => 'plain' + + t = Thread.new(response) { |resp| + resp.await_commit + _, _, body = resp.to_a + body.each do + @controller.latch.wait + body.close + break + end + } + t.join - got_error.wait + @controller.error_latch.wait assert_match 'Error while streaming', output.rewind && output.read end end @@ -333,20 +320,18 @@ module ActionController def test_ignore_client_disconnect @controller.latch = Concurrent::CountDownLatch.new - @controller.request = @request - @controller.response = @response + capture_log_output do |output| + get :ignore_client_disconnect - t = Thread.new(@response) { |resp| - resp.await_commit - _, _, body = resp.to_a - body.each do - body.close - break - end - } + t = Thread.new(response) { |resp| + resp.await_commit + _, _, body = resp.to_a + body.each do + body.close + break + end + } - capture_log_output do |output| - @controller.process :ignore_client_disconnect t.join Timeout.timeout(3) do @controller.latch.wait @@ -364,11 +349,8 @@ module ActionController end def test_live_stream_default_header - @controller.request = @request - @controller.response = @response - @controller.process :default_header - _, headers, _ = @response.prepare! - assert headers['Content-Type'] + get :default_header + assert response.headers['Content-Type'] end def test_render_text @@ -437,13 +419,13 @@ module ActionController def test_stale_without_etag get :with_stale - assert_equal 200, @response.status.to_i + assert_equal 200, response.status.to_i end def test_stale_with_etag @request.if_none_match = Digest::MD5.hexdigest("123") get :with_stale - assert_equal 304, @response.status.to_i + assert_equal 304, response.status.to_i end end diff --git a/actionpack/test/controller/send_file_test.rb b/actionpack/test/controller/send_file_test.rb index daa38ecbc4..c712c75c88 100644 --- a/actionpack/test/controller/send_file_test.rb +++ b/actionpack/test/controller/send_file_test.rb @@ -20,6 +20,47 @@ class SendFileController < ActionController::Base send_file(file_path, options) end + def test_send_file_headers_bang + options = { + :type => Mime::Type[:PNG], + :disposition => 'disposition', + :filename => 'filename' + } + + send_data "foo", options + end + + def test_send_file_headers_with_disposition_as_a_symbol + options = { + :type => Mime::Type[:PNG], + :disposition => :disposition, + :filename => 'filename' + } + + send_data "foo", options + end + + def test_send_file_headers_with_mime_lookup_with_symbol + options = { :type => :png } + + send_data "foo", options + end + + def test_send_file_headers_with_bad_symbol + options = { :type => :this_type_is_not_registered } + send_data "foo", options + end + + def test_send_file_headers_with_nil_content_type + options = { :type => nil } + send_data "foo", options + end + + def test_send_file_headers_guess_type_from_extension + options = { :filename => params[:filename] } + send_data "foo", options + end + def data send_data(file_data, options) end @@ -88,72 +129,38 @@ class SendFileTest < ActionController::TestCase # Test that send_file_headers! is setting the correct HTTP headers. def test_send_file_headers_bang - options = { - :type => Mime::Type[:PNG], - :disposition => 'disposition', - :filename => 'filename' - } - # Do it a few times: the resulting headers should be identical # no matter how many times you send with the same options. # Test resolving Ticket #458. - @controller.headers = {} - @controller.send(:send_file_headers!, options) - @controller.send(:send_file_headers!, options) - @controller.send(:send_file_headers!, options) - - h = @controller.headers - assert_equal 'image/png', @controller.content_type - assert_equal 'disposition; filename="filename"', h['Content-Disposition'] - assert_equal 'binary', h['Content-Transfer-Encoding'] + 5.times do + get :test_send_file_headers_bang - # test overriding Cache-Control: no-cache header to fix IE open/save dialog - @controller.send(:send_file_headers!, options) - @controller.response.prepare! - assert_equal 'private', h['Cache-Control'] + assert_equal 'image/png', response.content_type + assert_equal 'disposition; filename="filename"', response.get_header('Content-Disposition') + assert_equal 'binary', response.get_header('Content-Transfer-Encoding') + assert_equal 'private', response.get_header('Cache-Control') + end end def test_send_file_headers_with_disposition_as_a_symbol - options = { - :type => Mime::Type[:PNG], - :disposition => :disposition, - :filename => 'filename' - } + get :test_send_file_headers_with_disposition_as_a_symbol - @controller.headers = {} - @controller.send(:send_file_headers!, options) - assert_equal 'disposition; filename="filename"', @controller.headers['Content-Disposition'] + assert_equal 'disposition; filename="filename"', response.get_header('Content-Disposition') end def test_send_file_headers_with_mime_lookup_with_symbol - options = { - :type => :png - } - - @controller.headers = {} - @controller.send(:send_file_headers!, options) - - assert_equal 'image/png', @controller.content_type + get __method__ + assert_equal 'image/png', response.content_type end def test_send_file_headers_with_bad_symbol - options = { - :type => :this_type_is_not_registered - } - - @controller.headers = {} - error = assert_raise(ArgumentError) { @controller.send(:send_file_headers!, options) } - assert_equal "Unknown MIME type #{options[:type]}", error.message + error = assert_raise(ArgumentError) { get __method__ } + assert_equal "Unknown MIME type this_type_is_not_registered", error.message end def test_send_file_headers_with_nil_content_type - options = { - :type => nil - } - - @controller.headers = {} - error = assert_raise(ArgumentError) { @controller.send(:send_file_headers!, options) } + error = assert_raise(ArgumentError) { get __method__ } assert_equal ":type option required", error.message end @@ -169,10 +176,8 @@ class SendFileTest < ActionController::TestCase 'file.unk' => 'application/octet-stream', 'zip' => 'application/octet-stream' }.each do |filename,expected_type| - options = { :filename => filename } - @controller.headers = {} - @controller.send(:send_file_headers!, options) - assert_equal expected_type, @controller.content_type + get __method__, params: { filename: filename } + assert_equal expected_type, response.content_type end end diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index 06bf9dec74..40c97abd35 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -974,6 +974,11 @@ class ResponseDefaultHeadersTest < ActionController::TestCase headers.delete params[:header] head :ok, 'C' => '3' end + + # Render a head response, but don't touch default headers + def leave_alone + head :ok + end end def before_setup @@ -999,9 +1004,13 @@ class ResponseDefaultHeadersTest < ActionController::TestCase end test "response contains default headers" do + get :leave_alone + # Response headers start out with the defaults - assert_equal @defaults, response.headers + assert_equal @defaults.merge('Content-Type' => 'text/html'), response.headers + end + test "response deletes a default header" do get :remove_header, params: { header: 'A' } assert_response :ok diff --git a/actionpack/test/dispatch/response_test.rb b/actionpack/test/dispatch/response_test.rb index 024fd391d5..85cdcda655 100644 --- a/actionpack/test/dispatch/response_test.rb +++ b/actionpack/test/dispatch/response_test.rb @@ -4,7 +4,7 @@ require 'rack/content_length' class ResponseTest < ActiveSupport::TestCase def setup - @response = ActionDispatch::Response.new + @response = ActionDispatch::Response.create end def test_can_wait_until_commit @@ -149,14 +149,19 @@ class ResponseTest < ActiveSupport::TestCase status, headers, body = @response.to_a assert_equal "user_name=david; path=/", headers["Set-Cookie"] assert_equal({"user_name" => "david"}, @response.cookies) + end - @response = ActionDispatch::TestResponse.new + test "multiple cookies" do @response.set_cookie("user_name", :value => "david", :path => "/") @response.set_cookie("login", :value => "foo&bar", :path => "/", :expires => Time.utc(2005, 10, 10,5)) status, headers, body = @response.to_a assert_equal "user_name=david; path=/\nlogin=foo%26bar; path=/; expires=Mon, 10 Oct 2005 05:00:00 -0000", headers["Set-Cookie"] assert_equal({"login" => "foo&bar", "user_name" => "david"}, @response.cookies) + end + test "delete cookies" do + @response.set_cookie("user_name", :value => "david", :path => "/") + @response.set_cookie("login", :value => "foo&bar", :path => "/", :expires => Time.utc(2005, 10, 10,5)) @response.delete_cookie("login") status, headers, body = @response.to_a assert_equal({"user_name" => "david", "login" => nil}, @response.cookies) @@ -212,7 +217,7 @@ class ResponseTest < ActiveSupport::TestCase 'X-Content-Type-Options' => 'nosniff', 'X-XSS-Protection' => '1;' } - resp = ActionDispatch::Response.new.tap { |response| + resp = ActionDispatch::Response.create.tap { |response| response.body = 'Hello' } resp.to_a @@ -231,7 +236,7 @@ class ResponseTest < ActiveSupport::TestCase ActionDispatch::Response.default_headers = { 'X-XX-XXXX' => 'Here is my phone number' } - resp = ActionDispatch::Response.new.tap { |response| + resp = ActionDispatch::Response.create.tap { |response| response.body = 'Hello' } resp.to_a diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index 2cf39b68fb..0d6a3dc52d 100644 --- a/activemodel/lib/active_model/callbacks.rb +++ b/activemodel/lib/active_model/callbacks.rb @@ -103,6 +103,7 @@ module ActiveModel def define_model_callbacks(*callbacks) options = callbacks.extract_options! options = { + terminator: deprecated_false_terminator, skip_after_callbacks_if_terminated: true, scope: [:kind, :name], only: [:before, :around, :after] diff --git a/activemodel/lib/active_model/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb index 4b58ef66e3..52111e5442 100644 --- a/activemodel/lib/active_model/validations/callbacks.rb +++ b/activemodel/lib/active_model/validations/callbacks.rb @@ -23,6 +23,7 @@ module ActiveModel included do include ActiveSupport::Callbacks define_callbacks :validation, + terminator: deprecated_false_terminator, skip_after_callbacks_if_terminated: true, scope: [:kind, :name] end diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 82b07de482..99b500526d 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -96,7 +96,7 @@ module ActiveRecord end end - # Raises a <tt>ActiveRecord::DangerousAttributeError</tt> exception when an + # Raises an <tt>ActiveRecord::DangerousAttributeError</tt> exception when an # \Active \Record method is defined in the model, otherwise +false+. # # class Person < ActiveRecord::Base diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index a09437b4b0..2336d23a1c 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -22,7 +22,7 @@ module ActiveRecord # p1.save # # p2.first_name = "should fail" - # p2.save # Raises a ActiveRecord::StaleObjectError + # p2.save # Raises an ActiveRecord::StaleObjectError # # Optimistic locking will also check for stale data when objects are destroyed. Example: # @@ -32,7 +32,7 @@ module ActiveRecord # p1.first_name = "Michael" # p1.save # - # p2.destroy # Raises a ActiveRecord::StaleObjectError + # p2.destroy # Raises an ActiveRecord::StaleObjectError # # You're then responsible for dealing with the conflict by rescuing the exception and either rolling back, merging, # or otherwise apply the business logic needed to resolve the conflict. diff --git a/activerecord/lib/active_record/railties/databases.rake b/activerecord/lib/active_record/railties/databases.rake index b113baec33..d940ac244a 100644 --- a/activerecord/lib/active_record/railties/databases.rake +++ b/activerecord/lib/active_record/railties/databases.rake @@ -353,7 +353,7 @@ db_namespace = namespace :db do ActiveRecord::Tasks::DatabaseTasks.purge ActiveRecord::Base.configurations['test'] end - # desc 'Check for pending migrations and load the test schema' + # desc 'Load the test schema' task :prepare => %w(environment load_config) do unless ActiveRecord::Base.configurations.blank? db_namespace['test:load'].invoke diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 4a569fc242..1a2988ea77 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -11,6 +11,7 @@ module ActiveRecord :before_commit_without_transaction_enrollment, :commit_without_transaction_enrollment, :rollback_without_transaction_enrollment, + terminator: deprecated_false_terminator, scope: [:kind, :name] end diff --git a/activerecord/test/cases/attribute_test.rb b/activerecord/test/cases/attribute_test.rb index 0ec368f51d..c69782d93f 100644 --- a/activerecord/test/cases/attribute_test.rb +++ b/activerecord/test/cases/attribute_test.rb @@ -4,7 +4,6 @@ module ActiveRecord class AttributeTest < ActiveRecord::TestCase setup do @type = Minitest::Mock.new - @type.expect(:==, false, [false]) end teardown do diff --git a/activerecord/test/cases/type/date_time_test.rb b/activerecord/test/cases/type/date_time_test.rb index 62d3405be1..bc4900e1c2 100644 --- a/activerecord/test/cases/type/date_time_test.rb +++ b/activerecord/test/cases/type/date_time_test.rb @@ -5,6 +5,7 @@ module ActiveRecord module Type class IntegerTest < ActiveRecord::TestCase def test_datetime_seconds_precision_applied_to_timestamp + skip "This test is invalid if subsecond precision isn't supported" unless subsecond_precision_supported? p = Task.create!(starting: ::Time.now) assert_equal p.starting.usec, p.reload.starting.usec end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 69413fef47..309d842da1 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -277,20 +277,17 @@ The preferred method to halt a callback chain from now on is to explicitly `throw(:abort)`. - In the past, returning `false` in an ActiveSupport callback had the side - effect of halting the callback chain. This is not recommended anymore and, - depending on the value of - `Callbacks::CallbackChain.halt_and_display_warning_on_return_false`, will - either not work at all or display a deprecation warning. + In the past, callbacks could only be halted by explicitly providing a + terminator and by having a callback match the conditions of the terminator. * Add `Callbacks::CallbackChain.halt_and_display_warning_on_return_false` Setting `Callbacks::CallbackChain.halt_and_display_warning_on_return_false` - to `true` will let an app support the deprecated way of halting callback - chains by returning `false`. + to `true` will let an app support the deprecated way of halting Active Record, + Active Model and Active Model validations callback chains by returning `false`. Setting the value to `false` will tell the app to ignore any `false` value - returned by callbacks, and only halt the chain upon `throw(:abort)`. + returned by those callbacks, and only halt the chain upon `throw(:abort)`. The value can also be set with the Rails configuration option `config.active_support.halt_callback_chains_on_return_false`. @@ -300,7 +297,7 @@ For new Rails 5.0 apps, its value is set to `false` in an initializer, so these apps will support the new behavior by default. - *claudiob* + *claudiob*, *Roque Pinel* * Changes arguments and default value of CallbackChain's `:terminator` option diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 80c5fdba17..3db9ea2ac0 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -536,23 +536,12 @@ module ActiveSupport Proc.new do |target, result_lambda| terminate = true catch(:abort) do - result = result_lambda.call if result_lambda.is_a?(Proc) - if halt_and_display_warning_on_return_false && result == false - display_deprecation_warning_for_false_terminator - else - terminate = false - end + result_lambda.call if result_lambda.is_a?(Proc) + terminate = false end terminate end end - - def display_deprecation_warning_for_false_terminator - ActiveSupport::Deprecation.warn(<<-MSG.squish) - Returning `false` in a callback will not implicitly halt a callback chain in the next release of Rails. - To explicitly halt a callback chain, please use `throw :abort` instead. - MSG - end end module ClassMethods @@ -686,7 +675,8 @@ module ActiveSupport # # In this example, if any before validate callbacks returns +false+, # any successive before and around callback is not executed. - # Defaults to +false+, meaning no value halts the chain. + # + # The default terminator halts the chain when a callback throws +:abort+. # # * <tt>:skip_after_callbacks_if_terminated</tt> - Determines if after # callbacks should be terminated by the <tt>:terminator</tt> option. By @@ -764,6 +754,30 @@ module ActiveSupport def set_callbacks(name, callbacks) send "_#{name}_callbacks=", callbacks end + + def deprecated_false_terminator + Proc.new do |target, result_lambda| + terminate = true + catch(:abort) do + result = result_lambda.call if result_lambda.is_a?(Proc) + if CallbackChain.halt_and_display_warning_on_return_false && result == false + display_deprecation_warning_for_false_terminator + else + terminate = false + end + end + terminate + end + end + + private + + def display_deprecation_warning_for_false_terminator + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Returning `false` in Active Record and Active Model callbacks will not implicitly halt a callback chain in the next release of Rails. + To explicitly halt the callback chain, please use `throw :abort` instead. + MSG + end end end end diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index cda9732cae..4f47e0519f 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -766,13 +766,11 @@ module CallbacksTest end class CallbackFalseTerminatorWithoutConfigTest < ActiveSupport::TestCase - def test_returning_false_halts_callback_if_config_variable_is_not_set + def test_returning_false_does_not_halt_callback_if_config_variable_is_not_set obj = CallbackFalseTerminator.new - assert_deprecated do - obj.save - assert_equal :second, obj.halted - assert !obj.saved - end + obj.save + assert_equal nil, obj.halted + assert obj.saved end end @@ -781,13 +779,11 @@ module CallbacksTest ActiveSupport::Callbacks::CallbackChain.halt_and_display_warning_on_return_false = true end - def test_returning_false_halts_callback_if_config_variable_is_true + def test_returning_false_does_not_halt_callback_if_config_variable_is_true obj = CallbackFalseTerminator.new - assert_deprecated do - obj.save - assert_equal :second, obj.halted - assert !obj.saved - end + obj.save + assert_equal nil, obj.halted + assert obj.saved end end diff --git a/guides/source/action_view_overview.md b/guides/source/action_view_overview.md index d3c0a3666f..76454e77c7 100644 --- a/guides/source/action_view_overview.md +++ b/guides/source/action_view_overview.md @@ -148,7 +148,7 @@ end ``` #### Jbuilder -<a href="https://github.com/rails/jbuilder">Jbuilder</a> is a gem that's +[Jbuilder](https://github.com/rails/jbuilder) is a gem that's maintained by the Rails team and included in the default Rails Gemfile. It's similar to Builder, but is used to generate JSON, instead of XML. @@ -177,8 +177,8 @@ would produce: } ``` -See the <a href="https://github.com/rails/jbuilder#jbuilder"> -Jbuilder documention</a> for more examples and information. +See the [Jbuilder documention](https://github.com/rails/jbuilder#jbuilder) for +more examples and information. #### Template Caching diff --git a/guides/source/engines.md b/guides/source/engines.md index a3cff2a807..6bbd68ff78 100644 --- a/guides/source/engines.md +++ b/guides/source/engines.md @@ -150,7 +150,7 @@ When you include the engine into an application later on, you will do so with this line in the Rails application's `Gemfile`: ```ruby -gem 'blorgh', path: "vendor/engines/blorgh" +gem 'blorgh', path: 'engines/blorgh' ``` Don't forget to run `bundle install` as usual. By specifying it as a gem within @@ -639,7 +639,7 @@ However, because you are developing the `blorgh` engine on your local machine, you will need to specify the `:path` option in your `Gemfile`: ```ruby -gem 'blorgh', path: "path/to/blorgh" +gem 'blorgh', path: 'engines/blorgh' ``` Then run `bundle` to install the gem. diff --git a/railties/test/generators/app_generator_test.rb b/railties/test/generators/app_generator_test.rb index daf362357c..e5f10a89d3 100644 --- a/railties/test/generators/app_generator_test.rb +++ b/railties/test/generators/app_generator_test.rb @@ -1,7 +1,6 @@ require 'generators/generators_test_helper' require 'rails/generators/rails/app/app_generator' require 'generators/shared_generator_tests' -require 'mocha/setup' # FIXME: stop using mocha DEFAULT_APP_FILES = %w( .gitignore @@ -117,35 +116,33 @@ class AppGeneratorTest < Rails::Generators::TestCase run_generator [app_root] - Rails.application.config.root = app_moved_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) + stub_rails_application(app_moved_root) do + Rails.application.stub(:is_a?, -> *args { Rails::Application }) do + FileUtils.mv(app_root, app_moved_root) - FileUtils.mv(app_root, app_moved_root) + # make sure we are in correct dir + FileUtils.cd(app_moved_root) - # make sure we are in correct dir - FileUtils.cd(app_moved_root) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, - destination_root: app_moved_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file "myapp_moved/config/environment.rb", /Rails\.application\.initialize!/ - assert_file "myapp_moved/config/initializers/session_store.rb", /_myapp_session/ + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, + destination_root: app_moved_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file "myapp_moved/config/environment.rb", /Rails\.application\.initialize!/ + assert_file "myapp_moved/config/initializers/session_store.rb", /_myapp_session/ + end + end end def test_rails_update_generates_correct_session_key app_root = File.join(destination_root, 'myapp') run_generator [app_root] - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file "myapp/config/initializers/session_store.rb", /_myapp_session/ + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file "myapp/config/initializers/session_store.rb", /_myapp_session/ + end end def test_new_application_use_json_serialzier @@ -158,14 +155,12 @@ class AppGeneratorTest < Rails::Generators::TestCase app_root = File.join(destination_root, 'myapp') run_generator [app_root] - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :json/) + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :json/) + end end def test_rails_update_does_not_create_callback_terminator_initializer @@ -174,14 +169,12 @@ class AppGeneratorTest < Rails::Generators::TestCase FileUtils.rm("#{app_root}/config/initializers/callback_terminator.rb") - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_no_file "#{app_root}/config/initializers/callback_terminator.rb" + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_no_file "#{app_root}/config/initializers/callback_terminator.rb" + end end def test_rails_update_does_not_remove_callback_terminator_initializer_if_already_present @@ -190,14 +183,12 @@ class AppGeneratorTest < Rails::Generators::TestCase FileUtils.touch("#{app_root}/config/initializers/callback_terminator.rb") - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/initializers/callback_terminator.rb" + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file "#{app_root}/config/initializers/callback_terminator.rb" + end end def test_rails_update_set_the_cookie_serializer_to_marchal_if_it_is_not_already_configured @@ -206,14 +197,12 @@ class AppGeneratorTest < Rails::Generators::TestCase FileUtils.rm("#{app_root}/config/initializers/cookies_serializer.rb") - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :marshal/) + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file("#{app_root}/config/initializers/cookies_serializer.rb", /Rails\.application\.config\.action_dispatch\.cookies_serializer = :marshal/) + end end def test_rails_update_does_not_create_active_record_belongs_to_required_by_default @@ -222,14 +211,12 @@ class AppGeneratorTest < Rails::Generators::TestCase FileUtils.rm("#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb") - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_no_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb" + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_no_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb" + end end def test_rails_update_does_not_remove_active_record_belongs_to_required_by_default_if_already_present @@ -238,14 +225,12 @@ class AppGeneratorTest < Rails::Generators::TestCase FileUtils.touch("#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb") - Rails.application.config.root = app_root - Rails.application.class.stubs(:name).returns("Myapp") - Rails.application.stubs(:is_a?).returns(Rails::Application) - - generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell - generator.send(:app_const) - quietly { generator.send(:update_config_files) } - assert_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb" + stub_rails_application(app_root) do + generator = Rails::Generators::AppGenerator.new ["rails"], { with_dispatchers: true }, destination_root: app_root, shell: @shell + generator.send(:app_const) + quietly { generator.send(:update_config_files) } + assert_file "#{app_root}/config/initializers/active_record_belongs_to_required_by_default.rb" + end end def test_application_names_are_not_singularized @@ -456,13 +441,15 @@ class AppGeneratorTest < Rails::Generators::TestCase end def test_usage_read_from_file - File.expects(:read).returns("USAGE FROM FILE") - assert_equal "USAGE FROM FILE", Rails::Generators::AppGenerator.desc + assert_called(File, :read, returns: "USAGE FROM FILE") do + assert_equal "USAGE FROM FILE", Rails::Generators::AppGenerator.desc + end end def test_default_usage - Rails::Generators::AppGenerator.expects(:usage_path).returns(nil) - assert_match(/Create rails files for app generator/, Rails::Generators::AppGenerator.desc) + assert_called(Rails::Generators::AppGenerator, :usage_path, returns: nil) do + assert_match(/Create rails files for app generator/, Rails::Generators::AppGenerator.desc) + end end def test_default_namespace @@ -538,18 +525,31 @@ class AppGeneratorTest < Rails::Generators::TestCase def test_spring_binstubs jruby_skip "spring doesn't run on JRuby" - generator.stubs(:bundle_command).with('install') - generator.expects(:bundle_command).with('exec spring binstub --all').once - quietly { generator.invoke_all } + command_check = -> command do + @binstub_called ||= 0 + + case command + when 'install' + # Called when running bundle, we just want to stub it so nothing to do here. + when 'exec spring binstub --all' + @binstub_called += 1 + assert_equal 1, @binstub_called, "exec spring binstub --all expected to be called once, but was called #{@install_called} times." + end + end + + generator.stub :bundle_command, command_check do + quietly { generator.invoke_all } + end end def test_spring_no_fork jruby_skip "spring doesn't run on JRuby" - Process.stubs(:respond_to?).with(:fork).returns(false) - run_generator + assert_called_with(Process, :respond_to?, [:fork], returns: false) do + run_generator - assert_file "Gemfile" do |content| - assert_no_match(/spring/, content) + assert_file "Gemfile" do |content| + assert_no_match(/spring/, content) + end end end @@ -651,18 +651,37 @@ class AppGeneratorTest < Rails::Generators::TestCase template = %{ after_bundle { run 'echo ran after_bundle' } } template.instance_eval "def read; self; end" # Make the string respond to read - generator([destination_root], template: path).expects(:open).with(path, 'Accept' => 'application/x-thor-template').returns(template) + check_open = -> *args do + assert_equal [ path, 'Accept' => 'application/x-thor-template' ], args + template + end - bundler_first = sequence('bundle, binstubs, after_bundle') - generator.expects(:bundle_command).with('install').once.in_sequence(bundler_first) - generator.expects(:bundle_command).with('exec spring binstub --all').in_sequence(bundler_first) - generator.expects(:run).with('echo ran after_bundle').in_sequence(bundler_first) + sequence = ['install', 'exec spring binstub --all', 'echo ran after_bundle'] + ensure_bundler_first = -> command do + @sequence_step ||= 0 - quietly { generator.invoke_all } + assert_equal sequence[@sequence_step], command, "commands should be called in sequence #{sequence}" + @sequence_step += 1 + end + + generator([destination_root], template: path).stub(:open, check_open, template) do + generator.stub(:bundle_command, ensure_bundler_first) do + generator.stub(:run, ensure_bundler_first) do + quietly { generator.invoke_all } + end + end + end end protected + def stub_rails_application(root) + Rails.application.config.root = root + Rails.application.class.stub(:name, "Myapp") do + yield + end + end + def action(*args, &block) capture(:stdout) { generator.send(*args, &block) } end |