dinandmentink / nova-markdown

Adds a markdown editor component to laravel nova.
MIT License
36 stars 4 forks source link

Make the "visibility" option for uploaded images configurable. #24

Closed sanderkadastik closed 2 years ago

sanderkadastik commented 2 years ago

For images on a "s3" disk to be accessible via url, the "visibility" option should be set to "public". (Reference)

Now the images upload should work for the environments with an "s3" storage disk out of the box as well

dinandmentink commented 2 years ago

Thanks for the PR.

The behaviour you are adding is already available by configuring the disk. For example, this is the disk I am using. Note the 'visibility' => 'public', line at the end.

'public' => [
    'driver' => 's3',
    'key' => env('PUBLIC_DISK_ACCESS_KEY_ID'),
    'secret' => env('PUBLIC_DISK_SECRET_ACCESS_KEY'),
    'region' => env('PUBLIC_DISK_DEFAULT_REGION'),
    'bucket' => env('PUBLIC_DISK_BUCKET'),
    'url' => env('PUBLIC_DISK_URL'),
    'endpoint' => env('PUBLIC_DISK_ENDPOINT'),
    'use_path_style_endpoint' => env('PUBLIC_DISK_USE_PATH_STYLE_ENDPOINT', false),
    'visibility' => 'public',
],

For your usecase, where I assume you are using one s3 bucket for public and private files and set visibility on some of them you could:

's3-public' => [
    'driver' => 's3',
    'key' => env('AWS_ACCESS_KEY_ID'),
    'secret' => env('AWS_SECRET_ACCESS_KEY'),
    'region' => env('AWS_DEFAULT_REGION'),
    'bucket' => env('AWS_BUCKET'),
    'url' => env('AWS_URL'),
    'endpoint' => env('AWS_ENDPOINT'),
    'use_path_style_endpoint' => env('AWS_USE_PATH_STYLE_ENDPOINT', false),
    'throw' => false,
    'visibility' => 'public',
],

And then set nova-markdown.disk to s3-public. Note how you are creating a separate disk but with the same credentials as the s3 disk. The only difference is the visibility which is set to public. I myself would prefer having 2 separate s3 buckets for this, but that's just a preference.

So it's already possible to use separate buckets for public/private files with nova markdown and it's already possible to have the same bucket for public/private files, just by setting the nova-markdown.disk setting. Given that this is already possible by setting the value on the disk, I would rather not deal with this in the controller and config separately.

sanderkadastik commented 2 years ago

The idea behind this was to remove the configuration step for disks, because the way that the uploaded images are used, they should be publicly accessible anyway. In my opinion it would just give a slightly better out-of-the-box experience.

But I hear you! Thank you for your great work and suggestions! :)

dinandmentink commented 2 years ago

Ah. Laravel changed the default to be private for s3 buckets with v9 (probably because the flysystem dependency was updated, not sure). So I do see how it would make sense to change this.

The default config for nova-markdown.disk is public, which should still work out of the box with default configuration. Let me consider it a bit more tomorrow. I'll at least add a note to the readme to explain a disk with visibility true should be set.

dinandmentink commented 2 years ago

@sanderkadastik could you please reopen the pull request for a bit? I haven't been able to thoroughly look at it, but it's worthwhile to still have a look at. I'll probably have time somewhere coming week.

Edit: just found the reopen button :man_facepalming:. Reopening it for now.

dinandmentink commented 2 years ago

Thanks for the pr @sanderkadastik. After merging I'll add a comment to the readme and release it as v4.1.0.

Note. I do agree that allowing to set this option is good, because laravel docs are a bit ambiguous on whether the 'visibility' => 'public' option should work on s3 type filesystems. Given the nova-markdown usecase defaulting to true makes sense.