Create new Environment::getAsBoolean() method

[GuySartorelli]

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 and MY_ENV_VAR=false will both correctly return true and false from Environment::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 uses filter_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)

1. If the env var hasn't been set, should this be considered false-like or not-boolean?
2. If an env-var is not a valid boolean-like value, should we return null (which is what filter_var above returns) or throw an exception? Or just treat all non-boolean values as false?
   • If we throw an exception we could also implement an isBoolean() method or provide an optional argument to adjust whether it throws an exception or not.
3. Do we call the method getBoolean() or getAsBoolean()? Refer to discussion in the comments.

Acceptance criteria

• A new Environment::getAsBoolean() method is created which returns the environment variable as a boolean
• Injector config does not use this method when getting environment variables
• Usage of Environment::getEnv() in core and supported modules are evaluated to check if they should be using the new method

[michalkleiner]

My 2 cents for the decisions/questions:

1. if it's not set, it's not set and is empty, so return null
2. if it's set and not a boolean-like, then getAsBoolean should imho return false (we should NOT use the FILTER_NULL_ON_FAILURE flag).

[GuySartorelli]

1. if it's not set, it's not set and is empty, so return null

I tend to agree with that.

2. if it's set and not a boolean-like, then getAsBoolean should imho return false (we should NOT use the FILTER_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.

[michalkleiner]

2. if it's set and not a boolean-like, then getAsBoolean should imho return false (we should NOT use the FILTER_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.

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.

[GuySartorelli]

So it's really a question of naming rather than functionality? I'd be happy with naming it getBoolean() and use the safer behaviour.

[michalkleiner]

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.

[GuySartorelli]

just the latter with an optional param to be strict/er

I like that idea

[kinglozzer]

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

[michalkleiner]

@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.

[kinglozzer]

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