Support for different filesystems

[HuubVerbeek]

I recently ran into the need to dynamically switch the filesystem that is used by the PDF object. In order to do so I figured I'd overwrite the facade binding like this:

  $this->app->bind('dompdf.wrapper', function ($app) {
           $filesystem = $conditions
              ? Storage::drive('s3')
              : $app['files'];
                                         
            return new PDF($app['dompdf'], $app['config'], $filesystem, $app['view']);
        });

However Storage::drive('s3') returns an instance of an \Illuminate\Filesystem\FilesystemAdapter which is not an instance of \Illuminate\Filesystem\Filesystem that's required by the object's constructor.

Considering that all the filesystem adapters implement Illuminate\Contracts\Filesystem\Filesystem I suggest we allow an instance of this contract as constructor argument.

So this:

 public function __construct(Dompdf $dompdf, ConfigRepository $config, Filesystem $files, ViewFactory $view)

Would become:

use Illuminate\Contracts\Filesystem\Filesystem as FilesystemContract;

 public function __construct(Dompdf $dompdf, ConfigRepository $config, Filesystem|FilesystemContract $files, ViewFactory $view)

Interestingly, we will need to allow both because \Illuminate\Filesystem\Filesystem itself does not implement Illuminate\Contracts\Filesystem\Filesystem.

What do we think about this?

[angeljqv]

I suggest we allow an instance of this contract as constructor argument.

Looks like a really bad idea, just do:

Storage::disk('s3')->put('file_name.pdf', $pdf->->output());

[HuubVerbeek]

Looks like a really bad idea, just do:

Storage::disk('s3')->put('file_name.pdf', $pdf->->output());

Could you elaborate on the down sides of this idea?

Your suggestion works great if I always want to use the same file system in the same location. What if I want to dynamically switch between file systems in the same location? Why is it a bad idea to have the container resolve it?

[angeljqv]

Why is it a bad idea to have the container resolve it?

That not is the bad idea, changing the constructor signature is the bad idea, it could be a breaking change for someone

What if I want to dynamically switch between file systems in the same location?

Easy, change disk and locations dynamically, it uses arguments, you can use variables, obviously I put static texts for the example, do you know development basics?

What about #912?

[HuubVerbeek]

That not is the bad idea, changing the constructor signature is the bad idea, it could be a breaking change for someone

Aah but isn't it backward compatible as long as we also allow Filesystem $files? It is an expansion, not a modification as far as I understand. Obviously I could do something like this:

app()->bind('custom_filesystem', function(){
   $filesystem = $conditions
          ? Storage::disk('s3')
          : Storage::disk('something_else');
});

app()->make('custom_filesystem')->put('file_name.pdf', $pdf->->output()); 

But I don't see the point if we could also expand the constructor signature.

Concretely, in what way could this be a breaking change?

[angeljqv]

Seriously, it is so hard to understand why is a breaking change

First, union types starts at 8.0, source https://php.watch/versions/8.0/union-types, it breaks eveything for eveyone with php <8.0

Second, it breaks everything if somebody already overwrote contructor(signature changes)

Third, $conditions?? seriusly, that absurdly adds unnecessary complexity(not a breaking change, but it creates confusion and you are not going to help support)

Also, less duplicity, just

app()->bind('custom_filesystem', function(){
  return Storage::disk($conditions ? 's3' : 'local');
});

What about #912?

[HuubVerbeek]

First, union types starts at 8.0, source https://php.watch/versions/8.0/union-types, it breaks eveything for eveyone with php <8.0

Thanks! This was the thing I did not realize.

Third, $conditions?? seriusly, that absurdly adds unnecessary complexity(not a breaking change, but it creates confusion and >you are not going to help support)

I wasn't suggesting we should add this ;)

app()->bind('custom_filesystem', function(){
  return Storage::disk($conditions ? 's3' : 'local');
});

Nice!

[angeljqv]

What about #912? Maybe it could help you

[HuubVerbeek]

It looks nice! I have already solved my problem. Just wanted to understand why it is a bad idea. Now, I understand and agree.