Skip to content
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

Fixed config issue with AWS SDK v3 in AwsS3ResolverFactory #683

Closed
wants to merge 6 commits into from

Conversation

kieste
Copy link

@kieste kieste commented Dec 23, 2015

I just modified the pull request from glenn-- so that his changes pass PHP 5.3 checks.

@makasim
Copy link
Collaborator

makasim commented Dec 23, 2015

What if we add a new factory for v3 sdk? Same time we can deprecate curent aws factory.

It would be good if you can add some docs on when to use what factory.

Sounds good?

@makasim
Copy link
Collaborator

makasim commented Dec 25, 2015

@kieste are you planning to finish this PR?

@kieste
Copy link
Author

kieste commented Dec 25, 2015

Yes I will fix this before end of the year.

@kieste
Copy link
Author

kieste commented Dec 26, 2015

@makasim separated old and new SDK client config code into different factory classes. Added a note to the documentation.

@@ -32,7 +33,11 @@ public function build(ContainerBuilder $container)
$extension = $container->getExtension('liip_imagine');

$extension->addResolverFactory(new WebPathResolverFactory());
$extension->addResolverFactory(new AwsS3ResolverFactory());
if (defined('\Aws\Sdk::VERSION') && version_compare(\Aws\Sdk::VERSION, '3.0.0', '>=')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register both, and let a developer decided which one to use. Also would be good to add an exception if a resolver is used with wrong sdk.

@kieste
Copy link
Author

kieste commented Dec 26, 2015

@makasim after taking a closer look at my current setup I realized that I am not using the factories any more. (Currently I am creating the AwsS3Resolver as a service and injecting it directly into a proxy resolver.)

So I can't finish this pull request now with your suggested changes. (I must progress with my current project.) But I aggree to all of your comments. This totally makes sense. But it's too much effort for me at the moment.

@kieste kieste closed this Dec 26, 2015
@makasim makasim reopened this Dec 27, 2015
@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Dec 27, 2015
@makasim makasim mentioned this pull request Dec 27, 2015
@makasim makasim closed this Dec 27, 2015
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Dec 27, 2015
@makasim
Copy link
Collaborator

makasim commented Dec 27, 2015

Thanks for your contribution, I took care of your PR. Soon in the master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants