Add setter and deprecation for configurations hashes #35242
Conversation
| ActiveSupport::Deprecation.warn \ | ||
| "Returning a hash from ActiveRecord::Base.configurations is deprecated. Therefore calling `#{method}` on the hash is also deprecated. Please switch to using the `configs_for` method instead to collect and iterate over database configurations." | ||
| msg = if method == :[]= | ||
| "Setting `ActiveRecord::Base.configurations` with `:[]=` is deprecated. Use `ActiveRecord::Base.configurations=` directly to set the configurations instead." |
There was a problem hiding this comment.
That colon sticks out a bit to me
| "Setting `ActiveRecord::Base.configurations` with `:[]=` is deprecated. Use `ActiveRecord::Base.configurations=` directly to set the configurations instead." | |
| "Setting `ActiveRecord::Base.configurations` with `[]=` is deprecated. Use `ActiveRecord::Base.configurations=` directly to set the configurations instead." |
| @@ -178,6 +182,8 @@ def method_missing(method, *args, &blk) | |||
| configs_for(env_name: args.first) | |||
| when :values | |||
| configurations.map(&:config) | |||
| when :[]= | |||
| ActiveRecord::Base.configurations = DatabaseConfigurations.new(args[0] => args[1]) | |||
There was a problem hiding this comment.
Is this going to be a close enough approximation to work for most existing callers? I would've imagined something a bit more.. concat-y?
| msg = if method == :[]= | ||
| "Setting `ActiveRecord::Base.configurations` with `:[]=` is deprecated. Use `ActiveRecord::Base.configurations=` directly to set the configurations instead." | ||
| else | ||
| "`ActiveRecord::Base.configurations` no longer returns a hash. Methods that act on the hash like `#{method}` are deprecated and will be removed in Rails 6.1. Use the `configs_for` method to collect and iterate over the database configurations." |
There was a problem hiding this comment.
If the expectation is that super is going to raise, "deprecated" is a bit of a misnomer here.
Perhaps we should only produce this deprecation warning for each/first/fetch/values, and have the else case below raise a custom exception [with a similar, but harsher, message]?
| ActiveRecord::Base.configurations["readonly"] = config | ||
| end | ||
|
|
||
| assert_equal({ "adapter" => "sqlite3" }, ActiveRecord::Base.configurations["readonly"]) |
There was a problem hiding this comment.
Per above, while this sounds 💯 -- I love that we're able to provide this sort of transitional smoothing -- I think it's surprising (read: not how 5.2 acts) for the other configurations to have disappeared. (Assuming that is what's happening.)
eileencodes
force-pushed
the
add-setter-and-deprecation-for-configurations-hashes
branch
from
fc571ec
to
8f3499d
Compare
|
Ok I redid this PR. Here's what the changes consist of now:
Getters message: Setter message:
|
| @@ -102,6 +113,7 @@ def env_with_configs(env = nil) | |||
|
|
|||
| def build_configs(configs) | |||
| return configs.configurations if configs.is_a?(DatabaseConfigurations) | |||
| return configs if configs.is_a?(Array) | |||
There was a problem hiding this comment.
If we're calling [] = we're replacing already created configurations and those will be an array.
eileencodes
force-pushed
the
add-setter-and-deprecation-for-configurations-hashes
branch
3 times, most recently
from
c92e079
to
167200d
Compare
|
Update: chatted with @matthewd and we decided that adding a |
In chat Sam Saffron asked how to use the setter now that configurations
is no longer a hash and you need to do AR::Base.configurations["test"]=.
Technically you can do `ActiveRecord::Base.configurations = { the hash
}` but I realized the old way throws an error and is unintuitive.
To aid in the transition from hashes to objects this PR makes a few
changes:
1) Re-adds a deprecated hash setter `[]=` that will add a new hash
to the configurations list OR replace an existing hash if that
environment is already present. This won't be supported in future Rails
versions but a good error is important.
2) Changed to throw deprecation warnings on the methods we decided to support
for hash conversion and raise on the methods we don't support.
3) Refactored the setter/getter hash deprecation warnings messages and
rewrote them.
Getters message:
```
DEPRECATION WARNING: `ActiveRecord::Base.configurations` no longer
returns a hash. Methods that act on the hash like `values` are
deprecated and will be removed in Rails 6.1. Use the `configs_for`
method to collect and iterate over the database configurations.
```
Setter message:
```
DEPRECATION WARNING: Setting `ActiveRecord::Base.configurations` with
`[]=` is deprecated. Use `ActiveRecord::Base.configurations=` directly
to set the configurations instead.
```
4) Rewrote the legacy configurations test file to test all the public
methods in the DatabaseConfigurations class.
eileencodes
force-pushed
the
add-setter-and-deprecation-for-configurations-hashes
branch
from
167200d
to
06f9434
Compare
eileencodes
deleted the
add-setter-and-deprecation-for-configurations-hashes
branch
Minor changes to deprecation warning message after #35242
Is this a terrible idea? I want the transition from configuration hashes to objects to be smooth and given that @SamSaffron had to ask me what to do I think I could make this more app and gem friendly. If anyone has an idea of how to do this in another way let me know.
cc/ @tenderlove @matthewd @rafaelfranca
In chat Sam Saffron asked how to use the setter now that configurations
is no longer a hash when you need to do AR::Base.configurations["test"]=.
Now you can just do
ActiveRecord::Base.configurations = { the hash }butI realized it throws an error and is unintuitive. To aid in the
transition from hashes to object I've added the setter to the
method_missing call so that it can be used and throws a warning instead
of just erroring with undefined method.
While in here I also improved the deprecation warning.
Getters message:
Setter message: