[FrameworkBundle] Add CachePoolClearerPass for weak cache pool refs in cache clearers #19900
Conversation
fafff3c to
c09deb5
Compare
| } | ||
| if (isset($attr['clearer'])) { | ||
| $clearer = $container->getDefinition($attr['clearer']); | ||
| $clearer->addMethodCall('addPool', array(new Reference($id))); |
There was a problem hiding this comment.
I'd rather replace constructor argument with new one.
There was a problem hiding this comment.
This is how it is now, there is no constructor. I was about to add it, but wouldn't that enforce an unnecessary stronger binding between the actual current implementation and the way things works? Here, the only thing the code needs to know is that an addPool method can be called. Looks a looser binding than a constructor argument to me. Given that this is not critical perf code to say the least, I think it should be kept asis.
There was a problem hiding this comment.
The compiler pass is already implementation-aware, because addPool is not part of any interface as I can see. It's more like Psr6CacheClearerPass to be honest.
And I think it's OK for compiler pass to be locked on specific implementation, DIC is the place for this knowledge.
There was a problem hiding this comment.
So, what's your argument supporting the change? (you have mine :) )
There was a problem hiding this comment.
Given you bother with isset vs array_key_exists performance, I wonder why you didn't take into account this one. Same kind of micro-optimization for me. :)
And yet another mutable service looks painful, just for sake of clean code.
There was a problem hiding this comment.
Given you bother with isset vs array_key_exists performance
I don't know how to react to this. Is it ironic? Or are you wondering why here performance is not and issue, and why it is in the container methods? I'll take the second option. A clearer is called every once in a while. On the container, get can be called thousands of times. 1000xsmall-optim == big optim. That's why it matters there.
There was a problem hiding this comment.
I just wanted to say that I see no reason to prefer setter injection over constructor one. Same level of coupling for me, better performance of constructor injection, cleaner API, less side-effects, etc.
I see that the class has no constructor, but it's not an issue to add it.
Even if somebody added his own implementation, he'd look into this source code and find out that he needs to add addPool method or adapt constructor signature, i.e. same kind of changes. How setter is looser coupling?
There was a problem hiding this comment.
Even if somebody added his own implementation, he'd look into this source code and find out that he needs to add addPool
That's my point yes, and I find that having a convention that applies to a method is more open that one that applies to the constructor. That's why I said looser coupling.
Anyway, this is not directly related to this PR, which is just moving this line of code around, nothing new here.
If you really care, I propose you open a PR to change that.
c09deb5 to
65dab3e
Compare
| /** | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class CachePoolClearerPass implements CompilerPassInterface |
There was a problem hiding this comment.
Why this class is open for inheritance (not marked as final)?
65dab3e to
ce4bf44
Compare
ce4bf44 to
c4b9f7d
Compare
| $clearer = $container->getDefinition($attr['clearer']); | ||
| $clearer->addMethodCall('addPool', array(new Reference($id))); | ||
| } | ||
| if (array_key_exists('clearer', $attr)) { |
There was a problem hiding this comment.
I don't understand logic behind this code (array_reverse + break). Do you want to emulate overriding of the tag? Does it mean that a service cannot have 2 different clearers?
There was a problem hiding this comment.
cannot have 2 different clearers
True: neither in 3.1 nor with this PR. Could be worth it, I'll give it a try.
There was a problem hiding this comment.
But not in this PR, that's another topic :)
|
Thank you @nicolas-grekas. |
…che pool refs in cache clearers (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [FrameworkBundle] Add CachePoolClearerPass for weak cache pool refs in cache clearers | Q | A | ------------- | --- | Branch? | master | New feature? | yes | Tests pass? | yes | License | MIT On 3.1, when a cache pool is private and not injected anywhere, it is still added to its clearer service. The `CachePoolClearerPass` fixes this by referencing pools in clearers only after the removing passes. Commits ------- c4b9f7d [FrameworkBundle] Add CachePoolClearerPass for weak cache pool refs in cache clearers
On 3.1, when a cache pool is private and not injected anywhere, it is still added to its clearer service.
The
CachePoolClearerPassfixes this by referencing pools in clearers only after the removing passes.