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

Don't fallback to RACK_ENV when RAILS_ENV is not present #19404

Merged
merged 1 commit into from
Mar 19, 2015
Merged

Don't fallback to RACK_ENV when RAILS_ENV is not present #19404

merged 1 commit into from
Mar 19, 2015

Conversation

dmathieu
Copy link
Contributor

RACK_ENV is not the same as RAILS_ENV, and can't serve as a fallback when the later isn't present.

See http://www.hezmatt.org/~mpalmer/blog/2013/10/13/rack_env-its-not-for-you.html

In fact, it can even cause some issues if your app is using unicorn, as they're including internal rack middlewares relying on RACK_ENV to have the deployment value.
See https://github.com/defunkt/unicorn/blob/master/lib/unicorn.rb#L56-L79

Rack itself does that too.
https://github.com/rack/rack/blob/4e4ab39b0508aa3e59f5d7e53696ef6ae7c220ed/lib/rack/server.rb#L228

This removes support for falling back to RACK_ENV when RAILS_ENV is not available.

@senny senny added the railties label Mar 19, 2015
@arthurnn
Copy link
Member

Good catch. So this is only working right now, because for development RACK_ENV and RAILS_ENV are, coincidentally the same. And on production, RAILS_ENV is always set to production value.
👍 from me, @jeremy @tenderlove @spastorino what do you guys think?

@tenderlove
Copy link
Member

Agree with @dmathieu. It's not a valid fallback option.

tenderlove added a commit that referenced this pull request Mar 19, 2015
Don't fallback to RACK_ENV when RAILS_ENV is not present
@tenderlove tenderlove merged commit 7bdc763 into rails:master Mar 19, 2015
@arthurnn
Copy link
Member

@dmathieu can you add a Changelog entry for that?
thanks btw!

@dmathieu dmathieu deleted the remove_rack_env branch March 19, 2015 14:52
@dmathieu
Copy link
Contributor Author

@arthurnn: sure, will do.

@spastorino
Copy link
Contributor

Agreed 👍, maybe for another PR we should investigate a better way to integrate both things.

@jeremy
Copy link
Member

jeremy commented Mar 20, 2015

Considering this was intentionally, thoroughly, and publicly supported, we should go through a deprecation cycle to remove it.

@jeremy
Copy link
Member

jeremy commented Mar 20, 2015

After looking into the history, 👎 entirely. RACK_ENV grew out of a Rack server convention that multiple servers supported, then Rack::Server included. The fact that Rack::Server happened to have these default middleware stacks doesn't imply that it's the only allowed values, the only intended values, or part of the SPEC 😁

There's a long history of using RACK_ENV to communicate exactly what it is: the environment you're running the Rack app in, not the Rack environment.

jeremy added a commit that referenced this pull request Mar 20, 2015
Preserving RACK_ENV behavior.

This reverts commit 7bdc763, reversing
changes made to 45786be.
@dmathieu
Copy link
Contributor Author

@jeremy do as you wish. I just want to mention that it makes IE very slow on some versions of AWS's ELB, which can be a big issue for anyone.
If you think this shouldn't be fixed in rails, there's something that should be changed in rack, because there is definitely a bug here.

@mathieujobin
Copy link

was there any consideration falling back to APP_ENV ?

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

Successfully merging this pull request may close these issues.

None yet

7 participants