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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willzyx created an issue. See original summary.

willzyx’s picture

Status: Active » Needs review
FileSize
1.62 KB
willzyx’s picture

Title: Cache bins are not deleted deleted when the module that declares them is uninstalled » Cache bins are not deleted when the module that declares them is uninstalled
Issue tags: +Needs tests
Berdir’s picture

Status: Needs review » Needs work

Looks good to me, needs work for tests.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
5.1 KB

Went in a different direction from cache_bins_are_not-2695109-2.patch

Other changes to existing code:

  • Reduced nesting
  • No longer need to catch exceptions, interface signature no longer throws. Existing implementations catch.
  • Added tests.

Status: Needs review » Needs work

The last submitted patch, 8: 2695109-cache-bin-cleanup.patch, failed testing. View results

dpi’s picture

dpi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2695109-cache-bin-cleanup-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dpi’s picture

dpi’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

It 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. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
    @@ -498,34 +498,30 @@ class ModuleInstaller implements ModuleInstallerInterface {
    +    $cache_bin_services = array_filter(
    

    +1: This code is way more readable than before, nice work!

  2. +++ b/core/modules/system/tests/modules/module_cachebin/module_cachebin.services.yml
    @@ -0,0 +1,7 @@
    +services:
    +  module_cachebin.cache_bin:
    +    class: Drupal\Core\Cache\CacheBackendInterface
    +    tags:
    +      - { name: cache.bin }
    +    factory: cache.backend.database:get
    +    arguments: [module_cachebin]
    

    Its nice to add its dedicated test module. +1 for that

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 2695109-cache-bin-cleanup-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dpi’s picture

Thanks for the review @dawehner

Coding standards and test fix.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #15.

xjm’s picture

Priority: Normal » Major

Ooh, this sounds major. It actually might even border on critical. Promoting to major for now.

larowlan’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -498,34 +498,30 @@ class ModuleInstaller implements ModuleInstallerInterface {
+      isset($definitions['services']) ? $definitions['services'] : [],
...
+        $tags = isset($definition['tags']) ? $definition['tags'] : [];

seriously wish we had null coalesce ?? i.e. php7

This looks good to me

Sam152’s picture

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -498,34 +498,30 @@ class ModuleInstaller implements ModuleInstallerInterface {
+    $definitions = Yaml::decode(file_get_contents($service_yaml_file));
+
+    $cache_bin_services = array_filter(
+      isset($definitions['services']) ? $definitions['services'] : [],
+      function ($definition) {
+        $tags = isset($definition['tags']) ? $definition['tags'] : [];
+        foreach ($tags as $tag) {
+          if (isset($tag['name']) && ($tag['name'] == 'cache.bin')) {
+            return TRUE;
           }
         }
+        return FALSE;
+      }
+    );

Can't we use the container to do this somehow? I suppose this doesn't cover services provided by a ServiceProvider either?

Sam152’s picture

Test looks good.

+++ b/core/modules/system/tests/modules/module_cachebin/module_cachebin.services.yml
@@ -0,0 +1,7 @@
+  module_cachebin.cache_bin:
+    class: Drupal\Core\Cache\CacheBackendInterface
+    tags:
+      - { name: cache.bin }
+    factory: cache.backend.database:get
+    arguments: [module_cachebin]

+++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
@@ -60,4 +60,29 @@ class ModuleInstallerTest extends KernelTestBase {
+  /**
+   * Tests cache bins defined by modules are removed when uninstalled.
+   *
+   * @covers ::removeCacheBins
+   */
+  public function testCacheBinCleanup() {

If we're specifically covering factories here, lets mention that in the service ID and the name of the test method too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2695109-cache-bin-cleanup-17.patch, failed testing. View results

dawehner’s picture

Can't we use the container to do this somehow? I suppose this doesn't cover services provided by a ServiceProvider either?

This tricky bit about doing that is the following:

  • You don't have the container definitions exposed as API on runtime, so you cannot do that
  • On compile time on the other hand you should not init services, as something might not be build yet

Maybe I am missing something obvious?

Sam152’s picture

Couldn't a simple service collector work here? I was thinking something along the lines of:

  • Use some service collector to get all services with the tag before uninstallation.
  • Uninstall the module.
  • Use the same service collector after, call the remove method on the ones which have disappeared.

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.

dawehner’s picture

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

Sam152’s picture

I 100% agree as per #25:

we should delegate discussion of something like the above to another ticket

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Oops, I got distracted by writing a comment :) There was some random failure above, so let's get it back to RTBC?

Sam152’s picture

I had some feedback on the current patch in #22, but very nitty so happy to not block on that.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

We managed to last a long time without noticing that one..

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Sam152’s picture

Re: #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.

  • catch committed 2c2e85c on 8.5.x
    Issue #2695109 by dpi, willzyx, Sam152, dawehner: Cache bins are not...

  • catch committed 9cd1c42 on 8.4.x
    Issue #2695109 by dpi, willzyx, Sam152, dawehner: Cache bins are not...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.