Open GuySartorelli opened 4 months ago
My 2 cents for the decisions/questions:
null
getAsBoolean
should imho return false (we should NOT use the FILTER_NULL_ON_FAILURE
flag).
- if it's not set, it's not set and is empty, so return
null
I tend to agree with that.
- if it's set and not a boolean-like, then
getAsBoolean
should imho return false (we should NOT use theFILTER_NULL_ON_FAILURE
flag).
What's your thinking there? To me that seems more like someone has set an incorrect value for that variable and we shouldn't try to guess at what their intention was. I lean towards throwing a RuntimeException
in that scenario. We could provide people with a valueIsBoolean()
method as well if we want to provide developers with a way to avoid exceptions and try/catching.
- if it's set and not a boolean-like, then
getAsBoolean
should imho return false (we should NOT use theFILTER_NULL_ON_FAILURE
flag).What's your thinking there? To me that seems more like someone has set an incorrect value for that variable and we shouldn't try to guess at what their intention was. I lean towards throwing a
RuntimeException
in that scenario. We could provide people with avalueIsBoolean()
method as well if we want to provide developers with a way to avoid exceptions and try/catching.
Get 'whatever' as boolean. We shouldn't be guessing, as you say. If it was named getBoolean
then we can throw an exception if not boolean-like.
So it's really a question of naming rather than functionality? I'd be happy with naming it getBoolean()
and use the safer behaviour.
Well, the name ideally indicates what the functionality will be. We can have both (getBoolean
and getAsBoolean
) or just the latter with an optional param to be strict/er. I think we're on the same page, let's see what others think.
just the latter with an optional param to be strict/er
I like that idea
I’d be in favour of implementing only getBoolean()
and throwing an exception on a non-boolean value. If you’re trying to get a boolean value from an environment variable and someone has set a non-boolean value, that’s an error that should be highlighted. I can’t really imagine a valid use case for the getAsBoolean()
soft-fail version
@kinglozzer imagine just missing a typo in the secret value and having to do a full redeployment on SCP taking an hour while not being able to see what the value is.. I'd rather have a soft-fail since everything defaults to false if not truthy as seen by filter_var
which to me feels safer. But I'm probably not the target audience here.
Right, but equally you could try to set a value to true, miss a typo and it’ll silently fall back to false with no warning/error - that sounds worse to me. Environment variables tend to be critical to a site operating properly, I’d rather be notified if I’d set something incorrectly than it silently fall back to a value I may or may not have intended
Currently to check if an environment variable represents a true-like value, developers have to decide what they will accept as true-like.
Setting
MY_ENV_VAR=true
andMY_ENV_VAR=false
will both correctly returntrue
andfalse
fromEnvironment::getEnv('MY_ENV_VAR')
Setting
MY_ENV_VAR=off
will return the string"off"
which is truthy, even though the intention is clearly a false-like value.We should introduce a
getAsBoolean()
method which usesfilter_var($envVarValue, FILTER_VALIDATE_BOOL, FILTER_NULL_ON_FAILURE)
to determine if the value is a true-like, false-like, or not valid boolean value.See https://www.php.net/manual/en/filter.filters.validate.php for more information about what the
filter_var
line above would return.To decide (turn these into ACs)
null
(which is whatfilter_var
above returns) or throw an exception? Or just treat all non-boolean values asfalse
?isBoolean()
method or provide an optional argument to adjust whether it throws an exception or not.getBoolean()
orgetAsBoolean()
? Refer to discussion in the comments.Acceptance criteria
Environment::getAsBoolean()
method is created which returns the environment variable as a booleanEnvironment::getEnv()
in core and supported modules are evaluated to check if they should be using the new method