Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use getenv() instead of $_SERVER in detectEnvironment() #6257

Merged
merged 2 commits into from
Jul 30, 2022

Conversation

fcosrno
Copy link
Contributor

@fcosrno fcosrno commented Jul 12, 2022

Description
Update CodeIgniter.php to use getenv() instead of $_SERVER in detectEnvironment. The outcome of environment variables loading in either $_SERVER or $_ENV depends on the operating system and web server. For instance, in a Docker environment the .env file loads variables in $_ENV, resulting in detectEnvironment() not seeing CI_ENVIRONMENT when defined. Using getenv() instead of directly reading from $_SERVER solves this issue.

Fixes #6313

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Use getenv instead of $_SESSION in detectEnvironment
@kenjis kenjis changed the title fix: use getenv instead of $_SESSION in detectEnvironment fix: use getenv() instead of $_SERVER in detectEnvironment() Jul 12, 2022
system/CodeIgniter.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

in a Docker environment the .env file loads variables in $_ENV, resulting in detectEnvironment() not seeing CI_ENVIRONMENT when defined. Using getenv() instead of directly reading from $_SERVER solves this issue.

Can't reproduce.

I checked phpinfo() output in my Docker environment.

  • docker desktop 4.10.1
    • Engine: 20.10.17
    • Compose: v2.6.1
Environment
CI_ENVIRONMENT | development

PHP Variables
$_SERVER['CI_ENVIRONMENT'] | development
$_ENV['CI_ENVIRONMENT'] | development

@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

If you have .env file, setVariable() sets $_SERVER[$name] if it is empty().
I don't know why your $_SERVER['CI_ENVIRONMENT'] is not set.

protected function setVariable(string $name, string $value = '')
{
if (! getenv($name, true)) {
putenv("{$name}={$value}");
}
if (empty($_ENV[$name])) {
$_ENV[$name] = $value;
}
if (empty($_SERVER[$name])) {
$_SERVER[$name] = $value;
}
}

@fcosrno
Copy link
Contributor Author

fcosrno commented Jul 13, 2022

Yes @kenjis, that should be the case in the presence of a .env file, like when you COPY the file in the Dockerfile. But it's not the case when loading environment variables in other scenarios, like when using env_file in Docker Compose or in deployments without persistent date (no copying of .env files into containers). In my original comment I should have written "in a Docker environment the environment variables could load in $_ENV" and never in $_SERVER instead of explicitly calling out the .env file.

@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

Okay, I got your situation. You don't use .env file and set environment variable only.
And your PHP does not set $_SERVER['CI_ENVIRONMENT']. Maybe not FastCGI API.

By the way, why do we use $_SERVER['CI_ENVIRONMENT']?
It comes from environment variable, so why don't we use $_ENV['CI_ENVIRONMENT']?

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jul 13, 2022
@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

env() is slower than getenv().

$iterator->run(100000)

Test Time Memory
env() 0.3829 0 Bytes
getenv 0.3150 0 Bytes

@paulbalandan
Copy link
Member

It may be slower because it checks all available places ($_SERVER, $_ENV, getenv()). But in cases where putenv is disabled for security reason, wouldn't getenv return false even if CI_ENVIRONMENT is really set?

@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

  1. Disable putenv().

php.ini:

disable_functions = putenv
  1. Remove .env file.

  2. Make a controller.

class Home extends BaseController
{
    public function index()
    {
        return getenv('CI_ENVIRONMENT');
    }
}
  1. Run local server with Environment Variable.
$ CI_ENVIRONMENT=development php spark serve
  1. Navigate to http://localhost:8080/.

I see "development".

@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

It seems there are three options:

  1. use $_ENV
  2. use $_ENV or getenv() (If $_ENV is not set, call getenv())
  3. use env()

system/CodeIgniter.php Outdated Show resolved Hide resolved
@MGatner
Copy link
Member

MGatner commented Jul 13, 2022

Oops, I reviewed before I saw this discussion. I'm fine with .07s performance hit if env() covers more possible scenarios, but if it adds nothing then going with the more barebones approach seems good. One other advantage of env() is that a developer could theoretically override it if they wanted to do some fancy loading of their own.

@kenjis
Copy link
Member

kenjis commented Jul 29, 2022

variables_order in php.ini-production is GPCS.
So if not in CGI or FastCGI, $_ENV is not populated by default.
Using only $_ENV seems not good.

; This directive determines which super global arrays are registered when PHP
; starts up. G,P,C,E & S are abbreviations for the following respective super
; globals: GET, POST, COOKIE, ENV and SERVER. There is a performance penalty
; paid for the registration of these arrays and because ENV is not as commonly
; used as the others, ENV is not recommended on productions servers. You
; can still get access to the environment variables through getenv() should you
; need to.
; Default Value: "EGPCS"
; Development Value: "GPCS"
; Production Value: "GPCS";
; https://php.net/variables-order
variables_order = "GPCS"
https://github.com/php/php-src/blob/32d55f74229e7913db0d59ef874a401744479b6a/php.ini-production#L647

Warning
In both the CGI and FastCGI SAPIs, $_SERVER is also populated by values from the environment; S is always equivalent to ES regardless of the placement of E elsewhere in this directive.
https://www.php.net/manual/en/ini.core.php#ini.variables-order

@kenjis
Copy link
Member

kenjis commented Jul 29, 2022

It seems Apache SetEnv sets $_SERVER, not $_ENV.

It is important to note that this new variable will appear in $_SERVER, not $_ENV.
https://www.php.net/manual/ja/reserved.variables.environment.php#97105

If so, the following setting does not set $_ENV.

SetEnv CI_ENVIRONMENT development
https://codeigniter4.github.io/CodeIgniter4/general/environments.html#environment-apache

Concluseion: using env() is safe.

@MGatner
Copy link
Member

MGatner commented Jul 30, 2022

@fcosrno Are you available to confirm that env() fixes your issue? If so please update this PR.

If we don't hear back today I will merge #6257 (comment) and we can proceed with final review.

@fcosrno
Copy link
Contributor Author

fcosrno commented Jul 30, 2022

Yes, I can confirm env() fixes the issue.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Okay, all green!
@fcosrno Thank you for confirmation.

@kenjis kenjis merged commit a1a6358 into codeigniter4:develop Jul 30, 2022
@MGatner
Copy link
Member

MGatner commented Jul 31, 2022

💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: CI_ENVIRONMENT bug in container
5 participants