[11.x] check not empty before concat url

[MrTuffaha]

When using packages that adds a default prefix in filesystem config (example steffjenl/laravel-azure-blob-storage).

The FilesystemAdapter adds a "/" before the path as it only checks if prefix is set not also if its not empty, this may result in a url with double slashes "//".

this pull checks if prefix is not empty before combining it with path

[rodrigopedra]

Likely a breaking change:

<?php

function originalImplementation($url, $path)
{
    return rtrim($url, '/') . '/' . ltrim($path, '/');
}

function proposedImplementation($url, $path)
{
    if (empty($url)) {
        return trim($path, '/');
    }

    if (empty($path)) {
        return trim($url, '/');
    }

    return rtrim($url, '/') . '/' . ltrim($path, '/');
}

$urls = ['', 'https://url.with.slash/', 'https://url.without.slash'];
$paths = ['', '/', 'path', 'path/', '/path', '/path/'];

foreach ($urls as $url) {
    foreach ($paths as $path) {
        echo originalImplementation($url, $path), PHP_EOL;
    }

}

echo str_repeat('-', 80), PHP_EOL;

foreach ($urls as $url) {
    foreach ($paths as $path) {
        echo proposedImplementation($url, $path), PHP_EOL;
    }
}

Output:

$ php test.php 
/
/
/path
/path/
/path
/path/
https://url.with.slash/
https://url.with.slash/
https://url.with.slash/path
https://url.with.slash/path/
https://url.with.slash/path
https://url.with.slash/path/
https://url.without.slash/
https://url.without.slash/
https://url.without.slash/path
https://url.without.slash/path/
https://url.without.slash/path
https://url.without.slash/path/
--------------------------------------------------------------------------------


path
path
path
path
https://url.with.slash
https://url.with.slash/
https://url.with.slash/path
https://url.with.slash/path/
https://url.with.slash/path
https://url.with.slash/path/
https://url.without.slash
https://url.without.slash/
https://url.without.slash/path
https://url.without.slash/path/
https://url.without.slash/path
https://url.without.slash/path/

[MrTuffaha]

@rodrigopedra thanks for your observation, i have since updated the code so that if one parameter is empty it just returns the other parameter.

There is still change in the result when the $url parameter is empty it doesnt prefix the $path with a "/" by default which is the fix i am proposing

Here is the result from my new implementation:

                # empty string here
/
path
path/
/path
/path/
https://url.with.slash/
https://url.with.slash/
https://url.with.slash/path
https://url.with.slash/path/
https://url.with.slash/path
https://url.with.slash/path/
https://url.without.slash
https://url.without.slash/
https://url.without.slash/path
https://url.without.slash/path/
https://url.without.slash/path
https://url.without.slash/path/

[rodrigopedra]

Well, it is still a breaking change, as other people might be relying on this behavior.

I saw on the linked package that the prefix is user configurable, can't you just remove the starting slash on the prefix?

I tested the linked package on a sample project, with one of the sample env's provided on the package's README file, as such:

AZURE_STORAGE_LOCAL_ADDRESS=local
AZURE_STORAGE_NAME=devstoreaccount1
AZURE_STORAGE_KEY=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==
AZURE_STORAGE_CONTAINER=container

And added this key for the prefix:

AZURE_STORAGE_PREFIX=foo

Running this command:

Artisan::command('app:test', function () {
    $this->comment(Storage::disk('azure')->url('bar.txt'));
});

I got this output:

$ php artisan app:test
https://devstoreaccount1.blob.core.windows.net/container/foo/bar.txt

Can you provide the env values that you are using, which end up with a double slash?

You can provide only which AZURE_STORAGE_CONTAINER and AZURE_STORAGE_PREFIX you are using, as the other keys are sensitive.

[rodrigopedra]

Maybe you can ask the package maintainer to wrap the prefix with ltrim() when calling the adapter's constructor, to remove any prefixing slash?

Right here:

https://github.com/steffjenl/laravel-azure-blob-storage/blob/931b3e4e84bdc198560560fcaf420108c23a182a/src/AzureBlobStorageServiceProvider.php#L74

[GrahamCampbell]

Empty is the wrong function here. It does not check if the variable us the empty string, it checks if it looks like an enmpty-ish value, inc if it's 0. One should compare against '' instead.