Skip to content

[Process] Deprecate Process::inheritEnvironmentVariables() #32475

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
Jul 18, 2019
Merged

[Process] Deprecate Process::inheritEnvironmentVariables() #32475

merged 1 commit into from
Jul 18, 2019

Conversation

ogizanagi
Copy link
Contributor

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

IIUC, this method was kept as a BC layer from 3.4 to 4.0 to switch to the "inherit env vars" behavior, inciting developers to opt-in in 3.4. Since 4.0, env vars are always inherited, and this method doesn't allow to opt-out. So, time to remove it?


refs:

@nicolas-grekas
Copy link
Member

Thank you @ogizanagi.

@nicolas-grekas nicolas-grekas merged commit af9bad3 into symfony:4.4 Jul 18, 2019
nicolas-grekas added a commit that referenced this pull request Jul 18, 2019
…es() (ogizanagi)

This PR was merged into the 4.4 branch.

Discussion
----------

[Process] Deprecate Process::inheritEnvironmentVariables()

| Q             | A
| ------------- | ---
| Branch?       | 4.4 <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

IIUC, this method was kept as a BC layer from 3.4 to 4.0 to switch to the "inherit env vars" behavior, inciting developers to opt-in in 3.4. Since 4.0, env vars are always inherited, and this method doesn't allow to opt-out. So, time to remove it?

---

refs:

- #21470
- #22836

Commits
-------

af9bad3 [Process] Deprecate Process::inheritEnvironmentVariables()
@ogizanagi ogizanagi deleted the deprec/process/inheritEnv branch July 18, 2019 17:05
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@vincenttouzet
Copy link
Contributor

I was about to propose a new MR but I just saw this one. How can I do if do DONT want to inherit the env variable ? 🤔

I've tried to override the Process class but all is private inside it so I basically cannot override anything 😢

@ogizanagi
Copy link
Contributor Author

@vincenttouzet : Simply quoting @nicolas-grekas in #21470, here are the initial reasons for these changes:

About env vars: if one has unwanted env vars, one will still be able to remove them explicitly for the command. From my experience, not having eg PATH or HTTP_PROXY, etc. is more problematic. I'd prefer people to care about setting/unsetting the environment vars they know about, rather than allowing them to start with no ENV and discover later that they missed setting some var.

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.

5 participants