search

jrrdnx / craft-cloudflare-r2

MIT License
6 stars 3 forks source link

Prevent calls to `GetObjectAcl ` when copying and moving files #7

Closed FreekVR closed 1 year ago

FreekVR commented 1 year ago

Fixes #6 by marking all objects from R2 as "private" by default, rather than calling GetObjectAcl to determine the public/private status of an object when calling either copy() or move() (and possibly in some other places)

See https://developers.cloudflare.com/r2/api/s3/api/#unimplemented-object-level-operations

jrrdnx commented 1 year ago

Thanks for this @FreekVR! Sorry I hadn't gotten a chance to look into this yet, so this is much appreciated.

One thing, I'd like to update the existing visiblity() method in src/Fs.php:458 in lieu of creating the separate CloudflareR2Adapater.php, just to keep things simplified. If you can make that change I'll get this merged in!

FreekVR commented 1 year ago

@jrrdnx having the separate adapter is necessary I'm afraid as the visibility method of the Flysystem S3 adapter is the one that's calling the ACL method:

https://github.com/thephpleague/flysystem-aws-s3-v3/blob/3.x/AwsS3V3Adapter.php#L268 called here: https://github.com/thephpleague/flysystem-aws-s3-v3/blob/3.x/AwsS3V3Adapter.php#L419

If you had something else in mind though let me know, maybe I misunderstood :)

jrrdnx commented 1 year ago

@FreekVR Interesting. I don't have a chance to test at the moment, but I figured setting https://github.com/jrrdnx/craft-cloudflare-r2/blob/main/src/Fs.php#L458 to just return Visibility::PRIVATE; would accomplish the same thing, as this is what's being passed as the 4th argument to the AwsS3V3Adapter() on L253.

Not critical, just hoping to avoid creating more files if it's unnecessary. I should get a chance to test it in the next couple days, but if you get a chance first and I'm incorrect let me know.

FreekVR commented 1 year ago

I did try that as the first solution, but the $config argument that would be needed to bypass this API call doesn't seem to be passed by Craft to the Filesystem methods like copy().

I'm guessing the Aws adapter only applies the argument you mentioned to the bucket itself, and still checks if individual files differ in terms of ACL.

On Sat, 2 Sept 2023, 01:55 Jarrod, @.***> wrote:

@FreekVR https://github.com/FreekVR Interesting. I don't have a chance to test at the moment, but I figured setting https://github.com/jrrdnx/craft-cloudflare-r2/blob/main/src/Fs.php#L458 to just return Visibility::PRIVATE; would accomplish the same thing, as this is what's being passed as the 4th argument to the AwsS3V3Adapter() on L253 https://github.com/jrrdnx/craft-cloudflare-r2/blob/main/src/Fs.php#L253.

Not critical, just hoping to avoid creating more files if it's unnecessary. I should get a chance to test it in the next couple days, but if you get a chance first and I'm incorrect let me know.

— Reply to this email directly, view it on GitHub https://github.com/jrrdnx/craft-cloudflare-r2/pull/7#issuecomment-1703531078, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADF5CEHCDXZC5RJO57VRBLXYJYXRANCNFSM6AAAAAA4EMO5VY . You are receiving this because you were mentioned.Message ID: @.***>

jrrdnx commented 1 year ago

Thanks for all your work on this @FreekVR, v1.0.1 was just released!