[Feature Request] Allow to re-use an already existing S3Client instance

[Kocal]

Hi!

First, thank you for taking over the maintenance of this bundle, we all thought it was abandoned and would prevent us to migrate to Symfony 6 (due to dependencies issues between symfony/cache and league/cached-adapter IIRC).
This is why I've forked the projet and added many features (patches, taking a S3Client instance from the Container, using a PSR-6 cache pool, PHPStan, ...), and used it in our application.

So, in the futur we plan to drop my fork and re-use this bundle, but before that we need to find a way to prevent the BackendFactory to create a S3Client by itself, and use a S3Client instance from the Symfony Container.

I implemented this feature in Kocal/ckfinder-symfony-bundle#17 and Kocal/ckfinder-symfony-bundle#19, but I'm not super proud of it because I think it can be done in a better way.

Implementations idea

1. Configuration in the bundle

Ideally, we want to configure it at the backend configuration level, with a key client that will take a string representing a service identifier.

services:
    aws_s3_client:
        class: Aws\S3\S3Client
        arguments:
            - version: 'latest'
              region: '%aws_s3_region%'
              credentials:
                  key: '%aws_s3_client_key%'
                  secret: '%aws_s3_client_secret%'	
              # more options [...]

ckfinder:
  connector:
    backends:
      aws:
        name: 'aws'
        adapter: 's3'
        bucket: '%aws_s3_bucket%'
        client: 'aws_s3_client' // or `@aws_s3_client`

2. Dealing with the BackendFactory

Then we need to pass this S3Client instance to the BackendFactory.

In my fork, I've used a ServiceLocator:

• that is injected to the ConnectorFactory: https://github.com/Kocal/ckfinder-symfony-bundle/blob/main/src/DependencyInjection/CKSourceCKFinderExtension.php#L95-L97 and https://github.com/Kocal/ckfinder-symfony-bundle/blob/main/src/Factory/ConnectorFactory.php#L32
• that is used in the patched BackendFactory, see https://github.com/Kocal/ckfinder-symfony-bundle/blob/main/src/Patcher/BackendAwsS3ClientPatcher.php#L35

But it means that BackendFactory from the CKFinder PHP Connector should be updated on your side, since I don't think we want to re-introduce a patching system here.

---

Do you think this would be possible? Injecting a service reference in bundles in pretty common, e.g. with The PHP League's Flysystem bundle:

flysystem:
    storages:
        cloud.storage.aws_s3:
            adapter: 'aws'
            # ...
            options:
                client: 'aws_s3_client' # <-- here
                bucket: '%aws_s3_bucket%'

or with LiipImagineBundle:

    app.imagine.cache.resolver.aws_s3:
        class: Liip\ImagineBundle\Imagine\Cache\Resolver\AwsS3Resolver
        arguments:
            - '@aws_s3_client' # <-- here
            - '%aws_s3_bucket%'
        tags:
            - { name: 'liip_imagine.cache.resolver', resolver: 'aws_s3' }

Thanks!

[zaak]

Thank you for this suggestion @Kocal! Indeed, such an approach of defining everything through the configuration would be very convenient in Symfony.

It's super cool that you have managed to find the solution 👍 Although it is possible to provide your own S3 client without hacking the core code. It would be enough to create a plugin that registers a custom storage adapter. The registered adapter can directly use CKSource\CKFinder\Backend\Adapter\AwsS3 in the instantiation callback, just like the default one. The first argument of the constructor is S3ClientInterface so the existing S3 client instance could be passed here.

namespace CKSource\CKFinder\Plugin\MyCustomS3AdapterPlugin;
 
use CKSource\CKFinder\CKFinder;
use CKSource\CKFinder\Plugin\PluginInterface;
use PDO;
 
class MyCustomS3AdapterPlugin implements PluginInterface
{
    public function setContainer(CKFinder $app)
    {
        $backendFactory = $app->getBackendFactory();
 
        // Register a backend adapter named "my_s3".
        $backendFactory->registerAdapter('my_s3', function ($backendConfig) {
            $filesystemConfig = [
                'visibility' => $backendConfig['visibility'] ?? 'private',
            ];

            $prefix = isset($backendConfig['root']) ? trim($backendConfig['root'], '/ ') : null;

           // Based on how $client is passed in your code (and assuming it's an object)
           $client = ($backendConfig['client'] ?? null);

            return $this->createBackend($backendConfig, new AwsS3($client, $backendConfig['bucket'], $prefix), $prefix), $filesystemConfig);
        });
    }
 
    public function getDefaultConfig()
    {
        return [];
    }
}

Then you can simply reference such adapter in the config by adapter: 'my_s3' name, instead of using the default adapter: 's3'.

[Kocal]

Thanks for your response, I didn't know about CKFinder plugins system! 😅
So yeah it should be very easy to re-use an already existing S3Client instance.

However I have one question about the plugin location, I see in your example and in the documentation that the namespace should be CKSource\CKFinder\Plugin, and that's really weird, does it really mean that we should write our plugin inside Connector's /Plugin directory?

That's so weird, I would expect to write a class that implements PluginInterface where ever I wan't, having the bundle to find all services implementing PluginInterface, and calling CKFinder#registerPlugin(), but it looks like the bundle does not support it.

Thanks!

[zaak]

@Kocal: No, placing plugin in /plugins directory is not required. It is just one of the methods, especially useful when CKFinder is hosted as a standalone application.
In Symfony you can just pass the plugin instance to $ckfinder->registerPlugin(PluginInterface $plugin) method, like:

$container->get('ckfinder.connector')->registerPlugin($pluginInstance);

That's so weird, I would expect to write a class that implements PluginInterface where ever I wan't, having the bundle to find all services implementing PluginInterface, and calling CKFinder#registerPlugin(), but it looks like the bundle does not support it.

That's a cool idea for improvement 👍

[Kocal]

@zaak Hum okay, I will trust you, but to me it really looks like the documentation says that the plugin needs to be under CKSource\CKFinder\Plugin namespace/directory too 😬 :

For the auto-registration of PluginInterface implementations, I will try to work on it ASAP.

[zaak]

@Kocal: Good point, this might be confusing from Symfony's perspective (or any other integration in a modern PHP app). The documentation assumes CKFinder is hosted as a standalone application.