diff options
32 files changed, 468 insertions, 131 deletions
diff --git a/actionpack/lib/action_dispatch/journey/formatter.rb b/actionpack/lib/action_dispatch/journey/formatter.rb index a732e570f2..e288f026c7 100644 --- a/actionpack/lib/action_dispatch/journey/formatter.rb +++ b/actionpack/lib/action_dispatch/journey/formatter.rb @@ -3,7 +3,7 @@ require 'action_controller/metal/exceptions' module ActionDispatch module Journey # The Formatter class is used for formatting URLs. For example, parameters - # passed to +url_for+ in rails will eventually call Formatter#generate. + # passed to +url_for+ in Rails will eventually call Formatter#generate. class Formatter # :nodoc: attr_reader :routes diff --git a/actionpack/lib/action_dispatch/middleware/public_exceptions.rb b/actionpack/lib/action_dispatch/middleware/public_exceptions.rb index 53bedaa40a..cbb2d475b1 100644 --- a/actionpack/lib/action_dispatch/middleware/public_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/public_exceptions.rb @@ -7,11 +7,10 @@ module ActionDispatch end def call(env) - exception = env["action_dispatch.exception"] status = env["PATH_INFO"][1..-1] request = ActionDispatch::Request.new(env) content_type = request.formats.first - body = { :status => status, :error => exception.message } + body = { :status => status, :error => Rack::Utils::HTTP_STATUS_CODES.fetch(status.to_i, Rack::Utils::HTTP_STATUS_CODES[500]) } render(status, content_type, body) end @@ -19,7 +18,7 @@ module ActionDispatch private def render(status, content_type, body) - format = content_type && "to_#{content_type.to_sym}" + format = "to_#{content_type.to_sym}" if content_type if format && body.respond_to?(format) render_format(status, content_type, body.public_send(format)) else diff --git a/actionpack/test/controller/show_exceptions_test.rb b/actionpack/test/controller/show_exceptions_test.rb index 69bf4b7720..ff23b22040 100644 --- a/actionpack/test/controller/show_exceptions_test.rb +++ b/actionpack/test/controller/show_exceptions_test.rb @@ -75,7 +75,7 @@ module ShowExceptions get "/", {}, 'HTTP_ACCEPT' => 'application/json' assert_response :internal_server_error assert_equal 'application/json', response.content_type.to_s - assert_equal({ :status => '500', :error => 'boom!' }.to_json, response.body) + assert_equal({ :status => '500', :error => 'Internal Server Error' }.to_json, response.body) end def test_render_xml_exception @@ -83,7 +83,7 @@ module ShowExceptions get "/", {}, 'HTTP_ACCEPT' => 'application/xml' assert_response :internal_server_error assert_equal 'application/xml', response.content_type.to_s - assert_equal({ :status => '500', :error => 'boom!' }.to_xml, response.body) + assert_equal({ :status => '500', :error => 'Internal Server Error' }.to_xml, response.body) end def test_render_fallback_exception diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index 16188aec6b..94d2181e4d 100644 --- a/activemodel/lib/active_model/callbacks.rb +++ b/activemodel/lib/active_model/callbacks.rb @@ -120,30 +120,24 @@ module ActiveModel private def _define_before_model_callback(klass, callback) #:nodoc: - klass.class_eval <<-CALLBACK, __FILE__, __LINE__ + 1 - def self.before_#{callback}(*args, &block) - set_callback(:#{callback}, :before, *args, &block) - end - CALLBACK + klass.define_singleton_method("before_#{callback}") do |*args, &block| + set_callback(:"#{callback}", :before, *args, &block) + end end def _define_around_model_callback(klass, callback) #:nodoc: - klass.class_eval <<-CALLBACK, __FILE__, __LINE__ + 1 - def self.around_#{callback}(*args, &block) - set_callback(:#{callback}, :around, *args, &block) - end - CALLBACK + klass.define_singleton_method("around_#{callback}") do |*args, &block| + set_callback(:"#{callback}", :around, *args, &block) + end end def _define_after_model_callback(klass, callback) #:nodoc: - klass.class_eval <<-CALLBACK, __FILE__, __LINE__ + 1 - def self.after_#{callback}(*args, &block) - options = args.extract_options! - options[:prepend] = true - options[:if] = Array(options[:if]) << "value != false" - set_callback(:#{callback}, :after, *(args << options), &block) - end - CALLBACK + klass.define_singleton_method("after_#{callback}") do |*args, &block| + options = args.extract_options! + options[:prepend] = true + options[:if] = Array(options[:if]) << "value != false" + set_callback(:"#{callback}", :after, *(args << options), &block) + end end end end diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 341e85c59e..a3b27425f7 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -4,7 +4,7 @@ * Fixed a bug in `ActiveRecord#sanitize_sql_hash_for_conditions` in which `self.class` is an argument to `PredicateBuilder#build_from_hash` - causing `PredicateBuilder` to call non-existant method + causing `PredicateBuilder` to call non-existent method `Class#reflect_on_association`. *Zach Ohlgren* diff --git a/activerecord/lib/active_record/associations/builder/has_many.rb b/activerecord/lib/active_record/associations/builder/has_many.rb index 0d1bdd21ee..429def5455 100644 --- a/activerecord/lib/active_record/associations/builder/has_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_many.rb @@ -5,7 +5,7 @@ module ActiveRecord::Associations::Builder end def valid_options - super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :counter_cache] + super + [:primary_key, :dependent, :as, :through, :source, :source_type, :inverse_of, :automatic_inverse_of, :counter_cache] end def valid_dependent_options diff --git a/activerecord/lib/active_record/associations/builder/singular_association.rb b/activerecord/lib/active_record/associations/builder/singular_association.rb index 6a5830e57f..f06426a09d 100644 --- a/activerecord/lib/active_record/associations/builder/singular_association.rb +++ b/activerecord/lib/active_record/associations/builder/singular_association.rb @@ -1,7 +1,7 @@ module ActiveRecord::Associations::Builder class SingularAssociation < Association #:nodoc: def valid_options - super + [:remote, :dependent, :counter_cache, :primary_key, :inverse_of] + super + [:remote, :dependent, :counter_cache, :primary_key, :inverse_of, :automatic_inverse_of] end def constructable? diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 021832de46..8bdaeef924 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -305,6 +305,11 @@ module ActiveRecord reflection.options[:autosave] = true add_autosave_association_callbacks(reflection) + # Clear cached values of any inverse associations found in the + # reflection and prevent the reflection from finding inverses + # automatically in the future. + reflection.remove_automatic_inverse_of! + nested_attributes_options = self.nested_attributes_options.dup nested_attributes_options[association_name.to_sym] = options self.nested_attributes_options = nested_attributes_options diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 60eda96f08..0ba860a186 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -181,6 +181,7 @@ module ActiveRecord def initialize(*args) super @collection = [:has_many, :has_and_belongs_to_many].include?(macro) + @automatic_inverse_of = nil end # Returns a new, unsaved instance of the associated class. +attributes+ will @@ -289,15 +290,32 @@ module ActiveRecord alias :source_macro :macro def has_inverse? - @options[:inverse_of] + @options[:inverse_of] || find_inverse_of_automatically end def inverse_of - if has_inverse? - @inverse_of ||= klass.reflect_on_association(options[:inverse_of]) + @inverse_of ||= if options[:inverse_of] + klass.reflect_on_association(options[:inverse_of]) + else + find_inverse_of_automatically end end + # Clears the cached value of +@inverse_of+ on this object. This will + # not remove the :inverse_of option however, so future calls on the + # +inverse_of+ will have to recompute the inverse. + def clear_inverse_of_cache! + @inverse_of = nil + end + + # Removes the cached inverse association that was found automatically + # and prevents this object from finding the inverse association + # automatically in the future. + def remove_automatic_inverse_of! + @automatic_inverse_of = nil + options[:automatic_inverse_of] = false + end + def polymorphic_inverse_of(associated_class) if has_inverse? if inverse_relationship = associated_class.reflect_on_association(options[:inverse_of]) @@ -366,7 +384,84 @@ module ActiveRecord options.key? :polymorphic end + VALID_AUTOMATIC_INVERSE_MACROS = [:has_many, :has_one, :belongs_to] + INVALID_AUTOMATIC_INVERSE_OPTIONS = [:conditions, :through, :polymorphic, :foreign_key] + private + # Attempts to find the inverse association automatically. + # If it cannot find a suitable inverse association, it returns + # nil. + def find_inverse_of_automatically + if @automatic_inverse_of == false + nil + elsif @automatic_inverse_of.nil? + set_automatic_inverse_of + else + klass.reflect_on_association(@automatic_inverse_of) + end + end + + # Sets the +@automatic_inverse_of+ instance variable, and returns + # either nil or the inverse association that it finds. + # + # This method caches the inverse association that is found so that + # future calls to +find_inverse_of_automatically+ have much less + # overhead. + def set_automatic_inverse_of + if can_find_inverse_of_automatically?(self) + inverse_name = active_record.name.downcase.to_sym + + begin + reflection = klass.reflect_on_association(inverse_name) + rescue NameError + # Give up: we couldn't compute the klass type so we won't be able + # to find any associations either. + reflection = false + end + + if valid_inverse_reflection?(reflection) + @automatic_inverse_of = inverse_name + reflection + else + @automatic_inverse_of = false + nil + end + else + @automatic_inverse_of = false + nil + end + end + + # Checks if the inverse reflection that is returned from the + # +set_automatic_inverse_of+ method is a valid reflection. We must + # make sure that the reflection's active_record name matches up + # with the current reflection's klass name. + # + # Note: klass will always be valid because when there's a NameError + # from calling +klass+, +reflection+ will already be set to false. + def valid_inverse_reflection?(reflection) + reflection && + klass.name == reflection.active_record.try(:name) && + klass.primary_key == reflection.active_record_primary_key && + can_find_inverse_of_automatically?(reflection) + end + + # Checks to see if the reflection doesn't have any options that prevent + # us from being able to guess the inverse automatically. First, the + # +automatic_inverse_of+ option cannot be set to false. Second, we must + # have :has_many, :has_one, :belongs_to associations. Third, we must + # not have options such as :class_name or :polymorphic which prevent us + # from correctly guessing the inverse association. + # + # Anything with a scope can additionally ruin our attempt at finding an + # inverse, so we exclude reflections with scopes. + def can_find_inverse_of_automatically?(reflection) + reflection.options[:automatic_inverse_of] != false && + VALID_AUTOMATIC_INVERSE_MACROS.include?(reflection.macro) && + !INVALID_AUTOMATIC_INVERSE_OPTIONS.any? { |opt| reflection.options[opt] } && + !reflection.scope + end + def derive_class_name class_name = name.to_s.camelize class_name = class_name.singularize if collection? diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 936b83261e..bda7a76330 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -139,21 +139,20 @@ module ActiveRecord if values[:where].empty? || relation.where_values.empty? relation.where_values + values[:where] else - # Remove equalities from the existing relation with a LHS which is - # present in the relation being merged in. + sanitized_wheres + values[:where] + end + end - seen = Set.new - values[:where].each { |w| - if w.respond_to?(:operator) && w.operator == :== - seen << w.left - end - } + # Remove equalities from the existing relation with a LHS which is + # present in the relation being merged in. + def sanitized_wheres + seen = Set.new + values[:where].each do |w| + seen << w.left if w.respond_to?(:operator) && w.operator == :== + end - relation.where_values.reject { |w| - w.respond_to?(:operator) && - w.operator == :== && - seen.include?(w.left) - } + values[:where] + relation.where_values.reject do |w| + w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left) end end end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index ec128acf28..b1f0be3204 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -5,6 +5,88 @@ require 'models/interest' require 'models/zine' require 'models/club' require 'models/sponsor' +require 'models/rating' +require 'models/comment' +require 'models/car' +require 'models/bulb' + +class AutomaticInverseFindingTests < ActiveRecord::TestCase + fixtures :ratings, :comments, :cars + + def test_has_one_and_belongs_to_should_find_inverse_automatically + car_reflection = Car.reflect_on_association(:bulb) + bulb_reflection = Bulb.reflect_on_association(:car) + + assert_respond_to car_reflection, :has_inverse? + assert car_reflection.has_inverse?, "The Car reflection should have an inverse" + assert_equal bulb_reflection, car_reflection.inverse_of, "The Car reflection's inverse should be the Bulb reflection" + + assert_respond_to bulb_reflection, :has_inverse? + assert bulb_reflection.has_inverse?, "The Bulb reflection should have an inverse" + assert_equal car_reflection, bulb_reflection.inverse_of, "The Bulb reflection's inverse should be the Car reflection" + end + + def test_has_many_and_belongs_to_should_find_inverse_automatically + comment_reflection = Comment.reflect_on_association(:ratings) + rating_reflection = Rating.reflect_on_association(:comment) + + assert_respond_to comment_reflection, :has_inverse? + assert comment_reflection.has_inverse?, "The Comment reflection should have an inverse" + assert_equal rating_reflection, comment_reflection.inverse_of, "The Comment reflection's inverse should be the Rating reflection" + end + + def test_has_one_and_belongs_to_automatic_inverse_shares_objects + car = Car.first + bulb = Bulb.create!(car: car) + + assert_equal car.bulb, bulb, "The Car's bulb should be the original bulb" + + car.bulb.color = "Blue" + assert_equal car.bulb.color, bulb.color, "Changing the bulb's color on the car association should change the bulb's color" + + bulb.color = "Red" + assert_equal bulb.color, car.bulb.color, "Changing the bulb's color should change the bulb's color on the car association" + end + + def test_has_many_and_belongs_to_automatic_inverse_shares_objects_on_rating + comment = Comment.first + rating = Rating.create!(comment: comment) + + assert_equal rating.comment, comment, "The Rating's comment should be the original Comment" + + rating.comment.body = "Brogramming is the act of programming, like a bro." + assert_equal rating.comment.body, comment.body, "Changing the Comment's body on the association should change the original Comment's body" + + comment.body = "Broseiden is the king of the sea of bros." + assert_equal comment.body, rating.comment.body, "Changing the original Comment's body should change the Comment's body on the association" + end + + def test_has_many_and_belongs_to_automatic_inverse_shares_objects_on_comment + rating = Rating.create! + comment = Comment.first + rating.comment = comment + + assert_equal rating.comment, comment, "The Rating's comment should be the original Comment" + + rating.comment.body = "Brogramming is the act of programming, like a bro." + assert_equal rating.comment.body, comment.body, "Changing the Comment's body on the association should change the original Comment's body" + + comment.body = "Broseiden is the king of the sea of bros." + assert_equal comment.body, rating.comment.body, "Changing the original Comment's body should change the Comment's body on the association" + end + + def test_polymorphic_and_has_many_through_relationships_should_not_have_inverses + sponsor_reflection = Sponsor.reflect_on_association(:sponsorable) + + assert_respond_to sponsor_reflection, :has_inverse? + assert !sponsor_reflection.has_inverse?, "A polymorphic association should not find an inverse automatically" + + club_reflection = Club.reflect_on_association(:members) + + assert_respond_to club_reflection, :has_inverse? + assert !club_reflection.has_inverse?, "A has_many_through association should not find an inverse automatically" + end +end class InverseAssociationTests < ActiveRecord::TestCase def test_should_allow_for_inverse_of_options_in_associations diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 165b7454b7..6fe81e0d96 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -800,7 +800,9 @@ module NestedAttributesOnACollectionAssociationTests def test_validate_presence_of_parent_fails_without_inverse_of Man.accepts_nested_attributes_for(:interests) Man.reflect_on_association(:interests).options.delete(:inverse_of) + Man.reflect_on_association(:interests).clear_inverse_of_cache! Interest.reflect_on_association(:man).options.delete(:inverse_of) + Interest.reflect_on_association(:man).clear_inverse_of_cache! repair_validations(Interest) do Interest.validates_presence_of(:man) diff --git a/activerecord/test/cases/sanitize_test.rb b/activerecord/test/cases/sanitize_test.rb index f061c28e88..082570c55b 100644 --- a/activerecord/test/cases/sanitize_test.rb +++ b/activerecord/test/cases/sanitize_test.rb @@ -6,11 +6,10 @@ class SanitizeTest < ActiveRecord::TestCase end def test_sanitize_sql_hash_handles_associations - if current_adapter?(:MysqlAdapter, :Mysql2Adapter) - expected_value = "`adorable_animals`.`name` = 'Bambi'" - else - expected_value = "\"adorable_animals\".\"name\" = 'Bambi'" - end + quoted_bambi = ActiveRecord::Base.connection.quote("Bambi") + quoted_column_name = ActiveRecord::Base.connection.quote_column_name("name") + quoted_table_name = ActiveRecord::Base.connection.quote_table_name("adorable_animals") + expected_value = "#{quoted_table_name}.#{quoted_column_name} = #{quoted_bambi}" assert_equal expected_value, Binary.send(:sanitize_sql_hash, {adorable_animals: {name: 'Bambi'}}) end diff --git a/activerecord/test/models/club.rb b/activerecord/test/models/club.rb index 24a65b0f2f..7d7c205041 100644 --- a/activerecord/test/models/club.rb +++ b/activerecord/test/models/club.rb @@ -1,6 +1,6 @@ class Club < ActiveRecord::Base has_one :membership - has_many :memberships + has_many :memberships, :automatic_inverse_of => false has_many :members, :through => :memberships has_many :current_memberships has_one :sponsor diff --git a/activerecord/test/models/interest.rb b/activerecord/test/models/interest.rb index d5d9226204..f772bb1c7f 100644 --- a/activerecord/test/models/interest.rb +++ b/activerecord/test/models/interest.rb @@ -1,5 +1,5 @@ class Interest < ActiveRecord::Base - belongs_to :man, :inverse_of => :interests + belongs_to :man, :inverse_of => :interests, :automatic_inverse_of => false belongs_to :polymorphic_man, :polymorphic => true, :inverse_of => :polymorphic_interests belongs_to :zine, :inverse_of => :interests end diff --git a/activerecord/test/models/man.rb b/activerecord/test/models/man.rb index 4bff92dc98..49f002aa9a 100644 --- a/activerecord/test/models/man.rb +++ b/activerecord/test/models/man.rb @@ -1,7 +1,7 @@ class Man < ActiveRecord::Base has_one :face, :inverse_of => :man has_one :polymorphic_face, :class_name => 'Face', :as => :polymorphic_man, :inverse_of => :polymorphic_man - has_many :interests, :inverse_of => :man + has_many :interests, :inverse_of => :man, :automatic_inverse_of => false has_many :polymorphic_interests, :class_name => 'Interest', :as => :polymorphic_man, :inverse_of => :polymorphic_man # These are "broken" inverse_of associations for the purposes of testing has_one :dirty_face, :class_name => 'Face', :inverse_of => :dirty_man diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index 1134b09d8b..b81304b8e0 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -9,7 +9,7 @@ class Member < ActiveRecord::Base has_one :hairy_club, -> { where :clubs => {:name => "Moustache and Eyebrow Fancier Club"} }, :through => :membership, :source => :club has_one :sponsor, :as => :sponsorable has_one :sponsor_club, :through => :sponsor - has_one :member_detail + has_one :member_detail, :automatic_inverse_of => false has_one :organization, :through => :member_detail belongs_to :member_type diff --git a/activerecord/test/models/member_detail.rb b/activerecord/test/models/member_detail.rb index fe619f8732..a256c73c7e 100644 --- a/activerecord/test/models/member_detail.rb +++ b/activerecord/test/models/member_detail.rb @@ -1,5 +1,5 @@ class MemberDetail < ActiveRecord::Base - belongs_to :member + belongs_to :member, :automatic_inverse_of => false belongs_to :organization has_one :member_type, :through => :member diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 1dcacf0b12..6fe7e0f4fb 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -91,7 +91,26 @@ module ActiveSupport class Callback #:nodoc:# @@_callback_sequence = 0 - attr_accessor :chain, :filter, :kind, :options, :klass, :raw_filter + class Basic < Callback + end + + class Object < Callback + def duplicates?(other) + false + end + end + + def self.build(chain, filter, kind, options, _klass) + klass = case filter + when Array, Symbol, String + Callback::Basic + else + Callback::Object + end + klass.new chain, filter, kind, options, _klass + end + + attr_accessor :chain, :kind, :options, :klass, :raw_filter def initialize(chain, filter, kind, options, klass) @chain, @kind, @klass = chain, kind, klass @@ -99,10 +118,15 @@ module ActiveSupport normalize_options!(options) @raw_filter, @options = filter, options - @filter = _compile_filter(filter) + @key = compute_identifier filter + @source = _compile_source(filter) recompile_options! end + def filter + @key + end + def deprecate_per_key_option(options) if options[:per_key] raise NotImplementedError, ":per_key option is no longer supported. Use generic :if and :unless options instead." @@ -133,16 +157,12 @@ module ActiveSupport end def matches?(_kind, _filter) - if @_is_object_filter && !_filter.is_a?(String) - _filter_matches = @filter.to_s.start_with?(_method_name_for_object_filter(_kind, _filter, false)) - else - _filter_matches = (@filter == _filter) - end - - @kind == _kind && _filter_matches + @kind == _kind && filter == _filter end def duplicates?(other) + return false unless self.class == other.class + matches?(other.kind, other.filter) end @@ -167,7 +187,7 @@ module ActiveSupport # This double assignment is to prevent warnings in 1.9.3 as # the `result` variable is not always used except if the # terminator code refers to it. - result = result = #{@filter} + result = result = #{@source} halted = (#{chain.config[:terminator]}) if halted halted_callback_hook(#{@raw_filter.inspect.inspect}) @@ -179,7 +199,7 @@ module ActiveSupport <<-RUBY_EVAL #{code} if #{!chain.config[:skip_after_callbacks_if_terminated] || "!halted"} && #{@compiled_options} - #{@filter} + #{@source} end RUBY_EVAL when :around @@ -195,6 +215,15 @@ module ActiveSupport private + def compute_identifier(filter) + case filter + when String, ::Proc + filter.object_id + else + filter + end + end + # Compile around filters with conditions into proxy methods # that contain the conditions. # @@ -214,7 +243,7 @@ module ActiveSupport @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1 def #{name}(halted) if #{@compiled_options} && !halted - #{@filter} do + #{@source} do yield self end else @@ -232,11 +261,11 @@ module ActiveSupport conditions = ["true"] unless options[:if].empty? - conditions << Array(_compile_filter(options[:if])) + conditions << Array(_compile_source(options[:if])) end unless options[:unless].empty? - conditions << Array(_compile_filter(options[:unless])).map {|f| "!#{f}"} + conditions << Array(_compile_source(options[:unless])).map {|f| "!#{f}"} end @compiled_options = conditions.flatten.join(" && ") @@ -272,30 +301,27 @@ module ActiveSupport # Objects:: # a method is created that calls the before_foo method # on the object. - def _compile_filter(filter) - @_is_object_filter = false - + def _compile_source(filter) case filter when Array - filter.map {|f| _compile_filter(f)} + filter.map {|f| _compile_source(f)} when Symbol filter when String "(#{filter})" - when Proc + when ::Proc method_name = "_callback_#{@kind}_#{next_id}" @klass.send(:define_method, method_name, &filter) return method_name if filter.arity <= 0 - method_name << (filter.arity == 1 ? "(self)" : " self, Proc.new ") + method_name << (filter.arity == 1 ? "(self)" : "(self, ::Proc.new)") else method_name = _method_name_for_object_filter(kind, filter) - @_is_object_filter = true @klass.send(:define_method, "#{method_name}_object") { filter } _normalize_legacy_filter(kind, filter) scopes = Array(chain.config[:scope]) - method_to_call = scopes.map{ |s| s.is_a?(Symbol) ? send(s) : s }.join("_") + method_to_call = scopes.map{ |s| public_send(s) }.join("_") @klass.class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1 def #{method_name}(&blk) @@ -465,7 +491,7 @@ module ActiveSupport __update_callbacks(name, filter_list, block) do |target, chain, type, filters, options| mapped ||= filters.map do |filter| - Callback.new(chain, filter, type, options.dup, self) + Callback.build(chain, filter, type, options.dup, self) end options[:prepend] ? chain.prepend(*mapped) : chain.append(*mapped) diff --git a/activesupport/lib/active_support/core_ext/hash/diff.rb b/activesupport/lib/active_support/core_ext/hash/diff.rb index 5f3868b5b0..4359213380 100644 --- a/activesupport/lib/active_support/core_ext/hash/diff.rb +++ b/activesupport/lib/active_support/core_ext/hash/diff.rb @@ -1,3 +1,5 @@ +require 'active_support/deprecation' + class Hash # Returns a hash that represents the difference between two hashes. # diff --git a/activesupport/lib/active_support/i18n.rb b/activesupport/lib/active_support/i18n.rb index 22521a8e93..6cc98191d4 100644 --- a/activesupport/lib/active_support/i18n.rb +++ b/activesupport/lib/active_support/i18n.rb @@ -1,13 +1,13 @@ +require 'active_support/core_ext/hash/deep_merge' +require 'active_support/core_ext/hash/except' +require 'active_support/core_ext/hash/slice' begin - require 'active_support/core_ext/hash/deep_merge' - require 'active_support/core_ext/hash/except' - require 'active_support/core_ext/hash/slice' require 'i18n' - require 'active_support/lazy_load_hooks' rescue LoadError => e $stderr.puts "The i18n gem is not available. Please add it to your Gemfile and run bundle install" raise e end +require 'active_support/lazy_load_hooks' ActiveSupport.run_load_hooks(:i18n) I18n.load_path << "#{File.dirname(__FILE__)}/locale/en.yml" diff --git a/activesupport/lib/active_support/key_generator.rb b/activesupport/lib/active_support/key_generator.rb index 37124fb7ae..598c46bce5 100644 --- a/activesupport/lib/active_support/key_generator.rb +++ b/activesupport/lib/active_support/key_generator.rb @@ -4,7 +4,7 @@ require 'openssl' module ActiveSupport # KeyGenerator is a simple wrapper around OpenSSL's implementation of PBKDF2 # It can be used to derive a number of keys for various purposes from a given secret. - # This lets rails applications have a single secure secret, but avoid reusing that + # This lets Rails applications have a single secure secret, but avoid reusing that # key in multiple incompatible contexts. class KeyGenerator def initialize(secret, options = {}) diff --git a/activesupport/lib/active_support/lazy_load_hooks.rb b/activesupport/lib/active_support/lazy_load_hooks.rb index e489512531..e2b8f0f648 100644 --- a/activesupport/lib/active_support/lazy_load_hooks.rb +++ b/activesupport/lib/active_support/lazy_load_hooks.rb @@ -1,5 +1,5 @@ module ActiveSupport - # lazy_load_hooks allows rails to lazily load a lot of components and thus + # lazy_load_hooks allows Rails to lazily load a lot of components and thus # making the app boot faster. Because of this feature now there is no need to # require <tt>ActiveRecord::Base</tt> at boot time purely to apply # configuration. Instead a hook is registered that applies configuration once diff --git a/activesupport/lib/active_support/multibyte/unicode.rb b/activesupport/lib/active_support/multibyte/unicode.rb index cbc1608349..f1dfff738c 100644 --- a/activesupport/lib/active_support/multibyte/unicode.rb +++ b/activesupport/lib/active_support/multibyte/unicode.rb @@ -218,51 +218,31 @@ module ActiveSupport # Passing +true+ will forcibly tidy all bytes, assuming that the string's # encoding is entirely CP1252 or ISO-8859-1. def tidy_bytes(string, force = false) + return string if string.empty? + if force - return string.unpack("C*").map do |b| - tidy_byte(b) - end.flatten.compact.pack("C*").unpack("U*").pack("U*") + return string.encode(Encoding::UTF_8, Encoding::Windows_1252, invalid: :replace, undef: :replace) end - bytes = string.unpack("C*") - conts_expected = 0 - last_lead = 0 - - bytes.each_index do |i| + # We can't transcode to the same format, so we choose a nearly-identical encoding. + # We're going to 'transcode' bytes from UTF-8 when possible, then fall back to + # CP1252 when we get errors. The final string will be 'converted' back to UTF-8 + # before returning. + reader = Encoding::Converter.new(Encoding::UTF_8, Encoding::UTF_8_MAC) - byte = bytes[i] - is_cont = byte > 127 && byte < 192 - is_lead = byte > 191 && byte < 245 - is_unused = byte > 240 - is_restricted = byte > 244 + source = string.dup + out = ''.force_encoding(Encoding::UTF_8_MAC) - # Impossible or highly unlikely byte? Clean it. - if is_unused || is_restricted - bytes[i] = tidy_byte(byte) - elsif is_cont - # Not expecting continuation byte? Clean up. Otherwise, now expect one less. - conts_expected == 0 ? bytes[i] = tidy_byte(byte) : conts_expected -= 1 - else - if conts_expected > 0 - # Expected continuation, but got ASCII or leading? Clean backwards up to - # the leading byte. - (1..(i - last_lead)).each {|j| bytes[i - j] = tidy_byte(bytes[i - j])} - conts_expected = 0 - end - if is_lead - # Final byte is leading? Clean it. - if i == bytes.length - 1 - bytes[i] = tidy_byte(bytes.last) - else - # Valid leading byte? Expect continuations determined by position of - # first zero bit, with max of 3. - conts_expected = byte < 224 ? 1 : byte < 240 ? 2 : 3 - last_lead = i - end - end - end + loop do + reader.primitive_convert(source, out) + _, _, _, error_bytes, _ = reader.primitive_errinfo + break if error_bytes.nil? + out << error_bytes.encode(Encoding::UTF_8_MAC, Encoding::Windows_1252, invalid: :replace, undef: :replace) end - bytes.empty? ? "" : bytes.flatten.compact.pack("C*").unpack("U*").pack("U*") + + reader.finish + + out.encode!(Encoding::UTF_8) end # Returns the KC normalization of the string by default. NFKC is diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index f71e780c42..9659f141cb 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -801,4 +801,159 @@ module CallbacksTest assert_equal ["two", "one", "three", "yielded"], model.record end end + + class ConditionalTests < ActiveSupport::TestCase + def build_class(callback) + Class.new { + include ActiveSupport::Callbacks + define_callbacks :foo + set_callback :foo, :before, :foo, :if => callback + def foo; end + def run; run_callbacks :foo; end + } + end + + # FIXME: do we really want to support classes as conditionals? There were + # no tests for it previous to this. + def test_class_conditional_with_scope + z = [] + callback = Class.new { + define_singleton_method(:foo) { |o| z << o } + } + klass = Class.new { + include ActiveSupport::Callbacks + define_callbacks :foo, :scope => [:name] + set_callback :foo, :before, :foo, :if => callback + def foo; end + def run; run_callbacks :foo; end + } + object = klass.new + object.run + assert_equal [object], z + end + + # FIXME: do we really want to support classes as conditionals? There were + # no tests for it previous to this. + def test_class + z = [] + klass = build_class Class.new { + define_singleton_method(:before) { |o| z << o } + } + object = klass.new + object.run + assert_equal [object], z + end + + def test_proc_negative_arity # passes an empty list if *args + z = [] + object = build_class(->(*args) { z << args }).new + object.run + assert_equal [], z.flatten + end + + def test_proc_arity0 + z = [] + object = build_class(->() { z << 0 }).new + object.run + assert_equal [0], z + end + + def test_proc_arity1 + z = [] + object = build_class(->(x) { z << x }).new + object.run + assert_equal [object], z + end + + def test_proc_arity2 + assert_raises(ArgumentError) do + object = build_class(->(a,b) { }).new + object.run + end + end + end + + class CallbackTypeTest < ActiveSupport::TestCase + def build_class(callback, n = 10) + Class.new { + include ActiveSupport::Callbacks + define_callbacks :foo + n.times { set_callback :foo, :before, callback } + def run; run_callbacks :foo; end + def self.skip(thing); skip_callback :foo, :before, thing; end + } + end + + def test_add_class + calls = [] + callback = Class.new { + define_singleton_method(:before) { |o| calls << o } + } + build_class(callback).new.run + assert_equal 10, calls.length + end + + def test_add_lambda + calls = [] + build_class(->(o) { calls << o }).new.run + assert_equal 10, calls.length + end + + def test_add_symbol + calls = [] + klass = build_class(:bar) + klass.class_eval { define_method(:bar) { calls << klass } } + klass.new.run + assert_equal 1, calls.length + end + + def test_add_eval + calls = [] + klass = build_class("bar") + klass.class_eval { define_method(:bar) { calls << klass } } + klass.new.run + assert_equal 1, calls.length + end + + def test_skip_class # removes one at a time + calls = [] + callback = Class.new { + define_singleton_method(:before) { |o| calls << o } + } + klass = build_class(callback) + 9.downto(0) { |i| + klass.skip callback + klass.new.run + assert_equal i, calls.length + calls.clear + } + end + + def test_skip_lambda # removes nothing + calls = [] + callback = ->(o) { calls << o } + klass = build_class(callback) + 10.times { klass.skip callback } + klass.new.run + assert_equal 10, calls.length + end + + def test_skip_symbol # removes all + calls = [] + klass = build_class(:bar) + klass.class_eval { define_method(:bar) { calls << klass } } + klass.skip :bar + klass.new.run + assert_equal 0, calls.length + end + + def test_skip_eval # removes nothing + calls = [] + klass = build_class("bar") + klass.class_eval { define_method(:bar) { calls << klass } } + klass.skip "bar" + klass.new.run + assert_equal 1, calls.length + end + end end diff --git a/activesupport/test/dependencies_test.rb b/activesupport/test/dependencies_test.rb index 115a4e894d..4b1426bb2e 100644 --- a/activesupport/test/dependencies_test.rb +++ b/activesupport/test/dependencies_test.rb @@ -526,7 +526,6 @@ class DependenciesTest < ActiveSupport::TestCase m = Module.new m.module_eval "def a() CountingLoader; end" extend m - kls = nil with_autoloading_fixtures do kls = nil assert_nothing_raised { kls = a } diff --git a/activesupport/test/notifications_test.rb b/activesupport/test/notifications_test.rb index d63c59883a..33627a4e74 100644 --- a/activesupport/test/notifications_test.rb +++ b/activesupport/test/notifications_test.rb @@ -99,7 +99,7 @@ module Notifications @notifier.publish :foo @notifier.publish :foo - @notifier.subscribe("not_existant") do |*args| + @notifier.subscribe("not_existent") do |*args| @events << ActiveSupport::Notifications::Event.new(*args) end diff --git a/guides/code/getting_started/config/environment.rb b/guides/code/getting_started/config/environment.rb index 2d65111004..e7e341c960 100644 --- a/guides/code/getting_started/config/environment.rb +++ b/guides/code/getting_started/config/environment.rb @@ -1,5 +1,5 @@ -# Load the rails application. +# Load the Rails application. require File.expand_path('../application', __FILE__) -# Initialize the rails application. +# Initialize the Rails application. Blog::Application.initialize! diff --git a/railties/lib/rails/engine.rb b/railties/lib/rails/engine.rb index bfb9b456db..8000fc3b1e 100644 --- a/railties/lib/rails/engine.rb +++ b/railties/lib/rails/engine.rb @@ -447,7 +447,7 @@ module Rails self end - # Load rails generators and invoke the registered hooks. + # Load Rails generators and invoke the registered hooks. # Check <tt>Rails::Railtie.generators</tt> for more info. def load_generators(app=self) require "rails/generators" diff --git a/railties/lib/rails/engine/commands.rb b/railties/lib/rails/engine/commands.rb index 072d16291b..f39f926109 100644 --- a/railties/lib/rails/engine/commands.rb +++ b/railties/lib/rails/engine/commands.rb @@ -27,7 +27,7 @@ else puts <<-EOT Usage: rails COMMAND [ARGS] -The common rails commands available for engines are: +The common Rails commands available for engines are: generate Generate new code (short-cut alias: "g") destroy Undo code generated with "generate" (short-cut alias: "d") diff --git a/railties/lib/rails/generators/rails/app/templates/config/environment.rb b/railties/lib/rails/generators/rails/app/templates/config/environment.rb index 00a613ff04..ee8d90dc65 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/environment.rb +++ b/railties/lib/rails/generators/rails/app/templates/config/environment.rb @@ -1,5 +1,5 @@ -# Load the rails application. +# Load the Rails application. require File.expand_path('../application', __FILE__) -# Initialize the rails application. +# Initialize the Rails application. Rails.application.initialize! diff --git a/railties/test/generators/shared_generator_tests.rb b/railties/test/generators/shared_generator_tests.rb index 2724882a23..369a0ee46c 100644 --- a/railties/test/generators/shared_generator_tests.rb +++ b/railties/test/generators/shared_generator_tests.rb @@ -77,9 +77,9 @@ module SharedGeneratorTests end def test_template_raises_an_error_with_invalid_path - content = capture(:stderr){ run_generator([destination_root, "-m", "non/existant/path"]) } + content = capture(:stderr){ run_generator([destination_root, "-m", "non/existent/path"]) } assert_match(/The template \[.*\] could not be loaded/, content) - assert_match(/non\/existant\/path/, content) + assert_match(/non\/existent\/path/, content) end def test_template_is_executed_when_supplied |