-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: add 8.5 support #3179
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
base: master
Are you sure you want to change the base?
chore: add 8.5 support #3179
Conversation
fc5c30c to
116c59b
Compare
116c59b to
85cce0f
Compare
449cc4e to
4caa0f3
Compare
yenfryherrerafeliz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think falling back to empty string in every array access could be problematic, but we can leave this open for discussion.
| public function get($key) | ||
| { | ||
| $key = $key ?? ''; | ||
| if (!isset($this->items[$key])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the idea of evaluating if the key is null and not fallback to make the key an empty string. Why?, because an empty string is also a valid array key and this could cause unexpected behaviors. Also, when setting the key you also fallback to make it empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
if ($key == null || !isset($this->items[$key])) {
return null;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like falling back to an empty string either, but it's official guidance from the deprecation guide:
Using null as an array offset ¶
Using null as an array offset or when calling array_key_exists() is now deprecated. Instead an empty string should be used.
I'll try to come up with a better way around this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dug into this a bit further- avoided in areas where checks could be performed in another way, but in this class and MultiRegionClient, we break backward-compatibility if we do anything else. From the PHP Manual:
Null will be cast to the empty string, i.e. the key null will actually be stored under "".
In those classes we were setting/getting '' keys and that's behavior we should retain.
src/RetryMiddleware.php
Outdated
| return !empty($context) | ||
| && !empty($context['errno']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the extra condition could be redundant?, unless $context may be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in the latest update
| file_put_contents($awsDir . '/mycreds', $ini); | ||
| putenv("AWS_SHARED_CREDENTIALS_FILE={$awsDir}/mycreds"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is redundant. The same thing happens after line 561, unless I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slightly different format: for Linux, it's wrapped in an outer string
| public function testDownloadsObjectsWithAccessPointArn() | ||
| { | ||
| if (PHP_OS_FAMILY === 'Windows') { | ||
| $this->markTestSkipped('S3 access point ARN downloads have path handling issues on Windows'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?, customers can't do downloads using access points ARNs on Windows then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly. Will need to add to the backlog to investigate. I tried for a while to get this working
4caa0f3 to
b505e38
Compare
Description of changes:
Fixes PHP 8.5 deprecation warnings and adds 8.5 to test runs, adds Windows unit test workflow, adds JIT to applicable workflows, adds separate Codecov coverage workflow that runs weekly.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.