CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

Image column to support private s3 or compatible bucket

Open ziming opened this issue 3 years ago • 5 comments

WHY

BEFORE - What was wrong? What was happening before this PR?

If the file is stored in a private S3 or private S3 compatible bucket, the image fetching will fail

AFTER - What is happening after this PR?

If 'private_s3_bucket' key is true, use temporaryUrl() instead of url()

HOW

If 'private_s3_bucket' key is true, use temporaryUrl() instead of url().

Is it a breaking change?

No

How can we test the before & after?

Create a private S3 bucket, dump some files there and try.

Additional Notes

I named it 'private_s3_bucket' but in practice it could be a DigitalOcean, Linode or many other private s3 compatible bucket so maybe a different key name is a better choice. In some other libraries, I see that they use temporaryUrl() if user specified that the visibility is set to private (e.g. ->visibility('private') )

As for expiry time I give it 1 minute, I think that is way more than enough time for the private image to load. I did not provide a parameter to configure how long the duration should last.

ziming avatar Jun 27 '22 05:06 ziming

@ziming thanks for the contribution. How about temporarySignedUrl() ? Since we are doing changes here, shouldn't we tackle those too ?

Cheers

pxpm avatar Jun 27 '22 10:06 pxpm

Looking at these 2 files, I did not see temporarySignedUrl()

Only temporaryUrl()

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Filesystem/FilesystemAdapter.php

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Filesystem/AwsS3V3Adapter.php

I think temporarySignedUrl() is for normal laravel routes

https://laravel.com/docs/9.x/urls#signed-urls

Also see:

https://laravel.com/docs/9.x/filesystem#temporary-urls

ziming avatar Jun 27 '22 10:06 ziming

Looking at these 2 files, I did not see temporarySignedUrl()

Only temporaryUrl()

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Filesystem/FilesystemAdapter.php

https://github.com/laravel/framework/blob/9.x/src/Illuminate/Filesystem/AwsS3V3Adapter.php

I think temporarySignedUrl() is for normal laravel routes

https://laravel.com/docs/9.x/urls#signed-urls

Indeed you are right. 👍

Thanks for clarification, I will have a look at this later this week.

🥂

pxpm avatar Jun 27 '22 10:06 pxpm

I think we can call it 'temporary' instead of 'private_s3_bucket' to be consistent with how the upload field names it.

https://github.com/Laravel-Backpack/CRUD/blob/v5/src/resources/views/crud/fields/upload.blade.php

ziming avatar Jul 02 '22 15:07 ziming

I just changed the parameter to temporary

also added an (int) cast before passing into now()->addMinutes()

this way people can do temporary => true (recommended) or temporary => 2

The former will make it 1 minute and the later 2 minutes in case it is some rare huge image file or slow internet connection

ziming avatar Jul 15 '22 03:07 ziming

Hi @ziming

Thanks for this PR, really simple, clean and useful. As we decided with @tabacitu we separate parameter to active temporary URL and to add expiration time (in minutes), so this was modify on this PR: https://github.com/Laravel-Backpack/CRUD/pull/4879 will be check and merge.

Thanks again.

jcastroa87 avatar Jan 04 '23 15:01 jcastroa87

We will continue this PR on this link: https://github.com/Laravel-Backpack/CRUD/pull/4879

Thanks

jcastroa87 avatar Jan 04 '23 21:01 jcastroa87