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

Remove getenv() which is not thread safe #253

Closed
wants to merge 1 commit into from

Conversation

myteril
Copy link

@myteril myteril commented Oct 1, 2024

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe below

Description

putenv() and getenv() functions are not thread-safe. You can read the warning directly from vlucas/phpdotenv here: vlucas/phpdotenv#putenv-and-getenv

These functions can easily break the core functionality of web sites even under a little stress. In the pull request, I have removed getenv() from _env(). I ran the tests and all have been passed. You can check it from the screenshot below:

image

However, some Leaf modules (such as db) still use getenv() to get the environment variables. This pull request will affect them and break their functionality. Before merging, getenv should be replaced with _env in these modules.

Does this PR introduce a breaking change?

  • Yes
  • No

@mychidarko
Copy link
Member

@myteril can you check on the tests, thanks

@myteril
Copy link
Author

myteril commented Oct 2, 2024

I checked the tests. I think the failed test case is useless after this change and needs to be removed. By this pull request, _env will use the information from .env files only, not all environment variables including system's (except the ones defined in $_ENV). It does not make any sense to compare its output with the output of getenv which gets information directly from system's environment variables.

@mychidarko
Copy link
Member

@myteril noted. I'll keep this open in the mean time. I've seen countless folks who heavily on getenv/setenv in their code, suddenly switching away from this may probably have the same impact it did on the tests. I'll like to get more info on how severely it will impact others before I merge this

@mychidarko
Copy link
Member

I am closing this for now, but will revisit it in a later version

@mychidarko mychidarko closed this Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants