Add an option 'use_redirect_back_or_to_by_rails' to avoid definition conflicts of `redirect_back_or_to`

[atolix]

Fix: https://github.com/Sorcery/sorcery/issues/296

Rails 7 released a new method called redirect_back_or_to as a replacement for redirect_back. That may conflicts with the method by the same name defined by Sorcery.

This commit adds an option to set whether to use redirect_back_or_to defined by Rails 7, and add a method redirect_to_before_login_path as an alternative to sorcery's `redirect_back_or_to.

ref: https://github.com/rails/rails/pull/40671

Please ensure your pull request includes the following:

• [x] Description of changes
• [x] Update to CHANGELOG.md with short description and link to pull request
• [x] Changes have related RSpec tests that ensure functionality does not break

[willnet]

@joshbuker As far as I have reviewed, I think the implementation is fine, but I'm not sure if the comments and other texts are clear. Could you please review them?

[atolix]

@willnet Thank you for the review, and sorry for the late reply.

Could you please change the comment to use redirect_to_before_login_path instead of redirect_back_or_to in the initializer file ( https://github.com/sorcery/sorcery/blob/b3e7793e44ff649938302c9fd9bdb493179af06c/lib/generators/sorcery/templates/initializer.rb#L19 )?

fixed in https://github.com/Sorcery/sorcery/pull/373/commits/d78f5fed1eec4fbd66abd4d36f986299dcd9d234

Also, in the test dummy application's controller ( https://github.com/sorcery/sorcery/blob/b3e7793e44ff649938302c9fd9bdb493179af06c/spec/rails_app/app/controllers/sorcery_controller.rb ), redirect_back_or_to is used in actions other than test_return_to (which is used by your new test code). Please update these other actions to use redirect_to_before_login_path.

fixed in https://github.com/Sorcery/sorcery/pull/373/commits/3077b41590fc529bf1c2fc2c65ef00041c399883

[willnet]

@brendon I think the implementation in this PR is fine, but I’m not confident about the English wording or the method names. If anything looks off, please point it out.

[brendon]

Just on holiday at the moment but will look at this next week hopefully :)

[brendon]

This is an interesting approach. Is the idea that you'll allow a time period where using this overridden method issues a warning, and then at some time in the near future you change the default behaviour? Will people notice the warning in their logs?

Have you considered just signalling a breaking release version bump and changing the behaviour? Given new releases of sorcery aren't very frequent it might just be better to whip the band-aid off in one go?

[atolix]

@brendon @willnet

Thanks for the feedback. Just to clarify, are you suggesting removing Sorcery’s own redirect_back_or_to method entirely (and relying only on Rails’ version), or renaming Sorcery’s method instead?

Either way, I agree with simplifying it. Since it would be a breaking change, I just want to confirm that this is acceptable to move forward with.

[brendon]

Yes I think it makes sense to remove the custom redirect_back_or_to and rely on the Rails version. I imagine the breaking change would be that users would need to update their use of the method to conform to the Rails version's signature?

I haven't checked, but I assume the purpose of both methods is the same? or is there more nuance to the Sorcery version?

[willnet]

I support the approach in this PR to first show a warning.

Sorcery’s redirect_back_or_to prioritizes redirecting to the URL the user tried to access while not logged in (when that URL requires authentication).

https://github.com/Sorcery/sorcery/blob/301ada945d077ec8e8198d546a42edbddf77ea07/lib/sorcery/controller.rb#L99-L102

Rails’ redirect_back_or_to prioritizes redirecting to the referrer URL.

https://github.com/rails/rails/blob/2ea1d9bd035e9f41d535d673efbaf4173441ab34/actionpack/lib/action_controller/metal/redirecting.rb#L199-L207

If we switch from the Sorcery version to the Rails version all at once with no migration period, their argument signatures are almost the same, so no exception will be raised and Sorcery users might not notice. However, the application behavior will change.

Even if we log a message as in the current implementation of this PR, people might miss the logs, but it’s still better than switching abruptly. To ensure users notice this breaking change, we could consider raising an exception in the version after the one that includes this PR when redirect_back_or_to is used without config.use_redirect_back_or_to_by_rails = true.

Given new releases of sorcery aren’t very frequent it might just be better to whip the band-aid off in one go?

Once this PR and #351 are resolved, I’d like to discuss releasing the next version with @joshbuker . If I can get release permissions, I’d like to release more frequently than we do now.

[brendon]

Ok, that sounds good. Should we look at raising a rails deprecation warning instead of a straight Ruby warning using ActiveSupport::Deprecation? That way people who run with config.active_support.deprecation = :raise will get a more explicit warning.

[willnet]

