The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors

There is still a possible silent bugs due to cache stall during install, the install backend remains a partial implementation that doesn't save caches but still delete cruft thanks to delete() deletePrefix() and flush(), we might want to keep it until we get back working on the install issue.

The patch looks good. With the test itself, would it be simpler to just use the DatabaseBackend and InstallBackend classes directly in the test? Seems like we could avoid both the page requests and the helper module in that case. This is explicitly testing the interaction of those classes with each other, so I think it's fine if it doesn't go through the cache factory.

This patch is looking good (it is simple and quite straightforward), I won't do any whitespace review, if it passes tests it's enough for me. I'd say it's RTBC.

This looks fine to me too, marking RTBC.

#10: installer-cache-1394648-10.patch queued for re-testing.

I'll take the CNR change as an even less bold RTBC since this has come back green. I think the only thing that needs to be backported here is the tests (unless I missed something), so moving to 'normal task' for that. Committed/pushed to 8.x.

Just backported the test only as per catch in #19. Was curious to try out my git backporting method using rebase. It works well (not to say test will pass). :)

Fatal error: Class 'DatabaseBackend' not found in /var/lib/drupaltestbot/sites/default/files/checkout/modules/simpletest/tests/cache.test on line 449

This class has probably been renamed during a PSR-0 conversion; check the change notices or git blame history (http://www.kernel.org/pub/software/scm/git/docs/git-blame.html, http://drupal.org/node/1462360#comment-5677360) to find the new name. Then, update the patch.

attached is a patch that fixes the class names that were changed from 7.x->8.x. It also removes the tests for the methods that don't exist in the old implementation. This test is failing with the error:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8.simpletest320671test' doesn't exist: DELETE FROM {test} WHERE (cid = :db_condition_placeholder_0) ; Array ( [:db_condition_placeholder_0] => cache_one )

The tests run now. There was an assertion asserting false on something that would always be true

In DrupalFakeCache, isEmpty() is overriden to do this:

  function isEmpty() {
    return TRUE;
  }

There are assertions testing this in the test. That may be fine, but I feel like we need some more documentation on it. It certainly had me scratching my head.

Thanks @naxoc and @disasm!

The issue is filed against 7.x currently, so we do want to test the D7 patches. (So we should remove the -do-not-test from the filename.) Ideally, we should also post a tests-only D7 patch along with the complete patch, in order to verify that the backported tests are providing coverage. Like so:

Thanks for providing the interdiffs; it's very helpful.

+++ b/modules/simpletest/tests/cache.testundefined
@@ -446,13 +446,13 @@ class CacheInstallTestCase extends CacheTestCase {
     // Store an item in the database cache, and confirm that the installer's
     // cache backend recognizes that the cache is not empty.
     $database_cache->set('cache_one', 'One');
-    $this->assertFalse($install_cache->isEmpty());
+    $this->assertTrue($install_cache->isEmpty());
     $database_cache->clear('cache_one');
     $this->assertTrue($install_cache->isEmpty());

Agreed @naxoc; this change makes me nervous. It seems to directly contradict the comment there? Here's the same hunk in the D8 patch:

+++ b/core/modules/system/tests/cache.testundefined
@@ -457,3 +459,144 @@ class CacheIsEmptyCase extends CacheTestCase {
+    // Store an item in the database cache, and confirm that the installer's
+    // cache backend recognizes that the cache is not empty.
+    $database_cache->set('cache_one', 'One');
+    $this->assertFalse($install_cache->isEmpty());
+    $database_cache->delete('cache_one');
+    $this->assertTrue($install_cache->isEmpty());

Definitely asserting false, which means there's a difference in the functional code. We need to debug this further... Meanwhile, though, let's get those test-only and combined D7 patches for the bot to test. :)

Here are the patches renamed and reuploaded per #25.