Initialize deprecators before config/initializers run #46550

etiennebarrie · 2022-11-22T14:17:56Z

Motivation / Background

In config/initializers, I have some code that sets up an additional deprecator behavior (by taking the current behavior, which is an Array, prepending to it a new behavior, and setting the array as the new behavior). I want to do that only to Rails frameworks initializers since I only care about Rails deprecations in my situation.

When I updated my code to work with the new deprecators introduced in #46049 and later, I discovered that only 8 out of the total 13 frameworks have deprecators at this point in the boot process. That's because 5 of the frameworks are engines, and their own initializers by default run after all the engines initializers, which includes :load_config_initializers.

You can see an example app here https://github.com/etiennebarrie/rails-app/tree/deprecators:

$ bin/rails runner ''                                                                                                                                                                                       
{:application=>1}
{:development=>1}
{:deprecators=>8}

[Alternatively, I'd like a better way to configure all the Rails frameworks deprecators, without configuring any of the deprecators that gem engines may add to the application. Setting values on Rails.application.deprecators apply to all existing and later added deprecators, which isn't quite what I'm looking for. Plus I can only set a behavior, not query the existing one and add another one, but I understand that this makes sense for a deprecators collection. Very often people will want the same behavior to apply to all deprecators, in my specific case I only care about the Rails ones. I currently check the gem_name in the deprecation behavior, but given we have now multiple deprecators, I thought I might as well configure them separately.]

Detail

This pull request configures all the deprecator initializers to run before config/initializers.

Additional information

cc @jonathanhefner since you've been implementing all the deprecators.

Running before config/initializers is a bit arbitrary, and we may want to actually have the deprecators registered before. I tried by changing to :load_environment_config, and all the deprecators are then accessible in the environment file, but still not in application, and I think having config/application.rb and config/environments/development.rb so different isn't wanted.

There are two other ways I can see to fix this:

We could move the initializer to Railtie (which doesn't have any initializers yet), and by providing a method on Railtie, which would optionally return the framework name and deprecator, we could have the initializer run early in the process.
We could not use an initializer at all and use a before_initialize (to run before all the railties initializers, including config/initializers files) or even before_configuration (for the deprecators to be available as soon as the application is created in config/application.rb).

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
CI is passing.

jonathanhefner

Running before config/initializers is a bit arbitrary, and we may want to actually have the deprecators registered before. I tried by changing to :load_environment_config, and all the deprecators are then accessible in the environment file, but still not in application, and I think having config/application.rb and config/environments/development.rb so different isn't wanted.

...

We could not use an initializer at all and use a before_initialize (to run before all the railties initializers, including config/initializers files) or even before_configuration (for the deprecators to be available as soon as the application is created in config/application.rb).

Another possibility would be before: "active_support.deprecation_behavior", but I think before_configuration makes sense for the reasons you stated.

Note, though, that whatever we choose here will likely be imitated by library authors. So I would not depend on deprecators only containing Rails deprecator instances at any point in the boot process.

I currently check the gem_name in the deprecation behavior

I think that is more reliable. Also, that currently works with config.active_support.deprecation, yes?

After this PR, another possibility would be using deprecators.each and checking the gem_name:

Rails.application.deprecators.each do |deprecator|
  if deprecator.gem_name == "Rails"
    deprecator.behavior = ...
  end
end

When I added the framework deprecators, I debated whether to set a specific gem name for each deprecator. In the end, I left them as "Rails" because I felt that's what most people expect to see in warning messages. This seems like another reason to keep them that way.

rafaelfranca · 2022-11-22T20:36:16Z

I'm not sure applications/libraries should have access to the Rails deprecators outside of the Rails.application.deprecators API. Those deprecators are here to be used by the Rails framework, not by application/libraries. That being said, I'm not sure we should care about they being accessible in initializers.

etiennebarrie · 2022-11-23T13:45:21Z

I just pushed a commit, using before_configuration worked and made them all available at the same time, rather than a bit randomly during the initialization process.

I'm not sure applications/libraries should have access to the Rails deprecators outside of the Rails.application.deprecators API. Those deprecators are here to be used by the Rails framework, not by application/libraries.

I'm not sure I understand, I'm talking about accessing the Rails railties deprecators via the Rails.application.deprecators API here, so that seems to match your expectation. And I want to access them to set behavior on them, not use them to create warnings, so that should be fine too?

rafaelfranca · 2022-11-23T16:21:07Z

Got it. Tests are broken now. Can you take a look?

etiennebarrie · 2022-11-24T10:44:06Z

The test failures were legit, the before_configuration hooks run in Rails::Application.inherited, which means the initialize method of the application subclass isn't defined yet, and since we access the deprecators, it initializes the instance before initialize is defined. We could make deprecators a class method and delegate from the instance to the class to avoid that, but it seems a bit too much. I don't think we can expect deprecators to be configured before the configuration, for example this deprecation will never use the deprecation behavior that's configured, because the deprecation happens during configuration.

So I moved it back to be an initializer, running before active_support.deprecation_behavior.

It's an improvement over main because now all deprecators have their disallowed_warnings properly configured. This commit on my test app shows that if you set config.active_support.disallowed_deprecation_warnings that refers to an engine deprecation, on main it's not properly configured on the deprecator (because it's not yet registered in Rails.applicaiton.deprecators). On this branch since all framework deprecators are registered, it's properly picked up, and the disallowed behavior kicks instead of the normal behavior.

jonathanhefner · 2022-11-24T16:35:01Z

This commit on my test app shows that if you set config.active_support.disallowed_deprecation_warnings that refers to an engine deprecation, on main it's not properly configured on the deprecator (because it's not yet registered in Rails.applicaiton.deprecators).

Ah, that is a good catch. I had intentionally omitted Deprecators#disallowed_warnings= because disallowed warnings seem like they should be set per deprecator, and because config.active_support.disallowed_deprecation_warnings can use Deprecators#each. But, as you point out, the each is currently evaluated too early. Perhaps it would be worth adding Deprecators#disallowed_warnings= anyway. 🤔

jonathanhefner · 2022-11-25T21:29:54Z

This commit on my test app shows that if you set config.active_support.disallowed_deprecation_warnings that refers to an engine deprecation, on main it's not properly configured on the deprecator (because it's not yet registered in Rails.applicaiton.deprecators).

Perhaps it would be worth adding Deprecators#disallowed_warnings= anyway.

Fixing this in #46583. But making all framework deprecators available via Rails.application.deprecators to config/initializers still seems like a reasonable change to me.

Though I did not realize that active_support.deprecation_behavior actually runs after load_environment_config. (I pushed 2b2aea4 to document that more clearly.)

I tried by changing to :load_environment_config, and all the deprecators are then accessible in the environment file, but still not in application, and I think having config/application.rb and config/environments/development.rb so different isn't wanted.

Perhaps the difference is okay though? It's likely that different environments need different deprecator configurations. For example, in the default generated environments files: development.rb vs production.rb. Are there any other drawbacks for before: :load_environment_config?

etiennebarrie · 2022-11-28T09:46:39Z

Are there any other drawbacks for before: :load_environment_config?

Doing so moves the deprecator initializers at the very beginning of all the initializers, but I don't think there's any drawback to that. I pushed a commit with that change. I'll rebase for CI.

Since engine initializers run later in the process, we need to run this initializer earlier than the default. This ensures they're all registered before the environments are loaded.

rails-bot bot added actioncable actionmailbox actionmailer actionpack actiontext actionview activejob activemodel activerecord activestorage activesupport labels Nov 22, 2022

jonathanhefner reviewed Nov 22, 2022

View reviewed changes

etiennebarrie force-pushed the initialize-deprecators-before-config_initializers branch from 57072f6 to 8593131 Compare November 23, 2022 10:44

etiennebarrie force-pushed the initialize-deprecators-before-config_initializers branch from 8593131 to 9e89302 Compare November 24, 2022 10:41

etiennebarrie force-pushed the initialize-deprecators-before-config_initializers branch from 9e89302 to c540cd9 Compare November 28, 2022 09:20

Initialize deprecators before configuring them

3d6a7b2

Since engine initializers run later in the process, we need to run this initializer earlier than the default. This ensures they're all registered before the environments are loaded.

etiennebarrie force-pushed the initialize-deprecators-before-config_initializers branch from c540cd9 to 3d6a7b2 Compare November 28, 2022 09:47

rafaelfranca merged commit f6b386d into rails:main Nov 28, 2022

etiennebarrie mentioned this pull request Apr 19, 2023

WIP: Add disallow_deprecation_warnings:rails Shopify/rails#16

Closed

3 tasks

caius mentioned this pull request Dec 5, 2023

O&S isn't sending deprecations to O&S Own-and-Ship/oas_agent#29

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize deprecators before config/initializers run #46550

Initialize deprecators before config/initializers run #46550

Initialize deprecators before config/initializers run #46550

Initialize deprecators before config/initializers run #46550

Conversation

Motivation / Background

Detail

Additional information

Checklist

Choose a reason for hiding this comment