Add FormatExtensionResolver

[ossinkine]

Q	A
Branch?	2.0 (was branched from master)
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1198
License	MIT
Doc PR

I've added a new resolver to resolve issue with file extension when filter changes image format, see related issues.
Just add it as decorator to your primary resolver and images will be stored with proper extensions:

services:
    acme.imagine.cache.resolver.format_extension:
        class: Liip\ImagineBundle\Imagine\Cache\Resolver\FormatExtensionResolver
        arguments:
            - "@liip_imagine.cache.resolver.default"
            - "@liip_imagine.filter.configuration"
        tags:
            - { name: "liip_imagine.cache.resolver", resolver: "format_extension" }

[coveralls]

Coverage increased (+0.1%) to 83.776% when pulling 97b579c on ossinkine:format-extension-resolver into 469b178 on liip:2.x.

[robfrawley]

Are you able to include documentation regarding this new feature? Our documentation source files are located under the Resources/doc project directory and can be locally built using the build guide from the project Wiki to confirm your documentation changes and/or additions properly compile and render as expected. If you have no familiarity with reStructuredText (RST) (which is the markup language used for our docs) or you do not have the time or ability to update the docs for this feature, please advise and I will look into doing so myself prior to merging this PR.

[ossinkine]

@robfrawley I've added the documentation for this feature but unfortunatelly I was not able to build the doc according to build manual. Feel free to fix it if something is wrong.

[Perni1984]

@ossinkine many thanks for this!

[ossinkine]

@dbu Thank you for fixes 👍

[dbu]

looks good to me. okay to merge, @robfrawley ?

[acrobat]

@ossinkine why did you close this PR?

[ossinkine]

Looks like I accidentally deleted the branch while creating another PR.

[tarjei]

Hi, any news on this PR? heic files are becoming an issue so this is needed more an more.

[plandolt]

Looking forward to this resolver. Is there anything to add/help?

Another question is about a possible conflict when change the format on two or more files with the same filename except a different extension. e.g. 1.jpg and 2.png transformed to webp format will both result in 1.webp. But those file could be totally different. Should the extension instead of being replaced better being added? Or should this behaviour be configurable?

Plus: This one does not solve the problem of no redirect on the first request in which the cached version will be generated.

[ossinkine]

@scuben I have no idea why this PR still is not merged, so I don't how you can help 🤷‍♂️

What about file extensions, I though about that and it seemed to me that filenames like 1.jpg.webp are ugly enough. We are using hash names to avoid conflicts.

[trsteel88]

@lsmith77 are you able to merge this branch if it is fine? I'm looking to implement webp image formats (by defining format: webp) on additional filtersets and it doesn't alter the path name :(

[trsteel88]

Actually just looking at this code. It doesn't look like this is just something you can turn on. You would need to implement it in your services each time. Would it be better to have this as an option? Or alternatively, always replace (or append) the new extension?

[ossinkine]

@trsteel88 We can't always change the extension because it breaks BC. The such functionality in this library extends by decorating cache resolvers. Why do you think the adding an option is better then adding the decorator service?

@lsmith77 Indeed, what is keeping us from merging?

[lsmith77]

the main issue is time reviewing ..

@fbourigault do you have time to look into this PR?

[robfrawley]

@lsmith77 I can actually take a look at this later this week if no one else gets around to it.

[trsteel88]

@trsteel88 We can't always change the extension because it breaks BC. The such functionality in this library extends by decorating cache resolvers. Why do you think the adding an option is better then adding the decorator service?

Make it an option would mean BC would still be respected.

e.g. "redirect_format_extension" = false by default.

Then on any new installs, we could just specify true to make use of the automated redirect.

[trsteel88]

Not sure if this is a big deal, but the getBrowserPath() does not render the correct extension before the image is stored.

This means that the path will render as media/cache/resolve/foo/bar.jpg if the image does not exist yet. Then it will redirect to media/cache/foo/bar.webp

After the file is generated, it will then render as media/cache/foo/bar.webp which is correct.

[dbu]

thanks! and sorry for the delay.

i think for version 3 we should make this the default behaviour - it seems weird to not always do it. (also, with content negotiation for webp and stuff, did we currently only store one version, or does that take care of the extension?)