cache_clear_all will fail during module enable if the table being cleared does not exist yet. Usually, cache tables always exist at cache_clear time if they are defined in the module's schema method (as they should be). If a module adds a new cache table, then it should provide an update function to create it. However, when doing a major upgrade, the contrib module upgrade procedures at (referenced from both Drupal 7 and Drupal 8's UPGRADE.txt) instructs the user to enable the module before running updatedb. Enabling the module calls cache_clear_all, which will fail. The new table was not created by the install function, because the module was installed earlier, before the table existed in the schema, and it was not created by the update function because updatedb was not run yet. When upgrading from Drupal 6 to Drupal 7, the token module is one example of a contrib module that fails in this way.

The workaround is to ignore the error and run updatedb. If this is the sanctioned solution, perhaps the documentation should be changed to warn upgraders about this eventuality. Drush Site Upgrade could be modified to automatically try to run updatedb if the module enable fails, but this solution seems slightly inferior.

The attached patch adds a check in cache_clear_all to see if the table exists yet before clearing it. This allows the upgrade of modules that add cache tables to their schema to proceed cleanly. D8 has a different cache implementation, but it would not be hard to do the same thing there. I'd like to hear what the general consensus on the desired behavior should be here before doing that, though.

Edit: The patch here is not good, and should not be used. See #1 below.

#6 1477932-invalid-cache-bin-no-errors.patch2.61 KBDave Reid
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
drupal-7-cache-clear-all.patch606 bytesgreg.1.anderson
PASSED: [[SimpleTest]]: [MySQL] 38,304 pass(es).
[ View ]


bleen18’s picture

Couldn't this solution have bogus results on a site that uses Memcache (or whatever) instead of DB-based caching. On a site like that the DB table might not exist but the cache bin should be cleared anyway.

I realize that a module that creates its own cache table would still be creating the table even if a site uses Memcache, but I just want to bring up the point.

Assuming that this is not and issue and assuming that db_check_table is as fast as I think it is (cache_clear_all is already a sloooow process) then I think that silently skipping a cache bin that doesn't exist is a perfectly aceptable solution.

greg.1.anderson’s picture

I can't remember if there was some reason I didn't just test if _cache_get_object($bin) returns NULL. If that does work, it would be a cleaner solution. I'll give that a try when I have a chance to work on this again; thanks for the feedback.

xjm’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work
Issue tags:+Needs tests, +needs backport to D7

Thanks for the patch! If the bug also exists in D8, then we'd need that patch first, as per the backport policy.

Additionally, a couple notes about the patch: the inline comment block is wrapping a bit too early (it should go as close as possible to 80 chars without going over); and we'll need to have an automated test demonstrating the bug. I don't know enough about the implications of the change to evaluate beyond that. :)

Dave Reid’s picture

Ah! I ran into this with #1512934: When upgrading from 6.x, token_cache isn't created, causing error on enablement and the token module creating its cache_token table in token_update_7000() and drupal_flush_all_caches() being called before my module's update hook was run. I agree we should let cache_clear_all() fail gracefully in this case.

iamEAP’s picture

I wonder if it makes more sense to alter the logic behind when install_schema gets called instead of checking to make sure the table exists before clearing it. Seems like there's not much sense in adding extra overhead to cache_clear_all when this could be taken care of in module_enable.

Dave Reid’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
new2.61 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I think this is probably the best way this can be fixed with Drupal 8:

1. In DatabaseBackend::_construct() if db_table_exists() fails, then throw an exception about the invalid bin.
2. Catch errors in the cache() function and log it with watchdog_exception, and then use NullBackend as the backup so that calls to the cache continue to not cause more errors.

I've added I think what is close to enough test coverage for this to cache.test, but I wasn't sure if we needed to test for the watchdog exception. I could use some validation to ensure I'm on the right path for this. This will be easier to backport to D7 thankfully.

greg.1.anderson’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests

If #2 works, then there will be no additional overhead. I have not had time to work on this, though, so I don't yet know if that change is okay.

I would advise against #5, as this would likely have unintended consequences. In a major upgrade, install_schema is not called; instead, Drupal relies on the module's update hooks to manually modify the table entries so that they match the schema. The update hooks must run in order, as update hooks that come before a schema change might rely on the disposition of the old schema. If you went and changed the order of install_schema to force the db tables into their correct and most modern format, you might break some of the update hooks that had not run yet. Cache clear is already an expensive operation; I think it is better to be safe, even if it does cost extra cycles at cc time.

greg.1.anderson’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests

Accidentally saved over Dave Reid's status changes.

Dave Reid’s picture

A followup to #6 is that we could probably use a new CacheException class or similar so that we're not catching *everything*.

Dave Reid’s picture

Title:cache clear behavior in module enable interferes with major upgrade process when modules introduce new cache tables» Allow cache() and cache_clear_all() to fail gracefully if a database cache bin does not exist

Re-titling for the direction of this issue

Status:Needs review» Needs work

The last submitted patch, 1477932-invalid-cache-bin-no-errors.patch, failed testing.

greg.1.anderson’s picture

I am ambivalent about the throw/catch in #6. I agree that a missing cache table can be considered an error condition, and it's okay to catch an error condition and still continue processing. However, consider the benefits of re-implementing with a Factory method pattern:

In function cache():

    $factory = $class . "::build";
    $cache_objects[$bin] = $factory($bin);

In class DatabaseBackend:

  public static function build($bin)
    if (db_table_exists($bin)) {
      return new Drupal\Core\Cache\DatabaseBackend($bin);
    else {
      // Probably log this with watchdog
      return new Drupal\Core\Cache\NullBackend($bin);

(Additional static factory methods needed, one for each cache class.)

Note that now we can substitute a result object of a different class if necessary in our static factory method. I acknowledge that in this particular use case, the benefit is marginal. In other cache classes that may fail for similar reasons, we would also need to add another if () {} else { return new NullBackend($bin); } to handle each potential failure case, which is similar to throwing an exception (fail fast) in the alternate implementation in #6 (except that the code might already be failing fast, in which case nothing additional would need to be done).

However, for other potential use cases, if you need to provide a different class than the standard default when dynamically creating a class object, you're simply stuck if you're not using a static factory method to create the object. For this reason, in other OOP systems I work with, I always prefer to use factory methods in instances where the class of the object being built is dynamically constructed.

Presently, there are 28 instances of $var = new $something In Drupal 8; 10 of those are in Symfony. It would be quite a sharp left turn in this issue to pick up and start using static factory methods. Perhaps the use of static factories has already been discussed and rejected. In any event, I think that the patch in #6 is okay as written; I just wanted to make sure this alternative was considered. I apologize that this is not a great venue to do it in.

tstoeckler’s picture

Version:8.x-dev» 7.x-dev

Just stumbled on this. In D8 cache bins are now created automagically. But in D7 this is still a problem.

indydas’s picture

Came across this in D7 too. Concur with

Will this be fixed?

indydas’s picture

Issue summary:View changes

Add a warning against use of the patch here.