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

[8.x] Added ability to define temporary URL macro for storage #40100

Merged
merged 6 commits into from
Dec 20, 2021

Conversation

ash-jc-allen
Copy link
Contributor

Hi! This PR adds the ability for you to add your own getTemporaryUrl() logic for the Storage facade.

At the moment, you can define a getTemporaryUrl() method on a file system adapter and it'll get called. So, this addition mirrors that same approach so that you can add the functionality via a macro.

I think this would be really handy for if you don't have direct access to make changes to the adapter itself. For example, you could use the macro so that you can use temporary URLs when using the local storage disk. Of course, you'd still need to make sure you create a controller to handle the actual request, but I think it could tidy up the code a bit.

As a really basic example, you could have something like this:

Storage::macro('getTemporaryUrl', function ($path, $expiration, $options) {
    $options['path'] = $path;

    return URL::temporarySignedRoute('file.signed', $expiration, $options);
});

So, you can call the method in a much more Laravel-y way:

$url = Storage::temporaryUrl(
    'file.jpg', now()->addMinutes(5)
);

If this is something you might want to pull in, please let me know if there are any changes I need to make 😄

@taylorotwell
Copy link
Member

taylorotwell commented Dec 18, 2021

How does this work across multiple drivers? What I'm using local and s3 in the same application?

@ash-jc-allen
Copy link
Contributor Author

Ahh good point, I hadn't considered that. If you defined the macro, it would have overridden the s3 version.

I've just pushed up a new change that I think should handle this though. I've added a method called buildTemporaryUrlUsing. It acts like a kind of proxy through to the macro method but build a key that is specific to each filesystem driver.

For example, you could now use it like this:

// Define the logic using:

Storage::disk('local')->buildTemporaryUrlUsing(function ($path, $expiration, $options) {
    return 'using local';
});

// Not that you'd want to do this, but just as an example...
Storage::disk('s3')->buildTemporaryUrlUsing(function ($path, $expiration, $options) {
    return 'using s3';
});


// Call the logic using:

$url = Storage::temporaryUrl('file.jpg', now()->addMinutes(5)); // $url is: 'using local';

$url = Storage::disk('s3')->temporaryUrl('file.jpg', now()->addMinutes(5)); // $url is: 'using s3';

Do you think this would be a suitable approach?

@taylorotwell
Copy link
Member

I don't think you need macros at all.

@ash-jc-allen
Copy link
Contributor Author

Yeah, good shout to be fair! I've made some changes now so that it doesn't use macros and holds the closures in a static::$temporaryUrlBuilders array instead.

I also split the else-if out because I thought it was looking a bit bloated with the changes I'd just made, but I'm happy to switch that back if you'd prefer :)

@taylorotwell taylorotwell merged commit a13e74f into laravel:8.x Dec 20, 2021
@ash-jc-allen ash-jc-allen deleted the feature/temp-url-macro branch December 20, 2021 22:48
@ankurk91
Copy link
Contributor

ankurk91 commented Nov 9, 2022

Here is the correct syntax

This should go in AppServiceProvider, boot method.

<?php

use Illuminate\Support\Facades\Storage;

 Storage::disk('local')->buildTemporaryUrlsUsing(function ($path, $expiration, $options) {
            return $path.'?expires='.$expiration;
        });

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.

3 participants