From ddd75a363e4990a29ab54a645e1891440f6054e0 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Wed, 13 Dec 2017 12:06:13 -0700 Subject: [PATCH 1/3] Avoid changing method visibility when deprecating a method ActiveSupport::Deprecation::MethodWrapper#deprecate_methods changes method visibility to `public`. This seems unintentional and I will submit a PR to Rails to see if that can be updated. For the meanwhile, this update changes method visibility back to the original visibility. It's a bit more convoluted than might be expected because ActiveSupport uses `Module.prepend` to override the original method, so we can't just write `private :some_method` because that only affects the original method, whose visibility has not been changed. --- .../app/controllers/spree/admin/payment_methods_controller.rb | 2 ++ backend/app/controllers/spree/admin/resource_controller.rb | 2 ++ core/app/models/spree/order.rb | 1 + core/app/models/spree/payment_method.rb | 1 + core/spec/models/spree/order_spec.rb | 2 +- 5 files changed, 7 insertions(+), 1 deletion(-) diff --git a/backend/app/controllers/spree/admin/payment_methods_controller.rb b/backend/app/controllers/spree/admin/payment_methods_controller.rb index fe78bc85387..13c9e7c2347 100644 --- a/backend/app/controllers/spree/admin/payment_methods_controller.rb +++ b/backend/app/controllers/spree/admin/payment_methods_controller.rb @@ -52,6 +52,7 @@ def load_providers load_payment_method_types end deprecate load_providers: :load_payment_method_types, deprecator: Spree::Deprecation + instance_method(:load_providers).owner.send(:private, :load_providers) def load_payment_method_types @payment_method_types = Rails.application.config.spree.payment_methods.sort_by(&:name) @@ -64,6 +65,7 @@ def validate_payment_provider end deprecate validate_payment_provider: :validate_payment_method_type, deprecator: Spree::Deprecation + instance_method(:validate_payment_provider).owner.send(:private, :validate_payment_provider) def validate_payment_method_type requested_type = params[:payment_method].delete(:type) diff --git a/backend/app/controllers/spree/admin/resource_controller.rb b/backend/app/controllers/spree/admin/resource_controller.rb index 694766bf11e..9eba0d3fcd8 100644 --- a/backend/app/controllers/spree/admin/resource_controller.rb +++ b/backend/app/controllers/spree/admin/resource_controller.rb @@ -136,6 +136,7 @@ def parent_model_name alias_method :model_name, :parent_model_name deprecate model_name: :parent_model_name, deprecator: Spree::Deprecation + instance_method(:model_name).owner.send(:private, :model_name) def object_name controller_name.singularize @@ -173,6 +174,7 @@ def parent_data self.class.parent_data end deprecate :parent_data, deprecator: Spree::Deprecation + instance_method(:parent_data).owner.send(:private, :parent_data) def parent if parent? diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 96486c522d7..95fe345f03a 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -819,6 +819,7 @@ def update_params_payment_source end end deprecate update_params_payment_source: :set_payment_parameters_amount, deprecator: Spree::Deprecation + instance_method(:update_params_payment_source).owner.send(:private, :update_params_payment_source) def associate_store self.store ||= Spree::Store.default diff --git a/core/app/models/spree/payment_method.rb b/core/app/models/spree/payment_method.rb index 4d2c09b1432..70f7560ce6a 100644 --- a/core/app/models/spree/payment_method.rb +++ b/core/app/models/spree/payment_method.rb @@ -259,5 +259,6 @@ def gateway_class end end deprecate provider_class: :gateway_class, deprecator: Spree::Deprecation + instance_method(:provider_class).owner.send(:protected, :provider_class) end end diff --git a/core/spec/models/spree/order_spec.rb b/core/spec/models/spree/order_spec.rb index a92e74d2f22..dbe3463f5e8 100644 --- a/core/spec/models/spree/order_spec.rb +++ b/core/spec/models/spree/order_spec.rb @@ -1560,7 +1560,7 @@ def generate it 'is deprecated' do subject.instance_variable_set('@updating_params', {}) expect(Spree::Deprecation).to receive(:warn) - subject.update_params_payment_source + subject.send(:update_params_payment_source) end end From 0f71fd867f31bb182b8c481d98b0ccf8df0fd3c6 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 21 Dec 2017 09:39:49 -0700 Subject: [PATCH 2/3] Remove broken deprecation on Spree::PaymentMethod#provider_class Simply calling `deprecate` created the `provider_class` method on the base class (and all subclasses) but it did so in a way that would blow up with an error like: NoMethodError: super: no superclass method `provider_class' Defining the method also messed up the check on `respond_to? :provider_class` in the `gateway_class` method. Also, calling `deprecate` in the base class would not generate deprecation warnings for any `provider_class` methods defined in subclasses. --- core/app/models/spree/payment_method.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/app/models/spree/payment_method.rb b/core/app/models/spree/payment_method.rb index 70f7560ce6a..1025feac2e2 100644 --- a/core/app/models/spree/payment_method.rb +++ b/core/app/models/spree/payment_method.rb @@ -258,7 +258,5 @@ def gateway_class raise ::NotImplementedError, "You must implement gateway_class method for #{self.class}." end end - deprecate provider_class: :gateway_class, deprecator: Spree::Deprecation - instance_method(:provider_class).owner.send(:protected, :provider_class) end end From b334b76bb2610b991a626f9b3841302c9930d104 Mon Sep 17 00:00:00 2001 From: Jordan Brough Date: Thu, 21 Dec 2017 09:51:10 -0700 Subject: [PATCH 3/3] Different approach to fix method-visibility for private-method deprecations --- .../spree/admin/payment_methods_controller.rb | 7 ++----- .../app/controllers/spree/admin/resource_controller.rb | 10 +++++----- core/app/models/spree/order.rb | 3 +-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/backend/app/controllers/spree/admin/payment_methods_controller.rb b/backend/app/controllers/spree/admin/payment_methods_controller.rb index 13c9e7c2347..18c5b46d454 100644 --- a/backend/app/controllers/spree/admin/payment_methods_controller.rb +++ b/backend/app/controllers/spree/admin/payment_methods_controller.rb @@ -49,10 +49,9 @@ def collection end def load_providers + Spree::Deprecation.warn('load_providers is deprecated. Please use load_payment_method_types instead.', caller) load_payment_method_types end - deprecate load_providers: :load_payment_method_types, deprecator: Spree::Deprecation - instance_method(:load_providers).owner.send(:private, :load_providers) def load_payment_method_types @payment_method_types = Rails.application.config.spree.payment_methods.sort_by(&:name) @@ -61,11 +60,9 @@ def load_payment_method_types end def validate_payment_provider + Spree::Deprecation.warn('validate_payment_provider is deprecated. Please use validate_payment_method_type instead.', caller) validate_payment_method_type end - deprecate validate_payment_provider: :validate_payment_method_type, - deprecator: Spree::Deprecation - instance_method(:validate_payment_provider).owner.send(:private, :validate_payment_provider) def validate_payment_method_type requested_type = params[:payment_method].delete(:type) diff --git a/backend/app/controllers/spree/admin/resource_controller.rb b/backend/app/controllers/spree/admin/resource_controller.rb index 9eba0d3fcd8..525f3165fa3 100644 --- a/backend/app/controllers/spree/admin/resource_controller.rb +++ b/backend/app/controllers/spree/admin/resource_controller.rb @@ -134,9 +134,10 @@ def parent_model_name self.class.parent_data[:model_name].gsub('spree/', '') end - alias_method :model_name, :parent_model_name - deprecate model_name: :parent_model_name, deprecator: Spree::Deprecation - instance_method(:model_name).owner.send(:private, :model_name) + def model_name + Spree::Deprecation.warn('model_name is deprecated. Please use parent_model_name instead.', caller) + parent_model_name + end def object_name controller_name.singularize @@ -171,10 +172,9 @@ def load_resource_instance end def parent_data + Spree::Deprecation.warn('parent_data is deprecated without replacement.', caller) self.class.parent_data end - deprecate :parent_data, deprecator: Spree::Deprecation - instance_method(:parent_data).owner.send(:private, :parent_data) def parent if parent? diff --git a/core/app/models/spree/order.rb b/core/app/models/spree/order.rb index 95fe345f03a..ce1950335db 100644 --- a/core/app/models/spree/order.rb +++ b/core/app/models/spree/order.rb @@ -813,13 +813,12 @@ def process_payments_before_complete # } # def update_params_payment_source + Spree::Deprecation.warn('update_params_payment_source is deprecated. Please use set_payment_parameters_amount instead.', caller) if @updating_params[:order] && (@updating_params[:order][:payments_attributes] || @updating_params[:order][:existing_card]) @updating_params[:order][:payments_attributes] ||= [{}] @updating_params[:order][:payments_attributes].first[:amount] = total end end - deprecate update_params_payment_source: :set_payment_parameters_amount, deprecator: Spree::Deprecation - instance_method(:update_params_payment_source).owner.send(:private, :update_params_payment_source) def associate_store self.store ||= Spree::Store.default