NEW replace _ss_environment.php with .env and environment vars #6337

dhensby · 2016-11-25T18:25:16Z

This PR replaces our idiosyncratic _ss_environment.php convention with support for first class environment variables with a .env poly-fill for easy testing/development.

I've also removed the dependency on the Host header ($_SERVER['HTTP_HOST']) from the bootstrap so we no longer require $_FILE_TO_URL_MAPPING global for CLI execution. Devs can now set SS_HOST environment variable or Director.alternate_host config var.

Docs and tests have been updated

assertchris · 2016-11-27T21:59:46Z

Brilliant change. I'd suggest creating an env($key, $default = null) function which proxies to the underlying env function, but allows for defaults. Then things like getenv('SS_PROTECTED_ASSETS_PATH') can have sensible defaults, like env('SS_PROTECTED_ASSETS_PATH', 'assets/secure').

tractorcow · 2016-11-27T22:01:01Z

Could we not support both getenv() and defined() vars using a core environment helper?

class Environment {
	public static function get($var) {
		if (defined($var)) return constant($var);
		return getenv($var);
	}
}

This allows certain vars to be fixed (and thus protected from modification). there are security concerns related to allowing core vars to be modified.

dhensby · 2016-11-27T22:19:09Z

@assertchris good idea - I guess I'll just copy laravel ;) - though there is more a favour towards putting these methods into utility classes.

@tractorcow whilst I don't really think there's a significant issue in allowing our pseudo environment vars to be modified I think your suggestion is sound in regards to providing BC support.

One issue there is to address is how we now handle $_FILE_TO_URL_MAPPING - though we could just define a new env var for this?

assertchris · 2016-11-27T22:23:59Z

I think it would be better to resolve .env variables before constants, but good suggestion @tractorcow.

tractorcow · 2016-11-27T23:20:29Z

I would prefer to see $_FILE_TO_URL_MAPPING become less important and opt-in, and instead have a better default.

dhensby · 2016-11-27T23:24:27Z

I would prefer to see $_FILE_TO_URL_MAPPING become less important and opt-in, and instead have a better default.

I agree - there's currently the ability to set default baseurl and we could easily move this to an env var itself. .env won't apply across sites, so there's less of a need for a path to domain map.

tractorcow · 2016-11-27T23:24:34Z

My view is that if you declare security-sensitive vars such as SS_TRUST_* values, attempts to re-declare them via getenv() could potentially be an error condition (i.e. only if defined in multiple places AND differ).

Other values could be safely overridden, however, e.g for testing purposes. :)

I'm very security paranoid though... so I am perhaps arguing for unnecessary strictness in this case.

tractorcow · 2016-11-27T23:25:44Z

there's currently the ability to set default baseurl

Maybe just have SS_DEFAULT_BASE_URL instead as a substitute? Does that better suit the new architecture? We have BASE_URL but that's a core constant not an environment-level option.

SpiritLevel · 2016-11-28T02:02:28Z

tests/bootstrap/environment.php

@@ -2,33 +2,37 @@

 // Bootstrap _ss_environment.php


// Bootstrap environment.php ?

SpiritLevel · 2016-11-28T02:08:20Z

src/Assets/Flysystem/ProtectedAssetAdapter.php

