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

'formats' not used by Stream Loader #231

Closed
rdohms opened this issue Sep 3, 2013 · 16 comments
Closed

'formats' not used by Stream Loader #231

rdohms opened this issue Sep 3, 2013 · 16 comments

Comments

@rdohms
Copy link

rdohms commented Sep 3, 2013

if a profile configures the format parameter to, for example, jpg the url that is written for the image tag will override the extension:

/uploads/image.png becomes /uploads/image.jpg

In the FileSystemLoader this is overcome by iterating over the formats option until a file is found with one of the alternate extensions. (see code)

However, the same is not true for the StreamLoader. So essentially ImagineBundle changes the url, forcing the format, and then is never capable of finding the original file.

Iterating over alternative formats in the stream case may be a bit more resource heavy, so I think the solution would be to address the "why" urls are rewritten and try to handle that in the output not in passing the filename.

What do you guys think is our best way out here?

@lsmith77
Copy link
Contributor

lsmith77 commented Sep 3, 2013

The purpose is to be able to convert a file from one format to another.

@rdohms
Copy link
Author

rdohms commented Sep 3, 2013

@lsmith77 but the loader will still not be able to find the original file. Because it uses the given path as the "source path".

I agree with having "format" as an option, so that the generated result will be done in whatever format you specify, but the proper original path should be provided and not overwritten as is the case, maybe it should render something like /&force=jpg?

I don't mean 'why is the feature in there' i mean 'why is it implemented overriding the source url'

@rdohms
Copy link
Author

rdohms commented Sep 9, 2013

Any stance on this? I need to find a solution, would be better if the solution is in line with your goals so i can contribute it back to you.

@lsmith77
Copy link
Contributor

lsmith77 commented Sep 9, 2013

i might still not understand properly what you mean .. we need to embedd url into the html, this url should specify the format as is intended in the final output as otherwise its just a mess for outside systems (serving a jpg with .gif because the source image is a gif). so our url already needs to have the target extension. adding some hints as query parameters as to the original format etc while possible would also result in some unwanted behavior. for example many reverse proxies and browser caches react iffy to query parameters on otherwise static files.

@rdohms
Copy link
Author

rdohms commented Sep 9, 2013

@lsmith77 so how do you use the combination of StreamResolver + original image in png + desired output jpg ? Right now its not doable since the StreamResolver does not know how to search for the original image and just looks for original-name.jpg with the desired output extension.

@lsmith77
Copy link
Contributor

lsmith77 commented Sep 9, 2013

i don't know the StreamResolver .. it can of course be that for certain storage backends there is no way to support all features. which in this case might be not supporting the format option or making it possible (not sure how hard this is) for the storage backend to be able to add some query parameters in case it needs additional hints to find the original.

@rdohms
Copy link
Author

rdohms commented Sep 9, 2013

not being able to use format also leads to more errors like #93, since the mime-type is ignored.

I'll take a look at options.

@rdohms
Copy link
Author

rdohms commented Sep 9, 2013

also, sorry StreamLoader not Resolver.

@havvg
Copy link
Contributor

havvg commented Sep 16, 2013

Simply spoken, it's just missing the iteration code in the StreamLoader. However there is a caveat, not every backend behind the stream is capable of providing extended file information. The path is not necessarily a filename, it can be anything to address an image stream on the backend used.

I would suggest to extend the StreamLoader by adding a FileStreamLoader or something, which contracts the backend to be somewhat file-based. This loader may then try the formats to retrieve the source image.

@rdohms
Copy link
Author

rdohms commented Sep 16, 2013

@havvg Yes that is what i have done, its just a shame for the bundle to offer a situation where a frequent combination of its own options clashes and crashes it.
Which is why i was asking to see if we could solve that at the bundle level. But i'll leave it up to the creators to think about if its worth, my problem was solved with a custom Loader.

@havvg
Copy link
Contributor

havvg commented Sep 16, 2013

You got a generic solution for it, do you mind sending a PR for it?

@rdohms
Copy link
Author

rdohms commented Sep 16, 2013

@havvg its not very generic, but since we store the filename in the DB it checks the DB for the file based on its name (unique md5's) and gets the original extension. So it depends heavily on the way you store stuff.
Would be hard to do it in a generic way, but i could do a blog post with sample code to help people implement it maybe?

@lsmith77
Copy link
Contributor

It would be good to make the limitation clear in the docs and then provide your solution for an example work around

@LouTerrailloune
Copy link
Contributor

What about this:

Url in the form : /prefix/filter/path/filename.origext.wantedext

The controller extract wantedext
path/filename.origext is used by the loader.
wantedext is passed to FilterManager to check against wanted format and to generate response in this format
CacheManager and Resolver gets the full filename

We might output only one extention if the source file already contains one and the output format is the same.

This should work with any DataLoader.

@LouTerrailloune
Copy link
Contributor

There is a problem with my previous suggestion, there is no way to distinguish between an id without extension with extension added and an id which already has the good extension.

May be something like this whould be better:

/prefix/filter/path/filename.origext.to.wantedext

We have the same problem if the id ends by .to (less important however)

@havvg
Copy link
Contributor

havvg commented Dec 13, 2013

I'll close this. As mentioned in #277 we will take a look into this for v1.0.0.

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

4 participants