[LazyImage] Cache BlurHash, close #2 #1755
Conversation
Kocal
force-pushed
the
feat/GH-2
branch
4 times, most recently
from
017dda6 to
74e49d9
Compare
|
|
||
| static::assertSame( | ||
| 'L54ec*~q_3?bofoffQWB9F9FD%IU', | ||
| $cache->getItem(array_key_first($cache->getValues()))->get() |
There was a problem hiding this comment.
The generated cache key was different between my machine and the CI, so I'm using array_key_first($cache->getValues()) instead 🤷🏻
There was a problem hiding this comment.
Maybe would be enough to set framework.cache.prefix_seed to get stable namespaces/keys :)
There was a problem hiding this comment.
To be more clear:
- on my machine the cache key is
blurhash.70d3aaba5c301af7 - but on the CI it is
blurhash.788394be043a8824(see https://github.com/symfony/ux/actions/runs/8745742961/job/24001335802?pr=1755#step:7:108 & https://github.com/symfony/ux/actions/runs/8745742961/job/24001338516?pr=1755#step:7:73)
The different part is the result of hash('xxh3', '...') and is not related to the cache prefix seed 😕
And today we've learned, the namespace (generated through framework.cache.prefix) is not even used when adapter is an ArrayAdapter (https://github.com/symfony/cache/blob/7.0/DependencyInjection/CachePoolPass.php#L118-L119) :D
There was a problem hiding this comment.
Well, in fact it's super simple, since we compute the cache key with the filename, here __DIR__.'/../Fixtures/logo.png', paths are different between my machine and CI :)
But I we should keep this behaviour as is, I don't think we want to manipulate the filename (from public directory or not, from a URL or not, ...).
|
Rewritten as a decorator, tests have been adapted too |
carsonbot
added
Status: Reviewed
|
Thanks Hugo, only 270 issues to go! |
|
I just realized that the decorator
I'm working on it |
This PR was merged into the 2.x branch. Discussion ---------- [LazyImage] Fix cache implementation | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Following #1755 (comment), `BlurHash#createDataUriThumbnail` could not use the cache because it call `$this->encode()`, which refers to `BlurHash#encode` and not `CachedBlurHash#encode`. So I went back with my previous implementation by injecting (or not) a `CacheAdapter` implementation to `BlurHash`, and use it if necessary. I've cheated a bit to adapt test, because I really want to ensure `$cache` is used or not. Commits ------- a619c6c [LazyImage] Fix cache implementation
Hi everyone, this PR is a proposal for #2, the second oldest issue on this repository 🤯
I've also though about decorates the
BlurHashInterfaceif optioncacheis set, this way we don't modify the currentBlurHash. Let me know what is preferred :)