Add support for alternatiwe image formats like webp and avif#1651
Add support for alternatiwe image formats like webp and avif#1651
Conversation
…update handling of modern image formats (WebP, AVIF).
…tiation with tests.
…ative` and add comprehensive tests for alternative format handling.
…otiator` initialization, and enhance support for format-specific mime types and post-processors.
…pdate related methods, and improve support for flexible format negotiation.
…native_formats`, deprecate `isWebpSupported`, and update service definitions.
|
Hi @dbu . can you help me with cs fixer? I run cs fixer as you mentioned me last time, but nothing is fixed in me setup Thx |
webp configuration in favor of alternative_formats and ……agic driver can convert to avif, imagick convert to avif before postProcessor. so I add this option use_default_driver. only if this option is set to true (defaut) imagick will convert to avif. otherwise will be do it avifenc binary
|
Hi @dbu . Will you look at this PR please? |
|
Ok, But this PR is the same aproach as is already in this bundle. so nothing is changed. only some refactoring. |
dbu
left a comment
There was a problem hiding this comment.
i took a closer look now. the refactoring is well done and a much better and future proof way to achieve what the webp flag does. i am still unhappy that it deepens a pattern that i think we should move away from. in that way it sends a wrong signal to users who upgrade.
at the same time I don't have much time to work on the bundle, so it might be quite a while until i can address that. therefore lets merge this.
i commented on some details. can you please also add a changelog entry to explain the deprecation and the more generic solution, but also point out that client side resolving is the preferred way for supporting webp, avif etc?
| $webpGenerate = false | ||
| $alternativeFormats = false | ||
| ) { | ||
| if (\is_bool($alternativeFormats)) { |
There was a problem hiding this comment.
could we default the parameter to [] instead? otherwise not setting alternative formats triggers a deprecation
| $this->dispatcher = $dispatcher; | ||
| $this->defaultResolver = $defaultResolver ?: 'default'; | ||
| $this->webpGenerate = $webpGenerate; | ||
| $this->alternativeFormats = $alternativeFormats; |
There was a problem hiding this comment.
i would normalize this to have the property only accept array and translate false to [] and true to ['webp]. that way the BC is constrained to the constructor and the rest of the class simpler.
| * @param string $path | ||
| * @param string $filter | ||
| * @param string|null $resolver | ||
| * @param bool $webpSupported |
There was a problem hiding this comment.
| * @param bool $webpSupported | |
| * @param bool $webpSupported deprecated in favor of $alternativeFormatsSupported |
| * @param string $path | ||
| * @param string $filter | ||
| * @param string|null $resolver | ||
| * @param bool $webpSupported |
There was a problem hiding this comment.
| * @param bool $webpSupported | |
| * @param bool $webpSupported deprecated in favor of $alternativeFormatsSupported |
| - name: Run PHPUnit tests | ||
| env: | ||
| SYMFONY_DEPRECATIONS_HELPER: max[self]=0 | ||
| SYMFONY_DEPRECATIONS_HELPER: disabled=1 |
| service('liip_imagine.filter.manager'), | ||
| service('liip_imagine.cache.manager'), | ||
| '%liip_imagine.webp.generate%', | ||
| '%liip_imagine.webp.options%', |
There was a problem hiding this comment.
is there no risk of BC break? does the extension integrate the parameters into alternative_formats if the old parameters are set?
|
|
||
| Use WebP if supported | ||
| ~~~~~~~~~~~~~~~~~~~~~ | ||
| Use modern formats if supported (recommended) |
There was a problem hiding this comment.
i'd love to point to the client side resolving right at the start of this section, and mention that client side resolving is the preferred way of doing this.
| formats. | ||
| For better performance, you can use the ``<picture>`` tag to resolve supported | ||
| image formats on the client-side in the browser. This will complicate the HTML code | ||
| and require registering multiple filters that generate images in different formats. |
There was a problem hiding this comment.
| and require registering multiple filters that generate images in different formats. | |
| and require registering multiple filters that generate images in different formats. | |
| If you have a suggestion how a convenient setup for this would look, please open | |
| an issue on github to discuss the topic. |
|
Hi, i can preprare new PR, with idea you want. So lets freeze this PR and I will do on new one. Ok? You want say me something what os your idea closer? But... When i will do new PR and abamdonenthis idea, there will must be BC. So what is better, accpet this with non BC or release new version with BC. I like second option. |
|
if you can address my comments here, we can merge this pull request, then at least there is a better support for the server-side negotiation. i think there is not much work left to do, and then we have an improvement we can release. |
…update handling of modern image formats (WebP, AVIF).
Description
This PR refactors the way modern image formats (like WebP and AVIF) are handled within the bundle. Instead of having hardcoded logic for a single format, it introduces a generic Alternative Formats system.
Key changes:
alternative_formatsconfiguration tree.webpconfiguration is automatically normalized into the new structure using abeforeNormalizationstep inConfiguration.php.FormatNegotiatorservice to dynamically select the best image format based on the browser'sAcceptheader (supporting AVIF, WebP, etc.).FilterPathContainerto support any alternative format, while markingcreateWebp()as deprecated.This approach follows the discussion in issue #1314, providing a future-proof solution for modern image delivery while ensuring zero breaking changes for existing installations.
Proff of concept here: just6words.com