Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Allow processing env vars#23901
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
marfillaster commentedAug 16, 2017
👍 I totally approve this solution. The chained processing is just genius. |
ab01c8d tod75dcaeCompare| * | ||
| * @throws RuntimeException on error | ||
| */ | ||
| publicfunctiongetEnv($prefix,$name,\Closure$getEnv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
preferably the namespace parsing happens inContainer::getEnv no? Basically you need to do the same check each time implementing it. What aboutgetEnv($prefix, $name, $value, \Closure $getEnv = null)?
nicolas-grekasAug 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
namespace parsing already happens inContainer::getEnv.
What you see here is forwarding, which is something not all providers are required to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
never mind, thats simply$value = $getEnv($name) currently. Basically the trick is to not forgetreturn $getEnv($name) for defaulting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and i missed your comment in between.. but you're right the current implem is fine.
| if (true !==$format) { | ||
| $resolved =sprintf($format,$env); | ||
| }elseif ($placeholder ===$resolved =$bag->escapeValue($this->getEnv($env))) { | ||
| $resolved =$bag->all()[strtolower("env($env)")]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we fix#23527 (comment) somehow?
nicolas-grekasAug 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@yceruto here we are, with a newresolve handler to the rescue.
mnapoli commentedAug 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Is |
nicolas-grekas commentedAug 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@mnapoli I think you're forgetting about how things work. Despite your claim in#22407, a PHP DSL would not help at all reading from the env. The reason is compilation of the container. The DSL is read once, at compile time. |
mnapoli commentedAug 16, 2017
👍 sorry :) |
| if (isset($attr['prefix'])) { | ||
| $providers[$attr['prefix']] =newReference($id); | ||
| }elseif (!$r =$container->getReflectionClass($class =$container->getDefinition($id)->getClass())) { | ||
| thrownewInvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.',$class,$id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this mean that the service class existence is not resolved at compile time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, as done commonly in other passes: when it's not required at compile time, we usually don't validate that kind of things.
| 100 =>array( | ||
| $resolveClassPass =newResolveClassPass(), | ||
| newResolveInstanceofConditionalsPass(), | ||
| newRegisterEnvProvidersPass(), |
nicolas-grekasAug 16, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
registered before optimization passes so that env values can be used at compile time (typically using$container->resolveEnvPlaceholders().) This makes env providers a bit less flexible (they can't easily be manipulated by DI extensions) but that's consistent with the provided feature to me (env vars should be light to fetch.)
Pierstoval commentedAug 17, 2017
I cannot agree more on this PR 👍 |
ro0NL commentedAug 17, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The $_ENV['REAL'] ='{"real":true}';$c =newContainerBuilder();$c->setParameter('json_real','%env(json:REAL)%');$c->register('foo','stdClass')->setProperties(array('json_real' =>'%json_real%',));$c->compile(true); Somehow real and default envs get mixed up, probably due reference generation in EnvBag::get() $_ENV['REAL'] ='runtime';$c =newContainerBuilder();$c->setParameter('env(REAL)','compiletime');$c->setParameter('real','%env(REAL)%');$c->register('teatime','stdClass')->setProperties(array('real' =>'%real%','real_inline' =>'%env(REAL)%',));$c->compile(true);dump($c->get('teatime')); This looks bad 🤔 What im worried about is the current default strategy. With this feature things may work unexpected due the exact lookup. Also related; if in above example you leave out the default ("compiletime"); you'll get Yet we did a real lookup already.. so what's the deal here? Otherwise it seems to work just great :) |
nicolas-grekas commentedAug 17, 2017
@ro0NL thanks for the detailed report. Good news, both (the 3 in fact) your examples apply to 3.3 also, which means you found bugs. Nothing related to this PR specifically. Those happen only in |
kbond commentedAug 17, 2017
What is the reason for no |
nicolas-grekas commentedAug 18, 2017
@kbond no reason, fixed. |
26c28eb to019a582Compare| useSymfony\Component\DependencyInjection\ContainerBuilder; | ||
| useSymfony\Component\DependencyInjection\ContainerInterface; | ||
| useSymfony\Component\DependencyInjection\Definition; | ||
| useSymfony\Component\DependencyInjection\EnvProviderInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should beEnvVarProcessorInterface (same below)
| $container->registerForAutoconfiguration(ResourceCheckerInterface::class) | ||
| ->addTag('config_cache.resource_checker'); | ||
| $container->registerForAutoconfiguration(EnvProviderInterface::class) | ||
| ->addTag('container.env_provider'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be renamed as well
fabpot commentedSep 6, 2017
Still some |
019a582 to2499a76Compare2499a76 to1f92e45Comparefabpot commentedSep 7, 2017
Thank you@nicolas-grekas. |
This PR was merged into the 3.4 branch.Discussion----------[DI] Allow processing env vars| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | see description| License | MIT| Doc PR | -This PR is an updated version of#20276 ~~(it embeds#23899 for now.)~~It superscedes/closes:- [DI] Add support for secrets#23621 ping@dunglas- Runtime container parameter not found event filter#23669 ping@marfillaster- [DependencyInjection] [DX] Support for JSON string environment variables#23823 ping@Pierstoval- add support for composite environment variables#17689 ping@greg0ire- [DI] ENV parameters at runtime with PHP 7 strict types not working properly#20434 ping@sandrokeil- Issue when using a SQLite database and the DATABASE_URL env var#23527 ping@javiereguiluz#22151 is another story, so not fixed here.The way it works is via `%env(foo:BAR)%` prefixes, where "foo" can be bound to any services you'd like.By default, the following prefixes are supported:- `bool`, `int`, `float`, `string`, `base64`- `const` (for referencing PHP constants)- `json` (supporting only json **arrays** for type predictability)- `file` (eg for access to secrets stored in files.)- `resolve` (for processing parameters inside env vars.)New prefixes can be added by implementing the new `EnvProviderInterface`, and tagging with `container.env_provider` (see `Rot13EnvProvider` in tests.)Prefixes can be combined to chain processing, eg.`%env(json:base64:file:FOO)%` will be roughly equivalent to`json_decode(base64_decode(file_get_content(getenv('FOO'))))`.Commits-------1f92e45 [DI] Allow processing env vars
sroze commentedSep 12, 2017
Great job@nicolas-grekas 👏 |
…ameter (yceruto)This PR was merged into the master branch.Discussion----------Workaround to use DATABASE_URL with %kernel.project_dir% parameterSetting `DATABASE_URL=sqlite:///var/data/blog.sqlite` as default, causes this known error through `index.php`:```An exception occurred in driver: SQLSTATE[HY000] [14] unable to open database file```So the right relative path in this case should be `sqlite:///../var/data/blog.sqlite` BUT then it wouldn't work for `bin/console` entries. Then, relative paths shouldn't be the right way but absolute paths.The right thing is that soon (in 3.4 & 4.0) we can use environment variables with configuration parameterssymfony/symfony#23901, so the final configuration would be the one proposed. For now a workaround to make both scenarios work `index.php` and `bin/console`.Commits-------e45b5d5 Workaround to use DATABASE_URL with %kernel.project_dir% parameter
…ameter (yceruto)This PR was merged into the master branch.Discussion----------Workaround to use DATABASE_URL with %kernel.project_dir% parameterSetting `DATABASE_URL=sqlite:///var/data/blog.sqlite` as default, causes this known error through `index.php`:```An exception occurred in driver: SQLSTATE[HY000] [14] unable to open database file```So the right relative path in this case should be `sqlite:///../var/data/blog.sqlite` BUT then it wouldn't work for `bin/console` entries. Then, relative paths shouldn't be the right way but absolute paths.The right thing is that soon (in 3.4 & 4.0) we can use environment variables with configuration parameterssymfony/symfony#23901, so the final configuration would be the one proposed. For now a workaround to make both scenarios work `index.php` and `bin/console`.Commits-------e45b5d5 Workaround to use DATABASE_URL with %kernel.project_dir% parameter
…ameter (yceruto)This PR was merged into the master branch.Discussion----------Workaround to use DATABASE_URL with %kernel.project_dir% parameterSetting `DATABASE_URL=sqlite:///var/data/blog.sqlite` as default, causes this known error through `index.php`:```An exception occurred in driver: SQLSTATE[HY000] [14] unable to open database file```So the right relative path in this case should be `sqlite:///../var/data/blog.sqlite` BUT then it wouldn't work for `bin/console` entries. Then, relative paths shouldn't be the right way but absolute paths.The right thing is that soon (in 3.4 & 4.0) we can use environment variables with configuration parameterssymfony/symfony#23901, so the final configuration would be the one proposed. For now a workaround to make both scenarios work `index.php` and `bin/console`.Commits-------e45b5d5 Workaround to use DATABASE_URL with %kernel.project_dir% parameter
Uh oh!
There was an error while loading.Please reload this page.
This PR is an updated version of#20276
(it embeds#23899 for now.)It superscedes/closes:
#22151 is another story, so not fixed here.
The way it works is via
%env(foo:BAR)%prefixes, where "foo" can be bound to any services you'd like.By default, the following prefixes are supported:
bool,int,float,string,base64const(for referencing PHP constants)json(supporting only jsonarrays for type predictability)file(eg for access to secrets stored in files.)resolve(for processing parameters inside env vars.)New prefixes can be added by implementing the new
EnvProviderInterface, and tagging withcontainer.env_provider(seeRot13EnvProviderin tests.)Prefixes can be combined to chain processing, eg.
%env(json:base64:file:FOO)%will be roughly equivalent tojson_decode(base64_decode(file_get_content(getenv('FOO')))).