Alternative credential retrieval - threadsafe - #2722
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. |
|
Hey @paullallier, Sorry for the very delayed response. I was reviewing this today and I don't think this is something we can merge. There are too many tests that depend on credential 'emptiness' and there's not a great way for us to set these values and restore them when running unit tests. Something we'll keep in mind if we end up overhauling our credential providers though. Thanks again for the PR! |
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.