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] Make load() variadic#23242
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
chalasr commentedJun 20, 2017
| Q | A |
|---|---|
| Branch? | 4.0 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | Dotenv.php#L50 |
| License | MIT |
| Doc PR | n/a |
| * @throws PathException when a file does not exist or is not readable | ||
| */ | ||
| publicfunctionload($path/*,...$paths*/) | ||
| publicfunctionload(...$paths) |
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 would keep the$path, ...$paths signature as it conveys the fact that at least one path is required.
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.
got it, fixed
f962fec to96081cdComparechalasr commentedJun 20, 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.
Should we update the signature from |
| file_put_contents($path1,'FOO=BAR'); | ||
| file_put_contents($path2,'BAR=BAZ'); | ||
| (newDotEnv())->load($path1,$path2); |
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.
The test loading multiple paths should actually be added in 3.3
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.
See#23245
| * @throws PathException when a file does not exist or is not readable | ||
| */ | ||
| publicfunctionload($path/*, ...$paths*/) | ||
| publicfunctionload($path, ...$paths) |
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.
Why can't we changeload($path, ...$paths) toload(...$paths) in Symfony 4?
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.
@javiereguiluz see#23242 (comment).
Imho, we could have kept only... $paths, maybe throwing if$paths is empty, but having both in the signature looks a bit weird
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.
well, havingload($path, ...$paths) means that the signature itself tells people that at least 1 argument is required (and IDEs know it too then).load(...$paths) allows 0 arguments
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.
imo it's not that bad if the ...$paths is empty. No file loaded, no env variable created
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.
@Taluu writing$dotenv->load() in your code which is always a no-op because you misused it is a bad DX (and this is the experience talking: it was the initial API of the component when merging it in Symfony)
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, can sound like callingload() will automagically look for some.env but nothing happens.
This PR was merged into the 3.3 branch.Discussion----------[Dotenv] Test load() with multiple paths| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23242 (comment)| License | MIT| Doc PR | n/aCommits-------dabecde [Dotenv] Test load() with multiple paths
fabpot 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.
Can be rebase now that the test has been merged in 3.3.
This PR was merged into the 4.0-dev branch.Discussion----------[DI] Uncomment code| Q | A| ------------- | ---| Branch? | master <!-- see comment below -->| Bug fix? | no| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AI was about fixing all [other occurrences of commented code and `func_get/num_arg(s)`](https://github.com/symfony/symfony/search?utf8=%E2%9C%93&q=func_num_args&type=) as BC layer usages at once, but the following PRs will already cover it:- HttpFoundation:#22863- Serializer:#23241- Process:#22836- DotEnv:#23242So it only remains DI ones.Commits-------03f33b5 [DI] Uncomment code
chalasr commentedJun 21, 2017
rebased, not added string typehints, let me know if I should. |
fabpot commentedJun 21, 2017
For typehints, we should have a discussion about how we want to introduce them. |
fabpot commentedJun 21, 2017
Thank you@chalasr. |
This PR was merged into the 4.0-dev branch.Discussion----------[Dotenv] Make load() variadic| Q | A| ------------- | ---| Branch? | 4.0| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | [Dotenv.php#L50](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Dotenv/Dotenv.php#L50)| License | MIT| Doc PR | n/aCommits-------408a33e [Dotenv] Make load() variadic
chalasr commentedJun 21, 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.
@fabpot Actually, passing a value that can't be casted as string to this method would break. Given the class is final, I think adding them would be safe. |
stof commentedJun 22, 2017
@chalasr passign a non-castable valuealready breaks (inside the function). So I'm totally +1 for adding the typehint here (we cannot add them on non-final non-private methods due to BC, but we can here) |
chalasr commentedJun 22, 2017
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b [Yaml] use scalar type hints where possible6ce70e4 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b [Yaml] use scalar type hints where possible6ce70e4 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code
This PR was merged into the 4.0-dev branch.Discussion----------Add scalar typehints/return types| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no (final, already breaks if doc not respected)| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony#23242 (comment)| License | MIT| Doc PR | n/aCommits-------7b1715b078 [Yaml] use scalar type hints where possible6ce70e4bf9 Add scalar typehints/return types on final/internal/private code