Skip to content

Conversation

kha333n
Copy link

@kha333n kha333n commented Aug 5, 2025

This PR adds optional support for native copyObject when using R2-compatible S3 disks.

  • Added force_server_copy variable in disk configuration define is that R2 disk and use copyObject.
  • If the disk supports getClient and getBucket, the library will use native S3 copyObject.
  • Falls back to default copy method if unavailable.

This improves performance and avoids unnecessary file downloads/uploads.

@freekmurze
Copy link
Member

Instead of letting the users hard code this via config, it's better to detect the type of disk in shouldCopyFileOnDisk and enable if when needed.

@kha333n
Copy link
Author

kha333n commented Aug 19, 2025

Your point about detecting the disk type in shouldCopyFileOnDisk instead of hardcoding it in the config is valid.

However, the challenge is that Cloudflare R2 provides an S3-compatible API, so users typically use the same S3 disk configuration and SDK for both S3 and R2.

This makes it difficult to distinguish between them programmatically.

Given this, how can we reliably detect whether the storage is S3 or R2?

In my case, I’ve added a new disk named r2 in the configuration, but this won’t always be the case, as users may not define a separate disk for R2.

This leaves us with two options:

  1. Add a configuration option in the config file that users can set to explicitly indicate whether they’re using R2.
  2. Update the documentation to guide users to create a new disk configuration named r2 in the filesystems.php config file if they are using R2.

In my opinion, the first option is preferable because it provides a clearer and more flexible solution, which is why I chose it.

@kha333n
Copy link
Author

kha333n commented Aug 19, 2025

If you have some better idea in mind, you are welcomed to guide me and i'll update it. @freekmurze

@freekmurze
Copy link
Member

Maybe instead of adding a global option in our config file, let's make it work by letting people adding a configuration option at the configured disk itself, so we can read there.

A bit similar of how we ask people to ask to add dump configuration on the db connecting itself: https://spatie.be/docs/laravel-backup/v9/installation-and-setup#content-skipssl-in-mysqlmariadb-database-connection

@kha333n
Copy link
Author

kha333n commented Aug 26, 2025

@freekmurze That's a good point. I'll update it.

And thanks for the guidance.

@freekmurze
Copy link
Member

That looks much better 👍

Could you remove the unrelated styling changes to this PR only has the real changes for the functionality?
Could you also add tests?

@kha333n
Copy link
Author

kha333n commented Sep 8, 2025

@freekmurze Done.


function callProtected(object $obj, string $method, array $args = [])
{
$ref = new ReflectionClass($obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could require-dev our own spatie/invade package to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants