-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[FrameworkBundle] Make ValidatorCacheWarmer
and SerializeCacheWarmer
use kernel.build_dir
instead of kernel.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 7.3
Are you sure you want to change the base?
Conversation
e756930
to
38a2688
Compare
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 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
to
60296eb
Compare
4cc3f57
to
dfe9404
Compare
5a11934
to
9311e00
Compare
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
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/AbstractPhpFileCacheWarmer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Resources/config/validator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/CacheWarmer/ValidatorCacheWarmer.php
Outdated
Show resolved
Hide resolved
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
to
6418bca
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
to
ffc97f8
Compare
Can you please rebase on top of 7.2? |
cb11ce2
to
92560e0
Compare
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/ValidatorCacheWarmerTest.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/SerializerCacheWarmerTest.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Tests/CacheWarmer/SerializerCacheWarmerTest.php
Outdated
Show resolved
Hide resolved
92560e0
to
dd54123
Compare
ValidatorCacheWarmer
and SerializeCacheWarmer
use kernel.build_dir
instead of kernel.cache_dir
dd54123
to
de49d5c
Compare
de49d5c
to
e0f12d8
Compare
@nicolas-grekas Anything missing on this PR to get it merged ? :) |
…instead of `cache_dir` Signed-off-by: Quentin Devos <4972091+Okhoshi@users.noreply.github.com>
…instead of `cache_dir` Signed-off-by: Quentin Devos <4972091+Okhoshi@users.noreply.github.com>
e0f12d8
to
ae6f4e0
Compare
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