Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

Okhoshi
Copy link
Contributor

@OkhoshiOkhoshi commentedDec 9, 2023
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
Issuesnone
LicenseMIT

Follow up to#50391, set up theValidatorCacheWarmer 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

valtzu and Fan2Shrek reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.1 milestoneDec 9, 2023
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch 2 times, most recently frome756930 to38a2688CompareDecember 9, 2023 22:37
Copy link
Member

@nicolas-grekasnicolas-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
ContributorAuthor

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

@OkhoshiOkhoshi changed the titleFramework validator serialize build dir[FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dirDec 10, 2023
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch from38a2688 to60296ebCompareDecember 18, 2023 10:10
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch 2 times, most recently from4cc3f57 todfe9404CompareJanuary 9, 2024 21:55
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch 2 times, most recently from5a11934 to9311e00CompareMarch 23, 2024 21:58
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

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)

@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch from9311e00 to6418bcaCompareApril 4, 2024 17:05
@Okhoshi
Copy link
ContributorAuthor

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

@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch from6418bca toffc97f8CompareApril 4, 2024 17:19
@xabbuhxabbuh modified the milestones:7.1,7.2May 15, 2024
@nicolas-grekas
Copy link
Member

Can you please rebase on top of 7.2?

Okhoshi reacted with thumbs up emoji

@carsonbotcarsonbot changed the title[FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dirMake ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dirJul 1, 2024
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch from92560e0 todd54123CompareJuly 30, 2024 05:37
@carsonbotcarsonbot changed the titleMake ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir[FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dirJul 30, 2024
@OskarStarkOskarStark changed the title[FrameworkBundle] Make ValidatorCacheWarmer and SerializeCacheWarmer use kernel.build_dir instead of cache_dir[FrameworkBundle] MakeValidatorCacheWarmer andSerializeCacheWarmer usekernel.build_dir instead ofkernel.cache_dirJul 30, 2024
@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch fromdd54123 tode49d5cCompareDecember 7, 2024 09:26
@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch fromde49d5c toe0f12d8CompareApril 1, 2025 07:37
@Okhoshi
Copy link
ContributorAuthor

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

@OkhoshiOkhoshiforce-pushed theframework-validator-serialize-build-dir branch frome0f12d8 toae6f4e0CompareApril 22, 2025 07:31
@Okhoshi
Copy link
ContributorAuthor

@nicolas-grekas@OskarStark Could we still get this merged in 7.3 ?

@fabpotfabpot added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelMay 10, 2025
@@ -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([
Copy link
Member

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`
@fabpotfabpotforce-pushed theframework-validator-serialize-build-dir branch fromae6f4e0 todc59817CompareMay 10, 2025 08:40
@fabpot
Copy link
Member

Thank you@Okhoshi.

@fabpotfabpot merged commit392d0c9 intosymfony:7.3May 10, 2025
9 of 11 checks passed
@OkhoshiOkhoshi deleted the framework-validator-serialize-build-dir branchMay 10, 2025 08:43
@fabpotfabpot mentioned this pull requestMay 10, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
DeprecationFeatureFrameworkBundle❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

6 participants
@Okhoshi@nicolas-grekas@fabpot@OskarStark@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp