Alternative credential retrieval - threadsafe#2722
Alternative credential retrieval - threadsafe#2722paullallier wants to merge 10 commits intoaws:masterfrom
Conversation
|
Found a minor problem. This doesn't seem to fall through to null when
|
|
Could we get this merged eventually? Thanks! |
|
Any chance of a review and comments on how we could make this an acceptable merge, please? |
| // Use credentials from environment variables, if available | ||
| $key = getenv(self::ENV_KEY); | ||
| $secret = getenv(self::ENV_SECRET); | ||
| $key = getenv(self::ENV_KEY) ?: $_SERVER[self::ENV_KEY]; |
There was a problem hiding this comment.
The $_SERVER references should check to see if it is set. Also, we should fall back to false if it isn't. I think you could add a null coalescing operator to what's already here and add false on the right side of the operand.
There was a problem hiding this comment.
Thank you Sean. And I think that's what I SHOULD have done with the ENV_SESSION variable too. 12 month ago, I was less familiar with the difference between null coalesce and Elvis!
Do we need more tests?
Add a fall-through in case the $_SERVER variable isn't set (and re-introduce the equivalent change for ENV_SESSION)
|
I'm not sure why I thought @server would set the $_SERVER variable for the test. Fixed this, but is there a cleaner option? Sorry - I should have run the tests locally (which I have now done with PHP 8.3 at least). |
|
Sorry. I'd love to get this into a mergeable state, but getting it to pass the tests on all the test suites appears to be beyond my skill level. :-( |
|
Hi @paullallier, Apologies for the delay. We'll get these fixed up and get the PR merged as soon as possible. |
2 participants
Similar to PR 2155
Description of changes:
Fallback to $_SERVER variable for AWS credentials to avoid using non-threadsafe putenv()
Would you consider allowing this change so that we can avoid having to use the non-threadsafe putenv() function to set AWS credentials - which we're storing in a .env file?
I've tried to write a test for it - not sure it's going to work yet.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.