Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add back changes that fix deprecation warning related to autoloading in Rails 6 #255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add back changes that fix deprecation warning related to autoloading in Rails 6 #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9d48c5
Call sorcery config block where it is defined
Hirurg103 Aug 8, 2020
6fa3214
Add back changes that fix deprecation warning related to autoloading …
Hirurg103 Aug 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lib/sorcery/controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ def self.included(klass)
# rubocop:enable Lint/HandleExceptions
end
end
Config.update!
Config.configure!
end

module InstanceMethods
Expand Down
7 changes: 2 additions & 5 deletions lib/sorcery/controller/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,8 @@ def user_config(&blk)
end

def configure(&blk)
@configure_blk = blk
end

def configure!
@configure_blk.call(self) if @configure_blk
update!
blk.call(self) if blk
end
Comment on lines 56 to 59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configure essentially 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|
  ...
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

Expand Down
14 changes: 4 additions & 10 deletions lib/sorcery/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,12 @@ class Engine < Rails::Engine

# TODO: Should this include a modified version of the helper methods?
initializer 'extend Controller with sorcery' do
# FIXME: on_load is needed to fix Rails 6 deprecations, but it breaks
# applications due to undefined method errors.
# ActiveSupport.on_load(:action_controller_api) do
if defined?(ActionController::API)
ActionController::API.send(:include, Sorcery::Controller)
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)
Comment on lines +12 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution is confirmed as breaking for existing applications, although an alternative that does work is on the v1 rework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean #227 there? Or does this change cause other issues?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@athix you say that the deprecation warning from autoloading in Rails 6 (#209) has been fixed in Sorcery/sorcery-rework. When it is not going to be fixed in the main gem, then the question above is irrelevant and you can ignore it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#132 is related

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, got it. Thank you for the advice @athix

ActionController::Base.helper_method :current_user
ActionController::Base.helper_method :logged_in?
end
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/controller_activity_logging_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
login_user(user)
now = Time.now.in_time_zone
Timecop.freeze(now)

sorcery_controller_property_set(:register_logout_time, true)
expect(user).to receive(:set_last_logout_at).with(be_within(0.1).of(now))

logout_user
Expand Down