dustin10 / VichUploaderBundle

A simple Symfony bundle to ease file uploads with ORM entities and ODM documents.
MIT License
1.83k stars 519 forks source link

Files ALWAYS gets downloaded even if `Content-Disposition` is set to `inline` #1456

Closed DenuxPlays closed 3 months ago

DenuxPlays commented 3 months ago

This is due to the unnecessary download attribute in the tag here:

        {%- if download_uri -%}
            <a href="{{ asset_helper is same as(true) ? asset(download_uri) : download_uri }}" download>
                {{ translation_domain is same as(false) ? download_label : download_label|trans({}, translation_domain) }}
            </a>
        {%- endif -%}

It should be

        {%- if download_uri -%}
            <a href="{{ asset_helper is same as(true) ? asset(download_uri) : download_uri }}">
                {{ translation_domain is same as(false) ? download_label : download_label|trans({}, translation_domain) }}
            </a>
        {%- endif -%}

Files still get downloaded but now the Content-Disposition gets respected.

garak commented 3 months ago

What's the case when Content-Disposition is online?

DenuxPlays commented 3 months ago

What's the case when Content-Disposition is online?

It simply means that the browser tries to open the file inside the browser and if he can't he is still going to download it.

I overwrote your form template and simply removed the download attribute and it works as expected (even when not setting the header/ direct download link)

garak commented 3 months ago

So, it seems that the default template works fine with the default headers. If you want something different, overwriting the template is enough. I don't see any issue here.

DenuxPlays commented 3 months ago

So, it seems that the default template works fine with the default headers. If you want something different, overwriting the template is enough.

I don't see any issue here.

My issue is that the download Attribute is unnecessary. It works as expected without it and without it the standard header gets actually respected as expected.

garak commented 3 months ago

You already solved it with the custom template. Is there anything else you need?

DenuxPlays commented 3 months ago

You already solved it with the custom template. Is there anything else you need?

I thought this change can be applied directly here. As I think this is a general improvement/"bug" fix

garak commented 3 months ago

The whole point is about what the average user expects to get. The current implementation suits it. A custom implementation is still possible. So, I don't see the need for improvements or fixes.

DenuxPlays commented 3 months ago

The current experiene does not change. The user would still get what they expected but okay if you are not open to this change this disscussion is pointless