Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[DependencyInjection] MakeContainer::getEnv public#46819

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
malarzm wants to merge1 commit intosymfony:6.2frommalarzm:public-get-env

Conversation

@malarzm
Copy link
Contributor

@malarzmmalarzm commentedJun 30, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#34173,#45450 (comment)
LicenseMIT
Doc PRto be added if accepted

In our app we're controlling certain features' availability with feature switches defined in envs (to prevent non-tech folks from turning them on without our assitance). We'd like to show whether those switches are on or off in the administration panel. Sadly, there is no kosher way to get those values during runtime other than injecting every variable we want to show. By makinggetEnv public we're exposing not only raw values one could get withgetenv but more importantly processors. By the way we're also ironing out two not-so-great hacks to get env variables in Symfony itself (included in PR).

Code will need finishing if we agree this is good idea:

  • I'll deal with new interface method according to the rules
  • Documentation will be written

I just don't want to waste time on these in case the PR will be shot down :)

derrabus reacted with thumbs down emoji
@carsonbotcarsonbot added Status: Needs Review DependencyInjection Feature RFCRFC = Request For Comments (proposals about features that you want to be discussed) labelsJun 30, 2022
@carsonbotcarsonbot added this to the6.2 milestoneJun 30, 2022
@carsonbotcarsonbot changed the title[RFC][DependencyInjection] Make Container::getEnv public[DependencyInjection] Make Container::getEnv publicJun 30, 2022
@raziel057
Copy link
Contributor

@malarzm Could it help for the following topics?
lexik/LexikTranslationBundle#420
#33358

Currently it seems complicated to resolve environment variable on loading extensions.

@malarzm
Copy link
ContributorAuthor

Currently it seems complicated to resolve environment variable on loading extensions.

No, this PR barely exposes accessing env variables to the outside world and does not change any parameters vs envs vs extensions.

@stof
Copy link
Member

@raziel057 not reading environment variables inside extensions is not a bug. It is afeature. The whole promise of env placeholders in the DependencyInjection is that new values of the environment variable will be taken into account without requiring to clear the cache. So if a container extension uses a setting to change the structure of the container instead of injecting into a service, it cannot use an env placeholder.

@raziel057
Copy link
Contributor

@stof Thanks for your answer. The issue is that often when you deploy an app in a container for example you don't want to embed a config file for static parameters that can change if you deploy the same image in different environment. It would be just nice / easier to be able to specify if the ENV should be resolved statically or not.

I have seen lots of people simply define static ENV vars in docker compose files or Kubernetes Management Platforms like Rancher for example.

I just seen this have been already discussed here:#40794

@stof
Copy link
Member

stof commentedJul 1, 2022

@raziel057 for such case, I'm still usinghttps://github.com/Incenteev/ParameterHandler to create non-env-based parameters duringcomposer install, based either on env variables or an interactive prompt.

@nicolas-grekas
Copy link
Member

I'm 👎 for the reasons given by@stof. Closing therefore, thanks for suggesting.

@malarzm
Copy link
ContributorAuthor

I'm sorry but I'm missing the reasons? To me it looks like discussion was unrelated to the proposal itself as it was about how envs are behaving.

@xabbuhxabbuh reopened thisJul 11, 2022
@ro0NL
Copy link
Contributor

would this already be achievable using either theParameterBagInterface orContainerBagInterface?

nicolas-grekas reacted with thumbs up emoji

@malarzm
Copy link
ContributorAuthor

malarzm commentedJul 11, 2022
edited
Loading

would this already be achievable using either the ParameterBagInterface or ContainerBagInterface?

It wouldn't expose processors that way as its logic is defined by\Symfony\Component\DependencyInjection\Container::getEnv itself. For instance in our example in our envs we have:SOME_FEATURE='2022-07-11 13:00:00'; and we have dedicated processor%env(feature_switch:SOME_FEATURE)% in services definitions (the processor treats date-ish strings asbool:'s true). Now if we'd be relying on stuff defined in parameters I would be forbidden to call%env(bool:SOME_FEATURE)% somewhere down the line (continuing my example, in a controller showing status of a feature) as that parameter does not exist (nor its fallback as we're defining it withfeature_switch:).

@ro0NL
Copy link
Contributor

IMHO the way forward would be$this->parameterBag->get('env(csv:APP_FOO)')

which currently only works using a hack like

parameters:env(csv:APP_FOO):'%env(csv:APP_FOO)%'

making it work out-of-the-box would be more elegant though.

malarzm reacted with thumbs up emoji

@OskarStarkOskarStark changed the title[DependencyInjection] Make Container::getEnv public[DependencyInjection] MakeContainer::getEnv publicJul 14, 2022
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 29, 2022
edited
Loading

Rereading this again I'm 👎 here: exposing new APIs is increasing the maintenance cost, and here, ppl will jump into using the method and shooting themself in the foot (see comments from@raziel057 and@stof)

IMHO the way forward would be $this->parameterBag->get('env(csv:APP_FOO)')

Yes, that's a good way forward (it works with any parameter name, not onlyenv(...))`

valtzu reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Closing as explained, thanks for proposing.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

DependencyInjectionFeatureRFCRFC = Request For Comments (proposals about features that you want to be discussed)Status: Needs Review

Projects

None yet

Milestone

6.2

Development

Successfully merging this pull request may close these issues.

[RFC][Dotenv] Add a way to retrieve env vars not only via DI

7 participants

@malarzm@raziel057@stof@nicolas-grekas@ro0NL@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp