Avoid changing method visibility when deprecating a method #2449
Conversation
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.
|
I've submitted a Rails PR here: rails/rails#31433 to fix this in ActiveSupport. |
|
Thanks for finding this Jordan. I prefer to leave this as is and update to a new rails version when this is fixed upstream. |
|
@gmacdougall I think it's worth going ahead and fixing this. My fix was only merged into Rails master and Rails v5.2 is already in beta so I imagine it'll be a good while before that trickles back to Solidus. It's good to have correct method visibility for the apps using Solidus imo, even for deprecated methods. |
|
Nice catch.
I think rails master is still tracking 5.2, so it shouldn't be too long. Alternatively, we could use |
Great point, I'll update with that 👍 (at least for the ones where the method is defined in a way that's friendly to that) |
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.
|
@jhawthorn @gmacdougall I removed a broken deprecation no |
ActiveSupport::Deprecation::MethodWrapper#deprecate_methodschanges methodvisibility to
public. This seems unintentional and I will submit a PR toRails 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.prependto override the original method, so we can'tjust write
private :some_methodbecause that only affects the original method,whose visibility has not been changed.
If people think it's worth creating a helper for this let me know and I will do so.