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.

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

Delete AuthlogicLoadedTooLateError #681

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Conversation

jaredbeck
Copy link
Collaborator

This error no longer seems to be necessary.

I tested this in a new rails 6.0.0 application. In the following,
my debug output is prefixed with "->".

bin/rails server
-> loading rails_adapter.rb
=> Booting Puma
...
* Listening on tcp://localhost:3000
...
-> RailsImplementation included into ActionController::Base
-> RailsImplementation included into ActionController::API
-> loading application_controller.rb
Started GET "/" for 127.0.0.1 at 2019-09-11 13:33:05 -0400
Processing by HomeController#index as HTML
-> activate_authlogic
  Rendering home/index.html.erb within layouts/application

We can see that the activate_authlogic callback still occurs. So,
raising an AuthlogicLoadedTooLateError would be incorrect.

And, of course, I tested a normal sign in POST.

This error no longer seems to be necessary.

I tested this in a new rails 6.0.0 application. In the following,
my debug output is prefixed with "->".

```
bin/rails server
-> loading rails_adapter.rb
=> Booting Puma
...
* Listening on tcp://localhost:3000
...
-> RailsImplementation included into ActionController::Base
-> RailsImplementation included into ActionController::API
-> loading application_controller.rb
Started GET "/" for 127.0.0.1 at 2019-09-11 13:33:05 -0400
Processing by HomeController#index as HTML
-> activate_authlogic
  Rendering home/index.html.erb within layouts/application
```

We can see that the `activate_authlogic` callback still occurs. So,
raising an AuthlogicLoadedTooLateError would be incorrect.

And, of course, I tested a normal sign in POST.
@jaredbeck
Copy link
Collaborator Author

Delete AuthlogicLoadedTooLateError
This error no longer seems to be necessary.

In #678, (released in 5.0.3) @hasghari switched us from an eager include to (LazyLoadHooks). Perhaps, now that we use LazyLoadHooks, the timing is not as important? I really don't know much about it.

Copy link
Collaborator

@tiegz tiegz left a comment

Choose a reason for hiding this comment

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

Yay for removing old code 🎊

@jaredbeck just to be safe, should we test this with Rails 5.2 as well? (I briefly tried to track down where ActionController filters changed but had no luck)

@jaredbeck
Copy link
Collaborator Author

Thanks for the quick review, Tieg.

@jaredbeck just to be safe, should we test this with Rails 5.2 as well?

We test against rails 5.2 with CI. I don't have time for any additional manual testing today. Considering how broken 5.0.3 is, I'm feeling some pressure to get 5.0.4 out.

(I briefly tried to track down where ActionController filters changed but had no luck)

You were trying to determine why we needed AuthlogicLoadedTooLateError in the past? Yeah, I wish I understood that better. Maybe we should blame it.

@jaredbeck jaredbeck merged commit 3a04ece into master Sep 11, 2019
@jaredbeck jaredbeck deleted the fix_load_early_error branch September 11, 2019 21:35
@jaredbeck
Copy link
Collaborator Author

Released as 5.0.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants