Add missing resolveUri method to FlysystemStorage

[sovetski]

It resolves a lot of problems related to Flysystem.

Without this PR, if we are using S3, Cloudflare images or other cloud storage, the public url is not the CDN one but a local url which is incorrect.

For the people who will suggest using uri_prefix, as I mentionned here: https://github.com/dustin10/VichUploaderBundle/issues/1434#issuecomment-2041172155 In my case it is impossible to add uri_prefix manually, because in Cloudflare, the file name is not at the end, it is something like https://imagedelivery.net/qsdazd54azd98a4z69d5/image3.jpg/public the /public at the end makes it impossible because it is not just a prefix, we have to do some sprintf etc.

The problem existe since more than 8 years, I think it is time to automatise it with this PR.

I hope that it will be ok for maintainers.

Related:

https://github.com/dustin10/VichUploaderBundle/issues/1418 https://github.com/dustin10/VichUploaderBundle/issues/1434 https://github.com/dustin10/VichUploaderBundle/issues/502 https://github.com/dustin10/VichUploaderBundle/issues/1089 https://github.com/dustin10/VichUploaderBundle/issues/539

[sovetski]

@garak should be fine now

[alcohol]

This change broke our whole implementation. Why was this not done in a major release?

We're using flysystem as our storage backend.

Before this change, everything resolved fine, e.g. to a URI like:

https://bucket.ams3.digitaloceanspaces.com/media/cache/filter/subdir/c5b74a86da9c6a4e4c7132a2be83d3f43c9cade7.jpeg

After this change the following was resolved:

https://bucket.ams3.digitaloceanspaces.com/media/cache/filter/https---bucket.ams3.digitaloceanspaces.com/subdir/4ea7859db76e4befaf27ad97864ac36b4976e29d.jpg

We are using the helper to get the path, and then attempt to read it using our flysystem cache adapter, e.g.

$path = $this->helper->asset($entity);
$content = $this->defaultStorage->read($path);

[sovetski]

Hi @alcohol, what does your helper exactly? Can you put the code here? The problem can also be related to your custom adapter, do you have all required methods implemented? If you can provide your code here, it will be easier to debug it and see the problem.

[alcohol]

The helper is simply an instance of Vich\UploaderBundle\Templating\Helper\UploaderHelper.

Instead of returning a path it is now returning a full uri.

[sovetski]

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

[alcohol]

Our cached S3/Flysystem adapter

```php getCacheItem($path); if ($item->isHit()) { return $item->get() instanceof FileAttributes; } if ( ! $this->adapter->fileExists($path)) { return false; } $item->set(new FileAttributes(path: $path)); $this->saveCacheItem($item); return true; } public function directoryExists(string $path): bool { $item = $this->getCacheItem($path); if ($item->isHit()) { return $item->get() instanceof DirectoryAttributes; } if ( ! $this->adapter->directoryExists($path)) { return false; } $item->set(new DirectoryAttributes(path: $path)); $this->saveCacheItem($item); return true; } public function write(string $path, string $contents, Config $config): void { $this->adapter->write($path, $contents, $config); $this->addCacheEntry($path, new FileAttributes($path)); } public function writeStream(string $path, $contents, Config $config): void { $this->adapter->writeStream($path, $contents, $config); $this->addCacheEntry($path, new FileAttributes($path)); } public function read(string $path): string { try { $contents = $this->adapter->read($path); } catch (UnableToReadFile $e) { $this->purgeCacheItem($path); throw $e; } $item = $this->getCacheItem($path); if ( ! $item->isHit()) { $fileAttributes = new FileAttributes(path: $path); $item->set($fileAttributes); $this->saveCacheItem($item); } return $contents; } public function readStream(string $path) { try { $resource = $this->adapter->readStream($path); } catch (UnableToReadFile $e) { $this->purgeCacheItem($path); throw $e; } $item = $this->getCacheItem($path); if ( ! $item->isHit()) { $fileAttributes = new FileAttributes(path: $path); $item->set($fileAttributes); $this->saveCacheItem($item); } return $resource; } public function delete(string $path): void { try { $this->adapter->delete($path); } finally { $this->purgeCacheItem($path); } } public function deleteDirectory(string $path): void { try { foreach ($this->adapter->listContents($path, true) as $storageAttributes) { /** @var StorageAttributes $storageAttributes */ $this->purgeCacheItem($storageAttributes->path()); } $this->adapter->deleteDirectory($path); } finally { $this->purgeCacheItem($path); } } public function createDirectory(string $path, Config $config): void { $this->adapter->createDirectory($path, $config); $this->addCacheEntry($path, new DirectoryAttributes(path: $path)); } public function setVisibility(string $path, string $visibility): void { $this->adapter->setVisibility($path, $visibility); $item = $this->getCacheItem($path); if ($item->isHit()) { $currentAttributes = $item->get(); if ($currentAttributes instanceof FileAttributes) { $additionalAttributes = new FileAttributes(path: $path, visibility: $visibility); } elseif ($currentAttributes instanceof DirectoryAttributes) { $additionalAttributes = new DirectoryAttributes(path: $path, visibility: $visibility); } else { throw new RuntimeException('Cached item is not a file or a directory.'); } $item->set($this->mergeAttributes($currentAttributes, $additionalAttributes)); $this->saveCacheItem($item); } // We cannot create the cache item if it does not exist since we don't know if it's a file or a directory. } public function visibility(string $path): FileAttributes { try { return $this->getFileAttributes( path: $path, loader: fn (): FileAttributes => $this->adapter->visibility($path), accessor: static fn (FileAttributes $fileAttributes): ?string => $fileAttributes->visibility(), ); } catch (RuntimeException $previous) { throw UnableToRetrieveMetadata::visibility($path, $previous->getMessage(), $previous); } } public function mimeType(string $path): FileAttributes { try { return $this->getFileAttributes( path: $path, loader: fn (): FileAttributes => $this->adapter->mimeType($path), accessor: static fn (FileAttributes $fileAttributes): ?string => $fileAttributes->mimeType(), ); } catch (RuntimeException $previous) { throw UnableToRetrieveMetadata::mimeType($path, $previous->getMessage(), $previous); } } public function lastModified(string $path): FileAttributes { try { return $this->getFileAttributes( path: $path, loader: fn (): FileAttributes => $this->adapter->lastModified($path), accessor: static fn (FileAttributes $fileAttributes): ?int => $fileAttributes->lastModified(), ); } catch (RuntimeException $previous) { throw UnableToRetrieveMetadata::lastModified($path, $previous->getMessage(), $previous); } } public function fileSize(string $path): FileAttributes { try { return $this->getFileAttributes( path: $path, loader: fn (): FileAttributes => $this->adapter->fileSize($path), accessor: static fn (FileAttributes $fileAttributes): ?int => $fileAttributes->fileSize(), ); } catch (RuntimeException $previous) { throw UnableToRetrieveMetadata::fileSize($path, $previous->getMessage(), $previous); } } public function checksum(string $path, Config $config): string { $algo = $config->get('checksum_algo'); $metadataKey = isset($algo) ? 'checksum_'.$algo : 'checksum'; $attributeAccessor = function (StorageAttributes $storageAttributes) use ($metadataKey) { if (is_a($this->adapter, 'League\Flysystem\AwsS3V3\AwsS3V3Adapter')) { // Special optimization for AWS S3, but won't break if adapter not installed $etag = $storageAttributes->extraMetadata()['ETag'] ?? null; if (isset($etag)) { $checksum = trim($etag, '" '); } } return $checksum ?? $storageAttributes->extraMetadata()[$metadataKey] ?? null; }; try { $fileAttributes = $this->getFileAttributes( path: $path, loader: function () use ($path, $config, $metadataKey): FileAttributes { // This part is "mirrored" from FileSystem class to provide the fallback mechanism and be able to cache the result try { if ( ! $this->adapter instanceof ChecksumProvider) { throw new ChecksumAlgoIsNotSupported(); } $checksum = $this->adapter->checksum($path, $config); } catch (ChecksumAlgoIsNotSupported) { $checksum = $this->calculateChecksumFromStream($path, $config); } return new FileAttributes(path: $path, extraMetadata: [$metadataKey => $checksum]); }, accessor: $attributeAccessor ); } catch (RuntimeException $e) { throw new UnableToProvideChecksum($e->getMessage(), $path, $e); } return $attributeAccessor($fileAttributes); } public function listContents(string $path, bool $deep): iterable { foreach ($this->adapter->listContents($path, $deep) as $storageAttributes) { $item = $this->getCacheItem($storageAttributes->path()); if ($item->isHit()) { $cachedStorageAttributes = $item->get(); if ($cachedStorageAttributes instanceof FileAttributes && $storageAttributes instanceof FileAttributes) { $cachedStorageAttributes = $this->mergeAttributes(current: $cachedStorageAttributes, additional: $storageAttributes); } elseif ($cachedStorageAttributes instanceof DirectoryAttributes && $storageAttributes instanceof DirectoryAttributes) { $cachedStorageAttributes = $this->mergeAttributes(current: $cachedStorageAttributes, additional: $storageAttributes); } } else { $cachedStorageAttributes = null; } $item->set($cachedStorageAttributes ?? $storageAttributes); $this->saveCacheItem($item); yield $storageAttributes; } } public function move(string $source, string $destination, Config $config): void { try { $this->adapter->move($source, $destination, $config); } catch (UnableToMoveFile $e) { $this->purgeCacheItem($source); $this->purgeCacheItem($destination); throw $e; } $itemSource = $this->getCacheItem($source); $itemDestination = $this->getCacheItem($destination); if ($itemSource->isHit()) { /** @var StorageAttributes $sourceStorageAttributes */ $sourceStorageAttributes = $itemSource->get(); $destinationStorageAttributes = $sourceStorageAttributes->withPath($destination); $this->deleteCacheItem($itemSource); } else { $destinationStorageAttributes = new FileAttributes(path: $destination); } $itemDestination->set($destinationStorageAttributes); $this->saveCacheItem($itemDestination); } public function copy(string $source, string $destination, Config $config): void { try { $this->adapter->copy($source, $destination, $config); } catch (UnableToCopyFile $e) { $this->purgeCacheItem($source); $this->purgeCacheItem($destination); throw $e; } $itemSource = $this->getCacheItem($source); $itemDestination = $this->getCacheItem($destination); if ($itemSource->isHit()) { /** @var StorageAttributes $sourceStorageAttributes */ $sourceStorageAttributes = $itemSource->get(); $destinationStorageAttributes = $sourceStorageAttributes->withPath($destination); } else { $destinationStorageAttributes = new FileAttributes(path: $destination); } $itemDestination->set($destinationStorageAttributes); $this->saveCacheItem($itemDestination); } public function publicUrl(string $path, Config $config): string { if ( ! $this->adapter instanceof PublicUrlGenerator) { throw new RuntimeException('The adapter does not support public URLs.'); } return $this->adapter->publicUrl($path, $config); } public function temporaryUrl(string $path, DateTimeInterface $expiresAt, Config $config): string { if ( ! $this->adapter instanceof TemporaryUrlGenerator) { throw new RuntimeException('The adapter does not support temporary URLs.'); } return $this->adapter->temporaryUrl($path, $expiresAt, $config); } } ```

[alcohol]

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

It is not my helper, it is vendor code from this library. Previously we were getting $mapping->getUriPrefix().'/'.$path;, now we are getting the $fs->publicUrl($path);

[sovetski]

@alcohol if in your helper, you change the "resolveUri" by "resolvePath" does it work?

It is not my helper, it is vendor code from this library. Previously we were getting $mapping->getUriPrefix().'/'.$path;, now we are getting the $fs->publicUrl($path);

As I am not able to do it at the moment, you can try to edit the vendor's code just to see what happens and put the original code back? I am not using S3 or this helper, that is why I need more details from your side if possible, to be able to find a fix

[alcohol]

The behaviour documented here has changed if you use the FlysystemStorage implementation: https://github.com/dustin10/VichUploaderBundle/blob/master/docs/generating_urls.md#generating-a-url-in-a-controller

I am not saying this change is bad, but it drastically differs from what is documented and was previously default behaviour. The FlysystemStorage storage was falling back to the resolveUri method from the AbstractStorage implementation. By overriding that method the returned value can be very different suddenly. While this might be considered a "bugfix", I feel such a major change should have been done in a new major version.

[Cryde]

Hello

I had the same issue, I guess bundle like https://github.com/1up-lab/OneupFlysystemBundle/tree/main are not "ready" for this change because I can't see where we can configure public urls (https://flysystem.thephpleague.com/docs/usage/public-urls/) Or maybe I miss something ?

[garak]

Never used flysystem myself, please let me know if I need to revert this change

[sovetski]

@Cryde I don't know if it can help, but for Flysystem, I am using this package and it works as expected: thephpleague/flysystem-bundle, documentation: https://github.com/dustin10/VichUploaderBundle/blob/master/docs/storage/flysystem.md#integrating-with-thephpleagueflysystem-bundle

@garak as mentionned @alcohol, maybe you can put it in a major release instead?

[garak]

I could put it in a major, but a major release is not currently planned

[andyexeter]

This update broke things for us too. We are storing images in S3 using a Cloudfront URL as a uri_prefix. The uri_prefix config is now completely ignored and the S3 URL is returned when using the \Vich\UploaderBundle\Templating\Helper\UploaderHelper::asset method.

[sovetski]

This update broke things for us too. We are storing images in S3 using a Cloudfront URL as a uri_prefix. The uri_prefix config is now completely ignored and the S3 URL is returned when using the \Vich\UploaderBundle\Templating\Helper\UploaderHelper::asset method.

It solves a problem that shouldn't be there in the first place. I'm not opposed to reverting this change, but do you have a better approach to suggest?

Reverting is just a way to put the problem back; I find suggesting an improvement more interesting than wiping everything out at once.

If everyone agrees to test, I can submit a new pull request to fix the issue for existing projects. But again, just because it works with a makeshift solution doesn't mean it should never evolve (in my opinion)

[garak]

What about adding an alternate storage? We need to avoid breaking existing implementations

[andyexeter]

I understand that it solves a problem, but it does so by breaking existing functionality.

Forgive me if I'm misunderstanding, but looking at this code:

https://github.com/dustin10/VichUploaderBundle/blob/fec8c14c0c4ddd7f2bcd4b58fadb1222a7d2b195/src/Storage/FlysystemStorage.php#L121-L125

The code within the try block completely ignores the uri_prefix config, doesn't it? So it breaks for anyone using any uri_prefix?

[sovetski]

@andyexeter yes you are right, it prioritizes the publicUrl instead of uri_prefix. As @garak said, we should avoid to break existing implementations, I will do a PR to revert changes, and maybe in the future it will be implemented as a major change

[sovetski]

@garak the PR is ready to rollback changes: https://github.com/dustin10/VichUploaderBundle/pull/1444