Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs review
FileSize
5.07 KB

This is pounard's patch minus DrupalNullCache.

pounard’s picture

Sub.

David_Rothstein’s picture

Per my comments in #1082328: Provide a proper no-op cache.inc the above patch has some problems.

Here's a different attempt to allow DrupalNullCache to be used, though. Basically, the approach is to use DrupalNullCache until the database is available, and then immediately switch over to the database cache as soon as possible. It turns out this is actually the approach that was tried originally to fix this issue a long long time ago (#151280: After install, old cached variables are used instead of those chosen using installer), but things have gotten more complicated since then so it's no longer quite as simple.

I also wrote some tests in the attached patch (we can't test the installer directly yet, but we can do a pretty good job of simulating it).

After writing this, I'm not really sure if this approach is worth it or not, though. This patch seems to slow down the installer noticeably (lots of cache clearing that is no longer a no-op, I guess). Also, it has the potential to reveal some nasty Drupal caching bugs of its own that might get triggered only in the installer context. (For D8, that might not be so bad, though; any bugs this surfaces will be real caching bugs that are worth fixing regardless.)

Either way, I think the tests (and the basic structure here) are worth keeping, even if we don't wind up actually dropping the installer-only cache implementation, so let's see what happens.

pounard’s picture

The isAvailable() method is an alias of another patch I'm actually working on at #1164484: Cache backend consistency runtime check and graceful downgrade mecanism, bot issues should share this, at least, would avoid duplicating effort here.

You probably don't need the cache_implementation_override() functions, you can do it pragmatically in the install.inc file at the right places, right? Having a one line function that will be used only at only one place seems a bit of over-engeenering.

Basically, this patch looks like a lot of what we came up with catch, actually, it does the exact same thing except you recheck for the right backend each install.php hit, I prefer your method compared to the one that just got reverted based on DrupalFakeInstall, except I'd remove the procedural proxy functions you added which does not really serve any purpose since you can do whatever you do inside directly at the right place. For example, you are calling cache_is_available() why don't you just do cache('cache)->isAvailable() instead?

Another goal of not having a real cache in install.php is that the installer will never stop set and clear caches, all the time: doing real call to the database may prove itself less performant that actually working without cache at this point.

pounard’s picture

From what I remember, SimpleTest proceeds with installation using a specific procedure, and doesn't really let us test the installer, but I might be wrong here: it's a really hard thing to really test.

David_Rothstein’s picture

You probably don't need the cache_implementation_override() functions, you can do it pragmatically in the install.inc file at the right places, right? Having a one line function that will be used only at only one place seems a bit of over-engeenering.

Well, maybe. I thought it would be preferable to avoid having code outside the cache system call drupal_static_reset() on one of the cache system's internal variables, though; it's cleaner to provide an API for at least that part of it. Also, it's possible someone else besides the install code will eventually have a similar use case.

For example, you are calling cache_is_available() why don't you just do cache('cache)->isAvailable() instead?

In this case I was just following the current pattern in cache.inc, which seems to provide procedural wrappers for things even when they are simple (see for example http://api.drupal.org/api/drupal/includes--cache.inc/function/cache_is_e... which is basically the same kind of wrapper).

Another goal of not having a real cache in install.php is that the installer will never stop set and clear caches, all the time: doing real call to the database may prove itself less performant that actually working without cache at this point.

Yeah, exactly. As per my above comment, it does empirically seem to be slower. I am not sure if that's due to Drupal's caching being inefficient or if it's just inherent in the fact that the installer saves a lot of data (and therefore writes to and clears caches a whole lot), but it's the main reason I am a little hesitant about this patch after writing it.

From what I remember, SimpleTest proceeds with installation using a specific procedure, and doesn't really let us test the installer, but I might be wrong here: it's a really hard thing to really test.

Yes, we cannot test the installer directly. (#630446: Allow SimpleTest to test the non-interactive installer and #1215104: Use the non-interactive installer in WebTestBase::setUp() are the issues for changing that.)

But I think the test I wrote here comes close enough to be useful, in the sense that it reproduces the conditions that trigger the bug (and I confirmed that if we switch to using DrupalNullCache always, it does result in the expected test failure).

catch’s picture

In this case I was just following the current pattern in cache.inc, which seems to provide procedural wrappers for things even when they are simple

While that's the case now, it won't be for long: #1272706: Remove backwards compatibility layer for cache API.

sun’s picture

#1215104: Use the non-interactive installer in WebTestBase::setUp() actually requires the following change from this patch in order to work:

+++ b/includes/cache.inc
@@ -16,13 +16,12 @@
 function cache($bin = 'cache') {
+  $cache_objects = &drupal_static(__FUNCTION__, array());
...
-  // We do not use drupal_static() here because we do not want to change the
-  // storage of a cache bin mid-request.
-  static $cache_objects;

In turn, we might have a circular dependency here. Are you guys OK with committing this particular change in #1215104: Use the non-interactive installer in WebTestBase::setUp() ?

sun’s picture

Issue summary: View changes
Status: Needs review » Closed (duplicate)

This issue is obsolete, as the installer uses a memory backend already.