Skip to content

Commit

Permalink
Update boot strap to use our own .env file parsing
Browse files Browse the repository at this point in the history
In light of an issue with putenv behaving asyncronously
it results in a race condition (particularly on windows
machines it would seem) with apache in the least. So
we will use our own parsing and set to a cache instead
  • Loading branch information
NightJar committed Oct 15, 2017
1 parent e9e7bd6 commit e614ede
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 14 deletions.
1 change: 0 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
"symfony/config": "^3.2",
"symfony/translation": "^2.8",
"symfony/yaml": "~3.2",
"vlucas/phpdotenv": "^2.4",
"php": ">=5.6.0",
"ext-intl": "*",
"ext-mbstring": "*",
Expand Down
3 changes: 1 addition & 2 deletions docs/en/00_Getting_Started/03_Environment_Management.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ server.
For each of these environments we may require slightly different configurations for our servers. This could be our debug
level, caching backends, or - of course - sensitive information such as database credentials.

To solve this problem of setting variables per environment we use environment variables with the help of the
[PHPDotEnv](https://github.com/vlucas/phpdotenv) library by Vance Lucas.
To solve this problem of setting variables per environment we use environment variables, with the help of a `.env` file where this isn't feasible; `.env` is a common concept across many projects of varying languages.

## Security considerations

Expand Down
6 changes: 3 additions & 3 deletions docs/en/03_Upgrading/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ composer require silverstripe/asset-admin ^1.0

This will also install the `graphql` module for GraphQL API access to your SilverStripe system, which powers the `asset-admin` module.

## Migrate to dotenv
## Migrate to enviornment variables

SilverStripe 4 requires the use of `.env` and "real" environment variables instead of `_ss_environment.php` for your environment configuration.
SilverStripe 4 requires the use of environment variables instead of `_ss_environment.php` for your environment configuration. Consult the documentation of your web server to set these per host.

You'll need to move your constants to a new `.env` file before SilverStripe will build successfully.
If you are unable to set 'true' environment variables, you'll need to move your constants to a new `.env` shim file before SilverStripe will build successfully. Set them in the format `NAME=VALUE`.

If you are not able to move your webserver away from using `_ss_environment.php` files, you can use [this example file](https://gist.github.com/robbieaverill/74fbfff6f438c94f6325107e4d7b2a45) and include it at the top of your `mysite/_config.php` file. This will export your constants as environment variables.

Expand Down
47 changes: 47 additions & 0 deletions src/Core/Environment.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,53 @@ class Environment
*/
protected static $timeLimitMax = null;

/**
* In the case 'true' environment variables cannot be set,
* set the appropriate values into the in-memory superglobal.
*/
public static shimFromFile(string $path, string $name = '.env')
{
$dotEnvFile = $path . DIRECTORY_SEPARATOR . $name;
if(is_file($dotEnvFile) && is_readable($dotEnvFile)) {
$envVars = file_get_contents($dotEnvFile);
//'bash style' comments not valid since php 7.0
$envVars = preg_replace('/^#/', ';', $envVars);
$envVars = parse_ini_string($envVars);
//if the file was invalid $envVars is false and a foreach will error.
if (!$envVars) {
continue;
}
foreach ($envVars as $name => $value) {
if (!key_exists($name, $_ENV)) {
$_ENV[$name] = $value;
}
if (!key_exists($name, $_SERVER)) {

This comment has been minimized.

Copy link
@tractorcow

tractorcow Oct 15, 2017

Contributor

From our testing, both $_SERVER and $_ENV were vulnerable to the race condition. We need another store for these variables.

This comment has been minimized.

Copy link
@NightJar

NightJar Oct 15, 2017

Author Contributor

Thanks for the confirmation, that was one of my questions, besides what to name methods :)

$_SERVER[$name] = $value;
}
putenv("$name=$value");
if (function_exists('apache_setenv')) {
apache_setenv($name, $value);
}

This comment has been minimized.

Copy link
@tractorcow

tractorcow Oct 15, 2017

Contributor

This makes all env vars un-overridable; We probably should not do this.

This comment has been minimized.

Copy link
@NightJar

NightJar Oct 15, 2017

Author Contributor

That was my intention; in the same way that a developer should not be able to change the BASE_PATH mid way through execution, environment settings should also be constant IMO. Is there a decision to the contrary that I'm not savvy to @tractorcow ?

This comment has been minimized.

Copy link
@tractorcow

tractorcow Oct 15, 2017

Contributor

BASE_PATH isn't an env variable; It's the statement of a fact, hence it's a constant. :) I think we should keep the concerns separate.

This comment has been minimized.

Copy link
@NightJar

NightJar Oct 15, 2017

Author Contributor

Perhaps worth noting I'm also emulating the status quo here with reference to the use of dotenv's load, as opposed to overload in the current beta.

If we're going to change the behaviour, perhaps it would be OK to also load both .env files (project root & parent dir) for the use case as described in the docs (universal DB details, but different other settings per development project).

This comment has been minimized.

Copy link
@dhensby

dhensby Oct 16, 2017

Contributor

env vars should not be overwritten in later execution without a specific flag saying that's what's meant to be happening. Current behaviour disallows this so that an attacker (or anyone really) can't override env vars - they are statements of fact just as much as a constant is.

This comment has been minimized.

Copy link
@tractorcow

tractorcow Oct 16, 2017

Contributor

I've kept this behaviour in the updated rewrite for this reason.

if (!defined($name)) {
define($name, $value);
}
}
break;
}
}

/**
* Fetch appropriate variable from the environment variables
* try first 'true' environment variables, then the $_ENV superglobal
*
* @param string $name
* @return string Values stored in the environment
*/
public static env(string $name)
{
return getenv($name) ?: $_ENV[$name];
}

/**
* Extract env vars prior to modification
*
Expand Down
29 changes: 21 additions & 8 deletions src/includes/constants.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
<?php

use Dotenv\Dotenv;
use Dotenv\Exception\InvalidPathException;
use SilverStripe\Core\TempFolder;

/**
Expand Down Expand Up @@ -61,13 +59,28 @@
if (!getenv('SS_IGNORE_DOT_ENV')) {
call_user_func(function () {
foreach ([BASE_PATH, dirname(BASE_PATH)] as $path) {
try {
(new Dotenv($path))->load();
} catch (InvalidPathException $e) {
// no .env found - no big deal
continue;
$dotEnvFile = $path . DIRECTORY_SEPARATOR . '.env';
if(is_file($dotEnvFile) && is_readable($dotEnvFile)) {
$envVars = file_get_contents($dotEnvFile);
$envVars = preg_replace('/^#/', ';', $envVars);
$envVars = parse_ini_string($envVars);
//if the file was invalid $envVars is false and a foreach will error.
if (!$envVars) {
continue;
}
foreach ($envVars as $name => $value) {
$_ENV[$name] = $value;
$_SERVER[$name] = $value;
putenv("$name=$value");
if (function_exists('apache_setenv')) {
apache_setenv($name, $value);
}
if (!defined($name)) {
define($name, $value);
}
}
break;
}
break;
}
});
}
Expand Down

0 comments on commit e614ede

Please sign in to comment.