Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle] MakeValidatorCacheWarmer
andSerializeCacheWarmer
usekernel.build_dir
instead ofkernel.cache_dir
#52981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e756930
to38a2688
CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if it wouldn't be nicer to adapt the behavior based on whether the file is relative or absolute?
This way we don't need any deprecations nor any new methods:
- when the path is absolute, we use that as we do now
- when the path is relative, we put the file in the build dir
And of course we make the path relative by default.
I also considered that option, and haven't realized
However, the deprecations and the new method are still necessary to let the extending class make the choice where to store the file. If we go with the current code, and the logic that when the path is relative we store in the build dir, it would force every CacheWarmer extending the |
38a2688
to60296eb
Compare4cc3f57
todfe9404
Compare5a11934
to9311e00
Compare
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not convinced about the refactoring, it brings a potential BC break.
Why not just change the value in the DI?
We can't prevent people from doing mistakes anyway 🤷
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
I'm having a hard time reasoning about the way things work during warmup... Right now, we made warmers be called with $buildDir set to null during post-compile warmups.
cache warmers would then be able to compare $cacheDir and $buildDir to decide if there is a readonly cache they shouldn't touch. Wouldn't that make all your related PRs simpler? (The most important change being to set $cacheDir to the build-dir during compile-time warmups) |
9311e00
to6418bca
Compare@nicolas-grekas I took a step back regarding this whole build_dir/cache_dir situation, and you are right that we can't prevent people to do mistakes anyway 👍 I could indeed drastically simplify this PR once I accepted that fact 🙂. However, passing the buildDir as null when we run the warmers in post-compile warmup is equivalent to your suggestion, so I suggest to keep that as it is. |
6418bca
toffc97f8
CompareCan you please rebase on top of 7.2? |
cb11ce2
to92560e0
Comparesrc/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/SerializerCacheWarmerTest.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/SerializerCacheWarmerTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
92560e0
todd54123
CompareValidatorCacheWarmer
andSerializeCacheWarmer
usekernel.build_dir
instead ofkernel.cache_dir
dd54123
tode49d5c
Comparede49d5c
toe0f12d8
Compare@nicolas-grekas Anything missing on this PR to get it merged ? :) |
e0f12d8
toae6f4e0
Compare@nicolas-grekas@OskarStark Could we still get this merged in 7.3 ? |
@@ -164,7 +164,10 @@ | |||
->set('serializer.mapping.cache.symfony', CacheItemPoolInterface::class) | |||
->factory([PhpArrayAdapter::class, 'create']) | |||
->args([param('serializer.mapping.cache.file'), service('cache.serializer')]) | |||
->args([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can be reverted
…er` use `kernel.build_dir` instead of `kernel.cache_dir`
ae6f4e0
todc59817
CompareThank you@Okhoshi. |
392d0c9
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Follow up to#50391, set up the
ValidatorCacheWarmer
andSerializerCacheWarmer
to use thekernel.build_dir
instead ofkernel.cache_dir
. The value conveyed by their constructor parameter has been changed to prevent developers to configure the cache file outside of the build_dir.AbstractPhpFileCacheWarmer
has been reworked a bit too, to reduce the risk of misconfiguration.#SymfonyHackday