-
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
Add ReturnTypeWillChange attribute #2269
Conversation
@SamRemis can you maybe approve the workflow so I can spot any build failures? Then I can fix them before you have a chance to review this. |
@driesvints, |
Cool, thanks @SamRemis! No worries at all. Seems like all tests pass 👍 |
@SamRemis Heya, I definitely don't want to pressure you but is there any chance we can get this PR merged and tagged any time soon? Atm this is preventing us from going ahead with a passing PHP 8.1 build for Laravel. |
@driesvints No pressure at all! I apologize for the delay- I was looking into this and got swept up in other things. Looking at it again, it's definitely backwards compatible. I'm not quite ready to get to work on supporting PHP 8, but am happy to merge this if it helps you guys. I'll do my best to start work on getting it merged today. My only question is on the use statements; why are they necessary? They generate a warning: |
Hey @SamRemis. Thanks a lot!
Hmm, where do you see that? We didn't see any problems with that in Laravel when merging laravel/framework#37838. I can easily undo that if need be. |
@driesvints I got that warning by using this SDK to make a very basic script to fetch an object from S3. Excuse the bad cose because this was just a test file:
When I ran the above code on PHP 7.3, that warning was shown in the output |
@SamRemis that's because you're using it directly in that script. If you use any of the classes modified in this PR there shouldn't be an error normally. |
@driesvints You're right, tested it out that way and no warning came up :) Just need to test it down to 5.5, and no problems came. Thanks for your patience, going to merge it soon |
Thanks a bunch @SamRemis 🎉 |
Btw, just FYI: we ended up changing the syntax ourselves to |
This PR adds the new
ReturnTypeWillChange
attribute from PHP 8.1 to several internal method overrides so these are compatible with PHP 8.1. This is similarly to how we've done it in the Laravel framework: laravel/framework#37838At the moment the PHP 8.1 build for the Laravel framework is failing because AWS is not yet compatible with it (understandable because it's still a while out). By merging this PR, the AWS PHP SDK will be one step closer to PHP 8.1 compatibility and we can run our own build to prepare Laravel. Now with the recent PHP 8.1 Alpha 3 being out it's a good time to start testing: https://www.php.net/archive/2021.php#2021-07-08-1
See the failing build on
laravel/framework
here: https://github.com/laravel/framework/runs/3022524466#step:7:61PS.: I tried to add a PHP 8.0 build (and was planning on a 8.1 after it) but there seems to be much more work involved as the current test suite has greatly aged. I suggest doing a new 4.0 major release and dropping a few older PHP versions and maintain parallel 3.x and 4.x branches so you can at least modernize the codebase and upgrade to a newer PHPUnit version.