Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Dotenv] Use default value when referenced variable is not set#31546
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
nicolas-grekas commentedMay 19, 2019
That's nice. We can support things that the shell supports I think yes. |
j92 commentedMay 19, 2019
@nicolas-grekas Thanks for the feedback! I added the test for the empty string and indeed the added regexp does not have to be case insensitive. I'm not sure what you mean by:
|
nicolas-grekas commentedMay 19, 2019
|
j92 commentedMay 20, 2019
FOO=BAR=${FOO:-test}In this case, the value of BAR will be 'test'. |
| (?!\() # no opening parenthesis | ||
| (?P<opening_brace>\{)? # optional brace | ||
| (?P<name>'.self::VARNAME_REGEX.')? # var name | ||
| (?P<default_value>(?:(:-)([^\}]+)))? # optional default value |
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.
no need for the extra brackets I believe:(?P<default_value>:-[^\}]+)?
but more generally, the parsing logic doesn't handle quoted/interpolated values
eg.${ABC:-a\'aa} or worse${ABC:-a\'a{a}a} (which results ina'a{aa})
an idea could be to not deal with these and reject more chars than only}?
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.
Sounds good to not deal with these cases. It's a fallback mechanism which should contain simple values. Let's keep it KISS for now.
I will update the regex. And should we reject}{\' characters with a FormatException?
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.
I think so yes. I would also exclude double quotes and$, and maybe more special chars (I don't know them all)?
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.
@nicolas-grekas I removed the braces from the regex. The regex strips only the last} from the default value and checks later withstrpbrk('"{}$) for the invalid characters. This way we can throw a FormatException instead of simply ignoring the default value.
I hope my solution is okay, as my knowledge of regexes is not great. Please let me know what you think.
fabpot commentedJul 8, 2019
@j92 It looks like this pull request is almost finished. You needed to update the regex. Do you have time to finish this work? |
j92 commentedJul 10, 2019
@fabpot Thanks for the reminder. I updated the PR. When the code is correct, I will also update the documentation. |
nicolas-grekas left a comment
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.
2nd round :)
| $value = (string)getenv($name); | ||
| } | ||
| if ('' ===$value &&isset($matches['default_value'])) { |
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.
we should also check that there is both an opening and closing bracket
and when on is missing, we should append the content of the default_value index as a regular string
please add a test case covering this situation.
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.
@nicolas-grekas If you are talking about the following case:BAR=${FOO:-TEST, it is already handled with aFormatException : Unclosed braces on variable expansion.
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, thanks for the precision
one more question: can we enter this "if" when there's no opening bracket? I feel like yes, while this should fail, isn't it?
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.
You are right, I added a check for a missing opening brace and the following case:
BAR=$FOO:-anow throws a FormatException withVariable expansion "$FOO:-a" should be wrapped in braces
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
j92 commentedJul 11, 2019
@nicolas-grekas Thanks for helping out, I was stuck. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
j92 commentedJul 13, 2019
@xabbuh Thanks for the feedback, it's applied. And I am curious to hear your opinion about the excluded characters. |
Uh oh!
There was an error while loading.Please reload this page.
| ["FOO=\nBAR=\${FOO:-\'a{a}a}","Unsupported character\"'\" found in the default value of variable\"\$FOO\". in\".env\" at line 2.\n...\\nBAR=\${FOO:-\'a{a}a}...\n ^ line 2 offset 24"], | ||
| ["FOO=\nBAR=\${FOO:-a\$a}","Unsupported character\"\$\" found in the default value of variable\"\$FOO\". in\".env\" at line 2.\n...FOO=\\nBAR=\${FOO:-a\$a}...\n ^ line 2 offset 20"], | ||
| ["FOO=\nBAR=\${FOO:-a\"a}","Unclosed braces on variable expansion in\".env\" at line 2.\n...FOO=\\nBAR=\${FOO:-a\"a}...\n ^ line 2 offset 17"], | ||
| ["FOO=\nBAR=\$FOO:-a","Variable expansion\"\$FOO:-a\" should be wrapped in braces in\".env\" at line 2.\n...FOO=\\nBAR=\$FOO:-a...\n ^ line 2 offset 16"], |
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.
actually, this should be a legal value: it's just the$FOO var followed by the:-a string
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.
indeed:
$ FOO=foo$ BAR=$FOO:-a$echo$BARfoo:-a
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.
one more question: can we enter this "if" when there's no opening bracket? I feel like yes, while this should fail, isn't it?
My interpretation of your comment was that when there is no opening bracket, in the case of$FOO:-a for example, it should fail. Clearly I misunderstood your comment, sorry for that. Could you give me an example of the case that should fail? Thanks!
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.
my comment was certainly not clear, as I spotted this case after making it :)
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedAug 6, 2019
Thank you@j92. |
… not set (j92)This PR was squashed before being merged into the 4.4 branch (closes#31546).Discussion----------[Dotenv] Use default value when referenced variable is not set| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | yes <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR |symfony/symfony-docs#11956 <!-- required for new features -->In bash you have the option to define a default variable like this:```bashFOO=${VARIABLE:-default}```When VARIABLE is not set```bashFOO=${VARIABLE:-default} #FOO=default```When VARIABLE is set:```bashVARIABLE=testFOO=${VARIABLE:-default} #FOO=test```If others find this also a good idea, I will write documentation and add the Doc PR. But first I would like some feedback to check if anyone agrees with this feature.Commits-------790dbad [Dotenv] Use default value when referenced variable is not set
This PR was merged into the 4.4 branch.Discussion----------Added small section about default values<!--If your pull request fixes a BUG, use the oldest maintained branch that containsthe bug (seehttps://symfony.com/roadmap for the list of maintained branches).If your pull request documents a NEW FEATURE, use the same Symfony branch wherethe feature was introduced (and `master` for features of unreleased versions).-->Added a short description and a code sample of how to use default values when referenced variables are not set. The feature PR issymfony/symfony#31546.Commits-------1848e00 Added small section about default values
Uh oh!
There was an error while loading.Please reload this page.
In bash you have the option to define a default variable like this:
FOO=${VARIABLE:-default}When VARIABLE is not set
When VARIABLE is set:
If others find this also a good idea, I will write documentation and add the Doc PR. But first I would like some feedback to check if anyone agrees with this feature.