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

Added the ability to specify a custom file name #12

Closed
wants to merge 1 commit into from

Conversation

antolopez
Copy link

If the temporary URL has the filename parameter, it will be used as the file name when downloaded.

Example:

        Storage::disk('local')->temporaryUrl($path, now()->addMinutes(5),  ['filename' => $filename]);

Added the ability to specify a custom file name.
If the temporary URL has the filename parameter, it will be used as the file name when downloaded.
@abrardev99
Copy link
Owner

I don't think getting it from the request class is a good idea. Maybe we can add a route parameter here, which will accept the name through the Storage temporaryUrl function.
I will look into this weekend.

@abrardev99
Copy link
Owner

I looked into the code and I think you can already pass file by without this PR. Example

Storage::disk('local')->temporaryUrl('file.txt', now()->addMinutes(5), ['filename' => 'customname']);

Not just file name, you can pass other options as well (with respect to disk)

[
  'ResponseContentType' => 'application/octet-stream',
  'ResponseContentDisposition' => 'attachment; filename=file2.jpg',
]

@abrardev99 abrardev99 closed this Jun 15, 2024
@antolopez
Copy link
Author

Hi, regarding the fact of using the request to obtain the parameter, there is no problem because the request is signed, including the parameters, so the established parameters cannot be changed after creating the temporary URL on the server.

And as for the last thing you mentioned, the problem is not what can be passed, which, as you said, when creating the link, anything can be passed. The problem is that the package route code ignores any parameters that might have been passed, making it not feasible without overwriting the route.

If the route is not going to be modified, could you at least add a package configuration so that the route file is not included?

Something like this:

` public function configurePackage(Package $package): void
{
$configuredPackage = $package
->name('laravel-local-temporary-url')
->hasConfigFile();

    if (!config('local-temporary-url.ignore_routes'))
        $configuredPackage->hasRoute('routes');
}`

@abrardev99
Copy link
Owner

The route does not ignore any options you pass using

Storage::disk('local')->temporaryUrl('file.txt', now()->addMinutes(5), ['filename' => 'customname']);

Output:
image

See route https://github.com/abrardev99/laravel-local-temporary-url/blob/main/src/LocalTemporaryUrlServiceProvider.php#L25, its accepting options. I tried locally and its accept file finames

@antolopez
Copy link
Author

Hi, regarding the part you mentioned, there is no problem and nothing needs to be modified. The temporary URL generation is perfect.

The problem lies in the route that listens to those URLs, which does not take the parameters into account when generating the download.

https://github.com/abrardev99/laravel-local-temporary-url/blob/main/routes/routes.php

<?php

use Illuminate\Support\Facades\Route;
use Illuminate\Support\Facades\Storage;

foreach (config('local-temporary-url.disk') as $disk) {
    Route::get("$disk/temp/{path}", function (string $path) use ($disk) {
        return Storage::disk($disk)->download($path);
    })
        ->where('path', '.*')
        ->middleware(config('local-temporary-url.middleware'))
        ->name("$disk.temp");
}

That’s why I suggested adding the configuration to the package to ignore this route, allowing it to be customized if you don’t want to modify it, to permit custom names and/or other options for the file download.

@abrardev99
Copy link
Owner

This route is not meant to use "separately" by developers, its being used internally by temporaryUrl method.

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