8000 [DependencyInjection] env var support broken in 3.3.7 and 3.3.8 · Issue #24020 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] env var support broken in 3.3.7 and 3.3.8 #24020

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
gharlan opened this issue Aug 29, 2017 · 8 comments
Closed

[DependencyInjection] env var support broken in 3.3.7 and 3.3.8 #24020

gharlan opened this issue Aug 29, 2017 · 8 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@gharlan
Copy link
Contributor
gharlan commented Aug 29, 2017
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.7, 3.3.8

I'm using sf flex with propel. This is part of my config:

propel:
    database:
        connections:
            default:
                adapter:  pdo_mysql
                user:     '%env(DATABASE_USER)%'
                password: '%env(DATABASE_PASSWORD)%'
                dsn:      'pdo_mysql:host=%env(DATABASE_HOST)%;dbname=%env(DATABASE_NAME)%;charset=utf8'

With symfony/dependency-injection 3.3.7 and 3.3.8 I get this exception:

screenshot 2017-08-29 11 20 52

#24016 didn't fix this case.

When I switch back to 3.3.6 it works again.

ping @nicolas-grekas
@stof
Copy link
Member
stof commented Aug 29, 2017

@nicolas-grekas looks like the env placeholder is not replaced back properly anymore in the processed container after your fix.

@nicolas-grekas
Copy link
Member

@gharlan can you share a reproducer please?

@nicolas-grekas nicolas-grekas mentioned this issue Aug 29, 2017
Merged
@nicolas-grekas
Copy link
Member

@gharlan can you please try #24021

@gharlan
Copy link
Contributor Author
gharlan commented Aug 29, 2017 •
edited
Loading

It does not fix it. I can try to create a reproducer, but before, some infos about PropelBundle, maybe it already helps to find the reason:

The PropelBundle writes the config to a parameter:

https://github.com/propelorm/PropelBundle/blob/61ef13c80380b74609501e8d4a1927cfd4d1acec/DependencyInjection/PropelExtension.php#L50

And this parameter is read later and passed to the connection manager (Line 82):

https://github.com/propelorm/PropelBundle/blob/3.0/PropelBundle.php#L57

When I add a breakpoint here, and look into $this->container, there is a difference in container->dynamicParameters['propel.configuration'].

In 3.3.6 the env vars are resolved (%env(DATABASE_HOST)% -> localhost etc), in 3.3.7, 3.3.8 and your pr, they are only replaced by env_DATABASE_HOST_ad096d2943aaf75934bd7232501a52ec etc.

@nicolas-grekas
Copy link
Member

OK, I can reproduce, working on it now.

@nicolas-grekas nicolas-grekas mentioned this issue Aug 29, 2017
Merged
@nicolas-grekas
Copy link
Member

Now fixed in #24021 and also in propelorm/PropelBundle#461

This was referenced Aug 29, 2017
Closed
Merged
fabpot added a commit to symfonycorp/connect-bundle that referenced this issue Aug 29, 2017
@fabpot
bug #17 Fixed configuration processing (jeremyFreeAgent)
d246ab1 
This PR was merged into the 4.x-dev branch.

Discussion
----------

Fixed configuration processing

Not calling the `Extension::processConfiguration()` method prevents tracking env vars correctly, as spotted in symfony/symfony#24020.

Commits
-------

095497c Fixed configuration processing
@samvdb samvdb mentioned this issue Aug 30, 2017
Merged
@Simperfit
Copy link
Contributor

This can be closed @nicolas-grekas.

@xabbuh
Copy link
Member
xabbuh commented Aug 31, 2017

@Simperfit Are you sure? #24021 isn't merged yet.

@fabpot fabpot closed this as completed Aug 31, 2017
fabpot added a commit that referenced this issue Aug 31, 2017
@fabpot
bug #24021 [DI] Don't track merged configs when the extension doesn't…
b641a76 
… expose it (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Don't track merged configs when the extension doesn't expose it

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

This is driving me crazy :)

Commits
-------

a8e6aac [DI] Don't track merged configs when the extension doesn't expose it
@Lctrs Lctrs mentioned this issue Sep 6, 2017
Merged
This was referenced Oct 21, 2017
Closed
Merged
< 5767 /div>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@fabpot @javiereguiluz @nicolas-grekas @gharlan @stof @xabbuh @Simperfit @carsonbot
0