-
-
Notifications
You must be signed in to change notification settings - Fork 454
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
Extend dbal config and extension for result cache #1721
Conversation
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 guess this would be enough indeed 🤔
Why not making it consistent with ORM? There, it's like this DoctrineBundle/Resources/doc/configuration.rst Lines 702 to 704 in 0ad7b24
|
Yes, good point, @ostrolucky, consistency would be great. meaning the difference would be that there is a default ArrayCache registered if type pool is used without identifier? I guess that feature made more sense before #1352 - or do I miss sth? Edit: not saying it doesn't make sense to bring in that behavior as well, just only if I miss sth to understand before I do the change |
My comment was mostly aimed at the options, not internal workings. In this PR you accept only service ID, while in ORM version you can choose if you use service id or a pool. But perhaps you are right, since DBAL supports symfony pool only, perhaps we should bite the bullet and just accept that ORM and DBAL node will be different and accept just service id. Any opinions from other maintainers on this? |
I think its fine to just allow a service id. Actually every cache pool is registered as a service with the pool name as service id anyway, right? So this will allow using cache pools or any custom service (as long as its a PSR cache). The ORM config technically also still supports the deprecated |
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.
Fair enough. Let's keep it as this then. Test wouldn't be bad either, but can't really go wrong with such simple change either.
This should go into 2.11.x though. Could you rebase it @chr-hertel ? |
f5d5772
to
0a4a84e
Compare
Target changed. No need to rebase :) |
Too late :D |
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.
let's add a very simple test case so at least the feature is also "documented" via tests
0a4a84e
to
5df53b1
Compare
5df53b1
to
33878a4
Compare
Thanks @chr-hertel |
This is super embarrassing, what do I overlook, this can't be the solution for #114, right?
tested it successfully with symfony demo: