NGSTACK-901 implement decorator service for variation path generator #51
Conversation
… which will replace file extension with webp if image variation is configured for this format
bundle/DependencyInjection/Compiler/WebpFormatVariationPathGeneratorDecoratorPass.php
Outdated
Show resolved
Hide resolved
bundle/Core/Imagine/VariationPathGenerator/WebpFormatVariationPathGenerator.php
Outdated
Show resolved
Hide resolved
|
Except for the CS style nitpicks, LGTM 👍 |
| use Symfony\Component\DependencyInjection\Definition; | ||
| use Symfony\Component\DependencyInjection\Reference; | ||
|
|
||
| class WebpFormatVariationPathGeneratorDecoratorPass implements CompilerPassInterface |
There was a problem hiding this comment.
Do we really need the compiler pass just to register a decorator?
Can't we do this through the service config in YAML files?
There was a problem hiding this comment.
Half of my life you force me to write compiler passes, now you want me to add it in the config 😆
I wasn't initially sure if we want to make the decorator service enabled by default, or if we will need to introduce a switch through configuration. @vjeran wants this enabled by default in both legacy admin and symfony/ezplatform, so maybe no need for the compiler pass.
There was a problem hiding this comment.
Compiler passes should be used when modifying eixsting services! With this, we're not modifying an existing service, but adding a new one.
Besides, we're already using decorators directly in yaml files.
There was a problem hiding this comment.
Let's leave it like this for consistency sake, as we will be using the compiler pass to actually decorate the existing variation path generator in Ibexa 4.x with our own service. If we are using 4.x in combination with legacy admin, then this is needed to consolidate variation path patterns to match the ones from legacy. The compiler pass in that case will replace ibexa decorator class with our own.
|
I presume this does not need to be merged into |
|
@emodric no, this works only for ezplatform + ngadminui combination, so only applies to 1.7 branch Technically, we might merge this into master if I check the installed bundles, and make a separate logic in the compiler pass depending whether we have ibexa/core or ezpublish/kernel installed |
Okay, I'll leave it as is for now and not merge to master (or rather, create an empty merge), and we'll fix that one when we get there. |
Implement decorator service for variation path generator which will replace file extension with webp, if image variation is configured for this format.
The implementation is heavily based on ezsystems/ezplatform-kernel#361, except we don't just append ".webp" on full generated image path, but instead replace the generated file extension with 'webp'.
The reason for this is keeping backward compatibility with eZ Publish legacy kernel (used in Netgen Admin UI). Legacy image aliases can be configured to be generated in specific format/mime type (i.e. original image is JPG but variation will be generated as WebP). But in this case, the alias filenames will always contain only one file extension, the one specified by the alias mime type. So this should prevent duplicate images being generated on the file system, both legacy and Symfony variation generators will use same alias path patterns.
This decorator serves purpose only for the ezsystems/ezpublish-kernel with or without the legacy-based admin.
Later kernel versions (both ezplatform-kernel and ibexa) already contain this decorator service by default.