8000 feat: add support for $_ENV and $_SERVER in CredentialsLoader by jannes-io · Pull Request #612 · googleapis/google-auth-library-php · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 3 commits into from
Apr 15, 2025

Conversation

jannes-io
Copy link
Contributor
@jannes-io jannes-io commented Mar 5, 2025

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

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.
@jannes-io jannes-io requested a review from a team as a code owner March 5, 2025 09:11
Copy link
google-cla bot commented Mar 5, 2025

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.

@jannes-io
Copy link
Contributor Author

CLA signed.

@bshaffer
Copy link
Contributor

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 main in this repo? That should also trigger the tests, which need to run in order for me to merge this.

Thanks again!

@jannes-io
Copy link
Contributor Author
jannes-io commented Apr 11, 2025

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.

image

Let me know if I can help.

@bshaffer
Copy link
Contributor

@jannes-io You're right, my mistake! Okay, I created an empty commit to trigger the actions.

@bshaffer bshaffer merged commit 3e63576 into googleapis:main Apr 15, 2025
12 checks passed
@bshaffer
Copy link
Contributor

@jannes-io hmm, now I'm thinking, is there any real reason we want to load from _SERVER? It seems to me that _ENV would be sufficient and preferable.

@jannes-io
Copy link
Contributor Author

@jannes-io hmm, now I'm thinking, is there any real reason we want to load from _SERVER? It seems to me that _ENV would be sufficient and preferable.

Honestly? I don't have a reason for it besides symfony/dotenv placing all .env variables in both _SERVER and _ENV.

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? 😆

@bshaffer
Copy link
Contributor

@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)

@bshaffer
Copy link
Contributor

@jannes-io released in v1.47.0. Thanks for your contribution!!

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.

feat: add support for using $_ENV instead of getenv/putenv
2 participants
0