-
Notifications
You must be signed in to change notification settings - Fork 193
feat: add support for $_ENV and $_SERVER in CredentialsLoader #612
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
Conversation
Many modern frameworks use dotenv or similar methods of loading environment variables into the superglobals, but do not use putenv in doing so. Which makes it harder to use this library.
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
CLA signed. |
Hello @jannes-io! Thanks for your contributions! It looks like I don't have permissions to update your branch, so could you update your branch with the changes from Thanks again! |
Hi @bshaffer , The "Allow edits by maintainers" is enabled, so you should be able to add commits. But it seems like there are no updates to main? The branch appears to be up to date. ![]() Let me know if I can help. |
@jannes-io You're right, my mistake! Okay, I created an empty commit to trigger the actions. |
@jannes-io hmm, now I'm thinking, is there any real reason we want to load from |
Honestly? I don't have a reason for it besides symfony/dotenv placing all So my reasoning is that they probably had a good and well thought out reason for doing both, so 3rd party libs that read things from the environment should probably be checking both for existence of env vars? 😆 |
@jannes-io okay that's what I thought! Thanks for your quick reply. So it makes sense to WRITE to both (since SERVER also contains ENV), but I can't think of a reason to READ from both (symfony is writing, and we're reading). I think I'll submit a PR to remove it (I get that it's a little pedantic, but this is an auth library, so it's good to keep thing simple and by the book) |
@jannes-io released in v1.47.0. Thanks for your contribution!! |
Many modern frameworks use dotenv or similar methods of loading environment variables into the superglobals, but do not use putenv in doing so. Which makes it harder to use this library, having to use factories to dynamically build client libs and setting the authentication that way instead of using this default method of authenticating.
Partially fixes #432 (only applies to the credentials loader)
Relates to: symfony/symfony#31062
Backwards compatibility is provided by always first checking
getenv
. If you want me to extract this function and replace all occurrences of getenv with it, then that's fine too. Just let me know.BEGIN_COMMIT_OVERRIDE
feat: add support for $_ENV in CredentialsLoader (#612)
END_COMMIT_OVERRIDE