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

[9.x] Add S3 temporaryUploadUrl method to AwsS3V3Adapter #45753

Merged
merged 7 commits into from
Feb 10, 2023
Merged

[9.x] Add S3 temporaryUploadUrl method to AwsS3V3Adapter #45753

merged 7 commits into from
Feb 10, 2023

Conversation

aneeskhan47
Copy link
Contributor

@aneeskhan47 aneeskhan47 commented Jan 21, 2023

Hi,

This method temporaryUploadUrl is helpful for getting presign upload URL from S3 for upload. useful for js uploaders e.g filepondetc. so they can upload to this URL directly.

we just have to replace the GetObject command with PutObject.

Usage:

Storage:disk('s3')->temporaryUploadUrl('path/to/file.pdf', now()->addMinutes(5));

@aneeskhan47 aneeskhan47 changed the title [9.x] Add temporaryUploadUrl method to AwsS3V3Adapter [9.x] Add S3 temporaryUploadUrl method to AwsS3V3Adapter Jan 21, 2023
@ankurk91
Copy link
Contributor

This is going to be a huge lifesaver.
Specially in serverless cloud

// have full control over the base path for this filesystem's generated URLs.
if (isset($this->config['temporary_url'])) {
$uri = $this->replaceBaseUrl($uri, $this->config['temporary_url']);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this block of code actually needed for temporary upload URLs? Why or why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we all know, temporary_url option of disks config in config/filesystems.php file is used to override the host/base url of the generated temporary URLs.

temporaryUrl() already included this logic of replaceBaseUrl(), So I included it too in my proposed temporaryUploadUrl() method.

As you can see, temporaryUploadUrl() generated a URL with my custom host i.e. 'example.com' since I set it up in the temporary_url config.

image

@aneeskhan47
Copy link
Contributor Author

Pr for doc: laravel/docs#8480

@taylorotwell
Copy link
Member

I'm not sure this is totally complete. Depending on the options you pass, you may also need to get the required headers that should be included in the upload request (via the getHeaders() method of the singed request). That is how Vapor's temporary upload URL functionality works.

@aneeskhan47
Copy link
Contributor Author

aneeskhan47 commented Jan 27, 2023

Well I'm not sure about how vapor temporary upload urls work. But using this I was able to upload files from files uploaders like Uppy or dropzone etc. that can directly upload to s3. Meaning no file request comes to my server.

@aneeskhan47
Copy link
Contributor Author

Can you clarify the required headers you are talking about. I can complete this pr code if required ☺️

@taylorotwell
Copy link
Member

Sure - you can see the controller we use to request signed upload URLs here: https://github.com/laravel/vapor-core/blob/2.0/src/Http/Controllers/SignedStorageUrlController.php

@aneesdev
Copy link

Thanks, I'll look into it and complete the pr asap 😄

@aneeskhan47
Copy link
Contributor Author

@taylorotwell sorry for the delay. But I recently unfortunately caught a fever. For the mean time you can draft this PR. Once I get better. I'll complete it 😊.

Just wanted to let you know.

@driesvints driesvints marked this pull request as draft January 30, 2023 09:53
@driesvints
Copy link
Member

@aneeskhan47 get well soon!

@aneeskhan47
Copy link
Contributor Author

@taylorotwell I have added the bool requiredHeaders option. if passed. an array will be returned with url, headers. else just the string url.

@aneeskhan47 aneeskhan47 marked this pull request as ready for review January 30, 2023 16:11
@ankurk91
Copy link
Contributor

I think headers can always be included and this method can return array always.

@aneesdev
Copy link

I think headers can always be included and this method can return array always.

You are correct about this. but lets see what taylor thinks.

@aneeskhan47
Copy link
Contributor Author

@taylorotwell since Laravel v10 is coming in feb. how about this should be on 10.x branch instead of v9.x?

@aneeskhan47
Copy link
Contributor Author

it's been a long day without my friend and I'll tell you about it when I see you again 👀.

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.

5 participants