Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
After #2470693: Upgrade to Symfony 2.7.0 cache bins are not more deleted when the module that declares them is uninstalled. This because ModuleInstaller::removeCacheBins() do not take into account the factories definition usage introduced in the upgrade to symfony 2.7
Typical service definition after the upgrade:
cache.xxx:
class: Drupal\Core\Cache\CacheBackendInterface
tags:
- { name: cache.bin }
factory: cache_factory:get
arguments: [xxx]
In ModuleInstaller::removeCacheBins() you check for 'factory_service' and 'factory_method' (which are not more used):
if ($tag['name'] == 'cache.bin' && isset($definition['factory_service']) && isset($definition['factory_method']) && !empty($definition['arguments'])) {
//delete cache bin
}
therefore cache bins are never deleted.
This issue affects various core modules (eg toolbar, dynamic page cache, etc) in 8.0, 8.1 and 8.2 branches
Comment | File | Size | Author |
---|---|---|---|
#17 | interdiff-2695109-13-17.txt | 1.36 KB | dpi |
#17 | 2695109-cache-bin-cleanup-17.patch | 5.13 KB | dpi |
#13 | interdiff-2695109-10-13.txt | 1.05 KB | dpi |
#13 | 2695109-cache-bin-cleanup-13.patch | 5.12 KB | dpi |
#10 | 2695109-cache-bin-cleanup-5.patch | 5.13 KB | dpi |
Comments
Comment #2
willzyx CreditAttribution: willzyx commentedComment #3
willzyx CreditAttribution: willzyx commentedComment #4
BerdirLooks good to me, needs work for tests.
Comment #8
dpiWent in a different direction from cache_bins_are_not-2695109-2.patch
Other changes to existing code:
Comment #10
dpiIncorrect prefix
Comment #11
dpiComment #13
dpiLeft out the filter.
Comment #14
dpiComment #15
dawehnerIt is hard to believe we didn't have any kind of test coverage for it :)
I don't believe we no longer needs tests ...
+1: This code is way more readable than before, nice work!
Its nice to add its dedicated test module. +1 for that
Comment #17
dpiThanks for the review @dawehner
Coding standards and test fix.
Comment #18
jibranBack to RTBC as per #15.
Comment #19
xjmOoh, this sounds major. It actually might even border on critical. Promoting to major for now.
Comment #20
larowlanseriously wish we had null coalesce ?? i.e. php7
This looks good to me
Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCan't we use the container to do this somehow? I suppose this doesn't cover services provided by a ServiceProvider either?
Comment #22
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest looks good.
If we're specifically covering factories here, lets mention that in the service ID and the name of the test method too.
Comment #24
dawehnerThis tricky bit about doing that is the following:
Maybe I am missing something obvious?
Comment #25
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCouldn't a simple service collector work here? I was thinking something along the lines of:
Although, given this issue fixes a specific bug, we should delegate discussion of something like the above to another ticket as to not hold up the fix.
Comment #26
dawehner@Sam152
Do I understand you correctly, you want to keep a service from before the container rebuild around and use that afterwords? I think it could work.
For me though it feels like changing the strategy completely by moving away from open the yml file sounds more like a follow up. What the patch does right now is probably the minimal way to fix the problem, so maybe the better strategy would be to open up a follow up?
Comment #27
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI 100% agree as per #25:
Comment #28
dawehnerOops, I got distracted by writing a comment :) There was some random failure above, so let's get it back to RTBC?
Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI had some feedback on the current patch in #22, but very nitty so happy to not block on that.
Comment #30
catchWe managed to last a long time without noticing that one..
Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #31
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRe: #26 I think the ultimate solution is to probably move the responsibility of uninstallation to hook_uninstall vs a method on a service. I can't think of any other things that do this and it obviously has certain limitations.