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

[atolix]

Fix: #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: rails/rails#40671

Please ensure your pull request includes the following:

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

[willnet]

Since it is desirable for this setting to default to true in the future, I would like to add that "in a future version, config.use_redirect_back_or_to_by_rails = true will become the default."

[atolix]

I have changed the message in 8ebe075

[willnet]

I'm not very good at English, so I'm not sure if the comments and warning messages sound natural, but I think the implementation is fine.

[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?

[willnet]

I've reviewed your PR again.

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 )?

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.

[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 d78f5fe

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 3077b41

[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).

sorcery/lib/sorcery/controller.rb

Lines 99 to 102 in 301ada9

	def redirect_back_or_to(url, flash_hash = {})
	redirect_to(session[:return_to_url] || url, flash: flash_hash)
	session[:return_to_url] = nil
	end

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.

edfa250

[brendon]

Looks good to me now :)

[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.

d9a4da2

[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.

26aed80

[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!

eab0a74

[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 :)

[brendon]

Looks good to me. There's no test that the deprecation is raised. Is that something we want to add?

[atolix]

I’ve added a test in 1c92b27

[brendon]

I think this is ready to merge :)

[willnet]

@atolix Thank you for your contributions!