laravel-azure-storage icon indicating copy to clipboard operation
laravel-azure-storage copied to clipboard

(refactor/bugfix) remove prefix from getUrl method

Open CodeWrap opened this issue 3 years ago • 1 comments
trafficstars

Hi,

since the prefix is now appended to the URL in the Laravel framework, we no longer need to append the prefix ourselves in the getUrl method.

See https://github.com/laravel/framework/commit/7cb6dbfae0420d388d9a2b5a7a068d2185ad308b

Best regards

CodeWrap avatar Oct 01 '22 10:10 CodeWrap

Thanks for this PR.

It looks like this breaks one of the tests:

vendor/bin/pest

   PASS  Tests\Unit\AzureBlobStorageAdapterTest
  ✓ it correctly generates the file URL
  ✓ it supports the URL method
  ✓ it supports preceding slash
  ✓ it supports custom URL
  ✓ it supports custom URL with root container
  ✓ it supports temporary URL
  ✓ it handles invalid custom URL
  ✓ it handles custom prefix

   FAIL  Tests\Unit\ServiceProviderTest
  ✓ it sets up the storage correctly
  ✓ it sets up the config correctly
  ✓ it handles custom blob endpoints
  ✓ it can set a custom URL to override the endpoint
  ✓ it resolves the Azure client
  ✓ it sets up the retry middleware
  ⨯ it includes the prefix in the return URL

  ---

  • Tests\Unit\ServiceProviderTest > it includes the prefix in the return URL
  Failed asserting that two strings are equal.

  at tests/Unit/ServiceProviderTest.php:86
     82▕     $this->app['config']->set('filesystems.disks.azure.endpoint', $endpoint);
     83▕     $this->app['config']->set('filesystems.disks.azure.url', $customUrl);
     84▕     $this->app['config']->set('filesystems.disks.azure.prefix', $prefix);
     85▕ 
  ➜  86▕     $this->assertEquals("$customUrl/$container/$prefix/a.txt", Storage::url('a.txt'));
     87▕ });
     88▕ 

  --- Expected
  +++ Actual
  @@ @@
  -'http://cdn.com/MY_AZURE_STORAGE_CONTAINER/my_prefix/a.txt'
  +'http://cdn.com/MY_AZURE_STORAGE_CONTAINER/a.txt'

  Tests:  1 failed, 14 passed
  Time:   0.07s

Also, Psalm flags the following issue, which appears to be related:

vendor/bin/psalm
Target PHP version: 8.1 (inferred from current PHP version)
Scanning files...
Analyzing files...

░░

ERROR: TooManyArguments - src/AzureStorageServiceProvider.php:49:24 - Too many arguments for Matthewbdaly\LaravelAzureStorage\AzureBlobStorageAdapter::__construct - expecting 4 but saw 5 (see https://psalm.dev/026)
            $adapter = new AzureBlobStorageAdapter(
                $client,
                (string)$config['container'],
                isset($config['key']) ? (string)$config['key'] : null,
                isset($config['url']) ? (string)$config['url'] : null,
                isset($config['prefix']) ? (string)$config['prefix'] : '',
            );


------------------------------
1 errors found
------------------------------

Checks took 2.14 seconds and used 1,153.723MB of memory
Psalm was able to infer types for 100% of the codebase

If you could resolve these issues, I'll be happy to pull in the changes. If you could update the README accordingly to reflect the changes, that would be great too.

Given this is a potentially breaking change, I'm inclined to think it would be a good idea if I were to create a new 3.0 release for this.

matthewbdaly avatar Oct 01 '22 11:10 matthewbdaly

Closed due to not hearing back

matthewbdaly avatar Apr 12 '23 11:04 matthewbdaly