Since Sorcery now supports Rails 7.1 and above, we can use Rails.application.deprecators. I think that’s a great idea.

[brendon]

Since Sorcery now supports Rails 7.1 and above, we can use Rails.application.deprecators. I think that’s a great idea.

Excellent. @atolix, would you be ok with switching to that?

[atolix]

@brendon @willnet

Thanks for the suggestion. I've updated the code to use ActiveSupport::Deprecation.warn instead of warn so that it integrates with Rails' deprecation system.

https://github.com/Sorcery/sorcery/pull/373/commits/edfa250eb7d69e4002add537091771530e2227f8

[atolix]

CI failed on Rails 7.2 or above with the following error:

NoMethodError: private method warn' called for ActiveSupport::Deprecation:Class # ./lib/sorcery/controller.rb:103:in redirect_back_or_to' # ./spec/rails_app/app/controllers/sorcery_controller.rb:45:in test_redirect_back_or_to' # ./spec/controllers/controller_spec.rb:214:in block (6 levels) in <top (required)>'

I switched to ActiveSupport.deprecator.warn, which works fine on Rails 7.1+ and follows the latest Rails deprecation interface.

https://github.com/Sorcery/sorcery/pull/373/commits/d9a4da2f8c0424a6258fdda800dd6983d6348ae5

[atolix]

@brendon

Thanks for catching that. I've updated the code to define a dedicated Sorcery.deprecator instance instead of relying on ActiveSupport.deprecator, following the Rails documentation you linked.

https://github.com/Sorcery/sorcery/pull/373/commits/26aed80fa438e6d98ca7c5703bbf0aab9f90a2b7

[brendon]

@brendon

Thanks for catching that. I've updated the code to define a dedicated Sorcery.deprecator instance instead of relying on ActiveSupport.deprecator, following the Rails documentation you linked.

26aed80

Thanks for that :) Just one more worry from me. Will it be a problem having the deprecator stored as a class variable? Could this potentially cause problems with threading? That's out of my field of expertise but I'm always wary of class variables. It might be better to just initialise the deprecator locally when needed. @willnet?

[brendon]

I found this article: https://blog.saeloun.com/2024/11/19/rails-7-1-adds-application-deprecators-method/

They seem to store their deprecators globally so maybe it's ok :)

[willnet]

As shown in the Rails documentation, could you add deprecators like the following inside sorcery/engine.rb?

https://api.rubyonrails.org/classes/ActiveSupport/Deprecation.html

By doing this, the overall settings like the following will also apply to Sorcery’s deprecator.

config.active_support.deprecation = :raise

[willnet]

Thanks for that :) Just one more worry from me. Will it be a problem having the deprecator stored as a class variable? Could this potentially cause problems with threading? That's out of my field of expertise but I'm always wary of class variables. It might be better to just initialise the deprecator locally when needed. @willnet?

@brendon You need to be careful when dealing with global values like class instance variables, but it’s fine as long as they aren’t modified by multiple threads at the same time. Settings like the deprecator are created during initialization and aren’t changed concurrently, so it should be safe. It seems that each Rails component also uses class instance variables to keep their own deprecator. https://github.com/rails/rails/blob/252ee6c6b846c6ce4a82528d03c2bd6d89c7a0fa/actionpack/lib/abstract_controller/deprecator.rb#L7-L7

[atolix]

@willnet

As shown in the Rails documentation, could you add deprecators like the following inside sorcery/engine.rb?

Added deprecators registration in sorcery/engine.rb. Thanks for the feedback!

https://github.com/Sorcery/sorcery/pull/373/commits/eab0a7456ee8690ee25dee35d6d110d6e328174c

[brendon]

Thanks for that :) Just one more worry from me. Will it be a problem having the deprecator stored as a class variable? Could this potentially cause problems with threading? That's out of my field of expertise but I'm always wary of class variables. It might be better to just initialise the deprecator locally when needed. @willnet?

@brendon You need to be careful when dealing with global values like class instance variables, but it’s fine as long as they aren’t modified by multiple threads at the same time. Settings like the deprecator are created during initialization and aren’t changed concurrently, so it should be safe. It seems that each Rails component also uses class instance variables to keep their own deprecator. https://github.com/rails/rails/blob/252ee6c6b846c6ce4a82528d03c2bd6d89c7a0fa/actionpack/lib/abstract_controller/deprecator.rb#L7-L7

Thanks @willnet, that makes sense :)

[atolix]

I’ve added a test in https://github.com/Sorcery/sorcery/pull/373/commits/1c92b2770f7ae78a798282e294f0a8eb748683ee

[willnet]

@atolix Thank you for your contributions!