aws / aws-sdk-php-symfony

Apache License 2.0
350 stars 89 forks source link

Add Symfony 6 support #78

Closed sylvaindeloux closed 2 years ago

sylvaindeloux commented 2 years ago

Description of changes:

Composer config has been updated to allow installation on Symfony 6.0.* projects.

lugosium commented 2 years ago

Any news?

AlexLisenkov commented 2 years ago

@howardlopez could you take a moment to review this?

juniorb2ss commented 2 years ago

Any news?

SamRemis commented 2 years ago

Is this definitely backward compatible? It'll take me some time to review this and determine if there are any breaking changes, and what they may be

juniorb2ss commented 2 years ago

Is this definitely backward compatible? It'll take me some time to review this and determine if there are any breaking changes, and what they may be

I tried it locally and may have some changes that break regarding the IoC tests, some of them are failing. I didn't have enough time to fix them. Any news I will let you know.

Qbrain commented 2 years ago

Just to be sure: Is there any update on this from the AWS team? the PR dates from November, and by now I think this package should be Symfony 6 compatible?

SamRemis commented 2 years ago

Apologies for the delay; it takes a long time to review these since it requires a major deep dive. A lot of research goes into confirming what will be a backward breaking change in a new major version of a dependency, and how to mitigate any breaking changes this will cause.
Symfony does have a solid backward compatibility guarantee, but not all of the SDK's use cases are covered under it. I'm still looking into it, but am happy to look into any feedback from the community or docs linked on why this wouldn't break for current customers.

Qbrain commented 2 years ago

absolutely no problem, it was more of a status check, so thanks for the update! Take all the time you need obviously. :)

lugosium commented 2 years ago

FYI @SamRemis when cleaning cache in Symfony 6:

13:09:29 WARNING [app] Failed to generate ConfigBuilder for extension Aws\Symfony\DependencyInjection\AwsExtension. ["exception" => LogicException { …},"extensionClass" => "Aws\Symfony\DependencyInjection\AwsExtension"]

Maybe related, deprecation in Symfony debug toolbar:

Method "Symfony\Component\Config\Definition\ConfigurationInterface::getConfigTreeBuilder()" might add "TreeBuilder" as a native return type declaration in the future. Do the same in implementation "Aws\Symfony\DependencyInjection\Configuration" now to avoid errors or add an explicit @return annotation to suppress this message.

PascalMarechal commented 2 years ago

Any update for this?

jukuan commented 2 years ago

It does look for me that PHP code works fine for Symfony 6. I did exact same changes in my fork. Jjust few changes in composer.json file are needed (~6.0). Why it's so hard to merge that PR? :-) In any way it's better to have partial support of Symfony 6 than do not have any support.

roman-eonx commented 2 years ago

Does anyone know an alternative to this package that works with Symfony 6? This outdated dependency is the only one blocking us from updating Symfony :cry:

SamRemis commented 2 years ago

I don't know if there are any alternatives, but I can provide an update; been working on this today and manual tests look good, just double checking on any deprecations that could cause problems now.
Once that's done, I just need to migrate the travis tests to github actions. If I don't have time today, I'll work on it first thing Monday barring a major emergency. If I can't push to this PR, I'll make my own similar one with the same updates and added github actions.

SamRemis commented 2 years ago

Update: Looks like the github actions tests that I added are passing; gonna need a few more minutes to get some tests running in PHP 8.0 since the current unit tests aren't compatible. I likely won't be adding them to this PR because of syntax issues leading to an incompatibility with versions before 7.2.
Still trying to get this working with a polyfill or updated code, but if I can't get it working in a reasonable timeframe, I will just merge this until I can deprecate lower versions.

SamRemis commented 2 years ago

Will put out a tagged release shortly - just as a side note, I would happily accept community input on how to replace the following AwsExtensionTest unit tests: extension_should_escape_strings_that_begin_with_at_sign, extension_should_expand_service_references, and extension_should_validate_and_merge_configs. They are currently using mocks, which is incompatible with the return types added in Symfony 6; this repo will need to have those test automated (or removed if they're not useful) before we can officially add support of PHP 8.

lugosium commented 2 years ago

Thank you @SamRemis 👍