8000 3.3.7 killed DotEnv :( · Issue #24015 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

3.3.7 killed DotEnv :( #24015

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
PhilETaylor opened this issue Aug 28, 2017 · 16 comments
Closed

3.3.7 killed DotEnv :( #24015

PhilETaylor opened this issue Aug 28, 2017 · 16 comments
Labels
BC Break DependencyInjection Status: Needs Review
Milestone
3.3

Comments

@PhilETaylor
Copy link
Contributor
PhilETaylor commented Aug 28, 2017 •
edited
Loading

3.3.6 working fine.

Composer update to 3.3.7, cleared all caches..

config.yml like:

snc_redis:
    session:
        client: session
        prefix: "session"
        ttl: 604800
    clients:
        default:
            type: predis
            alias: default
            dsn: "redis://%env(redis_server1)%/%env(redis_db)%"
            logging: "%kernel.debug%"
        cache:
            type: predis
            alias: cache
            dsn: "redis://%env(redis_server1)%/%env(redis_db)%"
            options:
                connection_timeout: 10
                read_write_timeout: 30

turns into

screen shot 2017-08-28 at 22 20 04

note the:

[tcp://env_redis_server_b4db6b8d7bb2020da1133e356d30a3e5:6379]

that is NOT a server name :(
@chalasr chalasr added this to the 3.3 milestone Aug 28, 2017
@fabpot
Copy link
Member
fabpot commented Aug 28, 2017

I won't be able to have a look at this today, but if someone can make a PR to fix this, I can probably make a release tonight.

@fabpot
Copy link
Member
fabpot commented Aug 28, 2017

/cc @nicolas-grekas?

@nicolas-grekas
Copy link
Member

@PhilETaylor can you provide a reproducer asap please? eg a standard edition which has the issue?

@PhilETaylor
Copy link
Contributor Author

on the case :)

@nicolas-grekas nicolas-grekas mentioned this issue Aug 28, 2017
Merged
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Aug 28, 2017 •
edited
Loading

@PhilETaylor can you please check #24016, does it fix the issue?

@PhilETaylor
Copy link
Contributor Author

@nicolas-grekas 🎉 BINGO! - #24016 fixes my problem ;-) Incredible turnaround time!

fabpot added a commit that referenced this issue Aug 28, 2017
@fabpot
bug #24016 [DI] Fix tracking env var placeholders nested in object gr…
64bcf6d 
…aphs (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Fix tracking env var placeholders nested in object graphs

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

Commits
-------

a8397cf [DI] Fix tracking env var placeholders nested in object graphs
@fabpot fabpot closed this as completed Aug 28, 2017
@fabpot
Copy link
Member
fabpot commented Aug 28, 2017

3.3.8 released.

@PhilETaylor
Copy link
Contributor Author

thats insane... :-) thanks!

@chalasr
Copy link
Member
chalasr commented Aug 28, 2017

Thanks for the quick report.

@PhilETaylor
Copy link
Contributor Author

Upgraded to 3.3.8 and confirmed all is ok :) 💤 bedtime now :)

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

Hey @nicolas-grekas I think I have another one of these for you to think about - when deploying to production :-(

I could not replicate this on a fresh install, but it looks related to the 3.3.7 issue :)

in my config_prod.yml I have

framework:
    session:
        cookie_secure:  "%env(app_is_on_a_ssl)%"

and in .env

app_is_on_a_ssl=TRUE

and I get

Invalid type for path "framework.session.cookie_secure". Expected boolean, but got string.

screen shot 2017-08-29 at 10 14 18

@stof
Copy link
Member
stof commented Aug 29, 2017

@PhilETaylor env placeholders don't work fine for configuration nodes now allowing strings, as they are strings. This is not related to the 3.3.7 issue. This is the case since the introduction of env placeholders.
On a side note, env variables are always strings, so this may not do what you think here, even though the validation was not rejecting it (setting the env variable to FALSE would still be a truthy string)

@PhilETaylor
Copy link
Contributor Author

ah ok - still seems that this should be a perfectly acceptable assumption that I can use env placeholders where ever I like in a config yml right? Isnt that the whole point of DotEnv?

I note also that

framework:
    secret:          "%env(secret)%"

gives

screen shot 2017-08-29 at 10 40 04

and thus the env() placeholder is returning the wrong value... thus making it unuseable :(

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

Despite indeed being neither a bug nor related to the previous issue I would most certainly recommend making a new issue to track this, as it is a practical shortcoming of the current implementation with, as far as I ca B2A1 n see, no real workaround right now without reintroducing deployment information in the environments, which is exactly what env variables are meant to fix.

We may need some kind of syntax like %env(app_is_on_a_ssl, bool)% so we can coerce this during container compilation.

ah ok - still seems that this should be a perfectly acceptable assumption that I can use env placeholders where ever I like in a config yml right?

Yes and no. Methinks it should be possible to import env variables as ints, floats or bools according to strictly defined rules. We should certainly not magically decide that the string FALSE is in some occasions intended to be boolean false, in others the integer 0, and in yet others its literal string representation. The whole point of the exception you're getting is that the Configuration component forces you to be typesafe about it with good reason.

@nicolas-grekas
Copy link
Member

No need for a new issue, this is #22151, we're working on it already.

@PhilETaylor
Copy link
Contributor Author

ok in this case I think I need to move back to vlucas/phpdotenv and use SYMFONY__ prefixed env vars in .env again :-(

I'm all for being an early adopter, but it seems symfony/dotenv and env() placeholders is too young at the moment for me :) :)

Thanks for the speedy replies. Appreciated much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break DependencyInjection Status: Needs Review
Projects
None yet
Milestone
3.3
Development

No branches or pull requests
6 participants
@fabpot @nicolas-grekas @PhilETaylor @stof @curry684 @chalasr
0