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

Don't throw fatal errors for unused env vars #23520

Closed
javiereguiluz opened this Issue Jul 15, 2017 · 23 comments

Comments

Projects
None yet
9 participants
@javiereguiluz
Member

javiereguiluz commented Jul 15, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.3.x

While upgrading an app to Symfony 3.3 + Symfony Flex, I'm facing some issues. All of them look reasonable to me, except this one:

Fatal error: Uncaught Symfony\Component\DependencyInjection\Exception\EnvParameterException:

Environment variables "DATABASE_URL" are never used. Please, check your
container's configuration.

I think it's crazy to throw a fatal error just because I'm not using a defined env var. Could we please change this behavior? Thanks!

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 15, 2017

Member

I don't think it's a crazy decision we made. Of course this can be reconsidered, but the arguments in the original discussion should not be discarded too fast. See #19681

Member

nicolas-grekas commented Jul 15, 2017

I don't think it's a crazy decision we made. Of course this can be reconsidered, but the arguments in the original discussion should not be discarded too fast. See #19681

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 15, 2017

Member

I apologize if "crazy" sounded too harsh 😞

Member

javiereguiluz commented Jul 15, 2017

I apologize if "crazy" sounded too harsh 😞

@javiereguiluz javiereguiluz changed the title from [Don't throw fatal errors for unused env vars to Don't throw fatal errors for unused env vars Jul 15, 2017

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jul 15, 2017

Contributor

#19681 (comment) this one?

Maybe we can add an option to white-list some env vars usage 🤔 but were to initialize this configuration. Because it's part of the Container, and we can't (shouldn't) use static.

Contributor

sstok commented Jul 15, 2017

#19681 (comment) this one?

Maybe we can add an option to white-list some env vars usage 🤔 but were to initialize this configuration. Because it's part of the Container, and we can't (shouldn't) use static.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jul 17, 2017

Member

I think I agree with @javiereguiluz here. I've add this issue more than once and that's annoying. Let's talk about this :)

Member

fabpot commented Jul 17, 2017

I think I agree with @javiereguiluz here. I've add this issue more than once and that's annoying. Let's talk about this :)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

There is the unused case and the swallowed case, which is the bad one. The current exceptions help DX by spotting otherwise silent failures. Not sure we can separate both cases, thus really not sure this is a good idea to remove the exceptions...

Member

nicolas-grekas commented Jul 17, 2017

There is the unused case and the swallowed case, which is the bad one. The current exceptions help DX by spotting otherwise silent failures. Not sure we can separate both cases, thus really not sure this is a good idea to remove the exceptions...

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 19, 2017

Member

Related to #22726 and #22561

Member

nicolas-grekas commented Jul 19, 2017

Related to #22726 and #22561

@eXtreme

This comment has been minimized.

Show comment
Hide comment
@eXtreme

eXtreme Jul 28, 2017

Contributor

Having info about unused envs sound reasonable but this error also happens, as explained in #22561, by just overriding some configs in test env - which is very annoying and was a BC break for me (but happened only in test env for me, but could happen if someone had envs used in dev and then overwritten configs for prod).

Contributor

eXtreme commented Jul 28, 2017

Having info about unused envs sound reasonable but this error also happens, as explained in #22561, by just overriding some configs in test env - which is very annoying and was a BC break for me (but happened only in test env for me, but could happen if someone had envs used in dev and then overwritten configs for prod).

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 30, 2017

Member

While upgrading Symfony Demo to Flex this happened to us again:

1) App\Tests\Command\AddUserCommandTest::testCreateUserNonInteractive with data set #0 (false)
Symfony\Component\DependencyInjection\Exception\EnvParameterException:
Environment variables "DATABASE_URL" are never used. Please, check your container's configuration.

Not that easy to solve because the database URL is different in the test environment and we don't have a .env.test.

Member

javiereguiluz commented Jul 30, 2017

While upgrading Symfony Demo to Flex this happened to us again:

1) App\Tests\Command\AddUserCommandTest::testCreateUserNonInteractive with data set #0 (false)
Symfony\Component\DependencyInjection\Exception\EnvParameterException:
Environment variables "DATABASE_URL" are never used. Please, check your container's configuration.

Not that easy to solve because the database URL is different in the test environment and we don't have a .env.test.

@fefas

This comment has been minimized.

Show comment
Hide comment
@fefas

fefas Aug 9, 2017

@nicolas-grekas .. sorry if my message in the #22726 sounded harsh and I even didn't know if the solution I proposed on this PR would be a good one. My primary intention there was to bring this discussion up, because nobody (member or owner) answered to the issue I opened (#22561).

I'd read the original discussion (#19681) before I've even opened that issue to understand what I would be doing wrong. Actually, I didn't understand everything there because both I don't really know how Symfony works under the hood and the thread is too long. In this way, for me (as a final user), the exception thrown seems to be "crazy". I also saw the PR #22976 that is an improvement, because it helps us to understand what we did "wrong".

Anyway, my situation here now is: we are trying to deliver our application using docker containers and all of the configuration is made through environments variables. That helps us both for Continuous Integration and for easy and fast Deployments. Now, we can deploy a complete new environment using only one command with a new set of env variables that are sent on containers startup. Given the context, it's completely normal that some environments variable is overridden sometimes.

With that exception, Symfony DI guarantees that every environment variable used at least once on at least one configuration file is being using on the resulted cache file. I have two concerns:

  • The exception is thrown on a dumper class and, for me, that class should not handle with any DI logic
  • Is that really Symfony's matter? For example: if there is something wrong in the configuration, it should be caught by functional tests or something like that.

fefas commented Aug 9, 2017

@nicolas-grekas .. sorry if my message in the #22726 sounded harsh and I even didn't know if the solution I proposed on this PR would be a good one. My primary intention there was to bring this discussion up, because nobody (member or owner) answered to the issue I opened (#22561).

I'd read the original discussion (#19681) before I've even opened that issue to understand what I would be doing wrong. Actually, I didn't understand everything there because both I don't really know how Symfony works under the hood and the thread is too long. In this way, for me (as a final user), the exception thrown seems to be "crazy". I also saw the PR #22976 that is an improvement, because it helps us to understand what we did "wrong".

Anyway, my situation here now is: we are trying to deliver our application using docker containers and all of the configuration is made through environments variables. That helps us both for Continuous Integration and for easy and fast Deployments. Now, we can deploy a complete new environment using only one command with a new set of env variables that are sent on containers startup. Given the context, it's completely normal that some environments variable is overridden sometimes.

With that exception, Symfony DI guarantees that every environment variable used at least once on at least one configuration file is being using on the resulted cache file. I have two concerns:

  • The exception is thrown on a dumper class and, for me, that class should not handle with any DI logic
  • Is that really Symfony's matter? For example: if there is something wrong in the configuration, it should be caught by functional tests or something like that.
@fefas

This comment has been minimized.

Show comment
Hide comment
@fefas

fefas Aug 9, 2017

Sorry.. I pressed comment before writing the end of the message

I would love to contribute and to find a fix for this issue now reported by others developers too (the PR #23773 makes the same I tried). So, I am completely open to think about the problem and recreate the PR with new changes if you give me some direction. As @fabpot said, this exception is really annoying 😝 ...

So, I'd like to ask you: why do you believe it's not a good idea to remove this exception? How exactly this helps DX? Are there more concerns?

I'd like also to say one more thing: I totally respect the decision of the Symfony maintainers. I always learn so much by watching talks of you. You are outstanding developers and, because of that, it's really hard to say publicly that I don't agree with some part of the code you wrote.

fefas commented Aug 9, 2017

Sorry.. I pressed comment before writing the end of the message

I would love to contribute and to find a fix for this issue now reported by others developers too (the PR #23773 makes the same I tried). So, I am completely open to think about the problem and recreate the PR with new changes if you give me some direction. As @fabpot said, this exception is really annoying 😝 ...

So, I'd like to ask you: why do you believe it's not a good idea to remove this exception? How exactly this helps DX? Are there more concerns?

I'd like also to say one more thing: I totally respect the decision of the Symfony maintainers. I always learn so much by watching talks of you. You are outstanding developers and, because of that, it's really hard to say publicly that I don't agree with some part of the code you wrote.

@fefas

This comment has been minimized.

Show comment
Hide comment
@fefas

fefas Aug 9, 2017

We've just found (me and @xthiago) a workaround for the issue #22561:

# app/config/database.yml

doctrine:
    dba:
        connections:
            default:
                host: "%database_host%"
                dbname: "%database_dbname%"
                user: "%database_user%"
                password: "%database_password%"
// app/AppKernel.php

protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader)
{
    $this->loadEnvironmentVariables($container);

    // ...
}

protected function loadEnvironmentVariables(ContainerBuilder $container)
{
    $useTestDatabase = 'test' === $this->getEnvironment();

    if ($useTestDatabase) {
        $container->setParameter('database_host', getenv('DATABASE_TEST_HOST'));
        $container->setParameter('database_dbname', getenv('DATABASE_TEST_DBNAME'));
        // ...

        return;
    }

    $container->setParameter('database_host', getenv('DATABASE_HOST'));
    $container->setParameter('database_dbname', getenv('DATABASE_DBNAME'));
    // ...
}

Pay attention with cache generation. The environment variables MUST be defined during cache warm up.

fefas commented Aug 9, 2017

We've just found (me and @xthiago) a workaround for the issue #22561:

# app/config/database.yml

doctrine:
    dba:
        connections:
            default:
                host: "%database_host%"
                dbname: "%database_dbname%"
                user: "%database_user%"
                password: "%database_password%"
// app/AppKernel.php

protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader)
{
    $this->loadEnvironmentVariables($container);

    // ...
}

protected function loadEnvironmentVariables(ContainerBuilder $container)
{
    $useTestDatabase = 'test' === $this->getEnvironment();

    if ($useTestDatabase) {
        $container->setParameter('database_host', getenv('DATABASE_TEST_HOST'));
        $container->setParameter('database_dbname', getenv('DATABASE_TEST_DBNAME'));
        // ...

        return;
    }

    $container->setParameter('database_host', getenv('DATABASE_HOST'));
    $container->setParameter('database_dbname', getenv('DATABASE_DBNAME'));
    // ...
}

Pay attention with cache generation. The environment variables MUST be defined during cache warm up.

@ousmaneNdiaye

This comment has been minimized.

Show comment
Hide comment
@ousmaneNdiaye

ousmaneNdiaye Aug 11, 2017

I got the same error in my test env. All tests were broken after defining an env variables in my config (for Heroku deployment purpose).

Environment variables "FOOBAR" are never used. Please, check your container's configuration.

Not sure if it can helps but waiting a cleaner solution, I fix it by adding dummy parameters in my parameters.yml file.

/parameters.yml
app.dummy_param1: '%env(FOOBAR)%'

Now I can run my tests again

ousmaneNdiaye commented Aug 11, 2017

I got the same error in my test env. All tests were broken after defining an env variables in my config (for Heroku deployment purpose).

Environment variables "FOOBAR" are never used. Please, check your container's configuration.

Not sure if it can helps but waiting a cleaner solution, I fix it by adding dummy parameters in my parameters.yml file.

/parameters.yml
app.dummy_param1: '%env(FOOBAR)%'

Now I can run my tests again

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 16, 2017

Member

There is at least one false-positive case for this issue that should be fixed by #23903.
Did you mean others? I'd then need a more precise description of when it still happens.

Member

nicolas-grekas commented Aug 16, 2017

There is at least one false-positive case for this issue that should be fixed by #23903.
Did you mean others? I'd then need a more precise description of when it still happens.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 18, 2017

Member

If the issue still arises, please open a new issue with a more specific description - or better: a reproducer.

Member

nicolas-grekas commented Aug 18, 2017

If the issue still arises, please open a new issue with a more specific description - or better: a reproducer.

nicolas-grekas added a commit that referenced this issue Aug 18, 2017

bug #23903 [DI] Fix merging of env vars in configs (nicolas-grekas)
This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Fix merging of env vars in configs

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22561, #23520
| License       | MIT
| Doc PR        | -

Commits
-------

00b9273 [DI] Fix merging of env vars in configs
@garak

This comment has been minimized.

Show comment
Hide comment
@garak

garak Aug 28, 2017

Contributor

I just upgraded from eeaea83 to 4173f13 and got this error 😞

Environment variables "mailer_transport" are never used, but my config has:

parameters:
    env(mailer_transport): smtp

swiftmailer:
    transport: '%env(mailer_transport)%'
Contributor

garak commented Aug 28, 2017

I just upgraded from eeaea83 to 4173f13 and got this error 😞

Environment variables "mailer_transport" are never used, but my config has:

parameters:
    env(mailer_transport): smtp

swiftmailer:
    transport: '%env(mailer_transport)%'
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 28, 2017

Member

did you get the error before also?
is "disable_delivery" set?
can you provide a reproducer?

Member

nicolas-grekas commented Aug 28, 2017

did you get the error before also?
is "disable_delivery" set?
can you provide a reproducer?

@garak

This comment has been minimized.

Show comment
Hide comment
@garak

garak Aug 28, 2017

Contributor

If I go back to eeaea83 there's no error.
Yes, "disable_delivery" is true in dev environment, but I also tried bin/console -e=prod and got the same error.
I'll try to provide a reproducer

Contributor

garak commented Aug 28, 2017

If I go back to eeaea83 there's no error.
Yes, "disable_delivery" is true in dev environment, but I also tried bin/console -e=prod and got the same error.
I'll try to provide a reproducer

@garak

This comment has been minimized.

Show comment
Hide comment
@garak

garak Aug 28, 2017

Contributor

If I go back to eeaea83 there's no error.
Yes, "disable_delivery" is true in dev environment, but I also tried bin/console -e=prod and got the same error.
Edit: here is a reproducer try: https://bitbucket.org/garak/symfony-standard

Contributor

garak commented Aug 28, 2017

If I go back to eeaea83 there's no error.
Yes, "disable_delivery" is true in dev environment, but I also tried bin/console -e=prod and got the same error.
Edit: here is a reproducer try: https://bitbucket.org/garak/symfony-standard

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Aug 28, 2017

fixed in #24009

fabpot added a commit that referenced this issue Aug 28, 2017

bug #24009 [DI] Fix tracking env vars when merging configs (bis) (nic…
…olas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Fix tracking env vars when merging configs (bis)

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23520 again
| License       | MIT
| Doc PR        | -

Commits
-------

baaff20 [DI] Fix tracking env vars when merging configs (bis)
@renatomefi

This comment has been minimized.

Show comment
Hide comment
@renatomefi

renatomefi Oct 12, 2017

Contributor

In my case the bundle extension unsets a value depending on another one:

\\ Bundle Extension load
        if ($config['api']['use_proxy'] === false) {
            $container->setParameter('api_client_proxy_uri', null);
        }

Since the used variables comparison are checking for the same values I get the unused variables exception, and that's really inconvenient since they still have a purpose.
What's the solution in this case?

Contributor

renatomefi commented Oct 12, 2017

In my case the bundle extension unsets a value depending on another one:

\\ Bundle Extension load
        if ($config['api']['use_proxy'] === false) {
            $container->setParameter('api_client_proxy_uri', null);
        }

Since the used variables comparison are checking for the same values I get the unused variables exception, and that's really inconvenient since they still have a purpose.
What's the solution in this case?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 12, 2017

Member

call $container->resolveEnvPlaceholders() on the $config array in your DI extension, this meaning you take responsibility for dealing with env vars

Member

nicolas-grekas commented Oct 12, 2017

call $container->resolveEnvPlaceholders() on the $config array in your DI extension, this meaning you take responsibility for dealing with env vars

@renatomefi

This comment has been minimized.

Show comment
Hide comment
@renatomefi

renatomefi Oct 13, 2017

Contributor

@nicolas-grekas what's the solution when I don't own the bundle?

Contributor

renatomefi commented Oct 13, 2017

@nicolas-grekas what's the solution when I don't own the bundle?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 13, 2017

Member

Not use env vars for this specific option (not all options are meant to be compatible with env vars)
Or submit a PR on the bundle to add support for env vars if that makes sense.

Member

nicolas-grekas commented Oct 13, 2017

Not use env vars for this specific option (not all options are meant to be compatible with env vars)
Or submit a PR on the bundle to add support for env vars if that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment