Added support and readme for environment variables #48
Conversation
| return preg_replace_callback( | ||
| '/\{env:([^:\}\{]+?)\}/', | ||
| function ($matches) { | ||
| $resolvedValue = getenv($matches[1]); |
There was a problem hiding this comment.
it may be fine to start with a getenv() call here, but I'd suggest to wrap it with a private method in which you can more easily exchange the resolving of a variable name to a value. you can make it return string|null and give it a second (optional) $default = null parameter.
you can also make the private method do the throw then (or let it pass depending on configuration and need).
at the end of the day it is normally useful you can then inject an associative array of all environment parameters (with their names as keys) and fill it with $array = getenv() or $_ENV or both etc..
just my 2 cents, in the end you normally want to provide the environment as a whole there (can be easily represented as a PHP associative array).
There was a problem hiding this comment.
I'm not sure what you're getting at here. Maybe you have a use-case in mind I'm not seeing here..
There was a problem hiding this comment.
Then leave it as-is. That getting at can escalate quickly otherwise which is normally not good for the first implementation.
docs/config-import.md
Outdated
| ``` | ||
| vendorx/general/api_key: | ||
| default: | ||
| 0: {env:VENDORX_API_KEY} |
There was a problem hiding this comment.
Regarding the format, again just FYI: In https://www.mkdocs.org/user-guide/configuration/#environment-variables it presents a way to mark this with Yaml-Tags. The Yaml parser might support this already (and they also have a default fallback value which I find sweet normally). Not saying it should be done the same (that project is in python), but you may already get some aspects / suggestions. And from top of my head, I think the symfony yaml parser is in use and they may have environment variable support already. So perhaps also worth to look into.
There was a problem hiding this comment.
Yeah Symfony makes use of %env()% instead of the proposed {env:}. Keeping in line with the original Symfony syntax makes more sense to me too.
There was a problem hiding this comment.
And I guess they have a component for that already. But take care with the deps ;) The tagged variant looks like it could work with the symfony YAML component standalone. and YAML tags have the benefit that other yaml parsers can deal with it as well. so for portability I'd personally would favor that. Then you don't need any extra syntax /E: but no interpolation?!
There was a problem hiding this comment.
I guess you mean Symfony\Component\DependencyInjection\EnvVarProcessor but I don't see a way to have that do what we're looking for.
It doesn't have a standalone version; symfony/symfony#33758
There was a problem hiding this comment.
But I agree it would be nice to have complete parity with how Symfony does it, but it is not strictly needed to have the basic functionality to support simple string env vars merged into this extension.
There was a problem hiding this comment.
Yeah nice but then hogs/problems with deps is not nice. As Symfony is popular, I'm in good hope the format is already well understood which I think is a benefit. I think this PR starts to take off.
|
To me, this looks perfectly fine. We don't need to overcomplex the whole thing. It is a new feature, so there shouldn't be any issues regarding BC breaks. Maybe @therouv has another opinion, but I'd say merge it and move along. @peterjaap thank you for this feature contribution. No one said it before :( |
| @@ -28,11 +28,11 @@ public function resolveValue($value) | |||
| { | |||
| $this->value = $value; | |||
| return preg_replace_callback( | |||
| '/\{env:([^:\}\{]+?)\}/', | |||
| '/\%env\(([^:\%\%]+?)\)\%/', | |||
There was a problem hiding this comment.
Just one consideration I'd like to share while seeing the changes (but I already thought about it yesterday with the previous syntax):
What do you think about to only support common and portable environment parameter names?
I have something in mind like A-Z (uppercase only) and the underscore _. And a minimum length of three. Something along those lines just to keep problems out of the door. It would be good I could back this up with some good reference which I don't have at hand right now.
Perhaps also as this is PHP/HTTP context, also disallow any starting with PHP_ and HTTP_.
Your mileage may vary.
Thoughts?
There was a problem hiding this comment.
Why would you force people to have a specific env var schema? Let them name their stuff whatever they want I'd say. Not the responsibility of this extension, is it?
There was a problem hiding this comment.
Nah, not forcing the users, this is about portability and keeping some rough edges out. The behaviour can be declared undefined. Please share your use-case and your suggestion which would not make you feel forced into something. This would be helpful to me so that I can better understand your considerations and which kind of profile you have in your env to pass along configuration. From my book this is not to have NUL byte injections and such stuff, not to limit the user in what she's doing. Take care and great to see you joining the discussion, its been a long time! @DavidLambauer
There was a problem hiding this comment.
I do like this idea, I'll get one of my regex experts on it!
There was a problem hiding this comment.
Disallow list;
PHP_HTTP_SERVER_SCRIPT_QUERY_DOCUMENT_
There was a problem hiding this comment.
Regex playground https://regex101.com/r/NcUNtB/2
There was a problem hiding this comment.
Since the commands of this extension are only executed in CLI environment, these env vars are very limited by itself and not directly exposed to third-party traffic.
To be honest, I don't see much of a risk and not a need to restrict more. The configuration files are created manually by agencies/merchants/developers and they usually have control here. And if they explicitly choose to use such variables (maybe there's a reason, e.g. in multi-cluster setups where some configuration values are exposed in environment variables by hosting company), then the should be able to do this.
However, I can understand the intention and I'd be happy to merge a PR if you'd like to restrict that even further.
|
Hi @peterjaap really happy about your contribution (incl. docs and tests) – THANKS! Changes have been merged and new release was tagged. |
|
You may not have noticed it, but there could be other considerations not yet realized with the regular expression changes while you decided to stop reviewing the changes. Probably you were a bit hasty not fully tracking the longer history of this feature with the recent changes, but again, take care. @DavidLambauer |
|
Thanks @DavidLambauer @therouv @ktomk thanks for your valuable input, it made this PR definitely better. We'll stay on the lookout for future considerations, but at a certain point with open source it's definitely YMMV. |
|
too hasty? That's me ;) Nothing has changed @ktomk |
Please make sure these boxes are checked before submitting your PR - thank you!
Issue
This PR fixes issue #20.
Proposed changes
I took the existing PR #21 and processed @ktomk his suggestions, and added a description to the docs.