Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configuration defaults for sulu_get_media_url are wrong #248

Open
QuingKhaos opened this issue Aug 3, 2016 · 9 comments
Open

Configuration defaults for sulu_get_media_url are wrong #248

QuingKhaos opened this issue Aug 3, 2016 · 9 comments

Comments

@QuingKhaos
Copy link
Contributor

Q A
Bug? yes
New Feature? no
Sulu Version 1.3.0-RC2

Actual Behavior

https://github.com/sulu/sulu-docs/blob/develop/reference/twig-extensions/functions/sulu_get_media_url.rst mentions that attachment/pdf and image/jpeg mimetypes are configured by default to be inlined, which is not true.

app/console config:dump-reference sulu_media says:

sulu_media:
    disposition_type:
        default:              attachment # One of "inline"; "attachment"
        mime_types_inline:    []
        mime_types_attachment:  []

Expected Behavior

Either the sensible defaults like mentioned in the docs or adapting the docs, which options are default and still showing how you configure e.g. PDFs to be inline.

@danrot
Copy link
Contributor

danrot commented Aug 22, 2016

@patkar Is it possible that this is handled internally different, without the need of a configuration? Then the example would be confusing (or even wrong), but it would really work that way.

@QuingKhaos
Copy link
Contributor Author

I would need to test. As I examined the code at that moment, the sulu_get_media_url used only this configuration variables IIRC.

@alexander-schranz
Copy link
Member

The text is little confusing but it dont mean that this is default: Following configuration is optional it means you need todo the following configuration to get this to work.

@danrot
Copy link
Contributor

danrot commented Aug 22, 2016

Yeah, that's right... Also the configuration is a bit confusing... There has always to be one array empty?

Anybody willing to rewrite that in a less confusing way?

@alexander-schranz
Copy link
Member

No the text is really confusing because it describe how the config is then when you want this case. If the mime_type is not in mime_types_inline or mime_types_attachment it will use the default.

https://github.com/sulu/sulu/blob/1.3.0/src/Sulu/Bundle/MediaBundle/Twig/DispositionTypeTwigExtension.php#L56

@alexander-schranz
Copy link
Member

alexander-schranz commented Aug 22, 2016

Better would maybe 2 examples:

Set default to attachment and set specific mime_types to inline:

sulu_media:
  disposition_type:
    default: "attachment"
    mime_types_inline: ["application/pdf", "image/jpeg"]

Set default to inline and set specific mime_types to attachment:

sulu_media:
  disposition_type:
    default: "inline"
    mime_types_attachment: ["video/mp4"]

It don't make sense to use mime_types_attachment and mime_types_inline because one of them will always be the default.

And it should describe there that sulu default config is setting the disposition type to attachment for all asset downloads.

@danrot
Copy link
Contributor

danrot commented Aug 22, 2016

Still strange to have only these two values, especially because there are a lot more values allowed: http://www.iana.org/assignments/cont-disp/cont-disp.xhtml#cont-disp-1

But that's not the point here, we should clearly explain it better.

@alexander-schranz
Copy link
Member

alexander-schranz commented Aug 22, 2016

Then the config need to be a free array or something. Currently it allows only the 2 from Symfony defined disposition types: https://github.com/symfony/http-foundation/blob/2.8/ResponseHeaderBag.php#L24-L25

@alexander-schranz
Copy link
Member

alexander-schranz commented Aug 22, 2016

What I find a little strange that the default will only work when you use the get_media_url twig extension. Think setting of the disposition type by mimeType should be done inside the controller and not in this twig extension. Currently the controller will ignore this config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants