-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
PHP 8.0, 8.1, PHPUnit 9 support + automated check for php compatibility #2403
Conversation
@SamRemis , I'm pinging you since you mentioned you'd take a look at any PR's regarding usage of php 8.x in a comment on issue 2367 . Could you please take this PR into consideration to add support for php 8.x? |
Hi @Zombaya and @27pchrisl, Thanks a lot for all of the work on this. @SamRemis and I will review when we have a chance, which will take some time. We'll get back to you ASAP, likely by the end of next week. |
Very much looking forward to it :-). If I can be of any help, please let me know and I'll try to take some time to help. |
$this->assertContains($expected, $actual, $message); | ||
} | ||
|
||
public function assertStringNotContainsString($expected, $actual, $message = '') |
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.
This is a longer method name, I think assertStringContains() may work here?
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.
Agreed it is long! But it's new in phpunit (https://phpunit.readthedocs.io/en/9.5/assertions.html#assertstringcontainsstring). And PhpStorm includes a linter rule to hint it should be used. However, it is certainly dealer's choice so we could revert this.
$this->assertNotContains($expected, $actual, $message); | ||
} | ||
|
||
public function assertStringContainsStringIgnoringCase($expected, $actual, $message = '') |
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.
Same as above assertStringContainsIgnoreCase()
Hey @Zombaya, I know it's been a while— sorry for the delay. Could you rebase and remove the compatibility check? We've decided it's something we'll possibly revisit in the future. Thanks again for all of the effort on this. |
e029b8f
to
ed1e252
Compare
Hey @stobrien89, I've rebased everything again, added some small fixes because of changed tests and removed the commit about php-compatibility-check. I ran the compatibility-check manually once more before removing it and it succeeded without problems. If you ever want to re-integrate the tool, I made a backup of the branch or you could always check the commit itself. |
$request->expects($this->at(0)) | ||
$request->expects($this->exactly(2)) | ||
->method('getHeader') | ||
->with('X-Amz-User-Agent') | ||
->willReturn(["MockBuilder"]); | ||
|
||
$request->expects($this->at(1)) | ||
->method('withHeader') | ||
->with( | ||
'X-Amz-User-Agent', | ||
new \PHPUnit\Framework\Constraint\RegularExpression( | ||
'/aws-sdk-php\/' . Sdk::VERSION . '.* MockBuilder/' | ||
) | ||
)->willReturn($request); | ||
|
||
$request->expects($this->at(2)) | ||
->method('getHeader') | ||
->with('User-Agent') | ||
->willReturn(['MockBuilder']); | ||
->withConsecutive( | ||
['X-Amz-User-Agent'], | ||
['User-Agent'] | ||
) | ||
->willReturnOnConsecutiveCalls( | ||
["MockBuilder"], | ||
['MockBuilder'] | ||
); | ||
|
||
$request->expects($this->at(3)) | ||
$request->expects($this->exactly(2)) | ||
->method('withHeader') | ||
->with( | ||
'User-Agent', | ||
new \PHPUnit\Framework\Constraint\RegularExpression( | ||
'/aws-sdk-php\/' . Sdk::VERSION . '.* MockBuilder/' | ||
) | ||
)->willReturn($request); | ||
->withConsecutive( | ||
[ | ||
'X-Amz-User-Agent', | ||
new \PHPUnit\Framework\Constraint\RegularExpression( | ||
'/aws-sdk-php\/' . Sdk::VERSION . '.* MockBuilder/' | ||
) | ||
], | ||
[ | ||
'User-Agent', | ||
new \PHPUnit\Framework\Constraint\RegularExpression( | ||
'/aws-sdk-php\/' . Sdk::VERSION . '.* MockBuilder/' | ||
) | ||
] | ||
) | ||
->willReturnOnConsecutiveCalls( | ||
$request, | ||
$request | ||
); |
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.
$this->at()
is deprecated in phpunit 9 and will be removed in phpunit 10.
I've replaced the code using withConsecutive()
and willReturnOnConsecutiveCalls()
(Documentation).
This loses the assertion that the methods will be called in the order as specified previously, that's why I'm highlighting this change, as it might be of importance.
tests/Crypto/Polyfill/AesGcmTest.php
Outdated
} | ||
$ptLen = \random_int(0, 1024); | ||
$aadLen = \random_int(0, 1024); | ||
$i = 0; | ||
$ptLen = \random_int(1, 1024); | ||
$aadLen = \random_int(1, 1024); |
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.
The reason the pipeline for this MR previously failed, was because $aadLen
was 0, resulting in a failure when doing $aad = \random_bytes($aadLen + $i);
.
This is now avoided by having the minimum-value of both lengths to be 1.
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.
Thanks for putting in the effort to identify and correct this. I think we'll want to exclude this change for now (as annoying as this flaky test might be) since we're waiting to hear back from another team on the best path forward.
3cf1c24
to
1e476ae
Compare
@stobrien89 , I'm finished for today. I believe all warnings from php 8.1 about deprecated ways of using some functions or implicit type-conversions should be fixed, as well as deprecation-warnings from phpunit 9.x and I solved the very low possibility of a failed workflow because of generating an invalid stringlength. I believe this MR is now ready again. If you need any more changes or information, I'll be glad to help. |
@@ -114,7 +114,7 @@ public function __invoke() | |||
} | |||
if (empty($token)) { | |||
if ($this->tokenFileReadAttempts < $this->retries) { | |||
sleep(pow(1.2, $this->tokenFileReadAttempts)); | |||
sleep((int) pow(1.2, $this->tokenFileReadAttempts)); |
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've kept the result of the code the same, but it does lose some precision which might be wanted.
An alternative fix would be using usleep()
, to keep a higher resolution.
usleep((int) (1000*1000*pow(1.2, $this->tokenFileReadAttempts)));
(Same goes for the same change in other places where this was changed.)
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 do not know if you have seen but in the file aws-sdk-php/src/Credentials/InstanceProfileProvider.php there is the same fix to do in line 262 :
private function handleRetryableException(
\Exception $e,
$retryOptions,
$message
) {
$isRetryable = true;
if (!empty($status = $this->getExceptionStatusCode($e))
&& isset($retryOptions['blacklist'])
&& in_array($status, $retryOptions['blacklist'])
) {
$isRetryable = false;
}
if ($isRetryable && $this->attempts < $this->retries) {
- sleep(pow(1.2, $this->attempts));
+ sleep((int) pow(1.2, $this->attempts));
} else {
throw new CredentialsException($message);
}
}
this patch worked for me
tests/Crypto/Polyfill/AesGcmTest.php
Outdated
} | ||
$ptLen = \random_int(0, 1024); | ||
$aadLen = \random_int(0, 1024); | ||
$i = 0; | ||
$ptLen = \random_int(1, 1024); | ||
$aadLen = \random_int(1, 1024); |
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.
Thanks for putting in the effort to identify and correct this. I think we'll want to exclude this change for now (as annoying as this flaky test might be) since we're waiting to hear back from another team on the best path forward.
1e476ae
to
2ffe7cc
Compare
@stobrien89 , I fixed all your latest remarks and rebased everything on master once more. However, I'm running into a new issue with the CI regarding the test It's throwing some sort of error related to openssl, but I'm not being able to reproduce the error locally. Nor the underlying code nor the testcode have changed fundamentally with this PR, so I'm a bit out my depth here on how to fix this. |
PS aws-sdk-php seems to be affected by PHPCompatibility/PHPCompatibility#1339 |
2ffe7cc
to
b0ab727
Compare
@stobrien89, I found the source of the failing unittest Could you review this once more? |
Hi @Zombaya, So sorry for the delayed response— I missed your last message. I'll take another look. Thanks again for your patience! |
src/Api/Serializer/JsonBody.php
Outdated
@@ -29,7 +29,7 @@ public function __construct(Service $api) | |||
public static function getContentType(Service $service) | |||
{ | |||
return 'application/x-amz-json-' | |||
. number_format($service->getMetadata('jsonVersion'), 1); | |||
. number_format(($version = $service->getMetadata('jsonVersion')) !== null ? $version : 0.0, 1); |
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.
Could you make this multiple lines?
tests/bootstrap.php
Outdated
|
||
if (!class_exists('\PHPUnit\Framework\Constraint\RegularExpression')) { | ||
if ( ! defined('PHPUNIT_COMPOSER_INSTALL')) { |
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.
Can you change the not operator spacing in this file as well? i.e. (!class_exists...
tests/bootstrap.php
Outdated
@@ -22,10 +26,13 @@ class_alias('\PHPUnit_Framework_Error_Warning', '\PHPUnit\Framework\Error\Warnin | |||
|
|||
// Patch PHPUnit class for PHP 7.4+ and PHPUnit 5.x to avoid deprecation warning | |||
// Necessary because older versions of PHPUnit are no longer supported | |||
$version = PHPUnit_Runner_Version::id(); | |||
$version = class_exists('PHPUnit_Runner_Version') ? PHPUnit_Runner_Version::id() : PHPUnit\Runner\Version::id(); |
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.
let's use standard spacing here as well.
tests/CloudFront/UrlSignerTest.php
Outdated
$this->assertStringStartsWith( | ||
"http://abc.cloudfront.net/images/image.jpg?color=red&Expires={$ts}&Signature=", | ||
$url | ||
); | ||
$urlObject = new Uri($url); | ||
$query = \GuzzleHttp\Psr7\Query::parse($urlObject->getQuery()); | ||
$query = \GuzzleHttp\Psr7\Query::parse($urlObject->getQuery()); |
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 see a number of spaces like this one added that aren't really following our coding style; is this intentional? Was it just run through a linter?
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 assume @27pchrisl either configured his IDE to do this automatically or has the same preference as me, which is aligned =
-signs 🙂 .
I've removed it for now and reverted to what it was.
@@ -90,7 +90,7 @@ public function testCorrectlyOutputsHost( | |||
}); | |||
|
|||
$handler = $list->resolve(); | |||
$handler($command, new Request('POST', $endpoint)); | |||
$handler($command, new Request('POST', $endpoint))->wait(false); |
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.
why was this change necessary?
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.
This was done by @27pchrisl. I've unittested this with and without the wait()
and was not able to find a reason why.
I've removed it again.
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 found out why this was probably added.
When running this test using PHP 8.0 and PHPUnit 9.x, this test is marked as a risky test because it does not run any assertions. When adding ->wait(false)
, this disappears and the assertions are actually being done.
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.
Left a few comments, but I think we're getting close :)
src/Api/Serializer/JsonBody.php
Outdated
@@ -29,7 +29,12 @@ public function __construct(Service $api) | |||
public static function getContentType(Service $service) | |||
{ | |||
return 'application/x-amz-json-' | |||
. number_format($service->getMetadata('jsonVersion'), 1); | |||
. number_format( |
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.
This will never be null. We have a check to ensure that every one of our services that go through this code will have a json version. For now, this is just a deprecated warning, right? We should consider just suppressing instead.
src/S3/S3Client.php
Outdated
return false; | ||
} | ||
|
||
$bucketLen = $bucket !== null ? strlen($bucket) : 0; |
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.
This check is not necessary if we are checking for is_string above; it will never be null and always be a string, right?
@@ -0,0 +1,141 @@ | |||
<?php | |||
|
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.
Is there a reason we would use our own Polyfill instead of taking a dependency on something widely tested and maintained like https://github.com/Yoast/PHPUnit-Polyfills?
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.
Not at all, I will update this. I wasn’t sure what the AWS policy was on third party deps.
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.
@SamRemis although, having just taken a look at it, it's quite a substantial change at this point, so just checking it's something you're definitely asking for :)
But worth noting that even if we pull polyfills from PHPUnit-Polyfills we'll still need some of our own, for example to handle readAttribute()
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'd think it's worth having as little code to maintain as possible by using the polyfill, especially when there's a dependency that does exactly that. I'd prefer it, yes. @stobrien89, do you agree?
Also as a side note, sorry this is taking so long. It's a huge PR and carefully looking at every single file over and over again is very time consuming. Thank you for being patient, and I'll do my best to prioritize it higher. If you don't see me respond for a bit, feel free to @ me to give me a notification. That said, I'll try to look at this every day until it's merged.
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 definitely agree. Especially with us looking forward to the next major version, having as little code to maintain as possible will be ideal. Thanks again for all of the work on this @Zombaya and @27pchrisl!
tests/CloudFront/UrlSignerTest.php
Outdated
$ts = time() + 1000; | ||
$key = $_SERVER['CF_PRIVATE_KEY']; | ||
$kp = $_SERVER['CF_KEY_PAIR_ID']; | ||
$ts = time() + 1000; |
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.
spacing
tests/CloudFront/UrlSignerTest.php
Outdated
$ts = time() + 1000; | ||
$key = $_SERVER['CF_PRIVATE_KEY']; | ||
$kp = $_SERVER['CF_KEY_PAIR_ID']; | ||
$ts = time() + 1000; |
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.
spacing
tests/CloudFront/UrlSignerTest.php
Outdated
|
||
$invalidUri = 'http://abc.cloudfront.net/images/éüàçµñ圌.jpg?query key=query value'; | ||
$uri = new Uri($invalidUri); | ||
$uri = new Uri($invalidUri); |
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.
spacing
tests/CloudFront/UrlSignerTest.php
Outdated
'policy' => '{}', | ||
'private_key' => $_SERVER['CF_PRIVATE_KEY'], | ||
'key_pair_id' => $_SERVER['CF_KEY_PAIR_ID'] | ||
$url = $client->getSignedUrl(array( |
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.
spacing (next three lines)
@stobrien89 , I've added you and the other contributors here as contributor to the project so you should be able to push to the branch. Otherwise a second PR can be created to replace this one from your branch. |
@stobrien89 Did you manage to get the work you did above (#2403 (comment)) into this PR? |
Hi @Zombaya, @27pchrisl, @RobinHoutevelts, Apologies for the delay. Just pushed up the changes I mentioned in my last comment, including some changes related to #2498. From here, the next step would be to take a dependency on https://github.com/Yoast/PHPUnit-Polyfills. @SamRemis and I have discussed and with the next major version on the horizon, we'd rather not have another component to maintain. Thanks! |
…dms/phpunit-arraysubset-asserts
69a65c7
to
661ac2f
Compare
@stobrien89, @SamRemis, I've added a dependency on
Strategy
Base TestCaseI opted to only use The other option would be that every test extends this TestCase. This way you do not need to be very aware of what methods are deprecated when writing new TestCases. I felt that the first option was preferable. Main changesDifferent methodnames for setUp(), tearDown(), ...The library by Yoast provides different methodnames for fixtures than what was used in the -- function _setUp() {}
++ function set_up() {}
-- function _tearDown() {}
++ function tear_down() {}
-- public static function _setUpBeforeClass()
++ public static function set_up_before_class()
-- public static function _tearDownAfterClass()
++ public static function tear_down_after_class() $this->readAttribute() replaced by $this->getPropertyValue()
-- $this->assertArrayHasKey('s3', $this->readAttribute($p, 'manifest'));
++ $this->assertArrayHasKey('s3', $this->getPropertyValue($p, 'manifest')); Replaced $this->expectNotToPerformAssertions() by $this->addToAssertionCount(1)
I've left a comment in place to eventually replace it again with Other options would be writing a new compatibilityTrait inside our library, try to get a PR merged with -- $this->expectNotToPerformAssertions();
++ $this->addToAssertionCount(1); // To be replaced with $this->expectNotToPerformAssertions(); I could not find a lot of documentation on the method. I couldn't find it in the docs for example. Some documentation I found: |
Was any further progress made here? We've started to run into issues with PHP 8.1 |
@SamRemis , consider this a friendly notification 👼 Thanks everyone for all the hard work so far! |
@RobinHoutevelts thanks for the ping :) since the last update on Tuesday or Wednesday I've been working through this about 40 files a day, I'm gonna try to finish it up tomorrow or monday. Only issue I've found so far is a missing space, which I won't block on, so it's getting close :) |
Just to update, I have about 20 files left to go through, which I won't be able to do before today's release, but I should be able to finish today. Assuming I find no issues, which I haven't yet in the first 140 files, I will be able to merge tonight and this should go out with tomorrow's release. |
Absolute legend @Zombaya |
@SamRemis, @stobrien89, you made me one happy camper by getting this merged and released today! Many thanks and much kudos for the effort you spent and the thorough review you did multiple times. I was impressed with the level of detail you went over everything, as well as the respectfulness of the notes when something was not quite correct. Small shout-out to @27pchrisl for initiating the work on this PR and continuing to help after reviews have been made. I think it's safe to say this was as PR me and a lot of others were all looking forward to be merged into master 🙂. |
I've expanded on the excellent work done by @27pchrisl in PR #2332. I rebased his branch onto the latest version of master, upgraded the new tests to be able to use phpunit 9 and added a new CI-job that lints all code in
src
to make sure it is compatible with all supported versions of php (5.5-8.1) using phpcompatibility.This should add support for php 8.0 and php 8.1 to the project and make sure no code will trigger bugs because of changes in php.
This should probably fix #2378, #2367
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.