-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Environment variable typecast processors return null when used in conjunction with default: #50415
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
Comments
Looking at the code, it looks more intended than a bug. |
I'd like to close this one. You should use an explicit default fallback param instead of trying to rely on casting parameters:
bool_flag_default_value: false
services:
\EnvironmentService:
arguments:
$boolFlag: '%env(bool:default:bool_flag_default_value:OPTIONAL_ENV_VAR)%' Plz close the issue if it works (I didn't test it 😅) and if you think it's good enough 😄 |
alternatively, dont use 'optional envs' :) |
I know that is a possibility, but it's pretty verbose to be honest. And that was the reason why I wanted to try if it could be made simpler and shorter. Something like this would be ideal and explicit. services:
\EnvironmentService:
arguments:
$boolFlag: '%env(bool:default:bool(false):OPTIONAL_ENV_VAR)%' Having to define a separate parameter only to pass it to a single place is not great ux imo. But to reiterate from the beginning, when I initially came across this, my reaction was "okay, this is how it works, but it's not really clear from the documentation", hence symfony/symfony-docs#18283 which I made first before being suggested to create a bug report. So in my view it's now one of:
|
Is this tongue-in-cheek or serious? Having optional options for the app behavior is often necessary, and especially when the app is packaged in a Docker image, environment variables are the way to pass the configuration options in. Of course, another way would be generating a file in the image entrypoint but that seems to me to just add a layer of complexity for not really a good reason. |
i'm serious :) If the env var gets somehow unset, you wouldn't know ... which IMHO is a footgun.
why? |
Please let's keep this discussion focused, it's not the place to push anyone's personal preferences. |
…l (fancyweb) This PR was merged into the 5.4 branch. Discussion ---------- [DependencyInjection] Fix casting scalar env vars from null | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | - | Tickets | #50415 | License | MIT | Doc PR | - Using a casting env var processor on `null` returns `null` (it doesn't cast) which is not what one could expect and not what is documented. We could allow it. Commits ------- c4b16e5 [DependencyInjection] Allow casting env var processors to cast null
Symfony version(s) affected
5.4.22
Description
When typecast env-var processors (e.g.
bool:
) are used together withdefault
, the expression may evaluate tonull
.When
OPTIONAL_ENV_VAR
is undefined in the following example, thedefault:
processor will assignnull
to the result, and thebool:
processor will pass it through, even though the documentation states the return type isbool
. This is a problem when the value is passed as an non-nullable argument to a strictly-typed class.This is not indented and the processor should be strictly non-nullable, as per the initial discussion here symfony/symfony-docs#18283 .
How to reproduce
Pass the following as an argument to a Symfony service. Do not set the value of
OPTIONAL_ENV_VAR
.Symfony container build will fail due to
null
being passed as the$boolFlag
which has to be abool
.Possible Solution
No response
Additional Context
No response
The text was updated successfully, but these errors were encountered: