Skip to content

Remove deprecated non-symbol access to nested config_for hashes #37876

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

Conversation

etiennebarrie
Copy link
Contributor

Summary

This fixes an issue with #37870 where nested hashes returned by config_for could return Hash subclasses instead of strictly a Hash when calling to_hash.

Since we changed the name of the for option in HashWithIndifferentAccess, and the class dealing with deprecations takes an options hash, there's no exception but the option is silently ignored, which in turn changes the behavior of HashWithIndifferentAccess#to_hash.

Instead of fixing the code, this removes the deprecation code, which also solves the issue.

# before #37870 
Rails.application.config_for(:example)[:a_hash].to_hash[:other_hash]['key'] # => nil

# after #37870 
Rails.application.config_for(:example)[:a_hash].to_hash[:other_hash]['key']
# DEPRECATION WARNING: Accessing hashes returned from config_for by non-symbol keys is deprecated and will be removed in Rails 6.1. Use symbols for access instead. (called from <main> at (pry):1)
# => "value"

# after this PR
Rails.application.config_for(:example)[:a_hash].to_hash[:other_hash]['key'] # => nil

Other Information

I used the symbolize_names option YAML.load, which seems to be the first usage of it in Rails. This required me to symbolize the environment name, and the "shared" special key.
Another possibility would be to use deep_symbolize_keys as was done before #35198, but since we're loading the configuration file ourselves, it felt like an unnecessary traversal of the whole data structure. Let me know if that was a mistake.

@rafaelfranca
cc @byroot @Edouard-chin @paracycle @Morriar

@rails-bot rails-bot bot added the railties label Dec 3, 2019
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.

Can you add a CHANGELOG entry? We also need a entry in the upgrading guide since this is a deprecation removal.

@etiennebarrie etiennebarrie force-pushed the remove-deprecated-non-symbol-access-to-config_for branch from fdf183c to e5e9c55 Compare December 4, 2019 14:56
@rails-bot rails-bot bot added the docs label Dec 4, 2019
@etiennebarrie
Copy link
Contributor Author

Not sure how verbose I needed to be. 😬

@rafaelfranca rafaelfranca merged commit b4e7bb7 into rails:master Dec 4, 2019
@etiennebarrie etiennebarrie deleted the remove-deprecated-non-symbol-access-to-config_for branch December 4, 2019 19:33
composerinteralia added a commit to composerinteralia/rails that referenced this pull request Aug 15, 2020
Fixes rails#40031

While removing deprecated non-symbol access to nested `config_for`
hashes in rails#37876, we also broke `config_for` for anyone using the
[safe_yaml] gem. The problem is that `safe_yaml` patches `YAML.load` in
a way that doesn't honor the `symbolize_names` options (I believe this
is on purpose, to prevent symbol-based DOS attacks).

In the description of rails#37876 there is mention of the fact that this was
the first place in Rails we used `symbolize_names`, and that
`deep_symbolize_keys` had been used in the past.

This commit switches over to `deep_symbolize_keys` to allow `config_for`
to continue working for people using [safe_yaml].

[safe_yaml]: https://rubygems.org/gems/safe_yaml
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.

2 participants