Fix loading of parameters from `.env.*` files #3224

Merged
merged 1 commit into from Jun 21, 2016

Conversation

Projects
None yet
3 participants
@smotesko
Contributor

smotesko commented Jun 15, 2016

The documentation states that we can load params from files named .env or .env.something but the latter doesn't work because the regex only matches filenames ending with .env or .ini

src/Codeception/Configuration.php
@@ -646,7 +646,7 @@ private static function prepareParams($settings)
}
// .env and ini files
- if (preg_match('~(\.ini|\.env)$~', $paramStorage)) {
+ if (preg_match('~(\.ini$|\.env)~', $paramStorage)) {

This comment has been minimized.

@stipsan

stipsan Jun 15, 2016

Contributor

why limit .ini files?

@stipsan

stipsan Jun 15, 2016

Contributor

why limit .ini files?

This comment has been minimized.

@smotesko

smotesko Jun 15, 2016

Contributor

@stipsan I see two reasons to "limit" .ini files:

  1. They were "limited" before, so I don't change anything in that regard.
  2. I've never seen a config file named .ini.something. That's unconventional.

examples of file names that would match:

prod.ini
testing.ini
.env
.env.testing
@smotesko

smotesko Jun 15, 2016

Contributor

@stipsan I see two reasons to "limit" .ini files:

  1. They were "limited" before, so I don't change anything in that regard.
  2. I've never seen a config file named .ini.something. That's unconventional.

examples of file names that would match:

prod.ini
testing.ini
.env
.env.testing

This comment has been minimized.

@stipsan

stipsan Jun 15, 2016

Contributor

ok that makes sense regarding .ini.
I'm no regex wizard but I noticed a problem with your updated pattern though: https://regex101.com/r/hO5rU9/2

It's now matching stuff like .envy or anything with .env in it.

@stipsan

stipsan Jun 15, 2016

Contributor

ok that makes sense regarding .ini.
I'm no regex wizard but I noticed a problem with your updated pattern though: https://regex101.com/r/hO5rU9/2

It's now matching stuff like .envy or anything with .env in it.

This comment has been minimized.

@smotesko

smotesko Jun 16, 2016

Contributor

@stipsan Yes, I'm aware of it. Don't think if would be a problem because that's the last condition in the chain of possible options (after the .yml). If this regex doesn't match the next option is throwing an exception. See, it doesn't just match all the files in the project root. It matches files that are explicitly specified in codeception.yml, so if user wants to specify file named .envy there's no reason to deny it in this case, I think.

@smotesko

smotesko Jun 16, 2016

Contributor

@stipsan Yes, I'm aware of it. Don't think if would be a problem because that's the last condition in the chain of possible options (after the .yml). If this regex doesn't match the next option is throwing an exception. See, it doesn't just match all the files in the project root. It matches files that are explicitly specified in codeception.yml, so if user wants to specify file named .envy there's no reason to deny it in this case, I think.

This comment has been minimized.

@stipsan

stipsan Jun 16, 2016

Contributor

I see, that's good. I think that's a developer friendly approach 👍

I don't know if this functionality is covered in the testing suite already, as I've only recently started to participate here, but I think adding a test that catch the edge case you're fixing in this PR would prevent future regressions.

Basically just a test that makes sure .env.something is loaded correctly :)

@stipsan

stipsan Jun 16, 2016

Contributor

I see, that's good. I think that's a developer friendly approach 👍

I don't know if this functionality is covered in the testing suite already, as I've only recently started to participate here, but I think adding a test that catch the edge case you're fixing in this PR would prevent future regressions.

Basically just a test that makes sure .env.something is loaded correctly :)

This comment has been minimized.

@smotesko

smotesko Jun 16, 2016

Contributor

@stipsan 👌 I've looked into covering this with test and I see it requires mocking the PHP functions. It can be done, but needs some thought to not make a mess. I'll have to do it later.
I haven't found any PHP native functions mocking in this project so far, so I'll have to be the first one here to do it. Or maybe I miss something? I'm thinking to do it with a "namespace hack".

@smotesko

smotesko Jun 16, 2016

Contributor

@stipsan 👌 I've looked into covering this with test and I see it requires mocking the PHP functions. It can be done, but needs some thought to not make a mess. I'll have to do it later.
I haven't found any PHP native functions mocking in this project so far, so I'll have to be the first one here to do it. Or maybe I miss something? I'm thinking to do it with a "namespace hack".

This comment has been minimized.

@stipsan

stipsan Jun 16, 2016

Contributor

that sounds complicated indeed.
Perhaps @DavertMik or @Naktibalda know more about this?

@stipsan

stipsan Jun 16, 2016

Contributor

that sounds complicated indeed.
Perhaps @DavertMik or @Naktibalda know more about this?

This comment has been minimized.

@DavertMik

DavertMik Jun 16, 2016

Member

Thanks @stipsan for noticing it.

I think next pattern should be used:

(\.ini$|\.env(\.|$))
@DavertMik

DavertMik Jun 16, 2016

Member

Thanks @stipsan for noticing it.

I think next pattern should be used:

(\.ini$|\.env(\.|$))

This comment has been minimized.

@smotesko

smotesko Jun 21, 2016

Contributor

@DavertMik looks great, thanks. I've changed it.

@smotesko

smotesko Jun 21, 2016

Contributor

@DavertMik looks great, thanks. I've changed it.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 16, 2016

Member

Please update the regex

Member

DavertMik commented Jun 16, 2016

Please update the regex

Fix loading of parameters from `.env.*` files
The documentation [states](http://codeception.com/docs/06-ModulesAndHelpers#Dynamic-Configuration-With-Params) that we can load params from files named `.env` or `.env.something` but the latter doesn't work because the regex only matches filenames ending with `.env` or `.ini`

Change the `.env|.ini` regex
@smotesko

This comment has been minimized.

Show comment
Hide comment
@smotesko

smotesko Jun 21, 2016

Contributor

@DavertMik changed.

Contributor

smotesko commented Jun 21, 2016

@DavertMik changed.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jun 21, 2016

Member

Thanks!

Member

DavertMik commented Jun 21, 2016

Thanks!

@DavertMik DavertMik merged commit f6d73df into Codeception:2.2 Jun 21, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment