Remove deprecation warning using controller filters inside initializer by mgrunberg · Pull Request #7340 · activeadmin/activeadmin

mgrunberg · 2022-02-17T14:15:49Z

Defining Action filters (before_action, skip_before_action, etc) using ActiveAdmin.before_action on the initializer file produces the following warning.

DEPRECATION WARNING: Initialization autoloaded the constants ApplicationHelper, TimeHelper, DeviseHelper, ApplicationController, InheritedResources::Base, DeviseController, Devise::SessionsController, Devise::PasswordsController, Devise::UnlocksController, Devise::RegistrationsController, and Devise::ConfirmationsController.

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

NOTE: On Rails 7.0 it produces the following error.

lib/active_admin/base_controller/authorization.rb:3:in `<module:ActiveAdmin>': uninitialized constant InheritedResources::Base (NameError)
Did you mean?  Base64

ActiveAdmin module forwards the filter call to all controllers. That means that we are using classes meant to be autoloaded inside the initializer. That's the source of the warning and it's not allowed starging Rails 7.x (see the second red box of https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#config-autoload-paths).

This PR defers the forward until controllers are loaded by using ActiveSupport.on_load hook.

Closes #5772.

mgrunberg · 2022-02-17T14:19:25Z

I can't find a good way to write a test for this. The issue is at boot time, calling ActiveAdmin.before_action on the initializer. It's totally fine to do it inside a test.

The initializers run very early on test setup, even on feature tests. I tried to write a feature test that restart/reload the server but couldn't make it.

I test this PR manually on each rails version by adding: config.before_action -> (controller) { Rails.logger.info "HEY, before action #{controller.class.name}##{action_name}" } to config/initializers/active_admin.rb

tagliala · 2022-02-17T15:19:10Z

Hi,

I can see that all Rails 6.1 builds are failing. Errors seem related to this change:

undefined method `prepend_before_action' for #<Class:0x000055f32e0131b0>

https://github.com/activeadmin/activeadmin/runs/5233438250?check_suite_focus=true#step:9:87

deivid-rodriguez · 2022-02-17T15:32:31Z

I'm guessing this change is correct (passes realworld cucumber features), but the unit tests are broken by it. If that's the culprit, we need to adapt unit tests.

mgrunberg · 2022-02-17T16:26:41Z

Hi, I believe the failures are related to test execution order. I'm going to check to confirm and see how to fix it.

mgrunberg · 2022-02-19T02:18:52Z

