feat: containerise s3 client generation, support other client credentials

[tractorcow]

fixes #23

I copied the formatS3Config logic directly from https://github.com/laravel/framework/blob/c0f3ee8c8779d5cfb6bb3c37c93b84672e67f236/src/Illuminate/Filesystem/FilesystemManager.php#L258-L266 to ensure that we supported like-for-like construction of the s3 client.

This ensures that custom config (such as the below) will continue to work.

        'endpoint'                => env('AWS_ENDPOINT'),
        'use_path_style_endpoint' => env('AWS_USE_PATH_STYLE_ENDPOINT', false),
        'options'                 => [
            'CacheControl' => 'public, max-age=2629746, s-maxage=2629746, stale-while-revalidate=2629746',
        ],

Once the getDriver() method is available, obviously we can use that, but no reliance on that being available on any specific version of 9.x.

I made an opinionated call and de-privatised the init() method, so that should this method need to be overridden entirely, users could replace the entire UploadController (as I have done myself).

Similarly, I shifted the s3 client construction into a named service in case user code needs to replace with their own logic.

[ahmedkandel]

@tractorcow the use of formatS3Config is very good while i didn't get your point regarding making the init method "protected" not "private" ? If this gives the opportunity to extend the UploadController what about the routes that point to it ?

What is the benefit of moving the S3Client instantiation to the ToolServiceProvider ? will this make it swappable, can you provide a use case ?

Thanks.

[tractorcow]

Hi @ahmedkandel , an example use case is where we might need application-specific customisation of the s3 client.

E.g. if I wanted to wrap my client in a proxy, or customise the config, I could do the below without having to replace the entire UploadController.

// Override default s3 client for assets
$this->app->bind('novas3client', function () {
    // example customisation - filesystem based on role
    $disk = auth()->user && auth()->user->is_admin ? 'protected' : 'public'

   // example customisation - non-default config source
   $config = $this->formatS3Config(config("app.upload_config.{$disk}"));

   // example customisation - manipulate config dynamically
   $config['options']['Cache-Controle'] = 'no-cache';

    // example customisation - proxy client via custom class
    return new S3ProxyClient(new S3Client($config));
});

https://github.com/ahmedkandel/nova-s3-multipart-upload/pull/24 is another use case that could be solved via a custom bound client.

Regarding the UploadController class, I could still override it this way

$this->app->bind(UploadController::class, MyUploadController::class);

then override

class MyUploadController extends UploadController {
   protected function init($request) {
      parent::init($request);
      // whatever else I need to act on $this->s3client.
   }
}

I had to do this earlier; but because init was private, I had to override every method in the class, so my override was very big :)

[ahmedkandel]

Hi @tractorcow moving the S3Client instantiation to the ToolServiceProvider will make the code more organised but i think it is better to move the binding under register() method not boot() method, would also suggest to use ->singleton() instead of ->bind().

Extending the UploadController seems hacky and make no sense as you can replace the S3Client binding in your own service provider so it will be duplication.

Regarding the examples you showed above i would suggest other way to do them:

$this->app->bind('novas3client', function () {

    // instead of doing this in the binding
    $disk = auth()->user && auth()->user->is_admin ? 'protected' : 'public'
    // you can directly use this tool disk() method in your nova resource
    NovaS3MultipartUpload::make('Files')
        ->disk(auth()->user && auth()->user->is_admin ? 'protected' : 'public')

   // it is not good practice to move filesystem disk configuration outside config/filesystems.php
   $config = $this->formatS3Config(config("app.upload_config.{$disk}"));

   // all configuration options will be taken in consideration after this PR so no need to do it manually
   $config['options']['Cache-Controle'] = 'no-cache';

});

Commit the new modification so we can review and merge.

Thanks.

[ahmedkandel]

A simpler implementation would be:

$adapter = Storage::disk($this->tool->disk);

$this->s3Client = is_callable($adapter, 'getClient')
    ? $adapter->getClient()
    : $adapter->getDriver()->getAdapter()->getClient();

[tractorcow]

I agree with moving to register, but singleton() will not work due to the requirements to provide constructor arguments. We could call makeWith with different disks, and get different instances back.

Extending the UploadController seems hacky and make no sense as you can replace the S3Client binding in your own service provider so it will be duplication.

Yeah that's why I don't want to extend it. It was hacky, hence the service provider. :)

I also think your is_callable() idea would solve the problem and is pretty smart. I thought about something like that, but was cautious about using too much conditional introspection though, since you have two paths you need to test instead of one, and the provider solution works equally on all versions of laravel.

[tractorcow]

I have shifted the code to register() now

[tractorcow]

For context, this is what happens when you register with ->singleton(); The $parameters will prevent the singleton being returned.

// Container.php:739
        $needsContextualBuild = ! empty($parameters) || ! is_null($concrete);

        // If an instance of the type is currently being managed as a singleton we'll
        // just return an existing instance instead of instantiating new instances
        // so the developer can keep using the same objects instance every time.
        if (isset($this->instances[$abstract]) && ! $needsContextualBuild) {
            return $this->instances[$abstract];
        }

[tractorcow]

Also as an asid @ahmedkandel , thanks for making such a great module. We use s3 extensively so we'll keep coming back with future contributions as we are able to. :)

[ahmedkandel]

Thanks @tractorcow