| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | install system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
In a clean database, I updated my CVS to the latest Drupal 7 just now, and ran the installer.
The last step asks me to fill in my site name, time zone, etc. I enter "D7 testing" in the Site name field.
Then I'm taken to the "Installation complete" screen where "D7 testing" appears to be the name of the site (based on the header text in the theme, as well as the HTML page title). So far, so good.
Then I click on the "your new site" link (which takes me to the site's front page). Now the header text says "Drupal" instead of "D7 testing", and the HTML page title indicates the site name is "Drupal" instead of "D7 testing".
Wen I refresh that page in the browser, the header text and HTML page title change, indicating the name of my site is "D7 testing".
The previous time I tested this (a few minutes ago with the same Drupal code), I had clicked around in the admin section for a while before clicking on a Home link, and the page title was still wrong all that time, until I visited the Site configuration page and saved the config.
So there seems to be some need to clear something in memory, cache, etc. there? Not sure what is going on, but it was a bit disconcerting, so I thought I would at least report it.
Comments
#1
This doesn't really seem minor to me, it's been annoying me a lot lately, and I can imagine it to be confusing when the site title changes on a page refresh
I've been trying to figure this out, but at the end of the install process, all caches are cleared right before output is returned and after the caches are cleared it still appears to get the right site_name
bumping this up to normal and changing the title slightly
if I go from the finished page to the front page by clicking the top link, I see a site name that is wrong, when I refresh, I see a site name that is correct
if I go from the finished page to the help page by clicking the help link, and then I click home in the breadcrumbs, I see a site name that is correct
if I manually go from the finished page to /admin, I get a notice that cron hasn't run yet and there's no status update, even though install.php calls drupal_cron_run(); right before returning output in install_finished();
so I'm totally oblivious as to why this is happening
#2
Ok ... the problem has to do with what variables get set using variable_initialize() immediately after the install completes. On that page load (and only that page load) we need to make sure that the variables are not loaded from cache because the cache is not fully formed (it seems)...
I feel like this patch may be overkill, but it solves the problem. Thoughts?
#3
awesomeness!
after applying this patch I was unable to recreate the problem \o/
we should really make a test for this though
#4
Some notes on code style:
+++ includes/bootstrap.inc 24 Oct 2009 03:40:14 -0000@@ -1510,7 +1510,13 @@ function _drupal_bootstrap($phase) {
+ // If $conf == empty array it means this is the very first page view after install,
+ // so dont let variable_initialize get data from cache
Sentences are finished by a period.
+++ includes/bootstrap.inc 24 Oct 2009 03:40:14 -0000@@ -1510,7 +1510,13 @@ function _drupal_bootstrap($phase) {
+ if($conf == array()){
Use
if (...) {.+++ includes/bootstrap.inc 24 Oct 2009 03:40:14 -0000@@ -1510,7 +1510,13 @@ function _drupal_bootstrap($phase) {
+ }else{
New line before "else" block.
}else {
+++ includes/bootstrap.inc 24 Oct 2009 03:40:14 -0000@@ -1510,7 +1510,13 @@ function _drupal_bootstrap($phase) {
+ $conf = variable_initialize(array(),true);
Use
$conf = variable_initialize(array(), TRUE);.+++ includes/bootstrap.inc 24 Oct 2009 03:40:14 -0000@@ -671,9 +671,9 @@ function drupal_get_filename($type, $nam
+function variable_initialize($conf = array(), $nocache = false) {
Use
FALSEinstead offalse.This review is powered by Dreditor.
Apart from that it looks good.
#5
@seutje
I'm not sure how to write a test that checks for something specifically on the first page view after installation. To be honest I'm *very* new to writing tests so any help on that front would be appreciated. In the mean time, can I mark this as "reviewed & tested..."?
#6
I have a hard time breaking my coding habits when developing for Drupal ... thanks
#7
Looks like this #524728: Refactor install.php to allow Drupal to be installed from the command line is the issue to push through if we want tests for installation.
#8
Regarding that last patch, I'm concerned about this change:
- if ($cached = cache_get('variables', 'cache')) {+ if (!$nocache && $cached = cache_get('variables', 'cache')) {
It looks like if $nocache is TRUE, then the $cached variable possibly won't be initialized. Is it used elsewhere, farther down in the function? Just noting this so someone can check it...
#9
AFAICT the problem occurs because install.php uses DrupalFakeCache, so the {cache} table isn't cleared when variable_set() is called at the end of the install process. I suggest you try to disable the fake cache as soon as the database is available, or you can simply flush the database cache manually at the end of the request.
#10
Both #2 and #9 seemed to have tracked it down pretty well... I think something like the attached patch might be what we actually want? (It works for me.)
It turns out that there are all sorts of index.php hits during the installer (I'm not sure exactly why, perhaps something to do with the batch or form API), and that is how things were getting in the cache. So the simplest thing we can do is have the installer cache implementation be extra safe and try to actually clear it.
I don't think we want to try switching away from DrupalFakeCache - we had that before, but intentionally removed it in #557542: Cache module_implements() . It was causing issues with command-line installs, among other things. It's best to trust the cache as little as possible during the installer, in my opinion.
#11
Just filed a related patch with regard to clearing up the code comments surrounding DrupalFakeCache and why we do not switch away from it:
#615528: Incorrect code comment regarding DrupalFakeCache in the installer
#12
When I first saw this issue in the queue a few days ago, I wondered if it was related to a problem with Update manager that I had noticed, which is that on a clean install, all the menu items for update manager don't really work. Here's the text I was about to submit as a separate (duplicate) issue:
Lots of people have run into this. It's easily reproducible on current HEAD. Just start from a completely clean HEAD install, install the default profile, and then try to visit admin/reports/updates. It fails until you rebuild your menu. Oddly, the first time you visit the page, it works. Then it's gone. Then it works again once you rebuild your menu. Some more data:
As soon as the installer finishes and gives you the "Drupal installation complete" landing page, here's what {menu_router} looks like:
mysql> select path, title from menu_router where path RLIKE 'admin/reports/updates';+--------------------------------+------------------------------------+
| path | title |
+--------------------------------+------------------------------------+
| admin/reports/updates/list | List |
| admin/reports/updates | Available updates |
| admin/reports/updates/update | Update existing modules and themes |
| admin/reports/updates/install | Install new module or theme |
| admin/reports/updates/settings | Settings |
| admin/reports/updates/check | Manual update check |
+--------------------------------+------------------------------------+
6 rows in set (0.00 sec)
Then, as soon as you visit admin/reports/updates for the first time, {menu_router} is like so:
mysql> select path, title from menu_router where path RLIKE 'admin/reports/updates';Empty set (0.00 sec)
Finally, once you rebuild the menu, it's back to this:
mysql> select path, title from menu_router where path RLIKE 'admin/reports/updates';+--------------------------------+------------------------------------+
| path | title |
+--------------------------------+------------------------------------+
| admin/reports/updates/list | List |
| admin/reports/updates | Available updates |
| admin/reports/updates/update | Update existing modules and themes |
| admin/reports/updates/install | Install new module or theme |
| admin/reports/updates/settings | Settings |
| admin/reports/updates/check | Manual update check |
+--------------------------------+------------------------------------+
6 rows in set (0.01 sec)
and it stays like that forever after...
I just tried this patch and it fixes the menu problem entirely. Yay. ;) I'd RTBC this except there's a silly code-style problem. I'll re-roll in a second, stay tuned.
#13
Whoops. ;) I misread something. Looking again, the code-style is fine. Code is clean. Fixes a critical bug. RTBC.
#14
Agreed on RTBC, I started an abortive attempt at running cache_clear_all() on the final install page to no avail, this is the right way to do it.
#15
so how bout that test? ;)
#16
Pretty sure this can't be tested - as soon as you refresh the front page it fixes itself.
#17
@catch: not the menu. Elements of the menu stay broken until you rebuild it. But yeah, I don't think our testing infrastructure is good at testing install profiles...
#18
+1 on RTBC
#19
+++ includes/cache-install.inc 27 Oct 2009 02:56:56 -0000@@ -22,11 +19,21 @@ class DrupalFakeCache implements DrupalC
+ // If there is a database cache, attempt to clear it whenever possible,
+ // since any full bootstraps that occur during the installer might have
+ // left stale cached data in it.
I'm not sure this code comment adequately captures the discussion in this issue (dww's report is really good). Particularly, we should document why we're doing nothing in the catch branch. I could see someone trying to "fix" it later, and we don't have test coverage for this part of the code.
This review is powered by Dreditor.
#20
OK, here it is with a much-expanded code comment. A little tricky, since I'm still not sure I totally understand the chain of events here, but I think this should at least make someone think very very hard before deleting this code :)
#21
Still works ... and comments abound :)
#22
Holy cow!! You are hired for official patch documentation duties! ;)
Committed to HEAD. Thanks! :D
#23
sweet, this was annoying the crap outta me :P
#24
Sorry to reopen. But meh? DrupalFakeCache is supposed to be self-enclosed. Introducing an artificial dependency on DrupalDatabaseCache makes absolutely zero sense. What if the site uses something else then the database cache?
#25
This thought crossed my mind as well but didn't seem that important to me - I was wondering if someone else would bring it up :)
DrupalFakeCache appears in cache-install.inc and is only really intended to be used during the installer. Is there a use case for a site that wants to avoid the database cache even while they are installing Drupal?
I think we might consider just renaming it to DrupalInstallerCache or something like that...
#26
@David_Rothstein: DrupalFakeCache can be very useful during development, but more importantly, this fix is broken: there is no guarantee that the website is using the DrupalDatabaseCache *at all* in the first place.
#27
OK, as I understand it, here is the situation where it might break. Someone, before they even install Drupal, puts a variable in their settings.php file to use a different cache system. Then, I guess what you are saying is, they will still be hit by this bug, because that cache will be getting filled and the code here does nothing to clear it?
This seems to me to be a pretty big edge case - people obviously use different cache systems on live sites where they care about performance, but would anyone really bother trying to switch the cache before they actually install Drupal? (And up until a few weeks ago or so, I'm pretty sure this was impossible - the installer forced you to use DrupalDatabaseCache whether you wanted to or not.)
To solve it, though, could we just make the installer respect whatever is in settings.php if the cache system is already defined there? In other words, this code currently in install.php:
<?php$conf['cache_inc'] = 'includes/cache.inc';
$conf['cache_default_class'] = 'DrupalFakeCache';
?>
Could we just put a
if (!isset())check around it?#28
Agreed with David, I don't see how this possibly affects any new install, and not in a way which would do anything worse than re-introduce the original bug even if that was the case - which is a strange, edge-casey and temporary bug only linked to the first few minutes after install as it is.
There's also the fact that even if you use DrupalFakeCache when developing, but memcache.inc for production, your site is still going to be loading the DrupalDatabaseCache code in cache.inc - because we don't have variable_get('cache.inc') any more, and your modules will still be creating cache_* tables, so I don't see how it would materially impact that situation.
Which leads us only to code style / conventions - where I can agree a DrupalInstallCache might make sense over DrupalFakeCache - but given core use cases would be a renaming most likely.
#29
I think we can mark this as fixed ... if someone wants to revisit renaming "DrupalFakeCache" to "DrupalInstallCache" we can re-open or start a new thread...
#30
Automatically closed -- issue fixed for 2 weeks with no activity.