refactor environment configuration #859
Conversation
refacroring Environment access to allow configuration from multiple sources. Initially, the sources are env and php.ini, but this should allow us to add more sources in future without changing the API.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #859 +/- ##
============================================
+ Coverage 80.80% 81.25% +0.45%
- Complexity 1951 1971 +20
============================================
Files 248 250 +2
Lines 5122 5165 +43
============================================
+ Hits 4139 4197 +58
+ Misses 983 968 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| return DefaultResolver::instance(); | ||
| } | ||
|
|
||
| public static function getInt(string $key, int $default): int |
There was a problem hiding this comment.
Do we need the $default parameters or could we return null instead?
There was a problem hiding this comment.
We could return null, it would mean changing most of our factories to match. Providing a default has the benefit that the default is validated against known types and possible values, so seems safer to me.
There was a problem hiding this comment.
I missed that we throw if the key is not set and no default is given - changing the default parameter to nullable / default null to match the other methods is sufficient then.
There was a problem hiding this comment.
I'm not sure about OTel specifically, but in the past it's been important on occasion to be able to distinguish between "not set" and "default", when there are multiple things that control something. Usually this comes up because of legacy env vars. For instance, and this is totally made up for example: OTEL_HOST=collector.local but then later we realize that means it can't support https as is, so there's a new one OTEL_COLLECTOR_URI=https://collector.local. For the sake of falling back to an older config, you generally want to be able to distinguish between default and unset.
There was a problem hiding this comment.
@morrisonlevi we currently manage that scenario with ::has, although it might start to get gnarly with more variables:
$endpoint = Configuration::has(Variables::OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
? Configuration::getString(Variables::OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
: Configuration::getString(Variables::OTEL_EXPORTER_OTLP_ENDPOINT);| { | ||
| $value = getenv($variableName); | ||
| if ($value === false) { | ||
| $value = $_ENV[$variableName] ?? $default; |
There was a problem hiding this comment.
$value retreived from $_ENV can be any type. I think we should allow resolvers to return mixed.
There was a problem hiding this comment.
That's true, but there is a bunch of logic in Accessor to parse and validate string values, and I think it makes sense to leave it there.
There was a problem hiding this comment.
I think we should skip parsing if $value is not a string and just validate/coerce the type.
Limiting the type to string will become an actual issue when configuration files with support for more complex types are added to the spec.
There was a problem hiding this comment.
resolvers can return mixed now. Rather than skip parsing, I updated the parsers to allow their own type (eg bool|string)...
handle get_cfg_var array response add CompositeResolver and simplify Configuration class check $_SERVER instead of $_ENV for env vars
| { | ||
| $value = getenv($variableName); | ||
| if ($value === false) { | ||
| $value = $_SERVER[$variableName] ?? $default; |
There was a problem hiding this comment.
I think we should check $_SERVER first. For example vlucas/phpdotenv populates only $_SERVER and $_ENV by default, "Using getenv() and putenv() is strongly discouraged due to the fact that these functions are not thread safe".
Do we have to use local_only: true?
Warning If PHP is running in a SAPI such as Fast CGI, this function will always return the value of an environment variable set by the SAPI, even if putenv() has been used to set a local environment variable of the same name. Use the local_only parameter to return the value of locally-set environment variables.
There was a problem hiding this comment.
I think if we want to remove getenv/putenv support it can be in a separate PR. I was worried about what that would mean for all the tests which rely on putenv, but I think that phpunit's @backupGlobals feature should do (and not having to support both $_ENV and getenv should simplify tests a little)
Refactoring Environment access to allow configuration from multiple sources. Initially, the sources are env and php.ini, but this should allow us to add more sources in future without changing the API.