@tagliala, @deivid-rodriguez it was a flaky test. I'm running defined hooks on any class that includes ActiveAdmin::Devise::Controller. spec/unit/devise_spec.rb defines a class that includes the module but doesn't have callbacks (it's not an AbstractController). controller_filters_spec.rb defines callbacks. If it runs first, devise_spec runs define callbacks and fails.

The alternative is to move this https://github.com/activeadmin/activeadmin/pull/7340/files#diff-f0cf2650684e208d1c3f6a07e874099749954701701a0212c6bce155685606f8R34-R35 from the module into each defined class (ActiveAdmin::Devise::SessionControler, ActiveAdmin::Devise::RegistrationController, etc).

mgrunberg · 2022-02-21T12:58:46Z

Ended up using the alternative to prevent problems when spec/unit/device_spec.rb runs before spec/unit/application_spec.rb

javierjulio

@mgrunberg this is not an area I really know anything about so hoping the others can help. Thank you!

lib/active_admin/application.rb

deivid-rodriguez

This looks good to me! Can you rebase and squash to a single commit?

Defining Action filters (`before_action`, `skip_before_action`, etc) using `ActiveAdmin.before_action` on the initializer file produces the following warning. ``` DEPRECATION WARNING: Initialization autoloaded the constants ApplicationHelper, TimeHelper, DeviseHelper, ApplicationController, InheritedResources::Base, DeviseController, Devise::SessionsController, Devise::PasswordsController, Devise::UnlocksController, Devise::RegistrationsController, and Devise::ConfirmationsController. Being able to do this is deprecated. Autoloading during initialization is going to be an error condition in future versions of Rails. ``` NOTE: On Rails 7.0 it produces the following error. ``` lib/active_admin/base_controller/authorization.rb:3:in `<module:ActiveAdmin>': uninitialized constant InheritedResources::Base (NameError) Did you mean? Base64 ``` ActiveAdmin module forwards the filter call to all controllers. That mean that we are using classes meant to be autoloaded inside the initializer. That's not allowed in Rails 7.x (see the second red box of https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#config-autoload-paths). This commit defer the forward until controllers are loaded by using ActiveSupport.on_load hook. NOTE: I'm adding `ActiveSupport.run_load_hooks(:active_admin_controller, self)` on each devise controller to preserve original behaviour. Adding that to `ActiveAdmin::Devise::Controller` leads to flaky tests depending on what run first: `spec/unit/devise_spec.rb` or `spec/unit/application_spec.rb`.

mgrunberg · 2022-02-25T13:23:33Z

@deivid-rodriguez done

deivid-rodriguez · 2022-02-25T13:53:51Z

Awesome thanks so much!

deivid-rodriguez · 2022-02-25T14:17:03Z

This actually closes #5772, right?

mgrunberg · 2022-02-25T14:24:09Z

This actually closes #5772, right?

Yes, that correct

javierjulio

Thanks again! ❤️

This was referenced Feb 17, 2022

Add Rails 7 Support #7235

Merged

reimplement action filters using hooks tagliala/activeadmin#4

Closed

mgrunberg force-pushed the fix/remove-deprecation-warning-defining-action-filter-on-initializers branch from 88b14b0 to 7b19147 Compare February 19, 2022 02:06

mgrunberg force-pushed the fix/remove-deprecation-warning-defining-action-filter-on-initializers branch 2 times, most recently from 7d24902 to af076e0 Compare February 21, 2022 12:56

javierjulio requested a review from deivid-rodriguez February 23, 2022 22:53

javierjulio reviewed Feb 23, 2022

View reviewed changes

lib/active_admin/application.rb Show resolved Hide resolved

deivid-rodriguez added the type enhancement label Feb 25, 2022

deivid-rodriguez approved these changes Feb 25, 2022

View reviewed changes

mgrunberg force-pushed the fix/remove-deprecation-warning-defining-action-filter-on-initializers branch from 969d023 to 3d753d2 Compare February 25, 2022 13:22

deivid-rodriguez changed the title ~~Remove deprecation wraning using controller filters inside initializer~~ Remove deprecation warning using controller filters inside initializer Feb 25, 2022

javierjulio approved these changes Feb 25, 2022

View reviewed changes

javierjulio merged commit 67bfdb5 into activeadmin:master Feb 25, 2022

mgrunberg deleted the fix/remove-deprecation-warning-defining-action-filter-on-initializers branch February 25, 2022 18:38

tagliala mentioned this pull request Apr 10, 2022

Support Rails 7 #7196

Closed

Uh oh!

Conversation

mgrunberg commented Feb 17, 2022 • edited by deivid-rodriguez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgrunberg commented Feb 17, 2022

Uh oh!

tagliala commented Feb 17, 2022

Uh oh!

deivid-rodriguez commented Feb 17, 2022

Uh oh!

mgrunberg commented Feb 17, 2022

Uh oh!

mgrunberg commented Feb 19, 2022

Uh oh!

mgrunberg commented Feb 21, 2022

Uh oh!

javierjulio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

mgrunberg commented Feb 25, 2022

Uh oh!

deivid-rodriguez commented Feb 25, 2022

Uh oh!

deivid-rodriguez commented Feb 25, 2022

Uh oh!

mgrunberg commented Feb 25, 2022

Uh oh!

javierjulio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mgrunberg commented Feb 17, 2022 •

edited by deivid-rodriguez

Loading