matthewbdaly/laravel-azure-storage

getTemporaryUrl does not consider the prefix

rtheunissen opened this issue · 5 comments

I'm using a path prefix in the configuration, but I noticed that getTemporaryUrl only considers the container. I would expect that I could pass a relative path consistent to put and get to generate a URL to those files.

IIRC these came from separate pull requests.

I don't know when I will have the scope to look at this as right now I don't have anything I'm using this on and my Azure knowledge is a little rusty. If you want to have a go at a pull request I'm happy to consider it.

My only concern is that this would break BC for anyone who currently adds the prefix manually, as we are doing. We could strip the prefix from the path before we prepend it, but that's also not great when prefix/prefix is the intended path. How do you feel about breaking backward-compatibility to fix this? It's definitely a bug IMO:

public function getAzurePublicUrl(array $config, string $path, string $disk): string
{
    return Storage::disk($disk)->temporaryUrl(
        rtrim($config['prefix'] ?? '', '/') . '/' . $path,
        $expires = Carbon::now()->modify($config['expire'] ?? '+10 minutes'),
        $options = [],
    );
}

I think breaking BC is probably necessary at this point - I can't think of any easier alternative

Excellent I'll work on a PR for that sometime. 👍

Closing as no activity - feel free to reopen in future