Fix async S3 filesystem adapter throwing error if the full AWS SDK is not installed, even though it is not used in this case#2490
Open
zyberspace wants to merge 3 commits intosonata-project:4.xfrom
Conversation
…ld work without having the full AWS SDK installed
… not installed, even though it is not used in this case
phansys
reviewed
Jun 16, 2025
| * @phpstan-return iterable<array{array<string, mixed>, array<string, mixed>}> | ||
| */ | ||
| public function provideLoadWithFilesystemConfigurationV3Cases(): iterable | ||
| public function provideLoadWithFilesystemConfigurationS3Cases(): iterable |
Member
There was a problem hiding this comment.
"V3" refers to the SDK's version.
Author
There was a problem hiding this comment.
Hey, thanks for the reply. So i drop the name change?
Maybe something like provideLoadWithS3FilesystemConfigurationV3Cases would work?
Tests would then be testLoadWithS3FilesystemConfigurationV3 and testLoadWithS3FilesystemConfigurationAsync.
| if (class_exists(S3Client::class)) { | ||
| if ( | ||
| class_exists(S3Client::class) | ||
| && ($_ENV['PHPUNIT_TESTS_SONATA_MEDIA_FORCE_DISABLE_S3_CLIENT'] ?? 'false') !== 'true' |
Member
There was a problem hiding this comment.
This feels odd having a code/env value for test in prod config. Isn't any other way to do it @zyberspace ?
Member
|
If you rebase, the rector build will be fixed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Subject
When i followed the S3 setup documentation, I ran into an error that the
sonata.media.adapter.service.s3does not exist.As it turns out, even when using the
async-aws/simple-s3client like described in the documentation and therefore thesonata.media.adapter.service.s3.asyncservice, the container builder still expects thesonata.media.adapter.service.s3to exist, even though this service is not used in this case.The full error i received:
I added a failing test that reproduces this error. If you run the PHPUnit tests with the commit a9ebea4 you can see the failing test and the error described above is thrown.
After verifying that the new test "successfully" fails, i added another commit that fixes this.
I actually had a bit of trouble preventing the AWS SDK S3 service from registering because both clients are installed in the test environment, hence why the tests were also not throwing any errors before. If there is a better way to prevent that service from being registered in the test environment, please let me know.
(I also renamed the S3 tests from
...V3..to...S3.... If theV3name was on purpose, i am happy to change that back)I am targeting this branch, because this is a backwards compatible fix.
Changelog