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

Set Active Record configurations on after_initialize #42499

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

casperisfine
Copy link
Contributor

Otherwise you can't change config.active_record.* from config/initializers.

Until now it somewhat worked because the config would be assigned when ActiveRecord::Base would be first referenced. But it was quite brittle anyway.

I might merge this one quickly once I get a green CI as to fix the main branch. But I'd love to have second opinions on it.

@casperisfine
Copy link
Contributor Author

Ok, I'm running into more load order issues, so I pushed d7a857f to fix CI in the meantime. I'll try to get a proper fix tomorrow.

cc @Tonkpils

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

We should not be messing with after: options in the initializers. It also move all other initializers out of order. Can you check if not other initialzier changed the order because of this after option?

@@ -236,6 +228,18 @@ class Railtie < Rails::Railtie # :nodoc:
end
end

initializer "active_record.set_config", after: :load_config_initializers do |app|
Copy link
Member

Choose a reason for hiding this comment

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

This also wrok for config/initializers but Railties will still not be able to set config. I think this should happen as late is possible, maybe in an after_initialize. That would avoid the need to use the after: option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried that before and it wouldn't work, but it now does. Quite puzzling. Either way i think it's good to go now.

@eileencodes
Copy link
Member

d7a857f caused a lot more failures and active record has a lot of deprecation warnings in the tests. Can we revert the changes to maintain_test_schema in the meantime?

@rafaelfranca
Copy link
Member

Done in 0305815

@casperisfine casperisfine force-pushed the fix-activerecord-config-ordering branch 2 times, most recently from bc017bb to 8561bb7 Compare June 16, 2021 07:39
@casperisfine casperisfine changed the title Set Active Record configurations after config/initializers have ran Set Active Record configurations on after_initialize Jun 16, 2021
@casperisfine casperisfine force-pushed the fix-activerecord-config-ordering branch 2 times, most recently from b459889 to d2351fe Compare June 16, 2021 08:10
Otherwise you can't change `config.active_record.*` from `config/initializers`.

Until now it somewhat worked because the config would be assigned
when `ActiveRecord::Base` would be first referenced. But it was quite
brittle anyway.
@casperisfine casperisfine force-pushed the fix-activerecord-config-ordering branch from d2351fe to 538998d Compare June 16, 2021 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants