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
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
"twig/twig": "^2.14 || ^3.0"
},
"require-dev": {
"async-aws/simple-s3" : "^1.1",
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved
"aws/aws-sdk-php": "^3.0",
"dama/doctrine-test-bundle": "^6.7",
"doctrine/doctrine-bundle": "^2.3.2",
Expand All @@ -89,7 +90,6 @@
"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

"symfony/messenger": "^4.4 || ^5.4 || ^6.0",
"symfony/phpunit-bridge": "^6.1",
"symfony/security-csrf": "^4.4 || ^5.4 || ^6.0",
Expand All @@ -107,6 +107,7 @@
"sonata-project/doctrine-orm-admin-bundle": "<4.0"
},
"suggest": {
"async-aws/simple-s3": "If you want to use async AWS S3 adapter",
"liip/imagine-bundle": "If you want on-the-fly thumbnail generation or image filtering (scale, crop, watermark...).",
"sonata-project/classification-bundle": "If you want to categorize your media items.",
"sonata-project/doctrine-orm-admin-bundle": "If you want to persist entities.",
Expand Down
1 change: 1 addition & 0 deletions docs/reference/advanced_configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ Full configuration options:
cache_control: max-age=86400 # or any other
meta:
key1: value1 #any amount of metas(sent as x-amz-meta-key1 = value1)
async: true
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved

replicate:
primary: sonata.media.adapter.filesystem.s3
Expand Down
17 changes: 17 additions & 0 deletions docs/reference/amazon_s3.rst
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,23 @@ This is a sample configuration to enable amazon S3 as a filesystem and provider:
version: '%s3_version%' # defaults to "latest" (cf. https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/guide_configuration.html#cfg-version)
endpoint: '%s3_endpoint%' # defaults to null (cf. https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/guide_configuration.html#endpoint)

Async adapter
-------------
In order to use async S3 adapter, you will need to require the following package:

.. code-block:: bash

composer require async-aws/simple-s3

.. code-block:: yaml

# config/packages/sonata_media.yaml

sonata_media:
filesystem:
s3:
async: true

.. note::

This bundle is currently using KNP Gaufrette as S3 adapter.
Expand Down
3 changes: 3 additions & 0 deletions src/DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,9 @@ private function addFilesystemSection(ArrayNodeDefinition $node): void
->prototype('scalar')
->end()
->end()
->booleanNode('async')
->defaultFalse()
->end()
->end()
->end()

Expand Down
40 changes: 32 additions & 8 deletions src/DependencyInjection/SonataMediaExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

namespace Sonata\MediaBundle\DependencyInjection;

use Gaufrette\Adapter\AsyncAwsS3;
use Gaufrette\Adapter\AwsS3;
use Sonata\Doctrine\Mapper\Builder\OptionsBuilder;
use Sonata\Doctrine\Mapper\DoctrineCollector;
use Symfony\Component\Config\Definition\Processor;
Expand Down Expand Up @@ -260,8 +262,15 @@ public function configureFilesystemAdapter(ContainerBuilder $container, array $c

// add the default configuration for the S3 filesystem
if ($container->hasDefinition('sonata.media.adapter.filesystem.s3') && isset($config['filesystem']['s3'])) {
$async = (bool) $config['filesystem']['s3']['async'];
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved
$adapterClass = $async ? AsyncAwsS3::class : AwsS3::class;
$clientReference = new Reference(
$async ? 'sonata.media.adapter.service.s3.async' : 'sonata.media.adapter.service.s3'
);

$container->getDefinition('sonata.media.adapter.filesystem.s3')
->replaceArgument(0, new Reference('sonata.media.adapter.service.s3'))
->setClass($adapterClass)
->replaceArgument(0, $clientReference)
->replaceArgument(1, $config['filesystem']['s3']['bucket'])
->replaceArgument(2, ['create' => $config['filesystem']['s3']['create'], 'region' => $config['filesystem']['s3']['region'], 'directory' => $config['filesystem']['s3']['directory'], 'ACL' => $config['filesystem']['s3']['acl']]);

Expand All @@ -276,22 +285,37 @@ public function configureFilesystemAdapter(ContainerBuilder $container, array $c

$arguments = [
'region' => $config['filesystem']['s3']['region'],
'version' => $config['filesystem']['s3']['version'],
];

if (!$async) {
$arguments['version'] = $config['filesystem']['s3']['version'];
}

if (isset($config['filesystem']['s3']['endpoint'])) {
$arguments['endpoint'] = $config['filesystem']['s3']['endpoint'];
}

if (isset($config['filesystem']['s3']['secretKey'], $config['filesystem']['s3']['accessKey'])) {
$arguments['credentials'] = [
'secret' => $config['filesystem']['s3']['secretKey'],
'key' => $config['filesystem']['s3']['accessKey'],
];
if ($async) {
$arguments['accessKeyId'] = $config['filesystem']['s3']['accessKey'];
$arguments['accessKeySecret'] = $config['filesystem']['s3']['secretKey'];
} else {
$arguments['credentials'] = [
'secret' => $config['filesystem']['s3']['secretKey'],
'key' => $config['filesystem']['s3']['accessKey'],
];
}
}

$container->getDefinition('sonata.media.adapter.service.s3')
->replaceArgument(0, $arguments);
if ($async) {
$container->getDefinition('sonata.media.adapter.service.s3.async')
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved
->replaceArgument(0, $arguments);
$container->removeDefinition('sonata.media.adapter.service.s3');
} else {
$container->getDefinition('sonata.media.adapter.service.s3')
->replaceArgument(0, $arguments);
$container->removeDefinition('sonata.media.adapter.service.s3.async');
}
} else {
$container->removeDefinition('sonata.media.adapter.filesystem.s3');
$container->removeDefinition('sonata.media.filesystem.s3');
Expand Down
7 changes: 7 additions & 0 deletions src/Resources/config/gaufrette.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* file that was distributed with this source code.
*/

use AsyncAws\SimpleS3\SimpleS3Client;
use Aws\S3\S3Client;
use Gaufrette\Adapter\AwsS3;
use Gaufrette\Adapter\Ftp;
Expand Down Expand Up @@ -75,4 +76,10 @@
->args([[]])

->set('sonata.media.metadata.noop', NoopMetadataBuilder::class);

if (class_exists(SimpleS3Client::class)) {
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved
$containerConfigurator->services()
->set('sonata.media.adapter.service.s3.async', SimpleS3Client::class)
->args([[]]);
}
};
4 changes: 4 additions & 0 deletions tests/Controller/MediaAdminControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.

*/
$datagrid->expects(static::exactly(3))->method('setValue')->withConsecutive(
['context', null, 'another_context'],
['category', null, 1]
Expand Down
67 changes: 65 additions & 2 deletions tests/DependencyInjection/SonataMediaExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@

namespace Sonata\MediaBundle\Tests\DependencyInjection;

use AsyncAws\SimpleS3\SimpleS3Client;
use Aws\CloudFront\CloudFrontClient;
use Aws\S3\S3Client;
use Aws\Sdk;
use Gaufrette\Adapter\AsyncAwsS3;
use Gaufrette\Adapter\AwsS3;
use Imagine\Gd\Imagine as GdImagine;
use Imagine\Gmagick\Imagine as GmagicImagine;
use Imagine\Imagick\Imagine as ImagicImagine;
Expand Down Expand Up @@ -237,18 +241,31 @@ public function testLoadWithSonataAdminCustomConfiguration(): void
* @param array<string, mixed> $expected
* @param array<string, mixed> $configs
*/
public function testLoadWithFilesystemConfigurationV3(array $expected, array $configs): void
{
public function testLoadWithFilesystemConfigurationV3(
array $expected,
array $configs
): void {
if (!class_exists(Sdk::class)) {
static::markTestSkipped('This test requires aws/aws-sdk-php 3.x.');
}

$this->load($configs);

static::assertFalse($this->container->hasDefinition('sonata.media.adapter.service.s3.async'));
static::assertSame(
S3Client::class,
$this->container->getDefinition('sonata.media.adapter.service.s3')->getClass()
);

static::assertSame(
$expected,
$this->container->getDefinition('sonata.media.adapter.service.s3')->getArgument(0)
);

static::assertSame(
AwsS3::class,
$this->container->getDefinition('sonata.media.adapter.filesystem.s3')->getClass()
);
}

/**
Expand Down Expand Up @@ -344,6 +361,52 @@ public function dataFilesystemConfigurationAwsV3(): iterable
];
}

public function testLoadWithFilesystemConfigurationV3ASync(): void
{
if (!class_exists(SimpleS3Client::class)) {
static::markTestSkipped('This test requires async-aws/simple-s3.');
}

$expected = [
'region' => 'region',
'endpoint' => 'endpoint',
'accessKeyId' => 'access',
'accessKeySecret' => 'secret',
];

$configs = [
'filesystem' => [
's3' => [
'async' => true,
'bucket' => 'bucket_name',
'region' => 'region',
'version' => 'version',
'endpoint' => 'endpoint',
'secretKey' => 'secret',
'accessKey' => 'access',
],
],
];

$this->load($configs);

static::assertFalse($this->container->hasDefinition('sonata.media.adapter.service.s3'));
static::assertSame(
SimpleS3Client::class,
$this->container->getDefinition('sonata.media.adapter.service.s3.async')->getClass()
);

static::assertSame(
$expected,
$this->container->getDefinition('sonata.media.adapter.service.s3.async')->getArgument(0)
);

static::assertSame(
AsyncAwsS3::class,
$this->container->getDefinition('sonata.media.adapter.filesystem.s3')->getClass()
);
}

public function testMediaPool(): void
{
$this->load();
Expand Down