-        if(defined('SS_PROTECTED_ASSETS_PATH')) {
-            return SS_PROTECTED_ASSETS_PATH;
+        if(getenv('SS_PROTECTED_ASSETS_PATH')) {
+            return getenv('SS_PROTECTED_ASSETS_PATH');


Isn't it more efficient to avoid calling getenv twice and instead do something like below?

if($protetedAssetsPath=getenv('SS_PROTECTED_ASSETS_PATH')) { return $protectedAssetsPath; }

If so then there are a number of other places one reduce the calls to getenv :)

SpiritLevel · 2016-11-28T02:09:24Z

src/Control/Director.php

@@ -585,7 +585,7 @@ public static function is_https() {
 		// See https://support.microsoft.com/en-us/kb/307347
 		$headerOverride = false;
 		if (TRUSTED_PROXY) {
-			$headers = (defined('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) ? array(SS_TRUSTED_PROXY_PROTOCOL_HEADER) : null;
+			$headers = (getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) ? array(getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) : null;
 			if (!$headers) {


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:09:43Z

src/Control/HTTPRequest.php

@@ -708,7 +708,7 @@ public function allParsed() {
 	public function getIP() {
 		$headerOverrideIP = null;
 		if(TRUSTED_PROXY) {
-			$headers = (defined('SS_TRUSTED_PROXY_IP_HEADER')) ? array(SS_TRUSTED_PROXY_IP_HEADER) : null;
+			$headers = (getenv('SS_TRUSTED_PROXY_IP_HEADER')) ? array(getenv('SS_TRUSTED_PROXY_IP_HEADER')) : null;
 			if(!$headers) {


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:10:13Z

src/Core/Constants.php

-		if(defined('SS_TRUSTED_PROXY_IPS') && SS_TRUSTED_PROXY_IPS !== 'none') {
-			if(SS_TRUSTED_PROXY_IPS === '*') {
+		if(getenv('SS_TRUSTED_PROXY_IPS') !== 'none') {
+			if(getenv('SS_TRUSTED_PROXY_IPS') === '*') {
 				$trusted = true;


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:10:25Z

src/Core/Constants.php

-if (defined('SS_ALLOWED_HOSTS') && php_sapi_name() !== "cli") {
-	$all_allowed_hosts = explode(',', SS_ALLOWED_HOSTS);
+if (getenv('SS_ALLOWED_HOSTS') && php_sapi_name() !== "cli") {
+	$all_allowed_hosts = explode(',', getenv('SS_ALLOWED_HOSTS'));
 	if (!isset($_SERVER['HTTP_HOST']) || !in_array($_SERVER['HTTP_HOST'], $all_allowed_hosts)) {


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:10:44Z

src/Core/Startup/ParameterConfirmationToken.php

@@ -167,7 +167,7 @@ protected function currentAbsoluteURL() {
 		// See https://support.microsoft.com/en-us/kb/307347
 		$headerOverride = false;
 		if(TRUSTED_PROXY) {
-			$headers = (defined('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) ? array(SS_TRUSTED_PROXY_PROTOCOL_HEADER) : null;
+			$headers = (getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) ? array(getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) : null;


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:10:53Z

src/Dev/Deprecation.php

-		if(defined('SS_DEPRECATION_ENABLED')) {
-			return SS_DEPRECATION_ENABLED;
+		if(getenv('SS_DEPRECATION_ENABLED')) {
+			return getenv('SS_DEPRECATION_ENABLED');
 		}


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:11:06Z

src/Dev/Install/config-form.html

-													if($usingEnv && isset($fieldSpec['envVar']) && defined($fieldSpec['envVar'])) {
-														$value = constant($fieldSpec['envVar']);
+													if($usingEnv && isset($fieldSpec['envVar']) && getenv($fieldSpec['envVar'])) {
+														$value = getenv($fieldSpec['envVar']);


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:11:45Z

src/Dev/Install/install.php5

-        if( defined('SS_DATABASE_CLASS') ){
-            $type = $_REQUEST['db']['type'] = SS_DATABASE_CLASS;
+        if( getenv('SS_DATABASE_CLASS') ){
+            $type = $_REQUEST['db']['type'] = getenv('SS_DATABASE_CLASS');


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:12:09Z

src/Dev/Install/install.php5

-    if( defined('SS_DATABASE_CLASS') ){
-        $type = $_REQUEST['db']['type'] = SS_DATABASE_CLASS;
+    if( getenv('SS_DATABASE_CLASS') ){
+        $type = $_REQUEST['db']['type'] = getenv('SS_DATABASE_CLASS');


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:12:35Z

src/conf/ConfigureFromEnv.php

-	} else if(defined('SS_DATABASE_CHOOSE_NAME') && SS_DATABASE_CHOOSE_NAME) {
-		$loopCount = (int)SS_DATABASE_CHOOSE_NAME;
+	if(getenv('SS_DATABASE_NAME')) {
+		$database = getenv('SS_DATABASE_NAME');


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:12:50Z

src/conf/ConfigureFromEnv.php

+	if(getenv('SS_DATABASE_NAME')) {
+		$database = getenv('SS_DATABASE_NAME');
+	} else if(getenv('SS_DATABASE_CHOOSE_NAME')) {
+		$loopCount = (int)getenv('SS_DATABASE_CHOOSE_NAME');
 		$databaseDir = BASE_PATH;


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:13:06Z

src/conf/ConfigureFromEnv.php

-	if(defined('SS_DATABASE_PORT')) {
-		$databaseConfig['port'] = SS_DATABASE_PORT;
+	if(getenv('SS_DATABASE_PORT')) {
+		$databaseConfig['port'] = getenv('SS_DATABASE_PORT');


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:13:16Z

src/conf/ConfigureFromEnv.php

-	if (defined('SS_DATABASE_TIMEZONE')) {
-		$databaseConfig['timezone'] = SS_DATABASE_TIMEZONE;
+	if (getenv('SS_DATABASE_TIMEZONE')) {
+		$databaseConfig['timezone'] = getenv('SS_DATABASE_TIMEZONE');


Extra call to getenv ?

SpiritLevel · 2016-11-28T02:13:38Z

src/conf/ConfigureFromEnv.php

-	if(defined('SS_DATABASE_SCHEMA'))
-		$databaseConfig["schema"] = SS_DATABASE_SCHEMA;
+	if(getenv('SS_DATABASE_SCHEMA'))
+		$databaseConfig["schema"] = getenv('SS_DATABASE_SCHEMA');


Extra call to getenv ?

dhensby · 2017-01-31T13:25:52Z

Needs to link to more detailed docs, and an example on how to rewrite the old format to the new one.

I've added docs under environment management that show the new format. I've added to the changelog to link to this.

Should mention that devs need to check if their webservers are configured to deny requests to dot-prefixed files if the environment file is located in the webroot (or move it out of the webroot). This wasn't a problem before with PHP files, since they would execute on webserver requests, but not leak any information.

Added security issues section to environment management docs

Needs to point out conditional logic as bad practice (no longer possible), and suggest a better alternative

I'm not sure where to put this, it doesn't really make sense to put it in environment management docs ("Don't do this thing you can't do any more coz it's bad but if you have to do it this way").

Mention the removal of $_FILE_TO_URL_MAPPING (and how to achieve this now)

Added to change log

chillu · 2017-01-31T18:42:42Z

I'm not sure where to put this, it doesn't really make sense to put it in environment management docs ("Don't do this thing you can't do any more coz it's bad but if you have to do it this way").

In the 4.0.0.md upgrading guide - doesn't make sense as permanent documentation, but at the point of an upgrade it's relevant.

chillu · 2017-01-31T18:43:44Z

docs/en/04_Changelogs/4.0.0.md

+* Removed support for _ss_environment.php in favour of .env and first class environment variables
+* Environment variables now can be set in `.env` file placed in webroot or one level above
+* Environment variables will be read from the environment as well
+* `$_FILE_TO_URL_MAPPING` has been removed and replaced with using `Director.aleternate_host` or `SS_HOST` env var


Spelling ("aleternate")

What's wrong with my aleternate spelling :)

sminnee

A few more points. However, I'd be open to merging as-is and raising an alpha5 ticket for the subsequent changes if we want to just get the thing over the line.

sminnee · 2017-01-31T20:57:42Z

docs/en/00_Getting_Started/03_Environment_Management.md

+
+## Managing environment variables with `.env` files
+
+By default the `.env` must be placed in your webroot. If this file exists, it will be automatically loaded by the


Let's start saying "project root" rather than webroot. If, one blessed day, we allow a flexible webroot location, then these docs won't need updating.

We should note in these docs that the .env

Eg: "By default, the .env file must be placed either in your project root folder (i.e. next to your composer.json), or in its parent folder"

sminnee · 2017-01-31T21:01:06Z

docs/en/00_Getting_Started/03_Environment_Management.md

+| `SS_ALLOWED_HOSTS` | A comma deliminated list of hostnames the site is allowed to respond to |
+| `SS_MANIFESTCACHE` | The manifest cache to use (defaults to file based caching) |
+| `SS_IGNORE_DOT_ENV` | If set the .env file will be ignored. This is good for live to mitigate any performance implications of loading the .env file |
+| `SS_HOST` | The hostname to use when it isn't determinable by other means (eg: for CLI commands) |


This doesn't appear to let us force the generation of https:// URLs from CLI scripts. Perhaps we want to allow SS_HOST to be a URL fragment as well as a hostname?

This could be a future PR.

We've also lost the ability to indicate that a SilverStripe site is running in a non-root URL.

Was there a reason that we didn't make this setting SS_BASE_URL instead?

hmm - good point

sminnee · 2017-01-31T21:02:00Z

docs/en/05_Contributing/05_Making_A_SilverStripe_Core_Release.md


-    // Basic CLI hostname
+    # Basic CLI hostname
    global $_FILE_TO_URL_MAPPING;


This is out of date.

sminnee · 2017-01-31T21:04:29Z

src/Core/Constants.php

+                 BASE_PATH,
+                 dirname(BASE_PATH),
+             ) as $path) {
+        try {


Not a blocker, but I feel that an if(file_exists()) check would be better than throwing and swallowing an exception.

Exception is a bit of overhead, we can fix in separate ticket.

dhensby · 2017-01-31T21:42:15Z

OK - I've addressed all the feedback apart from the URL issues

sminnee · 2017-01-31T23:24:35Z

I'm happy for us to merge as-is and pick up SS_HOST vs SS_BASE_URL as a separate ticket.

dhensby · 2017-02-01T00:29:37Z

:') Can't believe it - whoop

dhensby · 2017-02-01T00:40:05Z

I've opened the follow-up issue

chillu · 2017-02-01T08:28:07Z

Awesome to see this merged! Buuuuut: It broke all travis builds apart from framework, since that's the only one that no longer relies on travis-support: silverstripe/silverstripe-travis-support#41. And funnily enough, we're using conditional logic in the _ss_env there ;) @dhensby, do you have time to look at this?

dhensby · 2017-02-01T08:43:52Z

I'll look at it today.

GrahamCampbell · 2019-01-26T17:55:52Z

The current version of phpdotenv is robust and thread-safe. The following example will work in a multithreaded environment. This avoids using the adapter that would have called putenv and getenv, which are not threadsafe.

<?php

use Dotenv\Environment\Adapter\EnvConstAdapter;
use Dotenv\Environment\Adapter\ServerConstAdapter;
use Dotenv\Environment\DotenvFactory;
use Dotenv\Dotenv;

$factory = new DotenvFactory([new EnvConstAdapter(), new ServerConstAdapter()]);

Dotenv::create($path, null, $factory)->load();

robbieaverill · 2019-01-28T09:57:36Z

Thanks for the update @GrahamCampbell!

@silverstripe/core-team do we want to do an RFC to re-implement the latest version of phpdotenv for SS5?

dhensby · 2019-01-28T16:31:50Z

New issue / thread please :)

maxime-rainville · 2019-01-28T19:55:30Z

I second Dan. Going through dozen of messages from 2 years ago is not conductive to a useful discussion.

GrahamCampbell · 2019-01-28T19:58:53Z

See #8764. :)

dhensby force-pushed the pulls/4/dotenv branch 5 times, most recently from 521cad4 to ee72f02 Compare November 27, 2016 21:48

dhensby force-pushed the pulls/4/dotenv branch from f6a9cb1 to 85f9a62 Compare November 27, 2016 22:37

SpiritLevel reviewed Nov 28, 2016

View reviewed changes

tests/bootstrap/environment.php Outdated

@@ -2,33 +2,37 @@

// Bootstrap _ss_environment.php

Copy link

Contributor

SpiritLevel Nov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Bootstrap environment.php ?

SpiritLevel reviewed Nov 28, 2016

View reviewed changes

dhensby force-pushed the pulls/4/dotenv branch 2 times, most recently from 20de2fa to 7edf25a Compare January 31, 2017 13:25

chillu reviewed Jan 31, 2017

View reviewed changes

NEW replace _ss_environment.php with .env and environment vars

873fd8c

sminnee requested changes Jan 31, 2017

View reviewed changes

DOCS Updated environment management docs to use .env file

6e096f6

dhensby force-pushed the pulls/4/dotenv branch from 7edf25a to 0a9ec3f Compare January 31, 2017 21:29

NEW Director::host() to determine host name of site

8c8231c

dhensby force-pushed the pulls/4/dotenv branch from 0a9ec3f to 8c8231c Compare January 31, 2017 21:36

sminnee approved these changes Jan 31, 2017

View reviewed changes

tractorcow merged commit b4536bf into silverstripe:master Feb 1, 2017

dhensby deleted the pulls/4/dotenv branch February 1, 2017 00:29

dhensby mentioned this pull request Feb 1, 2017

Better support for custom host/base url in CLI / env vars #6588

Closed

chillu mentioned this pull request Feb 1, 2017

Add .env support in parallel to _ss_environment.php silverstripe/silverstripe-travis-support#41

Closed

kinglozzer mentioned this pull request Mar 15, 2017

Undefined variable in framework/src/Dev/Install/config-form.html #6707

Closed

dhensby mentioned this pull request Jun 7, 2017

introduces SS_EXTRA_DOT_ENVS var for declaring additonal variables #7001

Closed

dhensby mentioned this pull request Dec 22, 2017

BASE_URL / Director.alternate_base_url issues & inconsistency #7728

Closed


		## Managing environment variables with `.env` files

		By default the `.env` must be placed in your webroot. If this file exists, it will be automatically loaded by the

NEW replace _ss_environment.php with .env and environment vars #6337

NEW replace _ss_environment.php with .env and environment vars #6337

Conversation

dhensby commented Nov 25, 2016 • edited Loading

assertchris commented Nov 27, 2016 • edited Loading

tractorcow commented Nov 27, 2016

dhensby commented Nov 27, 2016

assertchris commented Nov 27, 2016

tractorcow commented Nov 27, 2016

dhensby commented Nov 27, 2016

tractorcow commented Nov 27, 2016

tractorcow commented Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

SpiritLevel Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Jan 31, 2017

chillu commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sminnee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Jan 31, 2017

sminnee commented Jan 31, 2017

dhensby commented Feb 1, 2017

dhensby commented Feb 1, 2017

chillu commented Feb 1, 2017

dhensby commented Feb 1, 2017

GrahamCampbell commented Jan 26, 2019

robbieaverill commented Jan 28, 2019

dhensby commented Jan 28, 2019

maxime-rainville commented Jan 28, 2019

GrahamCampbell commented Jan 28, 2019

dhensby commented Nov 25, 2016 •

edited

Loading

assertchris commented Nov 27, 2016 •

edited

Loading

tractorcow commented Nov 27, 2016 •

edited

Loading

SpiritLevel Nov 28, 2016 •

edited

Loading