Skip to content

Null environment variable cast to empty string #50815

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.

Sign up for GitHub

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

Jump to bottom
Closed
fmarchalemisys opened this issue Jun 29, 2023 · 6 comments
Closed

Null environment variable cast to empty string #50815

fmarchalemisys opened this issue Jun 29, 2023 · 6 comments
Assignees
@fancyweb
Labels
Bug DependencyInjection Status: Needs Review

Comments

@fmarchalemisys
Copy link
Contributor

Symfony version(s) affected

6.3.1

Description

The following code used to work with symfony 6.3.0 but not with 6.3.1.

In services.yaml:

parameters:
    env(HTTP_UNIQUEID): ~

In a service:

    public function __construct(
        #[Autowire('%env(string:HTTP_UNIQUEID)%')] ?string $xRequestId
    ) {
        // symfony 6.3.0: $xRequestId is null
        // symfony 6.3.1: $xRequestId is ""
        $this->xRequestId = $xRequestId ?? $this->generateSafeUnpredictableId();
    }

With symfony 6.3.1, the $xRequestId is the empty string. It defeats the call to generateSafeUnpredictableId().

This issues was introduced by this change: symfony/dependency-injection@6c3c271#diff-0ebfb024d710d87ccb36098690a9aa8ec4cea8bae635a2c5f95813e3ceba3dabR184

This is a big BC break for a minor version change.

How to reproduce

In services.yaml:

parameters:
    env(HTTP_UNIQUEID): ~

In a service:

    public function __construct(
        #[Autowire('%env(string:HTTP_UNIQUEID)%')] ?string $xRequestId
    ) {
        // symfony 6.3.0: $xRequestId is null
        // symfony 6.3.1: $xRequestId is ""
        $this->xRequestId = $xRequestId ?? $this->generateSafeUnpredictableId();
        // $this->xRequestId is the very predictable empty string!
    }

Possible Solution

Replace

if (!\in_array($prefix, ['string', 'bool', 'not', 'int', 'float'], true)) {
    return null;
}

with

return null;

when $env is null in EnvVarProcessor.php.

Additional Context

Apache with the unique_id module generates a unique id for each request it processes. The unique id is stored in HTTP_UNIQUEID.

When the unique_id module is not enabled or caddy is used instead of apache, there are no HTTP_UNIQUEID. It must be generated on the fly by the service.

As a quick patch, I simply check if the env var is the empty string but this is a big BC break that's not easy to spot.
@fancyweb fancyweb self-assigned this Jun 29, 2023
@fancyweb
Copy link
Contributor

fancyweb commented Jun 29, 2023 •
edited
Loading

Thanks for reporting the issue with detailed info.

The problem is that the reported "bug" is precisely the fix we merged in #50517 😅 Basically, some people expect the cast to be applied, and others don't.

%env(SOME_VAR)% will also return '' now because the string prefix is applied by default btw.

I agree it causes unexpected behavior changes on 5.4, especially for the string prefix I guess.

Let's maybe revert it and reapply it on 6.4? What do you think @nicolas-grekas?

@nicolas-grekas
Copy link
Member

%env(SOME_VAR)% will also return '' now because the string prefix is applied by default btw.

maybe we can change that? when there's no explicit prefix, we keep null?

@fmarchalemisys
Copy link
Contributor Author

Is it possible to explicitly declare a nullable string like this:

#[Autowire('%env(?string:HTTP_UNIQUEID)%')] ?string $xRequestId

It's still a BC break but, at least, nullable can be allowed or not.

BTW, what happened to #29767 that introduced the nullable: prefix in 2019?

@MatTheCat
Copy link
Contributor

what happened to #29767 that introduced the nullable: prefix in 2019?

See #30504

@fancyweb fancyweb mentioned this issue Jun 30, 2023
Merged
@ndench
Copy link
Contributor

ndench commented Jul 3, 2023

IMHO, I think that if someone declares the variable like this, then they expect it to come through as nullable:

parameters:
    env(HTTP_UNIQUEID): ~

If they want it to default to empty string, they can always declare it like this instead:

parameters:
    env(HTTP_UNIQUEID): ''

nicolas-grekas added a commit that referenced this issue Jul 3, 2023
@nicolas-grekas
bug #50837 [DependencyInjection] Fix autocasting null env values to e…
1e1d91e 
…mpty string (fancyweb)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Fix autocasting null env values to empty string

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | -
| Deprecations? | -
| Tickets       | #50815 (comment)
| License       | MIT
| Doc PR        | -

This PR fixes autocasting `null` env values to `''`.
We continue to autocast all other values to string when there's no prefix.

It doesn't revert #50517, it just fixes an unintended side effect.

It doesn't fix #50815 request, it confirms the behavior: the solution for this issue would be to remove the `string:` prefix.

There's still no solution to have the "keep null as null" and "cast if not null" behavior.

Commits
-------

2c0815d [DependencyInjection] Fix autocasting null env values to empty string
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this issue Jul 3, 2023
@nicolas-grekas
bug #50837 [DependencyInjection] Fix autocasting null env values to e…
6858b2d 
…mpty string (fancyweb)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Fix autocasting null env values to empty string

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | -
| Deprecations? | -
| Tickets       | symfony/symfony#50815 (comment)
| License       | MIT
| Doc PR        | -

This PR fixes autocasting `null` env values to `''`.
We continue to autocast all other values to string when there's no prefix.

It doesn't revert symfony/symfony#50517, it just fixes an unintended side effect.

It doesn't fix symfony/symfony#50815 request, it confirms the behavior: the solution for this issue would be to remove the `string:` prefix.

There's still no solution to have the "keep null as null" and "cast if not null" behavior.

Commits
-------

2c0815d04e [DependencyInjection] Fix autocasting null env values to empty string
@nicolas-grekas
Copy link
Member

Use #[Autowire('%env(HTTP_UNIQUEID)%')] and it should work after #50837 is released (by the end of the month.)

@Wiktor6 Wiktor6 mentioned this issue Jun 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fancyweb fancyweb

Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@nicolas-grekas @MatTheCat @xabbuh @ndench @fancyweb @carsonbot @fmarchalemisys