Refactor getenv access using factory methods

[bcremer]

This is an alternative to PR #6.

This contains a compability break:

Old:

$xdg = new \XdgBaseDir\Xdg();

New:

// Using getenv internally
$xdg = \XdgBaseDir\Xdg::createFromEnvironment();

// OR: create using a given array
$xdg = \XdgBaseDir\Xdg::createFromArray([
    'XDG_CACHE_HOME' => '/tmp/xdg/cache',
    'XDG_CONFIG_HOME' => '/tmp/xdg/configs'
]);

This makes it possible to test the class without mocking the getenv function at all.

[nubs]

This definitely can be a nicer interface and I use similar things myself. The BC Break is the main reason I didn't take a different approach. Defining a __construct method whose only parameters are there primarily for mocking abilities can lead to some annoying BC breaks as well if some new required parameter is added (as the required parameter should likely be first in the constructors arguments ahead of the "mock" parameters).

This approach here also leaves a method that can only be tested by modifying the environment (namely createFromEnvironment) and doesn't handle the filesystem mocking (which is the external entity that is more likely to need mocking).

@bcremer, what are your thoughts on handling the filesystem mocking? Would you still recommend mocking getenv in this manner if Isolator was already being used to mock the filesystem?

[bobthecow]

It wouldn't be a BC break (and would be a cleaner API, IMO) if instead of creating factory methods you made the constructor something like this:

    private static $envKeys = array(
        'USER',
        'HOME',
        'XDG_CONFIG_HOME',
        'XDG_DATA_HOME',
        'XDG_CACHE_HOME',
        'XDG_CONFIG_DIRS',
        'XDG_DATA_DIRS',
        'XDG_RUNTIME_DIR',
    );

    public function __construct(array $env = array())
    {
        foreach (self::$envKeys as $key) {
            $this->env[$key] = array_key_exists($key, $env) ? $env[$key] : getenv($key);
        }
    }

That would let you mock as many or few of the environment variables as you want.

[nubs]

What I meant by BC break is this. Say you have your __construct method as implemented. You now need to add some new parameter to __construct that is more directly concerned with the library: it affects typical usage of the library rather than this $env parameter that is practically only there for the test suite. As an example, let's say this parameter is a flag $createMissingDirectories that controls whether this library tries to create directories that are missing or just throw an exception when attempting to access them. In this hypothetical situation, we want this parameter to be optional and default to true. It is preferable to add the parameter without breaking BC, but to do so you would have to add this parameter to the end of the constructor arguments __construct($env = array(), $createMissingDirectories = true). Doing this means that someone using the library always has to specify $env when trying to set this flag (e.g., new Xdg(array(), false)) which is less than ideal. The user typically never wants to have to bother themselves with $env as the default behavior it uses is the primary purpose of the library (parsing the environment variables in the actual environment). Therefore, in order to set the proper order of the parameters, backwards compatibility would likely be broken by putting the flag first __construct($createMissingDirectories = true, $env = array()).

[bcremer]

@bobthecow I like the the idea of partial env overrides using the constructor, I created a separate PR for that, see #9.

@nubs BC should not be an issue right now. This lib is not yet marked as stable.

For future BC stability an optional $config array can be added when needed:
public function __construct(array $env = [], array $config = [])

The scope of the Xdg class should stay narrow.
I would even like to get rid of the lazy generation of the runtime dir.
In my opinion this should belong into a wrapping class making the XDG class just an readonly env accessor.

[bcremer]

I close this PR in favour of #9.