Skip to content

[DependencyInjection] Stop considering empty env vars as populated #48705

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
Merged
merged 1 commit into from
Dec 28, 2022
Merged

[DependencyInjection] Stop considering empty env vars as populated #48705

merged 1 commit into from
Dec 28, 2022

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 19, 2022 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? no (DX improvement)
Deprecations? no
Tickets -
License MIT
Doc PR -

When a .env file declares an empty var (eg FOO=), env-var loaders are skipped.
This is a foot gun that has hit me many times when using secrets and that I consider a DX bug.
This PR fixes the issue by ensuring that env vars defined as empty strings don't prevent loaders from being called.

@carsonbot carsonbot added this to the 5.4 milestone Dec 19, 2022
@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] fix: fix typo about help [DependencyInjection] Fix considering empty env vars as populated Dec 19, 2022
@fabpot fabpot modified the milestones: 5.4, 6.3 Dec 27, 2022
@nicolas-grekas nicolas-grekas added DX DX = Developer eXperience (anything that improves the experience of using Symfony) and removed Bug labels Dec 27, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 5.4 to 6.3 December 28, 2022 11:32
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Fix considering empty env vars as populated [DependencyInjection] Stop considering empty env vars as populated Dec 28, 2022
@nicolas-grekas
Copy link
Member Author

PR rebased on 6.3

@fabpot
Copy link
Member

fabpot commented Dec 28, 2022

Thank you @nicolas-grekas.

@fabpot fabpot merged commit f79d848 into symfony:6.3 Dec 28, 2022
@nicolas-grekas nicolas-grekas deleted the di-empty-env branch December 28, 2022 14:39
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 9, 2023
Closed
@PhilETaylor
Copy link
Contributor

PhilETaylor commented Apr 20, 2023 •
edited
Loading

Just a question on this.

Prior to 6.3 it was common to have things like

###> sentry/sentry-symfony ###
#SENTRY_DSN="https://<redacted>@o96822.ingest.sentry.io/< redacted>"
SENTRY_DSN=""
###< sentry/sentry-symfony ###

in the .env during development, to quickly toggle between something being enabled or disabled. This is then references in config/sentry.php (just as an example)

return static function (ContainerConfigurator $containerConfigurator): void {
    $containerConfigurator->extension(
        'sentry',
        [
            'dsn' => env('SENTRY_DSN'),
        ]
    );
};

or in the .env.test.* files to have empty, but defined env vars so that the app can load, but those api's being untested did not need a valid string.

twitter_consumer_key=
twitter_consumer_secret=
twitter_access_token=
twitter_access_token_secret=

Is it the intention of this PR to introduce a b/c break where this would work in 6.2, but after upgrading to Symfony 6.3, this would now throw an throw new EnvNotFoundException(sprintf('Environment variable not found: "%s".', $name)); exception even though the env var is still there, its just an empty string?

If it was your intention for this to not be backward compatible, then cool - it worked :)

Just trying to work out if it's on purpose or a bug introduced.

@nicolas-grekas
Copy link
Member Author

Did you experience this exception or are you just wondering? If you're wondering, can you give it a try?

@PhilETaylor
Copy link
Contributor

I've experienced it on multiple projects upgrading to 6.3.x-dev sorry

Will dig deeper tomorrow and find a minimal reproducer for you

@PhilETaylor PhilETaylor mentioned this pull request Apr 21, 2023
Closed
@PhilETaylor
Copy link
Contributor

As promised, a scaled down reproducer is detailed here for you #50094

@nicolas-grekas nicolas-grekas mentioned this pull request May 11, 2023
Merged
nicolas-grekas added a commit that referenced this pull request May 12, 2023
@nicolas-grekas
bug #50301 [FrameworkBundle] Ignore vars from dotenv files in secrets…
eb6a111 
…:list (nicolas-grekas)

This PR was merged into the 6.3 branch.

Discussion
----------

[FrameworkBundle] Ignore vars from dotenv files in secrets:list

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Sidekick of #48705

Commits
-------

8561045 [FrameworkBundle] Ignore vars from dotenv files in secrets:list
@Okhoshi Okhoshi mentioned this pull request Jun 1, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Jun 9, 2023
@nicolas-grekas
bug #50530 [DependencyInjection] Fix support for false boolean env …
87a65d8 
…vars (Okhoshi)

This PR was merged into the 6.3 branch.

Discussion
----------

[DependencyInjection] Fix support for `false` boolean env vars

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

After upgrading `symfony/dependency-injection` package to version 6.3, some of our env vars couldn't be found anymore.

For some scripts, we are providing an adhoc default value to env vars by setting `$_SERVER` directly. However, since version 6.3 of the DependencyInjection component, setting an env var to `false` (the boolean value, like in the snippet below) doesn't work anymore, and Symfony reports that the variable cannot be found.

```php
$_SERVER['FOO'] = false;
```

It seems to be a side effect of the changes made in #48705.

Commits
-------

5101d18 [DependencyInjection] Fix support for `false` boolean env vars
@hanishsingla hanishsingla mentioned this pull request Oct 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@yceruto yceruto yceruto approved these changes

@chalasr chalasr chalasr approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx

@wouterj wouterj Awaiting requested review from wouterj

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
DependencyInjection DX DX = Developer eXperience (anything that improves the experience of using Symfony) Status: Needs Review
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
6 participants
@nicolas-grekas @fabpot @PhilETaylor @yceruto @chalasr @carsonbot