-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
Fix: use array adapter cache pools by default #1073
Conversation
default: | ||
return 'cache.app'; | ||
} | ||
$id = sprintf('cache.doctrine.orm.%s.%s', $objectManagerName, str_replace('_cache', '', $cacheName)); |
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.
every object-manager needs separate caches, right? Otherwise there could be weird key collisions I think
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 sure what the EM uses as key for the entry, but this is definitely safer than using shared caches.
0379639
to
f9143e3
Compare
Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble. How to do that?
|
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.
The separate cache pools are a really nice touch 👍
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.
Nice work David 👌🏻
@dmaicher build is failing due to a deprecation in the 4.4 job. Do you want to take a look at that in a separate PR? Otherwise I'll do so in the afternoon. |
b8c1378
to
33b4cca
Compare
Don't worry: #1075. Just need to rebase :) |
33b4cca
to
fe28c21
Compare
Thanks David! |
Here is the recipe change: symfony/recipes#695 |
@alcaeus @greg0ire @nicolas-grekas when we release this it will possibly cause some issues for users that don't also sync the latest recipe change (symfony/recipes#695) It will leave them without proper caching for prod environments 😢 Not sure how we can avoid this though... But maybe chances are high that new projects started with fresh 1.12/2.0 recipes are not running in production yet? 😄 🙈 |
I think we cannot avoid it, but before it was an implicit dep on my project, so I ended up automatically using 2.0 via composer update. I had to manually require 1.x. But nevermind we need to fix it and help the Community to get a working state back again 👍🏻 |
Fixing this is not a question of if, but how. The current upgrade path may cause some pain for people in production if they don't upgrade their flex recipes or don't update their config manually, but I believe we can account for that by communicating the change appropriately. |
This fixes #1069 and #1066
It restores the previous behavior from 1.11 where we use an array cache by default if no other cache is configured.
The cache pools will also show up in the profiler:
TODOs: