From 2c393330a487b2e2476e6e8ee27711f68df7e90b Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Wed, 29 Nov 2017 14:19:41 +0900 Subject: [PATCH] Override _read_attribute in Dirty plugin This is required due to a fix in AR 5.2: https://github.com/rails/rails/pull/28661 Previously, `__send__` was used to fetch the value of the attribute before changes, which worked fine with locale accessors like `title_en`. However, with the fix above, `@attributes` is now used, which does not contain translated attributes (let alone locale accessors). Since in general, locale accessors are an open-ended set, we can't treat them the way normal attributes are treated, so we resort to overriding `_read_attribute` to check if the attribute that has changed is one that Mobility changed, and if so use `__send__`. This fixes the issue, although overriding a private method is not ideal. --- lib/mobility/plugins/active_model/dirty.rb | 18 +++++++ lib/mobility/plugins/active_record/dirty.rb | 57 +++++++++++++++------ 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/lib/mobility/plugins/active_model/dirty.rb b/lib/mobility/plugins/active_model/dirty.rb index 22b86632b..c6d3640ba 100644 --- a/lib/mobility/plugins/active_model/dirty.rb +++ b/lib/mobility/plugins/active_model/dirty.rb @@ -32,6 +32,7 @@ def write(locale, value, options = {}) if model.changed_attributes.has_key?(locale_accessor) && model.changed_attributes[locale_accessor] == value model.send(:attributes_changed_by_setter).except!(locale_accessor) elsif read(locale, options.merge(fallback: false)) != value + model.send(:mobility_changed_attributes) << locale_accessor model.send(:attribute_will_change!, locale_accessor) end super @@ -63,6 +64,10 @@ def initialize(*attribute_names) private :restore_attribute! end + def included(model_class) + model_class.include ChangedAttributes + end + private # Get method suffixes. Creating an object just to get the list of @@ -74,6 +79,19 @@ def method_suffixes include ::ActiveModel::Dirty end.attribute_method_matchers.map(&:suffix).select { |m| m =~ /\A_/ } end + + # Tracks which translated attributes have been changed, separate from + # the default tracking of changes in ActiveModel/ActiveRecord Dirty. + # This is required in order for the Mobility ActiveRecord Dirty + # plugin to correctly read the value of locale accessors like + # +title_en+ in dirty tracking. + module ChangedAttributes + private + + def mobility_changed_attributes + @mobility_changed_attributes ||= Set.new + end + end end end end diff --git a/lib/mobility/plugins/active_record/dirty.rb b/lib/mobility/plugins/active_record/dirty.rb index 22946d6d9..963d5b988 100644 --- a/lib/mobility/plugins/active_record/dirty.rb +++ b/lib/mobility/plugins/active_record/dirty.rb @@ -34,23 +34,6 @@ def initialize(*attribute_names) define_attribute_methods if ::ActiveRecord::VERSION::STRING >= '5.1' end - # Overrides +ActiveRecord::AttributeMethods::ClassMethods#has_attribute+ to treat fallthrough attribute methods - # just like "real" attribute methods. - # - # @note Patching +has_attribute?+ is necessary as of AR 5.1 due to this commit[https://github.com/rails/rails/commit/4fed08fa787a316fa51f14baca9eae11913f5050]. - # (I have voiced my opposition to this change here[https://github.com/rails/rails/pull/27963#issuecomment-310092787]). - # @param [Attributes] attributes - def included(model_class) - names = @attribute_names - method_name_regex = /\A(#{names.join('|'.freeze)})_([a-z]{2}(_[a-z]{2})?)(=?|\??)\z/.freeze - has_attribute = Module.new do - define_method :has_attribute? do |attr_name| - super(attr_name) || !!method_name_regex.match(attr_name) - end - end - model_class.extend has_attribute - end - private def define_method_overrides @@ -94,6 +77,46 @@ def define_attribute_methods alias_method :"#{name}_in_database", :"#{name}_was" end end + + # Overrides +ActiveRecord::AttributeMethods::ClassMethods#has_attribute+ and + # +ActiveModel::AttributeMethods#_read_attribute+ to treat + # fallthrough attribute methods just like "real" attribute methods. + # + # @note Patching +has_attribute?+ is necessary as of AR 5.1 due to this commit[https://github.com/rails/rails/commit/4fed08fa787a316fa51f14baca9eae11913f5050]. + # (I have voiced my opposition to this change here[https://github.com/rails/rails/pull/27963#issuecomment-310092787]). + # @param [Attributes] attributes + def included(model_class) + super + names = @attribute_names + method_name_regex = /\A(#{names.join('|'.freeze)})_([a-z]{2}(_[a-z]{2})?)(=?|\??)\z/.freeze + has_attribute = Module.new do + define_method :has_attribute? do |attr_name| + super(attr_name) || !!method_name_regex.match(attr_name) + end + end + model_class.extend has_attribute + model_class.include ReadAttribute + end + + # Overrides _read_attribute to correctly dispatch reads on translated + # attributes to their respective setters, rather than to + # +@attributes+, which would otherwise return +nil+. + # + # For background on why this is necessary, see: + # https://github.com/shioyama/mobility/issues/115 + module ReadAttribute + # @note We first check if attributes has the key +attr+ to avoid + # doing any extra work in case this is a "normal" (non-translated) + # attribute. + def _read_attribute(attr, *args) + if @attributes.key?(attr) + super + else + mobility_changed_attributes.include?(attr) ? __send__(attr) : super + end + end + private :_read_attribute + end end end end