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

Configurable s3 adapter #2358

Merged
merged 5 commits into from
Feb 19, 2023

Conversation

ozahorulia
Copy link
Contributor

@ozahorulia ozahorulia commented Feb 16, 2023

Added configuration to set AWS S3 adapter class

I am targeting this branch, because it adds a configuration feature with full BC.

Closes #2357 .

Changelog

### Added
- Added new configuration parameter `sonata_media.filesystem.s3.async`

@ozahorulia
Copy link
Contributor Author

ozahorulia commented Feb 16, 2023

On a real project it fails because of this error:

Gaufrette\Adapter\AsyncAwsS3::__construct(): Argument #1 ($service) must be of type AsyncAws\SimpleS3\SimpleS3Client, ContainerEmExVd8\S3Client_76b2022 given,

I have to think how to pass async client instead of the simple one

@VincentLanglet
Copy link
Member

On a real project it fails because of this error:

Gaufrette\Adapter\AsyncAwsS3::__construct(): Argument #1 ($service) must be of type AsyncAws\SimpleS3\SimpleS3Client, ContainerEmExVd8\S3Client_76b2022 given,

I have to think how to pass async client instead of the simple one

Is there other adapters ?
If not, you may use a config async: true/false instead of the class.
And then if config = true, you define the async, else you define the original one.
This allows to have different constructors.

@ozahorulia
Copy link
Contributor Author

Is there other adapters ? If not, you may use a config async: true/false instead of the class. And then if config = true, you define the async, else you define the original one. This allows to have different constructors.

Yes, this is exactly what I'm going to do. Since there're indeed no other adapters, async options looks much more useful. I'm working on it.

src/DependencyInjection/SonataMediaExtension.php Outdated Show resolved Hide resolved
docs/reference/advanced_configuration.rst Outdated Show resolved Hide resolved
@@ -128,6 +128,10 @@ public function testListAction(): void
$this->configureSetFormTheme($formView, ['filterTheme']);
$this->configureSetCsrfToken('sonata.batch');
$this->configureRender('templateList', 'renderResponse');

/**
* @psalm-suppress DeprecatedClass
Copy link
Member

Choose a reason for hiding this comment

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

Can you solve the deprecation instead ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd leave it as it is right now, because phpunit doesn't provide any replacement for the withConsecutive method, and I'm way out of the context of this test case to thoroughly re-write the test. Any proposed workarounds, like using callbacks with match (switch if we still support 7.4) look very hideous to me.

src/Resources/config/gaufrette.php Show resolved Hide resolved
src/DependencyInjection/SonataMediaExtension.php Outdated Show resolved Hide resolved
src/DependencyInjection/SonataMediaExtension.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@@ -89,13 +90,13 @@
"sonata-project/classification-bundle": "^4.0",
"sonata-project/doctrine-orm-admin-bundle": "^4.0",
"symfony/browser-kit": "^4.4 || ^5.4 || ^6.0",
"symfony/filesystem": "^4.4 || ^5.4 || ^6.0",
Copy link
Member

Choose a reason for hiding this comment

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

One last question, why this was removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already in the "require" section with the same versions. Does it makes sense to have a duplicate entry in require-dev? I can roll it back if it was there for a reason :)

Copy link
Member

Choose a reason for hiding this comment

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

didn't see the require section. You were right to remove it then. thnaks

@VincentLanglet VincentLanglet merged commit a8b9bc4 into sonata-project:4.x Feb 19, 2023
@ozahorulia
Copy link
Contributor Author

@VincentLanglet Thank you! May I ask you to create a minor release, so I can use it in my projects?

@VincentLanglet
Copy link
Member

Done

@ozahorulia
Copy link
Contributor Author

Thank you!

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

Successfully merging this pull request may close these issues.

Configurable Gaufrette S3 adapter class
2 participants