aws / aws-sdk-php-laravel

A Laravel 5+ (and 4) service provider for the AWS SDK for PHP
http://aws.amazon.com/sdkforphp/
Apache License 2.0
1.64k stars 242 forks source link

Bump dependencies to allow for Laravel 9 #205

Closed jdanino closed 2 years ago

jdanino commented 2 years ago

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

etelford commented 2 years ago

It would be great to get this merged!

vitorn1234 commented 2 years ago

yep trying to update to 9.0 and this is breaking stuff :D updating the phpunit/phpunit might be helpfull for the php 8.0

mchorsley commented 2 years ago

@jdanino Could you update the README.md to show support for Laravel 9? May help to get the PR merged. Thanks for updating composer.json.

jdanino commented 2 years ago

@jdanino Could you update the README.md to show support for Laravel 9? May help to get the PR merged. Thanks for updating composer.json.

done

vitorn1234 commented 2 years ago

there is a big possibility there is no one looking at this, not even the issues are being reviewed for like 1 year, does anyone know anyone in amazon :D

SamRemis commented 2 years ago

Hi there, I'm from amazon. Gonna talk to the team about it during a meeting tomorrow and try to get this on the docket soon. If you'd like to reach us, I try to respond on gitter as quickly as I can, and tagging me helps me to see it sooner.

82rules commented 2 years ago

Would love to see this in asap!! this is a blocker for us upgrading to Laravel 9

lucca257 commented 2 years ago

please, accept the merge request asap! I can`t upgrade my project for laravel 9

SamRemis commented 2 years ago

This PR was also asking for the same thing; unfortunately this is a breaking change and we need to bump our minimum and maximum supported versions, which will take some time. We are discussing it now and working on making a final decision and then shortly thereafter will make an official public announcement if we do.
It's frustrating that this is holding people back from upgrading their versions, so I consider this a high priority. It's looking like we will be dropping support for 5.5, 5.6, and 7.0, so after we make the announcement, we need to give customers on those versions some time to upgrade. Since this is holding people back from upgrading, I will float the idea of fast-tracking this repo and the symfony repo in order to support the newest versions.

hailwood commented 2 years ago

@SamRemis Why do you need to give customers on those older versions time to upgrade? They can just continue to use the current version of the given packages until they're ready to upgrade, it's not like releasing a new version unpublished old versions.

NickBourque commented 2 years ago

@SamRemis Why do you need to give customers on those older versions time to upgrade? They can just continue to use the current version of the given packages until they're ready to upgrade, it's not like releasing a new version unpublished old versions.

@SamRemis - I, too, am curious about this. Why not tag a new version for this release now and drop support for old versions later? Getting an important release out shouldn't be dependent on when you can drop support for old versions. This is very important to us, and I would love to see this merged asap.

cy8urg commented 2 years ago

@SamRemis - Very important for me and my guys as well, why can you not release for 9 and let others upgrade as and when they need/want...? Please merge this...

SamRemis commented 2 years ago

I may be able to merge this one sooner, but I have to at least get to the point of officially announcing the deprecation campaign. This is one of my highest priorities since it's clearly important to the community, I'll merge this as soon as I can confirm no breaking changes.

It may be a fair bit of work to write and test something like this with a new version. In that case, it's not just pressing a merge button. If I were to merge this, it would likely break code. This needs unit test refactoring, versioning changes, function changes, and potentially a major version change.

jonnylink commented 2 years ago

It's a fair bit of work to write and test something like this with a new version. It's not just pressing a merge button. If I were to merge this, it would likely break code. This needs unit test refactoring, versioning changes, function changes, and most likely a major version change.

None of that makes any sense. This is a trivial change. The change here is just saying that it works with Laravel 9. It does.

NickBourque commented 2 years ago

It's a fair bit of work to write and test something like this with a new version. It's not just pressing a merge button. If I were to merge this, it would likely break code. This needs unit test refactoring, versioning changes, function changes, and most likely a major version change.

None of that makes any sense. This is a trivial change. The change here is just saying that it works with Laravel 9. It does.

Yeah I don't get it either. It's impossible to introduce a backward compatibility issue by merging this PR because all this is doing is saying "hi I work with Laravel 9", which is true with zero other code changes.

SamRemis commented 2 years ago

I deleted a comment tagging someone who no longer works for amazon

massimodellarovere commented 2 years ago

I have been a passionate user of Amazon Web Service and PHP development for many years. Unfortunately, the combination of AWS and PHP is a problem that has always been there and seems unsolvable. Amazon doesn't like PHP... It's True..

dump( $amazon->likephp()  ) = false
NickBourque commented 2 years ago

I deleted a comment tagging someone who no longer works for amazon

Could've spent that 3 seconds hitting the merge button and still had a second to spare

jonnylink commented 2 years ago

@SamRemis I appreciate that you are taking this seriously, and I'm sorry to push like this, but you're actually creating an unnecessary headache for AWS customers in this case. You do realize this is the Laravel package and not the PHP SDK itself, right? If so, can you please explain why you believe changing the composer requirements to allow Laravel 9 impacts any past version of Laravel?

We are literally talking about four files here: a facade, a service provider, and some config stuff that get overwritten anyway. Comments removed we are looking at less than 100 lines of code. Nothing in Laravel 9 has changed that would lead to any code changes.

SamRemis commented 2 years ago

@jonnylink thanks for understanding and patience; looks like my initial assessment of the significance was probably incorrect. I still need time to do my due diligence. This will be my first priority and I'll work on it as soon as I'm able, which is early next week.

If what I've read so far from the docs is a good indicator, it shouldn't take long at all once I've started.

SpartakusMd commented 2 years ago

Thank you @SamRemis for helping us with moving forward on this. As this is a library used by many, proper checks should be done and we understand it.

If it helps, we forked the project and made same changes as in this PR (basically allowing use of Laravel 9). We already use it in production on a couple of core projects. No issues found so far.

jonnylink commented 2 years ago

Thanks for making a second assessment, @SamRemis. When you get down to it, this package is really just a service provider for the SDK. As you can see in the upgrade guide there aren't any changes to the service providers, so there truly isn't anything to do. The unit tests as written should adequately cover things. Looking forward to this getting pushed out next week.

goyote commented 2 years ago

MERGE THIS!

etelford commented 2 years ago

I know this is annoying and inconvenient for some of us, but please be kind. You aren't without temporary or even permanent alternate solutions (like forking, creating your own service provider, etc.) if upgrading to Laravel 9 is that important to you.

jonnylink commented 2 years ago

Counter point: It seems pretty clear that this package (and AWS customers) aren't going to get the support at a professional level. This PR literally just adds ||^9.0 to a single line. If that takes over a month it's time for AWS to archive the package so that someone else can carry the torch.

EDIT: that said I'm also in agreement we don't need to be rude.

hailwood commented 2 years ago

@jonnylink Just because the aws package exists doesn't stop you from publishing your own on packagist if you feel that way.

image

jonnylink commented 2 years ago

@hailwood off-topic. The PR is good and should have been merged a month ago.

massimodellarovere commented 2 years ago

Waiting all this time for such a simple change is truly incredible. The answers of some of those responsible are ridiculous and do not deserve a debate. As for me I can also wait, I will move the update of my AWS customers to next month, but in the meantime I have presented to AWS Support as a premium user with 35 midrange customers my due reflections.

SamRemis commented 2 years ago

Resolved by https://github.com/aws/aws-sdk-php-laravel/pull/209