Skip to content

[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

Open
wants to merge 2 commits into
base: 7.3
Choose a base branch
from

Conversation

Okhoshi
Copy link
Contributor

@Okhoshi Okhoshi commented Dec 9, 2023

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues none
License MIT

Follow up to #50391, set up the ValidatorCacheWarmer and SerializerCacheWarmer to use the kernel.build_dir instead of kernel.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

@carsonbot carsonbot added this to the 7.1 milestone Dec 9, 2023
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch 2 times, most recently from e756930 to 38a2688 Compare December 9, 2023 22:37
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@Okhoshi
Copy link
Contributor Author

Okhoshi commented Dec 10, 2023

I wonder if it wouldn't be nicer to adapt the behavior based on whether the file is relative or absolute?

I also considered that option, and haven't realized symfony/filesystem was already required in the Bundle. Will change to that 👍

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

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 AbstractPhpFileCacheWarmer to use the build dir.
Or did I miss something ? :)

@Okhoshi Okhoshi changed the title Framework validator serialize build dir [FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir Dec 10, 2023
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from 38a2688 to 60296eb Compare December 18, 2023 10:10
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch 2 times, most recently from 4cc3f57 to dfe9404 Compare January 9, 2024 21:55
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch 2 times, most recently from 5a11934 to 9311e00 Compare March 23, 2024 21:58
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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 🤷

@nicolas-grekas
Copy link
Member

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.
Would it make sense to change this to be:

  • set cache_dir to build_dir during compile-time warmups
  • keep cache_dir and build_dir to their defined values 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)

@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from 9311e00 to 6418bca Compare April 4, 2024 17:05
@Okhoshi
Copy link
Contributor Author

Okhoshi commented Apr 4, 2024

@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.

@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from 6418bca to ffc97f8 Compare April 4, 2024 17:19
@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@nicolas-grekas
Copy link
Member

Can you please rebase on top of 7.2?

@carsonbot carsonbot changed the title [FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir Jul 1, 2024
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from 92560e0 to dd54123 Compare July 30, 2024 05:37
@carsonbot carsonbot changed the title Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir [FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir Jul 30, 2024
@OskarStark OskarStark changed the title [FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir [FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of kernel.cache_dir Jul 30, 2024
@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from dd54123 to de49d5c Compare December 7, 2024 09:26
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from de49d5c to e0f12d8 Compare April 1, 2025 07:37
@Okhoshi
Copy link
Contributor Author

Okhoshi commented Apr 1, 2025

@nicolas-grekas Anything missing on this PR to get it merged ? :)

Okhoshi added 2 commits April 22, 2025 09:31
…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>
@Okhoshi Okhoshi force-pushed the framework-validator-serialize-build-dir branch from e0f12d8 to ae6f4e0 Compare April 22, 2025 07:31
@Okhoshi Okhoshi requested a review from nicolas-grekas April 28, 2025 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants