Provide a proper no-op cache.inc

The null object cache implementation already exists, it's just misused, could this be merged with http://drupal.org/node/1164484 ? You commented there and both can be fixed one shot (and is definitely backportable).

Linking #605880: Settings and menus broken after install due to faulty caching and #1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support

For the first one, a less hacky solution could be to set a variable during installation that tells core to use only the fake implementation, then remove it during the last install step, so that during the full installation time, even during parallel ajax requests, no cache backend at all would be used.

Ok, same with the solution found by me and catch over the IRC to ensure no regression regarding #605880: Settings and menus broken after install due to faulty caching.

Note that I couldn't manage to reproduce the initial bug. In theory, this patch should wipe it out, but it remain in theory, please anyone that has been throught the original issue could retest this, it would be a lot of help.

Ohh the variable_del() does not seems to work, I have to review the patch upper.

I found the original bug. All is happening in install_finished() function. The drupal_flush_all_caches() is triggered before some calls to variable_set() and drupal_cron_run() which causes the those to cache data from installation script. Another solution instead of using the ugly original hack could have been to change the default backend in $conf in the very beginning of the function then move the cache flush to the end of this function.

Now that the temporary install variable that sets the cache backend to the null object implementation is fully working, there is no need to do this cache flush anymore, I simply removed it.

Here is the patch, that ensures no regression for good, and works gracefully.

Now that I found a cache entry on which I can be sure to reproduce the bug, I can write a unit test, I think, quite easily: because I add the default cache class variable entry, I can test for its existence after the Drupal instance has been installed: if present, then the variable cache has stalled, else all is ok.

Except for the test part, I was looking up at simple test setUp() method of DrupalWebTestCase and it installs only partially a Drupal site, using its own install procedure: we cannot test install (the cache part is skipped and no fake cache is used at all AFAIK). So we won't have test for this!

I'm not sure what do you think, but this change really is trivial, I think this is RTBC.

Ouh false positive, my site is behaving weirdly. Wait a bit.

variable_del() force the cache bin to init before it unsets the global, we have two way to fix this:
1. Force the global unset before calling variable_del() [ugly]
2. Patch variable_del() to unset the global before actually wiping the cache enty.

So real cause is the admin/timezone call in the latest form of installation procedure. As catch highlighted, this one bootstraps a real Drupal and forces variables to go cached.

Problem of caching variable during bootstrap is that the default cache backend is not set in $conf (because it's in database) so it caches via the default backend.

This seems to be a totally normal behavior, for us the problem remains that the system/timezone call does initialiaze and cache variables.

After a long fight with install procedure in parallel of a long chat with catch, we finally reverted the cache() static cache to use drupal_static(). This ensures we can remove currently set cache backend at install_finished() time and allows us to really clear all partial cache that may have been set during installation. This also ensures that later system cron run and other install_finished() business stuff will end up by correctly caching what's done in there.

Patch attached, no regression.

Seems good to me yes, let me fix it.

This is still bleeding over 80 chars and has a trailing space.

Not in my editor, max is 79 chars, no trailing space, if you are using some kind of tool to tell me that, then your tool is definitely wrong.

Fixed the indentation problem.

EDIT: Please requeue for testing.

AHHHH PLEASE STOPPPP!

We should definitely keep in mind that teh cache stall is only because of the system/timezone hit on the last form, over a partially installed Drupal. These kind of hits should be definitely get rid off, even if I spent somehting like 3 hours to find a not-so-hackish-at-least-less-than-the-original-patch that fix it does not mean that people can do that without GUILT!

As beejeebus suggested, I reverted the variable_del() statements order, even if it feels wrong somehow at least it does not imply any regression to revert it.

This patch looks good. I was wondering though, is DrupalFakeCache the best name?

When I was reviewing this patch, that name wasn't intuitive at first. There may be a more descriptive name? I can't think of a better name though.

Not a big deal and it shouldn't hold up this patch, but I figured I'd share my experience. :)

@Dries I agree, naming is bad, as it is a null object pattern implementation, either Null, Dummy or Neutral can fit, just as catch I'd go for Null personally, it's semantically more explicit.

Attached patch with the DrupalNullCache name, anticipating a "yes" here.

This is currently tagged for backport for D7. Can someone explain to me why it's safe to both move and rename this caching backend in D7 in the 9th stable point release? This seems like a D8-only fix to me.

It also refines a previous patch making it cleaner with less code lines without any regression: whichever looses code without loosing any feature and without inducing an API change (and even adding a cool feature for testing) seems like a good idea for stable?

@webchick: has it been commited to D8? I have some other patches waiting for being fixed following this one! I guess that the D7 backport may be arguable nevertheless D8 should move forward.

class DrupalFakeCache extends DrupalNullCache {} // Compat fix

I like the name DrupalNullCache so I committed the patch to 8.x.

I don't think this 'needs' to be backported. It 'could' be backported. Moving to 7.x for @webchick.

Yeah, I'd rather leave it at D8 myself. Dave Reid confirmed on IRC that he's using DrupalFakeCache in a few unit tests, so there is precedent for this already being used in contrib for D7.

If you feel strongly that's the wrong decision, speak up.

My one line compat fix still ensure no regression: we can also mark the compat class a @deprecated at the same time with a bit of documentation about DrupalNullCache. It's my opinion i'd understand you don't want it, but I think it's a loss not to.

If we do that, that approach needs to be ported and tested; it's not RTBC.

I agree, but it worth the shot, at least testing it right?

I don't really understand the point of changing it in D7, no. Especially since people are already counting on the existing functionality, and (presumably) already include_once()ing the file it exists in currently. Maybe an up-to-date issue summary would help.

The changes here are:
* D7 calls the class DrupalFakeCache, but it's not fake at all since it does database clear calls: it's an odd inconsistency (inconsistency between the name and what the class really does) and fixing that wouldn't hurt anyone.
* It would be better for people that'll discover this class to have a proper documentation and not some hacky code.
* Plus, it provides a patch which seems (I say seems, but I think it is I might be wrong) cleaner that was the previous one solving the same problem..

The include_once is the only thing that I fear: if people used include_once and not require_once, it should be fine and cause no further problems: I think they are not a lot in the world right now (you need to go and read the install code know this stuff exists, that excludes a lot of people).

By making no API change and by the fact the code moves to cache.inc we will make it reachable to developers and site builders that will read this code (it's more likely than anyone reads cache.inc more than *-install.inc): and providing a better documentation might make some people using the DrupalNullCache as it should be used (I think it's a valuable feature to have a proper and properly documented null object cache implementation).

That's pretty much all I have to say about this, I do not have any other argument here: the ball is into your hands.

I'm concerned about the original patch that was committed here.

Although it does seem to make sure the caches are consistent at the end of the installer (and therefore avoids the specific problem we ran into at #605880: Settings and menus broken after install due to faulty caching), what about the more general issue of cache consistency while the installer is running?

In other words, unless the installer clears the database cache every time a cache-clearing event happens (which it did before this patch was committed), any AJAX request that occurs during installation will be at risk of using outdated data. And if an AJAX callback calls variable_get() or other API functions and gets the wrong data back, all sorts of fun things could go wrong...

In the install profiles that ship with core, it looks like we only make AJAX requests to system/timezone and admin/config/search/clean-urls/check, which don't seem to care about the state of the database (and both happen on the same form anyway so there's less of a risk that one will populate the cache with stale data and the other will later use it). So I don't think we have any problems there. But what about other install profiles? People out there are definitely writing some crazy install profiles with fancy UIs, and we don't want to break their ability to use as much AJAX as they want to and in whatever stage of the install process they want to.

So I think we need to add back DrupalFakeCache for now (probably renamed to DrupalInstallCache while we're at it). We can still keep DrupalNullCache, of course, but we shouldn't use it in the installer for the time being. A separate issue could be opened to explore if there are other ways to do that.

I'd say that no AJAX calls at all should be made, hitting a partially installed site *is* an issue. I agree with your concern though, but during the whole installation process when important stuff (installing modules and misc. task submit handlers) are done in a no cache at all context (install.php / install.inc), so in theory all the installation process is totally fine: only the AJAX calls are not and those are only one I could figure out (the system/timezone was the only one populating caches).

This needs to be rolled back and rerolled as an addition instead of a replacement. Sorry, but this elevated the problem into a major bug.

Fair enough

As such the only time it could possibly make an AJAX callback is:

1. in hook_install() (I hope it wouldn't, that would be strange).

2. in hook_form_alter() on the install configuration screen - which is also where those system/timezone and etc. requests come from.

Nope, install profiles can also implement hook_install_tasks() and hook_install_tasks_alter(). This allows them to change the installer completely. Many profiles are using this to e.g. add a series of extra screens at the end (multi-step forms, etc), and any of those could easily have AJAX requests.

I think I have a patch that might allow this all to work though. I will post it on the other issue.

For this issue, I agree that just adding DrupalNullCache is a good candidate for backport to D7.

Whatever profiles can add, they surely should not call AJAX request over the site being installed, it's heretic to do that: they can use all the API they want as long it still run inside the install.php bootstrap where all is OK.

Well, we can't tell them not to use AJAX at all - that would be too restrictive.

Maybe it's possible to make it so the AJAX system can be run entirely inside install.php, though. That would be another avenue for dealing with this problem; I'm guessing it would be hard to do, but not really sure.

Right, that's basically what the patch I posted at #1297136: Use DrupalNullCache for the installer tries to do. We basically have to switch to the database cache as soon as the database is available.

In most practical cases these extra screens will all be at the very end of the installer, but they don't have to be. For example, if someone wanted to build an install profile that used http://drupal.org/project/project_browser to allow the user to download/enable modules during install, something crazy like that, in that case they'd probably swap out earlier stages of the installer (such as the module installation batch) with their own custom code also.

Hum, maybe this kind of crazy feature should happen on site first hit and not in the install context anymore, as a wizzard or such. It'd make much more sense IMHO. Isn't it a good reflexion topic? It would allow to do really crazy complex/complete stuff while the installer would remain simple and pretty much failproof!

Yeah, there are a couple issues for that already. (See #525594: Installation should consist of 2 phases instead of one and #1226380: Generate minimal mocked or SQLite environment and use to install Drupal in full environment.) Lots of unanswered questions about exactly how that would work, though.

Yes, this sound a proper path to explore IMHO! Installer supporting requests to sites it install sounds like a uterly complex thing to achieve safely.

EDIT: I woke up one of those issues, which was untouched since 2009. Hope this would lead to an interesting design to solve this problem.

Removing Novice tag.

I'm unsure what is supposed to be backported. Can anyone clarify?

Copied DrupalNullCache from D8, removed the methods that were only part of the interface in D8, and that's it. No added docs or references to it in the codebase.

67: drupal-1082328-67.patch queued for re-testing.

Ok, so the code looks fine. Not sure how to test it. Would be good to get this into Core. It's been sitting here for 2 years but still seems to work fine.

Do we still want this?