Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What do you mean by "append using the driver's capabilities"? #74

Open
stratboy opened this issue Jan 21, 2024 · 2 comments
Open

What do you mean by "append using the driver's capabilities"? #74

stratboy opened this issue Jan 21, 2024 · 2 comments

Comments

@stratboy
Copy link

Hi, in your source code I find this comment:

      Laravel's local disk implementation is quite inefficient for appending data to existing files
      To be at least a bit more efficient, we build the final content ourselves, but the most efficient
      Way to do this would be to append using the driver's capabilities

But what exactly do you mean? Could you show an example?
I'm using AS3, thus the League\Flysystem\AwsS3V3\AwsS3V3Adapter, but it doesn't feature any 'append' method or the like.

Thank you

@Sopamo
Copy link
Owner

Sopamo commented Jan 22, 2024

I think that's exactly the reason why Laravel doesn't generally use a "native" append method, because not all drivers (storage providers) allow you to append data. I don't know if AWS doesnt allow it or if it's just not implemented in the driver, but I do know that some providers have a way to append data.
The idea was to at some point to add specific code here to check which driver is being used, if it has an append method and use it if available. I would be open to PRs for that, but I think it needs a bit of refactoring so if someone wants to take that on, it might be a good idea to open an issue first and discuss a possible way on how to implement it exactly.

@stratboy
Copy link
Author

stratboy commented Jan 23, 2024

I think that's exactly the reason why Laravel doesn't generally use a "native" append method, because not all drivers (storage providers) allow you to append data. I don't know if AWS doesnt allow it or if it's just not implemented in the driver, but I do know that some providers have a way to append data. The idea was to at some point to add specific code here to check which driver is being used, if it has an append method and use it if available. I would be open to PRs for that, but I think it needs a bit of refactoring so if someone wants to take that on, it might be a good idea to open an issue first and discuss a possible way on how to implement it exactly.

Mmm ok, thank you. If I find some time maybe I'll look into it.
In the meantime, I wanted to suggest: why not use original filepond's headers instead of the equivalents server fields in chunk()? I mean:

$offset = $request->server('HTTP_UPLOAD_OFFSET');
$length = $request->server('HTTP_UPLOAD_LENGTH');

could be written as

$length = $request->header('Upload-Length'); // total size
$offset = $request->header('Upload-Offset');

The above headers are documented in filepond's docs, so they maybe generate less confusion if a coder take a look at this source code. Also, maybe we could name $length as $total_size instead, but this I admit is more a personal preference. :)

What do you think?

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

No branches or pull requests

2 participants