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

Ban env functions #207

Closed
wants to merge 1 commit into from
Closed

Ban env functions #207

wants to merge 1 commit into from

Conversation

gmponos
Copy link
Contributor

@gmponos gmponos commented Jun 20, 2020

Break down #200 and started by the env functions.. also sort the functions to the ones that are alias and the ones that are bad practice to use.

@gmponos gmponos requested a review from a team as a code owner June 20, 2020 19:59
greg0ire
greg0ire previously approved these changes Jun 20, 2020
@simPod
Copy link
Contributor

simPod commented Jun 20, 2020

Why is it a bad practice?

@gmponos
Copy link
Contributor Author

gmponos commented Jun 20, 2020

Inside the xml I have a linked issue..

In short words these function are not thread safe and one thread can affect the other

Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

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

getenv() is the only way to access parent envs from within children process. It's not always possible to avoid using it.

@gmponos
Copy link
Contributor Author

gmponos commented Jun 20, 2020

Hm.. interesting scenario..

Shouldn't environmental variables be set on the machine? Once they are set on the machine aren't they set for both processes?

So.. why parent/child processes should use env parameters to communicate with each other?

@ostrolucky
Copy link
Member

It's not always set "on machine". FOO=bar php baz.php

@gmponos
Copy link
Contributor Author

gmponos commented Jun 21, 2020

Hi.. I also have a code where I spawn children processes from a parent and I do this too

FOO=bar php baz.php

But I am passing FOO from the parent to the children during the spawn. The only reason I see that a child will want to read the FOO with getenv is because FOO might have changed.. which for me is a bad practice.

Still I think that this will cover 99% of the case.

Is your concern only for getenv and not about putenv? If so I will leave it here for a while.. maybe someone else has more feedback to provide.. and depending will remove it later.

@simPod
Copy link
Contributor

simPod commented Jun 27, 2020

Do we have an example where usage of getenv or putenv is actually required then? I'm confused by the discussion a little bit.

@Ocramius
Copy link
Member

See doctrine/automatic-releases: only putenv() used there, but I agree that setenv() should be banned.

@gmponos
Copy link
Contributor Author

gmponos commented Jun 27, 2020

I agree that setenv() should be banned.

@Ocramius you mean getenv ?

@Ocramius
Copy link
Member

I edited the previous message 😅

@greg0ire greg0ire changed the base branch from master to 8.2.x October 25, 2020 10:05
@greg0ire greg0ire dismissed their stale review October 25, 2020 10:05

The base branch was changed.

@greg0ire
Copy link
Member

greg0ire commented Apr 3, 2021

ping @gmponos do you plan on iterating on this? I'm trying to see what could ship in 9.0.0

@ostrolucky ostrolucky closed this May 10, 2021
@gmponos gmponos deleted the env-functions branch May 26, 2021 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants