[Dotenv] Use default value when referenced variable is not set #31546

j92 · 2019-05-19T16:51:20Z

Q	A
Branch?	4.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#11956

In bash you have the option to define a default variable like this:

FOO=${VARIABLE:-default}

When VARIABLE is not set

FOO=${VARIABLE:-default} #FOO=default

When VARIABLE is set:

VARIABLE=test
FOO=${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.

nicolas-grekas · 2019-05-19T17:36:33Z

That's nice. We can support things that the shell supports I think yes.
Can you add tests with the empty string? How does the shell behave when a car exists as the empty string also?
For the added regexp, why make it case insensitive? I think it's not needed.

j92 · 2019-05-19T18:52:18Z

@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:

How does the shell behave when a car exists as the empty string also?

nicolas-grekas · 2019-05-19T19:38:16Z

s/car/var - a var can be defined but empty, what's the behavior in this situation?

j92 · 2019-05-20T06:10:18Z

FOO=
BAR=${FOO:-test}

In this case, the value of BAR will be 'test'.

nicolas-grekas · 2019-05-20T15:33:40Z

src/Symfony/Component/Dotenv/Dotenv.php

+            (?!\()                                # no opening parenthesis
+            (?P<opening_brace>\{)?                # optional brace
+            (?P<name>'.self::VARNAME_REGEX.')?    # var name
+            (?P<default_value>(?:(:-)([^\}]+)))?  # optional default value


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 in a'a{aa})
an idea could be to not deal with these and reject more chars than only }?

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?

I think so yes. I would also exclude double quotes and $, and maybe more special chars (I don't know them all)?

@nicolas-grekas I removed the braces from the regex. The regex strips only the last } from the default value and checks later with strpbrk('"{}$) 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 · 2019-07-08T13:10:02Z

@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 · 2019-07-10T18:08:19Z

@fabpot Thanks for the reminder. I updated the PR. When the code is correct, I will also update the documentation.

nicolas-grekas

2nd round :)

nicolas-grekas · 2019-07-11T06:29:21Z

src/Symfony/Component/Dotenv/Dotenv.php

@@ -456,6 +457,14 @@ private function resolveVariables($value)
                $value = (string) getenv($name);
            }

