Skip to content

[Dotenv] Deprecate useage of "putenv" #31062

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

Merged
merged 1 commit into from
Apr 10, 2019
Merged

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Apr 10, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets
License MIT
Doc PR n/a

From discussions on symfony/recipes#571, I think it is a good idea to make people opt-in to using putenv.

In Symfony 5.0 we will just change the value of the constructor. As an alternative, we could decide we want to remove putenv in Symfony 5.0. If so, I would also deprecate $usePutenv=true.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(once comments are fixed)

@Nyholm
Copy link
Member Author

Nyholm commented Apr 10, 2019

Thank you for the reviews

@fabpot fabpot force-pushed the putenv-deprecate branch from 047c3f5 to 8e45fc0 Compare April 10, 2019 16:25
@fabpot
Copy link
Member

fabpot commented Apr 10, 2019

Thank you @Nyholm.

@fabpot fabpot merged commit 8e45fc0 into symfony:master Apr 10, 2019
fabpot added a commit that referenced this pull request Apr 10, 2019
This PR was squashed before being merged into the 4.3-dev branch (closes #31062).

Discussion
----------

[Dotenv] Deprecate useage of "putenv"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | n/a

From discussions on symfony/recipes#571, I think it is a good idea to make people opt-in to using `putenv`.

In Symfony 5.0 we will just change the value of the constructor. As an alternative, we could decide we want to remove `putenv` in Symfony 5.0. If so, I would also deprecate `$usePutenv=true`.

Commits
-------

8e45fc0 [Dotenv] Deprecate useage of \"putenv\"
@Nyholm Nyholm deleted the putenv-deprecate branch April 10, 2019 20:05
@Nyholm
Copy link
Member Author

Nyholm commented Apr 10, 2019

Thank you for merging

nicolas-grekas added a commit that referenced this pull request Apr 11, 2019
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Dotenv] Improve Dotenv messages

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | no
| License       | MIT
| Doc PR        | no

This PR improves a little bit of some messages from #31062

The first, passive sentences may be more suitable here because the value couldn't change by itself. It is changed by us - human.

The second, if we use **The default value of $usePutenv" argument of "%s\'s constructor**, we have to pass `__CLASS__` as the second parameter of `sprintf` function instead of `__METHOD__`. So, I suggest using **The default value of $usePutenv" argument of "%s"**.

Finally, the deprecation warning of `Dotenv::__construct()` is very long. Let's separate it into 2 pieces for readable reason.

Commits
-------

e871a6a Improve Dotenv messages
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
fabpot added a commit that referenced this pull request May 7, 2019
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Dotenv] Test do not use putenv

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The related pull request is #31062.

If the `$usePutenv` flag is set to `false`, `putenv` won't be executed. I just add a small test for this situation.

Commits
-------

6d1a76e Test do not use putenv
@fabpot fabpot mentioned this pull request May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants