search

chaps-io / access-granted

Multi-role and whitelist based authorization gem for Rails (and not only Rails!)
MIT License
773 stars 41 forks source link

Updated Railtie to be Zeitwerk compatible when Rails version >= 6 #53

Closed dmorehouse closed 3 years ago

dmorehouse commented 3 years ago

This fixes Zeitwerk detecting constants loaded during initialization.

pokonski commented 3 years ago

Sorry for the delay with reviewing this! I'll have a look as soon as possible, somehow missed a notification about this 😓

pokonski commented 3 years ago

@dmorehouse can you share what error you see in Rails 6 without your patch? I just tested the Rubygems latest version with Rails 6.1 and I see no errors during initialization. Maybe something changed in 6.1?

dmorehouse commented 3 years ago

@pokonski I suspect the message was a deprecation warning in 6.0 and they might have removed the warning (but not the problematic behavior) in 6.1. To be sure let me work up a Rails 6.0 project and a 6.1 project and I'll report back.

dmorehouse commented 3 years ago

After digging in, I've found that the deprecation warning happens in a new Rails 6.0 project but not a Rails 6.1 project because Rails 6.1 appears to have made subtle changes to the autoloader (I debugged autoloading and processing the access_granted/railtie.rb was in the same call stack and called in the same initializer order in both 6.0 and 6.1).

This issue happens in a brand new Rails 6.0 project created using rails _6.0.4_ new project and produces the following message (which goes to development.log) after adding access_granted to the Gemfile.

DEPRECATION WARNING: Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper.

Being able to do this is deprecated. Autoloading during initialization is going
to be an error condition in future versions of Rails.

Reloading does not reboot the application, and therefore code executed during
initialization does not run again. So, if you reload ActionText::ContentHelper, for example,
the expected changes won't be reflected in that stale Module object.

These autoloaded constants have been unloaded.

Please, check the "Autoloading and Reloading Constants" guide for solutions.
 (called from <main> at /redacted/ag_rails_6.0/config/environment.rb:5)

Per the Rails 6.0 upgrade guide about autoloading - https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#autoloading-when-the-application-boots. Initializers can not autoload constants (class names) the solution is to use Rails.application.reloader.to_prepare however in your case you want to wait until ActionController::Base and ActionController::API are loaded. When I upgraded my project from Rails 5.2 -> 6.0, I noticed I had to upgrade a bunch of gems for Rails 6 / zeitwerk compatibility. Here's a couple gems that I needed to update and how they handled the Rails 6 railtie lazy initialization which I think are comparable to access_granted's railtie. https://github.com/mileszs/wicked_pdf/commit/6fa659976b6797a477d076fbb851e614ce348d42#diff-872e0a26631d6103ce120b7e32313dbec795e73994f865312451a08023cb6a99 https://github.com/ErwinM/acts_as_tenant/commit/fea563c811d2798b5997a6fa174f6581fb389f06#diff-c44e52750815152c39879f43563fd7a265f7f031602b0ea3f6e986403ae159ea

FYI - The on_load hooks I use have been around for 10+ years, however to make this a no impact fix for < Rails 6.0 I chose to only use the hooks for Rails >= 6 so that if an existing project using access_granted counted on a side effect of loading ActionController::Base or ActionController::API during initialization the behavior would be the same.

Let me know if there's any other details I can provide to help you assess this change.

pokonski commented 3 years ago

Awesome, thanks for the detailed explanation :)

The on_load hooks I use have been around for 10+ years, however to make this a no impact fix for < Rails 6.0 I chose to only use the hooks for Rails >= 6 so that if an existing project using access_granted counted on a side effect of loading ActionController::Base or ActionController::API during initialization the behavior would be the same.

Good call, but I am also okay with making this a breaking change and bumping version number. I'll have a look :)

pokonski commented 3 years ago

I merged this and will prepare a Rubygems release shortly, once again thanks for the help!

dmorehouse commented 3 years ago

Great thanks so much for working with me on this. I look forward to updating my Gemfile to use the latest version :-)

pokonski commented 3 years ago

@dmorehouse this is now live as 1.3.3, thanks for the reminder!