Vichuploaderbundle: Option 'imagine_pattern' generates a wrong link

[murtukov]

| Q | A
| -------------------------- | -----
| Bug report? | yes
| Feature request? | no
| BC Break report? | no
| Symfony version | 4.0.6
| VichUploaderBundle version | 1.8.2
| LiipImagineBundle version | 2.0.x-dev
| SonataAdminBundle version | 3.33.0

I am using VichUploaderBundle + LiipImagineBundle + SonataAdminBundle.

config/packages/vich_uploader.yaml

vich_uploader:
    db_driver: orm

    mappings:
        user_img:
            uri_prefix:         /img/user
            upload_destination: "%kernel.root_dir%/../public/img/user"
            namer: Vich\UploaderBundle\Naming\HashNamer
            directory_namer: Vich\UploaderBundle\Naming\SubdirDirectoryNamer
            delete_on_update:   true
            delete_on_remove:   true

config/packages/imagine.yaml

liip_imagine:
    resolvers:
        default:
            web_path: ~

    filter_sets:
        cache: ~
        my_thumb:
            quality: 75
            filters:
                thumbnail: { size : [120, 90], mode : outbound }
                background: { size : [122, 92], position : center, color : '#eee' }

If I use this code somewhere in a template:

<img src="{{ vich_uploader_asset(object, 'imageFile') | imagine_filter('my_thumb') }}" />

it works perfectly and generates a correct link to thumbnail, like this:

http://example.com/media/cache/my_thumb/img/user/78/78c737520d6620f766d5a636da9afa13e89bf47f.jpg

However if I use the VichImageType with the option 'imagine_pattern' it generates a link without uri_prefix, like this:

http://example.com/media/cache/my_thumb/78/78c737520d6620f766d5a636da9afa13e89bf47f.jpg

It's the same entity in both cases.
I use the VichImageType in SonataAdminBundle, but since the official documentation says:

When defining fields in your admin classes you can use any of the standard Symfony field types and configure them as you would normally

it should make no difference.

I found the "problem", which was located in the file Vich\UploaderBundle\Form\Type\VichImageType.
If you change the line 64, then everything works fine:

| | |
| --- | --- |
| before | $path = $this->storage->resolvePath($object, $form->getName(), null, true); |
| after| $path = $this->storage->resolveUri($object, $form->getName(), null, true); |

Thanx

[garak]

This was changed in 514425c3115024cd680ee02113918b56cb639ad9 by @ossinkine
Feel free to open a PR

[ossinkine]

@murtukov vich_uploader_asset(object, 'imageFile') returns public path for browser, but imagine_filter('my_thumb') receives local relative path in your data root. So vich_uploader_asset(object, 'imageFile') | imagine_filter('my_thumb') is not make sense and http://example.com/media/cache/my_thumb/img/user/78/78c737520d6620f766d5a636da9afa13e89bf47f.jpg looks right.

[murtukov]

@ossinkine I'm sorry, I accidentally put links in wrong places in the description of this issue, it should be opposite. Corrected it, thanx.

[garak]

Can we close this?

[chapa]

Hi,

I was facing the same problem and I resolved *(see edit ↓) it with this configuration :

vich_uploader:
    mappings:
        product_image:
            uri_prefix: ""
            upload_destination: "%kernel.project_dir%/web/uploads/images/products"

liip_imagine:
    loaders:
        product:
            filesystem:
                data_root: "%kernel.project_dir%/web/uploads/images/products"
    filter_sets:
        product_thumbnail:
            data_loader: product
            filters:
                thumbnail: { size: [350, 150], mode: outbound }

liip_imagine.loaders.product.filesystem.data_root is configured to be se same value as vich_uploader.mappings.product_image.upload_destination, and the product_thumbnail filter for LiipImagine use the data loader defined above.

I also had to set vich_uploader.mappings.product_image.url_prefix to an empty string, to force the vich_uploader_asset twig function to return /my_image.png instead of /uploads/my_image.png which LiipImagine doesn't understand.

With all that, both vich_uploader_asset twig function and VichImageType's thumbnail work.

But I'm not sure it's supposed to work that way, it feels like I'm hacking the system to get what I want.

To go further

I find that AbstractStorage::resolvePath() (used in VichImageType::buildView()) calls $this->doResolvePath() with $mapping->getUploadDir($obj) as second parameter.

This parameter is supposed to be The directory in which the file is uploaded, but PropertyMapping::getUploadDir() doesn't use the uri_prefix configuration, which should be included because it's a part of the relative path of the file.

What do you think ? Is my thinking correct ?

Edit : actually this solution is not completely working, I just realized that the download link is wrong in the twig template (doesn't contains full relative path)

[ossinkine]

@chapa

I also had to set vich_uploader.mappings.product_image.url_prefix to an empty string, to force the vich_uploader_asset twig function to return /my_image.png instead of /uploads/my_image.png which LiipImagine doesn't understand.

vich_uploader_asset function returns absolute path by which the image is available on your website, but not the filesystem path on your webserver. So you should not pass the result of calling vich_uploader_asset to LiipImagine.

This parameter is supposed to be The directory in which the file is uploaded, but PropertyMapping::getUploadDir() doesn't use the uri_prefix configuration, which should be included because it's a part of the relative path of the file.

This parameter is the upload directory path (which you've specified in vich_uploader.mappings.product_image.upload_destination). AbstractStorage::resolvePath() returns relative path in your filesystem relative to PropertyMapping::getUploadDir() and this path is passed to Liip's CacheManager::getBrowserPath. CacheManager can find this file because liip_imagine.loaders.product.filesystem.data_root and vich_uploader.mappings.product_image.upload_destination are the same directory. uri_prefix is not used here, it used only in AbstractStorage::resolveUri.

[c33s]

@garak pointed me here from the issue i opened (#871). i read through this issue but its not clear for me what the conclusio is.

Feel free to open a PR
@garak is still valid? would simply reverting 514425c be an option?

@ossinkine what is the benefit of using resolvePath instead of resolveUri? to me it looks like it causes more trouble than it helps.

as i stated in #871 resolvePath is broken on windows because of the usage of DIRECTORY_SEPERATOR. additionally what i read above reads more like a workaround/hack.

tried the workaround with defining an empty uri_prefix but this does not help. my files are not located in the public web folder, the original/full-size images are in var/data/images and are made accessible through liip imagine with:

liip_imagine:
    loaders:
        default:
            filesystem:
                data_root:
                    - '%kernel.project_dir%/var/data/images'

i would really vote for a revert

[garak]

@c33s a PR to replace DIRECTORY_SEPARATOR would be merged immediately.

[ossinkine]

@c33s we using resolvePath instead of resolveUri because Liip receives path but not uri.
What issue with DIRECTORY_SEPARATOR, it's not clear for me?

[murtukov]

@garak I'm not sure whether this issue was resolved. It's my first issue report so I dont really know how things work here. Maybe I shouldn't close this issue. I'm still using the "hack" that I described in the decription above: replacing the line 64.

[c33s]

@garak and what about reverting the change back to the old behavior? or at least add a config value for this. i haven't found out why this was changed yet. i only can say (assuming the backslash changed to a slash) with the following setup:

• imagine/imagine: v0.7.1
• liip/imagine-bundle: 1.9.1
• vich/uploader-bundle: 1.8.3
• php 7.1
• custom directory namer
• files not in the public accessible web path

it does not work with resolvePath but it instantly works with resolveUri. so what is the benefit of using path? what was broken before? sadly the commit 514425c does not reference a detailed issue description with examples. the PR also contains no test showing the reason for this switch.

we now have at least four people which are having problems with this change:

• @murtukov
• @chapa
• @cadavre (https://github.com/dustin10/VichUploaderBundle/commit/514425c3115024cd680ee02113918b56cb639ad9#commitcomment-27808813)
• @c33s

without this "fix" it simply works out of the box for me. without workarounds or special config (empty url prefix).

@ossinkine but why it don't work when using resolvePath WITH liip imagine and works perfectly by only editing the vendor file to use resolveUri?

@ossinkine have you read my comment in #871? using the backslash often leads that the backslash is passed to something which should be an url. as most of the time linux/mac people writing libs, they don't notice that in windows env a backslash is passed to the url and gets escaped as %5C which leads to urls like that http://example.com/media/cache/resolve/my_thumb/d/0/1%5Cfilename.jpg but windows and php can open a path with forward slashes. so this is seperator is of no real use (expect for easily detecting windows or if you export a path for a file list where a windows user would be confused to see / instead of \)

[garak]

At this point, I'm considering to revert 514425c, since last comment makes sense.
Unless @ossinkine is providing more context, I'll do later today (or, at the latest, tomorrow)

[c33s]

@garak no hurry from my side lots of GDPR work for me currently

[ossinkine]

@c33s As I described in #831 old behavior only works if URI that returned by the Storage equals to relative path for Imagine. In other words, this will work only if Liip data root is web directory. With another data root this will not work because URI is not equal to relative path anymore.

[c33s]

@ossinkine thanks for referencing the issue.

... this will work only if Liip data root is web directory. With another data root this will not work

not entirely true, my images are not in the web root and i am using the symfony4 directory structre where the web directory is named public (if this is relevant).

as liip_imagine supports multiple data_root s it still works this way.

this is my liip_imagine config

liip_imagine:
    loaders:
        default:
            filesystem:
             data_root:
                    - '%kernel.project_dir%/var/data/images'
                    - '%kernel.project_dir%/public'
                    - '%kernel.project_dir%/assets/images'

if the fix is fixing stuff for people a simple option to allow both would be a good thing. or create a good documentation to let developers easily get easyadmin & liip imagine & vich uploader up and running with a complexer setup then putting all the data in the public web dir and with enabled custom namer

[ossinkine]

@c33s I'm not clear what is your issue with new behavior. Could you please provide config for liip_imagine and vich_uploader and shortly describe your issue one more time.
For about good documentation, obviously it is necessary while there are similar issues.

[ossinkine]

@murtukov Could you please try to fix your issue by specifying correct config for liip_imagine and vich_uploader and close this issue or describe why don't.

[c33s]

@ossinkine murtukov already closed this issue as fixed, see: https://github.com/dustin10/VichUploaderBundle/issues/858#issuecomment-377723012

the problem is, that the image uri used in the image src below the file upload form ist incorrect. which is correct if i change $this->storage->resolvePath to $this->storage->resolveUri

my config:

liip_imagine:
    driver: "gd"
    resolvers:
        default:
            web_path:
                web_root: '%kernel.project_dir%/public'
                cache_prefix: 'cache/images'
    loaders:
        default:
            filesystem:
                data_root:
                    - '%kernel.project_dir%/var/data/images'
                    - '%kernel.project_dir%/public'
---
vich_uploader:
    db_driver:                  orm
    mappings:
        stamp_image:
            uri_prefix:         ''
            upload_destination: '%kernel.project_dir%/var/data/images/stamps'
            inject_on_load:     true
            directory_namer:
                service:        App\Namer\Vich\MyDirectoryNamer
---             
easy_admin:
    entities:
        Stamp:
            class: App\Entity\Stamp
            form:
                fields:
                    ...
                    - { property: 'imageFile', type: 'vich_image', type_options: { allow_delete: false, image_uri: true, imagine_pattern: 'stamp_thumb' }}
#                    - { property: 'imageFile', type: 'vich_image', type_options: { imagine_pattern: 'stamp_thumb' }}
#                    - { property: 'imageFile', type: 'vich_image' }
                    ...

i will do some tests with different settings and post the result here as soon as i have time for it.

[ossinkine]

@c33s
Also @murtukov said he doesn't sure the issue was resolved.

Your example does not work because you specify different directories as data root for liip_imagine and vich_uploader. Specifying vich_uploader.mappings.stamp_image.upload_destination to %kernel.project_dir%/var/data/images should fix the issue. Also you can add %kernel.project_dir%/var/data/images/stamps to liip loader data root.
Replacing resolvePath with resolveUri looks like a fix because you specified empty uri_prefix and added %kernel.project_dir%/public as liip loader data root, but this is a hack.

[c33s]

@ossinkine you are right. looks like i haven't cleared the cache as i tried this variant of settings (i tried many variants -.-). thank you very much for your time & help.
as soon i have time, i will try to put together a small doc for that. there are really many settings which have to play nicely together.

can someone of you confirm the bug in easyadmin https://github.com/EasyCorp/EasyAdminBundle/issues/2240? the solution @ossinkine told me, only works if i use the image url but this leads that the thumbnail is the same image (size) as the lightbox image.

edit: @garak the comment from ossinkine fixed it for me and looks like a real solution and not a workaround.

edit2: what about the directory_seperator? i still see this as risky. a PR simply replacing them with / is quite easy but should we make a config for that?

[garak]

@c33s thanks for your time, I'll be happy if we manage to find a shared solution, and adding something to docs could be a big plus (saving other users to look into issues).
Unfortunately I can't help with EasyAdminBundle, never used it.
About slashes/directory_separator: it looks like it's just overkill here and so we simply don't need it. Feel free to propose a PR to remove it and I'll merge it ASAP

[garak]

Any news on this?

[c33s]

i will update the ticket with infos & create a PR as soon as i can. my current workload doesn't allow it yet. i assume it will be end of the month.

[murtukov]

@garak
@ossinkine
@c33s

Hi guys, it's been a long time since I opened this issue. Sorry for a poor feedback from my side, I didn't have enough time for this issue.
Now I took time to read all the messages above and all related issues.

What is the current state?
Well if I now create a new Symfony4 project and install following bundles:
VichUploaderBundle 1.8.2
LiipImagineBundle 2.0.x-dev
I would be able to use resolvers in Twig templates:
{{ vich_uploader_asset(user, 'imageFile') | imagine_filter('my_thumb') }}

as well as in controllers/services:

$relativePath = $this->vichService->asset($user, 'imageFile');
$link = $this->liip->getBrowserPath($relativePath, 'my_thumb');

Both would generate the same correct link:
http://example.com/media/cache/my_thumb/img/user/78/78c737520d6620f766d5a636da9afa13e89bf47f.jpg

Here is my configuration:

vich_uploader:
    db_driver: orm

    mappings:
        user_img:
            uri_prefix:         /img/user
            upload_destination: '%kernel.project_dir%/public/img/user'
            namer:              Vich\UploaderBundle\Naming\HashNamer
            directory_namer:    Vich\UploaderBundle\Naming\SubdirDirectoryNamer

liip_imagine:
    resolvers:
        default:
            web_path: ~

    filter_sets:
        cache: ~
        my_thumb:
            quality: 75
            filters:
                thumbnail: { size: [255, 400], mode: inset, allow_upscale: false }

Everything seems to work fine, however, if i try to use VichImageType in my Sonata Admin classes, it generates links without the prefix. Example:

$formMapper
    ->with('Avatar')
        ->add('imageFile', VichImageType::class, [
            'required' => false,
            'label' => 'Avatar',
            'download_uri' => false,
            'imagine_pattern' => 'my_thumb',
        ])
    ->end();

The code above would generate the following link:
http://example.com/media/cache/my_thumb/78/78c737520d6620f766d5a636da9afa13e89bf47f.jpg
As you can see the /img/user part isn't there

Conclusion:
For me the problem remains the same: I can't use VichImageType, because it generates links without uri_prefix because it uses resolvePath instead of resolveUri

So if you revert 514425c it would solve the problem and I would be able to generate links in templates, controllers and sonata classes.

Thanx.

[garak]

What if we add a new option? So everyone can choose if using resolvePath or resolveUri

[ossinkine]

@murtukov You have wrong configuration, so this behavior is expected. Please look at the above comments again.

[murtukov]

@ossinkine
How is my configuration wrong? These are basically configurations from official docs from VichUploaderBundle and LiipImagineBundle. And it worked perfectly (as expected) until i tried to use SonataAdmin. Which part of my configuration is wrong?

@garak Do you mean an option for VichUploaderType? In which cases would it be usefull to use resolvePath instead of resolveUri in VichUploaderType?

[benjamin-hubert]

same problem here as @murtukov. Can confirm the behaviour.

[ossinkine]

@murtukov @gouaille Please look at my comment above https://github.com/dustin10/VichUploaderBundle/issues/858#issuecomment-390905583

[garak]

After experiencing the same issue myself, I decided to revert changes done in 514425c.
A new tag was released for branch 1.8, while master can be used as ^1.10@dev (for now)

[murtukov]

@garak
Finally. Thank you.

[ossinkine]

What if we add a new option? So everyone can choose if using resolvePath or resolveUri

So since users are still confused, this would be a good solution

[garak]

@ossinkine can you provide a PR?

[ossinkine]

I'm busy in the near future, I'll do later

[BlackWiCKED]

@garak This commit broke my code (43adde0). I upload images to a private S3 repository, and store cached files in a public bucket (these two are separate), and since version 1.10.0 came out it won't work anymore. Was fine with version 1.9.2.

The problem is that instead of the filename (path) the _resolveUri_ function returns the absolute URL to the private file - which is obviously not accessible - and the cache resolver won't recognize it either. Function _resolvePath_ did what I needed.

[garak]

@BlackWiCKED stay with 1.9

[gnutix]

@garak That doesn't feel like a very sustainable solution, to hang on an old tag, especially with the latest release containing stuff like "solve compatibility with newer Symfony versions".

What about dealing with this piece of code in a configurable closure instead ? Something like :

if ($options['imagine_pattern']) {
    if (null === $this->cacheManager) {
        throw new \RuntimeException('LiipImagineBundle must be installed and configured for using "imagine_pattern" option.');
    }

    $path = $options['imagine_compute_path']($this->storage, $form, $object);
    if (null !== $path) {
        $view->vars['image_uri'] = $this->cacheManager->getBrowserPath($path, $options['imagine_pattern']);
    }
}

// ...

$resolver->setDefaults([
    'imagine_compute_path' => function (StorageInterface $storage, FormInterface $form, $object) {
        return $storage->resolveUri($object, $form->getName(), null);
    },
]);

// ...

$builder->add(
    'file',
    VichImageType::class,
    [
        'download_uri' => false,
        'imagine_pattern' => 'some_imagine_filter',
        'imagine_compute_path' => function (StorageInterface $storage, FormInterface $form, $object) {
            return $storage->resolvePath($object, $form->getName(), null, true);
        },
    ]
);

Maybe it would even be worth including this line as part of the configurable closure $this->cacheManager->getBrowserPath($path, $options['imagine_pattern']); as it misses some extra arguments (like the $runtimeConfig which might come in handy (or in another PR?)...

WDYT?

[garak]

@gnutix adding an option was already considered sometimes ago, and it's still a possibility.
I would keep it as simplest as possible: just add a new option, with a default value on the current behavior (to keep BC), and a new value on the alternate solution.

[gnutix]

@garak please review https://github.com/dustin10/VichUploaderBundle/pull/1021

[ossinkine]

I am preparing a PR where old behavior is also worked but deprecated and is described why, please be patient

[BlackWiCKED]

As you suggested @garak, I stayed with version 1.9, but keep getting deprecated error messages for Symfony 4.3. As far as I know, this bug was already fixed in #999, just probably was not back-ported to version 1.9. Which is fine I guess, I'd rather move forward to a new version than back-porting all the patches, but there were no messages in this thread since July and the PRs were abandoned in the meantime.

Not sure how can I help to wrap this up, we already identified where the issue is, a config parameter to decide whether _resolveUri_ or _resolvePath_ should be used, seems the right way to me, and of course keeping the old behavior by default (as it was in version 1.9) to maintain backward compatibility.

[garak]

@BlackWiCKED you can take diff from #1021 and open your PR

[BlackWiCKED]

@garak Done: #1045 , please review.