lazy twig extension: handle asset versioning, added imagine_filter_cache, better test coverage #1397
Conversation
dbu
force-pushed
the
sync-lazy-twig
branch
3 times, most recently
from
1d46052 to
242d0bf
Compare
|
@Schyzophrenic @GKFX you looked into this in the past, so maybe you have some feedback on this approach? |
|
@dbu I had a very brief look at this. I could be wrong but if you clean the path of everything after the ? I assume you won't be able to handle the http queries of a distant host such as an image provider where the parameters are critical to serve the right image. |
|
i inject the configured version string and check if the url ends with exactly that string. if it does not, i don't touch the url. |
dbu
changed the title
So I was actually thinking of a URL like this one: https://images2.productserve.com/?w=200&h=200&bg=white&trim=5&t=letterbox&url=ssl%3Awww.cdiscount.com%2Fpdt2%2F9%2F2%2F5%2F1%2F700x700%2FNIN4902370537925.jpg&feedId=38753&k=421fa9c064b91b3719c999b1a900cfb30d7e23de |
|
that should now work again when using |
You're right I see it now! Thanks for the precision |
| return; | ||
| } | ||
|
|
||
| $versionStrategyDefinition = $container->getDefinition('assets._version__default'); |
There was a problem hiding this comment.
getting the default symfony versioning service to copy the configuration from it, if available.
is there a better solution for this?
| */ | ||
| private function str_ends_with(string $haystack, string $needle): bool | ||
| { | ||
| return mb_substr($haystack, -mb_strlen($needle)) === $needle; |
There was a problem hiding this comment.
no need to depend on mbstring for that (As UTF-8 is self-synchronizing, you won't do a mess when using non-mbstring functions to look for the end of the string being `?%%s')
There was a problem hiding this comment.
ah, here we probably don't strictly need it. we use mbstring in a couple of places already. i add ext-mbstring to composer.json and leave it here as well (php-cs-fixer also wants to have the mbstring variants, would need to disable that rule otherwise, and that could mess in other places where we do need it)
| return; | ||
| } | ||
|
|
||
| $versionStrategyDefinition = $container->getDefinition('assets._version__default'); |
| } | ||
|
|
||
| $versionStrategyDefinition = $container->getDefinition('assets._version__default'); | ||
| if (!is_a($versionStrategyDefinition->getClass(), StaticVersionStrategy::class, true)) { |
There was a problem hiding this comment.
if you want to be safe, you should probably resolve parameters in the class name here (the DI component still supports them, even though it is considered a bad extension point and so core services stopped using that pattern). As that code is meant to detect custom strategies too, you cannot assume that it is a core definition.
There was a problem hiding this comment.
do i need the thing below, or on this line $container->resolveEnvPlaceholders($versionStrategyDefinition->getClass())?
There was a problem hiding this comment.
i experimented with this and saw that parameters are resolved when we get to this compiler pass. probably indeed PassConfig::TYPE_BEFORE_REMOVING
| } | ||
| $version = $versionStrategyDefinition->getArgument(0); | ||
| $format = $versionStrategyDefinition->getArgument(1); | ||
| $format = $container->resolveEnvPlaceholders($format); |
There was a problem hiding this comment.
@stof is this what you meant with resolving the parameters?
There was a problem hiding this comment.
no. I said parameters, not env placeholders (env placeholders cannot be validated at compile-time, as their value can change at runtime by definition).
I meant $container->getParameterBag()->resolveValue($format) (although you might not need it as it is a BEFORE_REMOVING pass, and so parameters might already have been resolved by the DI component, to be checked)
There was a problem hiding this comment.
to debug, i changed the FrameworkExtension to set the format with a parameter and the assets.php service definition to use a class name parameter.
both get resolved before this compiler pass runs. so i only removed the wrong resolveEnvPlaceholders i added but don't add the parameter bag resolving.
dbu
force-pushed
the
sync-lazy-twig
branch
2 times, most recently
from
4818440 to
f144a0b
Compare
While looking over things, i came up with a different approach that is less invasive than
parse_urland as a bonus actually nicely handles asset versioning. this accidentally even provides a simplistic solution for #168 (with just one global version, which could be helpful when changing filter sets)