Environment variables object

[nikolaposa]

Extracted logic for getting/setting variables into a separate EnvironmentVariables object in order to have a clear separation of responsibilities between loading values from a file and accessing values using native PHP facilitators.

EnvironmentVariables object can be used standalone, for read-only purposes, as an alternative for using getenv() function or global variables. It can also injected into a DI container so that environment variables are available when constructing services. For example:

$container->set('env', EnvironmentVariables::createImmutable());

$container->set('service', function (ContainerInterface $container) {
    $env = $container->get('env');
    return new My\Service($env['FOO'], $env['BAR']);
});

[j4m3s]

This looks like a worthwhile refactoring, and makes a lot of sense from a concerns and testing perspective. You've even used protected variables for open/closed principles!

I'm not a maintainer I'm afraid, but see this as valuable, particularly as it opens the door for people to extend EnvironmentVariables to put their own end to the $_ENV vs $_SERVER vs putenv in a clean and swappable fashion.

[Caroga]

+1

[Caroga]

Any updates would be much appreciated.

[GrahamCampbell]

Since this class is not final, we need "static" here, not "self". Alternatively, it could be desirable to actually make the class final.

[nikolaposa]

If we go down the "final" path, it would be good to do protected ->private modifications also, but since there's no specific interface that describes EnvironmentVariables implementations, I think there's no point in making this class final.

So I'll fix this method to support late static binding as you suggested.

[vlucas]

@GrahamCampbell can you approve this review since the references to 'self' here have been changed to 'static'?

[nikolaposa]

Why is the build failing, those error messages does not make any sense? Race condition or something, I guess I should sync my branch with changes on the master.

[nikolaposa]

Note that after syncing with changes on the master branch introduced with #215, I had to slightly adapt them. To be honest, I really don't understand why those changes were needed at the first place.

[piotr-cz]

It's sometimes helpful to load variables into an array without setting them in environment.
My use case is populating phinx environments config file as it allows migrations on specific environment from CLI:

phinx migrate --environment=testing

Related issues: #113, #163,

[vlucas]

I like this overall. What are your thoughts on reading in all the ENV vars (maybe merging $_ENV, $_SERVER, etc. and caching them locally, then having get return those cached env vars first? I only bring this up because many people seem to have issues with getenv, etc. being called from multiple different areas in their project at different times.

[nikolaposa]

@vlucas That is definitely something that can be considered, but I think it kinda goes beyond the scope of the initial idea. This was a simple refactor for a better separation of concerns, as well as to encapsulate/abstract logic for dealing with environment variables, and use it in an object-oriented fashion, as opposed to using getenv() or $_ENV directly. I didn't want to introduce any changes that might affect existing behaviour.

Therefore, I see yours and many other ideas as a follow up on this one, which after being merged opens opportunities for other improvements as well. I would rather improve things gradually, in smaller increments, rather than having one big PR with lots of (unrelated) changes in it.

[vlucas]

@GrahamCampbell can you approve this review since the references to 'self' here have been changed to 'static'?

[Korbeil]

@GrahamCampbell any news about this approval ?
(since the self() has been replaced by static())

[GrahamCampbell]

I'm working on rebasing this now. Having to check that every change that was made since is being copied into your new class.

[GrahamCampbell]

Replaced by #300. Gone with a more abstract approach, so it can be truly flexible.