Skip to content

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

Closed
melkamar opened this issue May 24, 2023 · 7 comments

Comments

@melkamar
Copy link

Symfony version(s) affected

5.4.22

Description

When typecast env-var processors (e.g. bool:) are used together with default, the expression may evaluate to null.

When OPTIONAL_ENV_VAR is undefined in the following example, the default: processor will assign null to the result, and the bool: processor will pass it through, even though the documentation states the return type is bool. This is a problem when the value is passed as an non-nullable argument to a strictly-typed class.

\EnvironmentService:
  arguments:
    $boolFlag: '%env(bool:default::OPTIONAL_ENV_VAR)%'

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.

final class EnvironmentService
{
    public function __construct(
        private bool $boolFlag,
    ) {}
}
# services.yaml
\EnvironmentService:
  arguments:
    $boolFlag: '%env(bool:default::OPTIONAL_ENV_VAR)%'

Symfony container build will fail due to null being passed as the $boolFlag which has to be a bool.

Possible Solution

No response

Additional Context

No response

@fancyweb
Copy link
Contributor

Looking at the code, it looks more intended than a bug.

@fancyweb
Copy link
Contributor

I'd like to close this one. null is a "special" value that is not cast by casting env var processors. WDYT @nicolas-grekas?

You should use an explicit default fallback param instead of trying to rely on casting null:

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 😄

@ro0NL
Copy link
Contributor

ro0NL commented May 31, 2023

alternatively, dont use 'optional envs' :)

@melkamar
Copy link
Author

melkamar commented Jun 1, 2023

You should use an explicit default fallback param instead of trying to rely on casting null

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:

  • merge the documentation PR to make it explicit that null can be a result of the typecast-processor when used with default, or
  • change the behavior so that typecast processors always return non-nullable values.

@melkamar
Copy link
Author

melkamar commented Jun 1, 2023

alternatively, dont use 'optional envs' :)

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.

@ro0NL
Copy link
Contributor

ro0NL commented Jun 1, 2023

i'm serious :) default hides undefined envs

If the env var gets somehow unset, you wouldn't know ... which IMHO is a footgun.

Having optional options for the app behavior is often necessary

why?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 1, 2023

Please let's keep this discussion focused, it's not the place to push anyone's personal preferences.
Using optional env vars is supported and perfectly fine.

nicolas-grekas added a commit that referenced this issue Jun 1, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants