EZP-29749: As an Administrator I want to configure Imagine ProxyResolver #2470
Conversation
|
I checked how it works if I get rid of @bdunogier / @andrerom do you think it makes sense to introduce |
|
That would be more clear on our side yes, the randomens feature is not really relevant anymore with http2 anyway so just complicates explaining the feature (and understanding how to configure it). |
kmadejski
dismissed stale reviews from alongosz and andrerom
Changed approach.
|
@andrerom / @alongosz / @adamwojs / @bdunogier I've changed the approach to allow only one host to be configured. Could you please review? EE PR will be changed accordingly after approve. |
|
|
||
| if ($configResolver->hasParameter('image_host') && | ||
| ($imageHost = $configResolver->getParameter('image_host')) !== '') { | ||
| $this->hosts = [$imageHost]; |
There was a problem hiding this comment.
Looked a bit deeper, and from what I can see we are basically hitting this part of the code right?
https://github.com/liip/LiipImagineBundle/blob/master/Imagine/Cache/Resolver/ProxyResolver.php#L90
It has inline comment "BC".
However looking at blame, it does not look to be deprecated so +1 from my side but thought I would mention it.
There was a problem hiding this comment.
Yes, exactly, we are using that part of the code.
As for me this BC comment is due to the fact, that when they changed Resolver behaviour, it was kind of BC break, please take a look: https://github.com/liip/LiipImagineBundle/pull/687/files#diff-f1e2882ffa991d3494a674108aa8000f
There was a problem hiding this comment.
yes, that is the blame I also looked at
| ezpublish.image_alias.imagine.cache_resolver_decorator_factory: | ||
| class: '%ezpublish.image_alias.imagine.decorated_cache_resolver_factory.class%' | ||
| arguments: | ||
| - '@ezpublish.config.resolver.chain' |
There was a problem hiding this comment.
Any specific reason for .chain here? Usually we use @ezpublish.config.resolver.
There was a problem hiding this comment.
Actually not, wasn't sure which one should I use :) Do you want me to change it to @ezpublish.config.resolver?
Co-Authored-By: kmadejski <kamil.madejski@yahoo.com>
|
@micszo I think it's ready for QA! |
|
So far tested with 1.7.8 and 1.13.4. Didn't find issues. |
|
I may have stumbled upon a bug on this: https://jira.ez.no/browse/EZP-30333. |
|
Thanks @bdunogier, I'll take a look on that soon. |
6.7/6.13/2.3/masterAs discussed with @bdunogier regarding the issue reported by the customer, this PR contains decorator for imagine cache resolver. It uses
ProxyResolverprovided by LiipImagine (ref: https://github.com/liip/LiipImagineBundle/blob/master/Resources/doc/cache-resolver/proxy.rst).Moreover, PR introduces the new siteaccess-aware configuration
image_hostswhich can be used for defining one host used then for image URLs generation.Note for QA:
Cache\AliasGeneratorDecoratorPlaceholderAliasGenerator@DominikaK I will appreciate your help with documenting this feature 🙂
TODO:
$ composer fix-cs).