Swap out calls to getenv() for $_ENV

jacobbednarz commented 10 years ago

This fixes the issues with PHP's getenv() not being able to fetch the correct hosting environment variables on Acquia.

jacobbednarz commented 10 years ago

...and of course it breaks the build :sweat:

cpliakas commented 10 years ago

Looking at the code, it might be helpful to add a getenv method to the CloudEnvironmentTest that mimics the PHP function. That way we can test if the array key exists or not in order to avoid errors that will break the build.

Thanks for your contribution and continued work on this issue.

jacobbednarz commented 10 years ago

Looking at the code, it might be helpful to add a getenv method to the CloudEnvironmentTest that mimics the PHP function

Would you be happy with something like this?

$environment = (!empty($_ENV['AH_SITE_ENVIRONMENT'])) ?: getenv('AH_SITE_ENVIRONMENT');
$this->setEnvironment($environment);

cpliakas commented 10 years ago

Personally I would rather go all-in with one solution, e.g. $_ENV over getenv() and not mix the two.

Maybe something like $environment = isset($_ENV['AH_SITE_ENVIRONMENT']) ? $_ENV['AH_SITE_ENVIRONMENT'] : false;

Unfortunately we cannot use short-hand ternary with empty() otherwise $environment will evaluate to true and not the value of $_ENV['AH_SITE_ENVIRONMENT'].

scor commented 10 years ago

FWIW, Drush, Drupal core and Symfony all use getenv(). The only places I was able to find $_ENV was in isolated cases in some tests... So it might be wiser to understand the underlying issue before making such change from a popular method to our own.

webbj74 commented 10 years ago

Separate from the Travis-CI env config issue, it might be useful to see the output of phpinfo() called in the same context that getenv('AH_SITE_ENVIRONMENT') appears to fail. This might help us understand the underlying issue.

@jacobbednarz could you post a pastebin link to the output in your situation? As you're probably aware, phpinfo may output some info you consider sensitive, so scrub/occult as needed.

jacobbednarz commented 10 years ago

FWIW, Drush, Drupal core and Symfony all use getenv(). The only places I was able to find $_ENV was in isolated cases in some tests... So it might be wiser to understand the underlying issue before making such change from a popular method to our own.

I would definitely like to know the difference (even if it is just for my reference) however I wouldn't like it to hold up this pull request. Should there be a valid reason to only use getenv() we can always swap it to use it.

Personally I would rather go all-in with one solution, e.g. $_ENV over getenv() and not mix the two.

Sure! Will add it in.

Unfortunately we cannot use short-hand ternary with empty() otherwise $environment will evaluate to true and not the value of $_ENV['AH_SITE_ENVIRONMENT'].

Doh, missed this one in my initial tests. :p

@jacobbednarz could you post a pastebin link to the output in your situation? As you're probably aware, phpinfo may output some info you consider sensitive, so scrub/occult as needed.

phpinfo() output below. If you want the actual account details to look on the box yourself, feel free to drop me a message and I can send you the actual hostnames.

cpliakas commented 10 years ago

I definitely hear the concerns about moving away from getenv() however I want to help @jacobbednarz use the SDK and create an method or some sort of abstraction that allows us to swap it out for a more reliable method without being disruptive then I think we should accept a PR if the build doesn't break.

That is my $0.02.

webbj74 commented 10 years ago

My local

Running export AH_SITE_ENVIRONMENT=dev before phpunit has the following effect:

$_ENV['AH_SITE_ENVIRONMENT'] == 'dev' $_SERVER['AH_SITE_ENVIRONMENT'] == 'dev' getenv('AH_SITE_ENVIRONMENT') == 'dev'

On Travis CI

Adding this to .travis.yml:

env:
  global:
    - AH_SITE_ENVIRONMENT=dev

has the following effect on phpunit:

$_ENV['AH_SITE_ENVIRONMENT'] is unset $_SERVER['AH_SITE_ENVIRONMENT'] == 'dev' getenv('AH_SITE_ENVIRONMENT') == 'dev'

On Acquia Hosting

Testing the values after including the "settings.inc" for a site has the following:

$_ENV['AH_SITE_ENVIRONMENT'] == 'dev' $_SERVER['AH_SITE_ENVIRONMENT'] is unset getenv('AH_SITE_ENVIRONMENT') is FALSE

It looks like the Acquia "settings.inc" does the following:

./mysite-settings.inc:80:  if (!isset($_ENV['AH_SITE_GROUP'])) {
./mysite-settings.inc:81:    $_ENV['AH_SITE_GROUP'] = $ah_site_group;

It appears to me that getenv's variable list is set to $_ENV and $_SERVER when PHP first initializes. Altering the superglobals after the fact has no effect on getenv. Similarly putenv does not appear to alter the superglobals:

Acquia (PHP 5.3.2-1ubuntu4.19) $ php -a
Interactive shell

php > printf("getenv('FOO') == %s\n", getenv('FOO') ? getenv('FOO') : 'FALSE');
getenv('FOO') == FALSE
php > $_ENV['FOO'] = 'bar';
php > printf("getenv('FOO') == %s\n", getenv('FOO') ? getenv('FOO') : 'FALSE');
getenv('FOO') == FALSE
php > $_SERVER['FOO'] = 'bar';
php > printf("getenv('FOO') == %s\n", getenv('FOO') ? getenv('FOO') : 'FALSE');
getenv('FOO') == FALSE
php > putenv('FOO=test');
php > printf("getenv('FOO') == %s\n", getenv('FOO') ? getenv('FOO') : 'FALSE');
getenv('FOO') == test
php > print "\$_ENV['FOO'] == {$_ENV['FOO']}\n";
$_ENV['FOO'] == bar
php > print "\$_ENV['SERVER'] == {$_SERVER['FOO']}\n";
$_ENV['SERVER'] == bar
php >

This behavior seems consistent on Acquia's various CLI versions...

/usr/bin/php
/usr/local/php5.3/bin/php
/usr/local/php5.5/bin/php

Conclusion

So it seems to me like Acquia's method for setting values for CLI is what is inconsistent here, not acquia-sdk-php. I feel Acquia should do a putenv("AH_SITE_GROUP={$ah_site_group}"); in addition to $_ENV['AH_SITE_GROUP'] = $ah_site_group; I think there are a handful of affected files:

mysite-settings.inc
drupal_6-settings.inc
ssaml-settings.inc
ssamml2-settings.inc
samltesting-settings.inc

jacobbednarz commented 10 years ago

Thanks for the confirmation @webbj74 - It mimics the results I seen within our Acquia environments. Regarding the proposed putenv() update, would you like to open a ticket internally for that and just keep an eye on the progress and then perhaps ping back to this repository when/if it is fixed?

In the mean time, is everyone happy if I just add a helper method to the CloudEnvironmentTest file and get this build passing?

jacobbednarz commented 10 years ago

And just utilise the $_ENV['AH_SITE_ENVIRONMENT'] for the library itself?

webbj74 commented 10 years ago

Have a look at my approach in https://github.com/webbj74/acquia-sdk-php/commit/4c0a73

This passed tests without needing to modify .travis.yml (though for the record it also passed tests with the config I mentioned previously).

I can create a separate PR for this if needed (I don't have perms to add commits to this one).

jacobbednarz commented 10 years ago

Nice! :thumbsup:

Yeah, if you want to use that, I can close mine off. Thanks for the continued support with this - appreciate it.

acquia / acquia-sdk-php