+            if ('' === $value && isset($matches['default_value'])) {


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.

@nicolas-grekas If you are talking about the following case:
BAR=${FOO:-TEST, it is already handled with a FormatException : Unclosed braces on variable expansion.

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?

You are right, I added a check for a missing opening brace and the following case:

BAR=$FOO:-a

now throws a FormatException with Variable expansion "$FOO:-a" should be wrapped in braces

src/Symfony/Component/Dotenv/Dotenv.php

j92 · 2019-07-11T06:51:22Z

@nicolas-grekas Thanks for helping out, I was stuck.

src/Symfony/Component/Dotenv/.env

src/Symfony/Component/Dotenv/Dotenv.php

j92 · 2019-07-13T11:45:12Z

@xabbuh Thanks for the feedback, it's applied. And I am curious to hear your opinion about the excluded characters.

src/Symfony/Component/Dotenv/Dotenv.php

nicolas-grekas · 2019-07-30T18:59:09Z

src/Symfony/Component/Dotenv/Tests/DotenvTest.php

+            ["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"],


actually, this should be a legal value: it's just the $FOO var followed by the :-a string

indeed:

$ FOO=foo $ BAR=$FOO:-a $ echo $BAR foo:-a

@nicolas-grekas

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!

my comment was certainly not clear, as I spotted this case after making it :)

src/Symfony/Component/Dotenv/Tests/DotenvTest.php

xabbuh · 2019-08-06T07:27:44Z

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  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#11956  In bash you have the option to define a default variable like this: ```bash FOO=${VARIABLE:-default} ``` When VARIABLE is not set ```bash FOO=${VARIABLE:-default} #FOO=default ``` When VARIABLE is set: ```bash VARIABLE=test FOO=${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  Added a short description and a code sample of how to use default values when referenced variables are not set. The feature PR is symfony/symfony#31546. Commits ------- 1848e00 Added small section about default values

carsonbot added Status: Needs Review Feature labels May 19, 2019

nicolas-grekas reviewed May 20, 2019

View reviewed changes

nicolas-grekas added this to the next milestone May 20, 2019

nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:11

xabbuh added the Dotenv label Jul 9, 2019

xabbuh mentioned this pull request Jul 9, 2019

[Dotenv] Fatal error for valid environment variable syntax. #31530

Closed

j92 force-pushed the default-value branch from b8c5020 to d2228e8 Compare July 10, 2019 18:00

nicolas-grekas reviewed Jul 11, 2019

View reviewed changes

j92 force-pushed the default-value branch from c6ad5a7 to 642374f Compare July 13, 2019 08:33

xabbuh requested changes Jul 13, 2019

View reviewed changes

src/Symfony/Component/Dotenv/.env Outdated Show resolved Hide resolved

src/Symfony/Component/Dotenv/Dotenv.php Outdated Show resolved Hide resolved

src/Symfony/Component/Dotenv/Dotenv.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 13, 2019

j92 force-pushed the default-value branch from 24d9c2f to e59d8e7 Compare July 13, 2019 11:42

carsonbot removed the Status: Needs Work label Jul 13, 2019

xabbuh approved these changes Jul 13, 2019

View reviewed changes

carsonbot added the Status: Reviewed label Jul 13, 2019

j92 mentioned this pull request Jul 13, 2019

Added small section about default values symfony/symfony-docs#11956

Merged

j92 force-pushed the default-value branch from 44e681b to 304f52a Compare July 24, 2019 18:33

nicolas-grekas reviewed Jul 30, 2019

View reviewed changes

nicolas-grekas reviewed Jul 31, 2019

View reviewed changes

src/Symfony/Component/Dotenv/Tests/DotenvTest.php Show resolved Hide resolved

nicolas-grekas approved these changes Aug 6, 2019

View reviewed changes

[Dotenv] Use default value when referenced variable is not set

790dbad

xabbuh force-pushed the default-value branch from a55f30c to 790dbad Compare August 6, 2019 07:27

xabbuh merged commit 790dbad into symfony:4.4 Aug 6, 2019

j92 deleted the default-value branch August 6, 2019 07:35

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dotenv] Use default value when referenced variable is not set #31546

[Dotenv] Use default value when referenced variable is not set #31546

j92 commented May 19, 2019 •

edited

Loading

nicolas-grekas commented May 19, 2019

j92 commented May 19, 2019

nicolas-grekas commented May 19, 2019

j92 commented May 20, 2019

nicolas-grekas May 20, 2019

j92 May 20, 2019

nicolas-grekas May 22, 2019

j92 Jul 10, 2019 •

edited

Loading

fabpot commented Jul 8, 2019

j92 commented Jul 10, 2019

nicolas-grekas left a comment

nicolas-grekas Jul 11, 2019

j92 Jul 13, 2019

nicolas-grekas Jul 24, 2019

j92 Jul 24, 2019

j92 commented Jul 11, 2019

j92 commented Jul 13, 2019

nicolas-grekas Jul 30, 2019

xabbuh Jul 31, 2019

j92 Jul 31, 2019

nicolas-grekas Jul 31, 2019

xabbuh commented Aug 6, 2019

[Dotenv] Use default value when referenced variable is not set #31546

[Dotenv] Use default value when referenced variable is not set #31546

Conversation

j92 commented May 19, 2019 • edited Loading

nicolas-grekas commented May 19, 2019

j92 commented May 19, 2019

nicolas-grekas commented May 19, 2019

j92 commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j92 Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

fabpot commented Jul 8, 2019

j92 commented Jul 10, 2019

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j92 commented Jul 11, 2019

j92 commented Jul 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Aug 6, 2019

j92 commented May 19, 2019 •

edited

Loading

j92 Jul 10, 2019 •

edited

Loading