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

Updating default settings of ApplicationController.renderer is not working #22975

Closed
prathamesh-sonpatki opened this issue Jan 8, 2016 · 6 comments
Milestone

Comments

@prathamesh-sonpatki
Copy link
Member

We generate an initializer application_controller_renderer to allow users to update the default config options for rendering templates outside of controller with following content.

# ApplicationController.renderer.defaults.merge!(
#   http_host: 'bigbinary.com',
#   https: true
# )

But the ApplicationController.renderer.defaults hash is frozen - https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/renderer.rb#L45. So it raises an error if we try to modify it.

@matthewd
Copy link
Member

matthewd commented Jan 8, 2016

Hmm. It seems undesirable that you'd be changing the defaults for all renderers, rather than just those for controllers inherited from ApplicationController. But it's also not very useful to configure only ApplicationController's renderer, and miss those for its subclasses. 😕

@prathamesh-sonpatki
Copy link
Member Author

@matthewd I agree. I think this is just an example to show how the defaults can be customized. But it does not work because of the frozen hash. Should we use DEFAULTS.dup here - https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/renderer.rb#L48? to allow users updating this hash?

@tomprats
Copy link
Contributor

@prathamesh-sonpatki + @matthewd: Is this something I should create a pull request for? I ran into this too and it seems a little odd that a generated file's commented code doesn't work if you try to use it

@pctj101
Copy link

pctj101 commented Feb 14, 2016

A few notes for visual clarity (those involved probably already know firsthand)

  1. Yes it is confusing for the generated file to offer code which prevents startup of server.

Example:

 ApplicationController.renderer.defaults.merge!(
   http_host: 'example.com',
   https: true
 )

Result on 'rails s'

config/initializers/application_controller_renderer.rb:3:in `merge!': can't modify frozen Hash (RuntimeError)
  1. Furthermore, setting ApplicationController#default_url_options does not seem to work. For example after sign in devise, I am redirected to http, even when my default protocol is https.
def default_url_options
  { protocol: "https" }
end
  1. Since the new way doesn't work, and the old way seems to be ignored, perhaps it would be helpful to offer some text guidance.

I'd be happy to help, but viewing the above comments, it's not clear what direction is the best.

If anyone needs me to write some comments, let me know.

@maclover7
Copy link
Contributor

Putting this on the 5.0.0 milestone, since this initializer (and the current issue) was first added in the 5.0 betas.

@maclover7 maclover7 added this to the 5.0.0 milestone Apr 20, 2016
maclover7 added a commit to maclover7/rails that referenced this issue Apr 20, 2016
Previously, users were trying to modify a frozen Hash. Includes a
regression test :)

Fixes rails#22975
@maclover7
Copy link
Contributor

Opened #24661 to dup the DEFAULTS hash, and included a regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants