Rails env for empty string env vars #27399

Merged
merged 1 commit into from Jan 4, 2017

Projects

None yet

6 participants

@sinogermany
Contributor
sinogermany commented Dec 19, 2016 edited

The story:

I've got a weird issue where Rails.application.config_for(:app_config) returns nil.
So I looked into the console and found out that Rails.env is an empty string.
That was because I'm using docker-compose and it's setting RAILS_ENV to an empty string because I specified in docker-compose.yml but not set in my local env vars.
That was an easy fix on my side but I always thought rails handles that edge case.
That's why I'm submitting this PR to give rails the ability to fall back to development mode when this happens.

The changes:

I've added 2 tests with RAILS_ENV and RACK_ENV being set to an empty string and Rails.env should be development. Also added the message to CHANGELOG.md.

P.S.

This is my first commit to rails repo and I tried to follow the guidelines for contributing. I skipped some steps that seems irrelevant but if I'm missing any step please let me know.

Daniel

@matthewd matthewd was assigned by rails-bot Dec 19, 2016
@rails-bot

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@maclover7 maclover7 added the railties label Dec 19, 2016
@sinogermany
Contributor
sinogermany commented Dec 19, 2016 edited

@matthewd Travis and Code Climate both display the lovely green color now. Let me know if there's anything missing for this PR :)

railties/CHANGELOG.md
+ *Daniel Deng*
+
+* Make `Rails.env` fall back to `development` when `RAILS_ENV` and `RACK_ENV` is an empty string.
+
@eileencodes
eileencodes Dec 19, 2016 Member

Your name should go on the bottom instead of the top of the change notice.

@@ -78,6 +78,18 @@ def restore_default_config
end
end
+ test "Rails.env falls back to development if RAILS_ENV == '' and RACK_ENV == nil" do
@eileencodes
eileencodes Dec 19, 2016 Member

With tests I think it's usually better to write out what you're expecting versus writing code. So in that case this would be "Rails.env falls back to development if RAILS_ENV is blank and RACK_ENV is nil"

@sinogermany
Contributor

@eileencodes thanks for your review comments, they make sense to me. Please see the extra commit e3bcbdc . Cheers!

@sinogermany
Contributor
sinogermany commented Dec 20, 2016 edited

hm... there's a travis error for mysql2, not sure what happened? I don't think it's relevant to my changes though, coz I can't see the commit e3bcbdc breaking anything. Could anyone help and have a look at that error please? Thanks heaps!

/home/travis/build/rails/rails/vendor/bundle/ruby/2.3.0/gems/mysql2-0.4.5/lib/mysql2/client.rb:120:in `_query': Mysql2::Error: Cannot load from mysql.proc. The table is probably corrupted: DROP PROCEDURE IF EXISTS ten; (ActiveRecord::StatementInvalid)
@maclover7
Member

@sinogermany restarted the failing build, is green now

@sinogermany
Contributor

@sinogermany restarted the failing build, is green now

Thanks @maclover7 !
Anyone who has any other questions feel free to let me know.

Cheers,

Daniel

@@ -67,7 +67,7 @@ def root
# Rails.env.development? # => true
# Rails.env.production? # => false
def env
- @_env ||= ActiveSupport::StringInquirer.new(ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "development")
+ @_env ||= ActiveSupport::StringInquirer.new(ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || "development")
@rafaelfranca
rafaelfranca Dec 21, 2016 Member

presence is not present in this file. We need to require it. The test suite is passing because some of part of the test suite is requiring it and it is being used.

@sinogermany
sinogermany Dec 22, 2016 Contributor

Thanks @rafaelfranca . Nice catch! I've required the file now, please see a1c0cca . Cheers!

@rafaelfranca

Could you squash your commit?

@sinogermany
Contributor

Could you squash your commit?

@rafaelfranca done. Thanks for the reminding :)

@rafaelfranca
Member
rafaelfranca commented Jan 4, 2017 edited

Now you have two commits again 😄. Don't worry with conflicts after you squash, I can solve them.

@sinogermany
Contributor

Now you have two commits again 😄. Don't worry with conflicts after you squash, I can solve them.

LOL I removed the conflict merging commit, hopefully I haven't done something that bothers you agian :-D

@rafaelfranca
Member

Merged in 50cb1b6 and backported in 807b0cb

@rafaelfranca rafaelfranca merged commit 498370c into rails:master Jan 4, 2017

1 check passed

codeclimate no new or fixed issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment