symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
828 stars 299 forks source link

[LazyImage] Be friendlier with the path passed to data_uri_thumbnail() #342

Closed weaverryan closed 3 months ago

weaverryan commented 2 years ago

Hi!

The main usage of lazy image looks like this

<img
   src="{{ data_uri_thumbnail('build/images/large.jpg', 40, 30) }}"
   {{ stimulus_controller('symfony/ux-lazy-image/lazy-image', {
       src: asset('build/images/large.jpg')
   }) }}
>

The path passed to data_uri_thumbnail() (build/images/large.jpg) is loaded simply via file_get_contents('build/images/large.jpg'). That seems to work for me... though not in my tests... and it feels "shaky" at best to pass in a non-absolute path.

Possible solution: if the path to the file cannot be found, we try again using the "public" path. Basically, something like this:

public function createDataUriThumbnail(string $filename, ...): string
{
    if (!file_exists($filename)) {
        $path = $this->projectDir.'/public/'.$filename;
        if (file_exists($path)) {
            $filename = $path;
        }
    }

    return $this->blurHash->createDataUriThumbnail($filename, $width, $height, $encodingWidth, $encodingHeight);
}

It's a bit ugly, bit it would be much more useful. An alternative would be some sort of general-purpose Twig filter for this:

src="{{ data_uri_thumbnail('build/images/large.jpg'|asset_file_path, 40, 30) }}"

But, I'm not sure I love this, and that seems like something that belongs in Symfony itself.

ToshY commented 2 years ago

hey @weaverryan :wave:

I have couple remarks related to this functionality. First off, to get on the same page, the file_get_contents you mention I assume you mean the following line in the encode method:

$image = $this->imageManager->make(file_get_contents($filename));

First off, if I have to believe the documentation of Internention's Image::make, the usage of file_get_contents seems unnecessary, as make can already handle file paths and URLs (without the need for file_get_contents). That would also mean that the possible solution with file_exists seems really specific for local files, while the current implementation also allowed for URLs to be used. This actually brings me to my next point.

Removing file_get_contents would make the methods generally more useful, as it allows different data strings to be passed, like binary string data, base64 encoded strings and even images (read as string) from external storage (with Flysystem).

Therefor, I think the retrieval of the content should not be handled by the blurhash methods, and instead the content should be passed directly. In terms of twig, that would mean something like this:

{% set imageData = readLocalFile('build/images/large.jpg') %}
<img src="{{ data_uri_thumbnail(imageData, 40, 30) }}

Note: readLocalFile in the above would be a custom Twig extension to read the file either using file_get_contents, Flysystem, or other valid method to get the file content.

Let me know what you think 😄

YetiCGN commented 1 year ago

This is also important for us, because the images passed to that function are assetized. So we currently jump through hoops like this to make it work:

data_uri_thumbnail(asset('build/images/big_one.webp')[1:])
Narthall commented 9 months ago

Same thing for me, using data_uri_thumbnail in conjunction with AssetMapper does not work in dev environment. Even using the solution pointed out by YetiCGN does not work, as it is then looking for instance for the assetized file instead of the one provided by AssetMapper.

This code:

data_uri_thumbnail(asset('images/big_one.webp')[1:])

Throws the following error:

An exception has been thrown during the rendering of a template ("Warning: file_get_contents(assets/images/big_one-1849ff3aa6f75832a97449cb2b58bbaf.webp): Failed to open stream: No such file or directory").

carsonbot commented 3 months ago

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

carsonbot commented 3 months ago

Hello? This issue is about to be closed if nobody replies.

Kocal commented 3 months ago

Closing since #1781 added a way to override the file_get_contents behavior.