New definition type for reading environment variables. #176

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@jmalloc
Contributor

jmalloc commented Aug 6, 2014

Overview

This PR contains an implementation of a new definition type EnvironmentVariableDefinition which allows for easy reading of a system environment variable with fallback to a default value if the environment variable is not defined.

Rationale

I've had the need to create several container definitions that look something like the following:

return [
    'my-app.dsn' => DI\factory(
        function (DI\Container $container) {
            $url = isset($_SERVER['DATABASE_URL'])
                ? $_SERVER['DATABASE_URL']
                : 'postgresql://user:pass@localhost/db';
        }
    )
];

I figured this might be a common situation for other developers, especially those deploying their applications to systems like Heroku, or otherwise following the principles of 12-factor applications.

Usage

The following snippet is equivalent to the example given above:

return [
    'my-app.dsn' => DI\env('DATABASE_URL', 'postgresql://user:pass@localhost/db')
];

The first parameter to DI\env() is the name of the environment variable to read, the second parameter is the default value to use in the event that the environment variable is not defined.

If the environment variable is not defined and no default is provided, a DefinitionException is thrown when attempting to resolve the value.

Finally, default values can reference other definitions using DI\link():

return [
    'my-app.default-dsn' => 'postgresql://user:pass@localhost/db',
    'my-app.dsn' => DI\env('DATABASE_URL', DI\link('my-app.default-dsn'))
];

Tests

Unit tests and integration tests have been implemented.

Remaining Issues

I think this PR is usable as it stands, however one thing I was unsure of is how strict the isResolvable() implementation needs to be. For example, it does not currently follow references made with DI\link() to check that they are also resolvable.

Edit It looks perhaps I need to do some work on a definition dumper for this new definition type.

Hopefully this a is a feature that you would consider desirable, and of course if there's anything that needs to be corrected please let me know.

@mnapoli mnapoli added the enhancement label Aug 6, 2014

@mnapoli mnapoli added this to the 4.3 milestone Aug 6, 2014

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 6, 2014

Member

Hi,

This looks awesome! This is definitely good to be merged, hopefully for the next version (4.3).

I just have a few remarks:

  • as you mentioned, it would be good to have a definition dumper
  • getenv returns false if the environment variable is undefined: that prevents to have false in the environment variable because in that case we would get either an exception or the default value.

Do you see any solution to that? I don't :( Using $_SERVER will not work in CLI, and $_ENV is deprecated/sometimes unsupported so that's not good.

Also if you have time, that would be awesome if you could add a small test for this case:

return [
    // null default value
    'optional-null' => DI\env('unknown', null),
];

just to check that we can set null default values. Judging by the code it should definitely work (you used func_num_args) but that would add some guarantees for that special case and that would ensure there is no regression later.

Member

mnapoli commented Aug 6, 2014

Hi,

This looks awesome! This is definitely good to be merged, hopefully for the next version (4.3).

I just have a few remarks:

  • as you mentioned, it would be good to have a definition dumper
  • getenv returns false if the environment variable is undefined: that prevents to have false in the environment variable because in that case we would get either an exception or the default value.

Do you see any solution to that? I don't :( Using $_SERVER will not work in CLI, and $_ENV is deprecated/sometimes unsupported so that's not good.

Also if you have time, that would be awesome if you could add a small test for this case:

return [
    // null default value
    'optional-null' => DI\env('unknown', null),
];

just to check that we can set null default values. Judging by the code it should definitely work (you used func_num_args) but that would add some guarantees for that special case and that would ensure there is no regression later.

@mnapoli mnapoli referenced this pull request Aug 6, 2014

Merged

4.3 #174

6 of 6 tasks complete
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 6, 2014

Member

Actually regarding my comment about getenv and false values, someone on SO pointed to me that you probably can't set a boolean value in an env variable (it would just be a string) so it's probably not a problem.

Member

mnapoli commented Aug 6, 2014

Actually regarding my comment about getenv and false values, someone on SO pointed to me that you probably can't set a boolean value in an env variable (it would just be a string) so it's probably not a problem.

@jmalloc

This comment has been minimized.

Show comment
Hide comment
@jmalloc

jmalloc Aug 6, 2014

Contributor

Thanks for the feedback :)

As far as I'm aware environment variables can only ever be strings, so I don't think the false check is cause for concern.

I'll try to get the dumper added later today.

Contributor

jmalloc commented Aug 6, 2014

Thanks for the feedback :)

As far as I'm aware environment variables can only ever be strings, so I don't think the false check is cause for concern.

I'll try to get the dumper added later today.

@jmalloc

This comment has been minimized.

Show comment
Hide comment
@jmalloc

jmalloc Aug 6, 2014

Contributor

Oh, and the other test case you mentioned!

Contributor

jmalloc commented Aug 6, 2014

Oh, and the other test case you mentioned!

@jmalloc

This comment has been minimized.

Show comment
Hide comment
@jmalloc

jmalloc Aug 7, 2014

Contributor

All done.

Contributor

jmalloc commented Aug 7, 2014

All done.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 7, 2014

Member

Awesome! I have merged this manually into the 4.3 branch (#174) so it will be in the next release.

Member

mnapoli commented Aug 7, 2014

Awesome! I have merged this manually into the 4.3 branch (#174) so it will be in the next release.

@mnapoli mnapoli closed this Aug 7, 2014

mnapoli added a commit that referenced this pull request Aug 12, 2014

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 12, 2014

Member

I have just released PHP-DI 4.3.0 which includes this feature. Thanks!

http://php-di.org/news/11-php-di-4-3-released.html

Member

mnapoli commented Aug 12, 2014

I have just released PHP-DI 4.3.0 which includes this feature. Thanks!

http://php-di.org/news/11-php-di-4-3-released.html

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