Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: 4.4
from

Conversation

Projects
None yet
5 participants
@j92
Copy link
Contributor

commented May 19, 2019

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

This comment has been minimized.

Copy link
Member

commented May 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 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:

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 19, 2019

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

@j92

This comment has been minimized.

Copy link
Contributor Author

commented May 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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 20, 2019

Member

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 }?

This comment has been minimized.

Copy link
@j92

j92 May 20, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas May 22, 2019

Member

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

This comment has been minimized.

Copy link
@j92

j92 Jul 10, 2019

Author Contributor

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

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

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 Jun 2, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jul 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 j92 force-pushed the j92:default-value branch from b8c5020 to d2228e8 Jul 10, 2019

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

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

@nicolas-grekas
Copy link
Member

left a comment

2nd round :)

@@ -456,6 +457,14 @@ private function resolveVariables($value)
$value = (string) getenv($name);
}
if ('' === $value && isset($matches['default_value'])) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 11, 2019

Member

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.

This comment has been minimized.

Copy link
@j92

j92 Jul 13, 2019

Author Contributor

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

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 src/Symfony/Component/Dotenv/Dotenv.php Outdated
@j92

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

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

@j92 j92 force-pushed the j92:default-value branch from c6ad5a7 to 642374f Jul 13, 2019

@@ -0,0 +1 @@
FOOBAR=${BAR:=production}

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 13, 2019

Member

It looks like this file was committed accidentally.

This comment has been minimized.

Copy link
@j92

j92 Jul 13, 2019

Author Contributor

Aah yes, sorry for that.

@@ -456,6 +457,14 @@ private function resolveVariables($value)
$value = (string) getenv($name);
}
if ('' === $value && isset($matches['default_value'])) {
if (false !== strpbrk($matches['default_value'], '\'"{$')) {

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 13, 2019

Member

Can't we prevent this from happening by already excluding these characters in the regex?

This comment has been minimized.

Copy link
@j92

j92 Jul 13, 2019

Author Contributor

I thought that for users that expect their default value to contain one of these characters, it would be nicer to get a clear error message instead of having to figure out the internals of this function to see which characters are excluded.

@@ -456,6 +457,14 @@ private function resolveVariables($value)
$value = (string) getenv($name);
}
if ('' === $value && isset($matches['default_value'])) {
if (false !== strpbrk($matches['default_value'], '\'"{$')) {
throw $this->createFormatException(sprintf('Unsupported character found in the default value of variable "$%s".', $name));

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jul 13, 2019

Member

If not, I think we should embed the matched string here like this:

if (false !== $tokenMismatch = strpbrk($matches['default_value'], '\'"{$')) {
    throw $this->createFormatException(sprintf('Unsupported token "%s" found in the default value of variable "$%s".', $tokenMismatch, $name));
}

This comment has been minimized.

Copy link
@j92

j92 Jul 13, 2019

Author Contributor

Thanks, even better 👍

@j92 j92 force-pushed the j92:default-value branch from 24d9c2f to e59d8e7 Jul 13, 2019

@j92

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

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

@xabbuh

xabbuh approved these changes Jul 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.