Add back changes that fix deprecation warning related to autoloading in Rails 6 #255
Conversation
[Sorcery#227] Before this commit sorcery config block was saved into a variable and called after including `Sorcery::Controller` into `ActionController::Base` (in the included block). In Sorcery#209 the initialization code was changed to include `Sorcery::Controller` into `ActionController::Base` after loading action controller (in the onload block) to fix deprecation warning related to autoloading. After this change calling the user model in the rails development console started to fail with "To use reset_password submodule, you must define a mailer" error (Sorcery#227), because sorcery config block was not called (calling the user model doesn't load action controller) and `::Sorcery::Controller::Config.user_config` was nil Because of this issue changes in Sorcery#209 have been reverted (Sorcery#234) There is no need to delay calling sorcery config block until `Sorcery::Controller` is included into `ActionController::Base` This commit changes the code to call sorcery config where it is defined. It will allow to apply changes in Sorcery#209 to fix deprecation warning from autoloading
…in Rails 6 [Sorcery#209] Deprecation warning "Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper." has been fixed in Sorcery#209, but then reverted because of "To use reset_password submodule, you must define a mailer" error occurring when calling the user model in the rails development console (Sorcery#227) c9d48c5 allows to add back changes from Sorcery#209 that fix the warning without breaking the user model in the rails development console
| ActiveSupport.on_load(:action_controller_api) do | ||
| ActionController::API.include(Sorcery::Controller) | ||
| end | ||
|
|
||
| # FIXME: on_load is needed to fix Rails 6 deprecations, but it breaks | ||
| # applications due to undefined method errors. | ||
| # ActiveSupport.on_load(:action_controller_base) do | ||
| if defined?(ActionController::Base) | ||
| ActionController::Base.send(:include, Sorcery::Controller) | ||
| ActiveSupport.on_load(:action_controller_base) do | ||
| ActionController::Base.include(Sorcery::Controller) |
There was a problem hiding this comment.
This solution is confirmed as breaking for existing applications, although an alternative that does work is on the v1 rework.
There was a problem hiding this comment.
Do you mean #227 there? Or does this change cause other issues?
There was a problem hiding this comment.
Per FIXME, changing to on_load causes existing applications to generate undefined method errors. This has been pretty consistent every time we've tried switching.
There was a problem hiding this comment.
Are there any other Sorcery/sorcery issues related to switching to the on_load syntax other than #227? If yes, could you please point me to them?
There was a problem hiding this comment.
When it is not going to be fixed in the main gem, then the question above is irrelevant and you can ignore it
The rework (sorcery-core) will become the main gem once v1 ships, so in that sense it should be resolved in the main gem. As for back-porting it to the current 0.15.X code-base, my free time is already completely consumed by the v1 rework so it's unlikely to happen until after 1.0.0 is live regardless.
Honestly, it would probably make sense for you to create a temporary fork with your changes here and use that until v1 is released. 0.15 shouldn't be receiving any updates in the meantime, so you won't have to worry about pulling down upstream changes until v1 goes live and you can cut over to that version.
There was a problem hiding this comment.
Ok, got it. Thank you for the advice @athix
| def configure(&blk) | ||
| @configure_blk = blk | ||
| end | ||
|
|
||
| def configure! | ||
| @configure_blk.call(self) if @configure_blk | ||
| update! | ||
| blk.call(self) if blk | ||
| end |
There was a problem hiding this comment.
With how the config works in v1, I don't know that immediately calling the block makes sense. configure essentially sets the global config, which gets executed on duplicates rather than the primary instance. I'll definitely keep this in mind though, and see if it can be simplified to a single method when testing.
There was a problem hiding this comment.
configureessentially sets the global config, which gets executed on duplicates rather than the primary instance.
While inspecting the code I found out that Sorcery::Controller::Config class (not instance) is intended to be used in controllers and it is not relevant where we set class variables (in the configure! method after action controller is laded or in the sorcery initializer when calling configure with a block)
Instances of Sorcery::Model::Config are configured in ::Sorcery::Controller::Config.user_config block and used in user models
There is another way to fix #227: move configure user block out of the configure controller block. Sorcery initializer will look like this:
# Rails.application.config.sorcery.configure is a way too long,
# Sorcery.configure looks more elegant
Sorcery.configure_controller do |config|
...
end
Sorcery.configure_user do |config|
...
endThere was a problem hiding this comment.
I definitely like calling Sorcery.configure over Rails.application.config.sorcery.configure, I'll see about implementing that for sure.
I've reworked how the config works quite a bit in the rework, but if there ends up still being a configure_user block, I'll extract it out like you've suggested. I like that separation better.
|
@Hirurg103 you might want to take a look at Sorcery/sorcery-rework. The bulk of the rework is changing how Sorcery handles the config and plugin (submodule) loading, which fixes the deprecation warnings as a consequence. |
|
Also important to note, the |
joshbuker
added
the
implemented in v1
|
Thank you for pointing me to Sorcery/sorcery-rework and fixing deprecation warnings there. Shall I close this PR? |
|
@Hirurg103 we can leave it open for now, it serves as a good reminder for me to test for method undefined errors, as well as deprecation warnings. I plan on closing out all the old PRs that are marked |
This PR introduces the following changes:
Add back changes from Fix deprecation warnings in Rails 6 #209 that fix deprecation warning "Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper."
Call sorcery config block where it is defined to fix "To use reset_password submodule, you must define a mailer" error. This error started to occur after adding changes from Fix deprecation warnings in Rails 6 #209 when calling the user model in the rails development console.