8000 [DependencyInjection] resolve env variables by jderusse · Pull Request #31000 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] resolve env variables #31000

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
wants to merge 1 commit into from
Closed

[DependencyInjection] resolve env variables #31000

wants to merge 1 commit into from

Conversation

jderusse
Copy link
Member
@jderusse jderusse commented Apr 8, 2019 •
edited
Loading
Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? yes (change the visibility of a method in a not-final class)
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

The documentation (https://symfony.com/doc/current/configuration/environment_variables.html#config-env-vars) give the following example:

parameters:
    env(HOST): '10.0.0.1'
    env(SENTRY_DSN): 'http://%env(HOST)%/project'
sentry:
    dsn: '%env(resolve:SENTRY_DSN)%'

This example works because the parameter env(HOST) is literally defined in the parameters sections. Reading the documentation we could think that env(HOST) is resolved by trying to read the HOST env variable then fallback to the default parameter, but it's not the case.

If I write the following same application using .env file I would have the error The parameter "env(HOST)" must be defined.

# .env
HOST="10.0.0.1"
SENTRY_DSN="http://%env(HOST)%/project"

This PR fix it. my use case is

# .env
DATABASE_URL="mysql://root:root@localhost/db_name"

# .env.prod
DATABASE_URL="mysql://my_user:%env(secret:DATABASE_PASSWORD)%@service.com/db_name"

note: to access the getEnv() which calls the list of env processors, I've to change the visibility of the method. Tell me we have another way to do it.

@jderusse jderusse changed the base branch from master to 3.4 April 8, 2019 07:28
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 8, 2019 •
edited
Loading

Way too hacky sorry 👎 (the implementation - since it relies on strong coupling with Container - but more importantly the feature: env vars referencing env vars make no sense from an ops pov)

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 8, 2019
@jderusse jderusse mentioned this pull request Apr 8, 2019
Merged
@jderusse
Copy link
Member Author
jderusse commented Apr 8, 2019

Closing this PR as this is not the purpose of the resolve processor. Opening symfony/symfony-docs/pull/11355 to fix the documentation instead

env vars referencing env vars make no sense from an ops pov

disagree with that.

This is common usecase to reference variables in variables in order to avoid duplication and mistakes when changing a value.

DATABASE_PASWORD=foo
DATABASE_URL=mysql://user:${DATABASE_PASWORD}@server

start_database -p $DATABASE_PASWORD
start_app --database $DATABASE_URL

source prod.config.env
DATABASE_URL=mysql://user:${DEFINED_IN_SOURCED_FILE}@server

@jderusse jderusse closed this Apr 8, 2019
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Apr 8, 2019
@OskarStark
minor #11355 Fix resolve example (jderusse)
7aed366 
This PR was merged into the 3.4 branch.

Discussion
----------

Fix resolve example

The `resolve` processor can only resolve `parameters` whereas the documentation give the feeling that we could resolve env variables too. Wich is not the case (see symfony/symfony#31000).

This PR change the example in order to be unambiguous.

Commits
-------

3e60b9d Fix resolve example
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Apr 8, 2019 •
edited
Loading

DATABASE_URL=mysql://user:${DATABASE_PASWORD}@server

this is not an env var referencing an env var, but a bash script creating an env var from another one :)

@jderusse jderusse deleted the fix-resolve-env branch August 2, 2019 12:16
@jderusse jderusse mentioned this pull request Oct 16, 2019
Merged
@jderusse jderusse mentioned this pull request Apr 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BC Break Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@jderusse @nicolas-grekas @carsonbot
0