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.
Comment | File | Size | Author |
---|---|---|---|
#20 | installer-stale-cache-605880-20.patch | 2.27 KB | David_Rothstein |
#10 | installer-stale-cache-605880-10.patch | 1.33 KB | David_Rothstein |
#6 | 605880_2.patch | 1.68 KB | bleen |
#2 | 605880.patch | 1.67 KB | bleen |
Comments
Comment #1
seutje CreditAttribution: seutje commentedThis 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
Comment #2
bleen CreditAttribution: bleen commentedOk ... 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?
Comment #3
seutje CreditAttribution: seutje commentedawesomeness!
after applying this patch I was unable to recreate the problem \o/
we should really make a test for this though
Comment #4
stBorchertSome notes on code style:
Sentences are finished by a period.
Use
if (...) {
.New line before "else" block.
Use
$conf = variable_initialize(array(), TRUE);
.Use
FALSE
instead offalse
.This review is powered by Dreditor.
Apart from that it looks good.
Comment #5
bleen CreditAttribution: bleen commented@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..."?
Comment #6
bleen CreditAttribution: bleen commentedI have a hard time breaking my coding habits when developing for Drupal ... thanks
Comment #7
matason CreditAttribution: matason commentedLooks 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.
Comment #8
jhodgdonRegarding that last patch, I'm concerned about this change:
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...
Comment #9
c960657 CreditAttribution: c960657 commentedAFAICT 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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedBoth #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.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedJust 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
Comment #12
dwwWhen 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:
Then, as soon as you visit admin/reports/updates for the first time, {menu_router} is like so:
Finally, once you rebuild the menu, it's back to this:
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.
Comment #13
dwwWhoops. ;) I misread something. Looking again, the code-style is fine. Code is clean. Fixes a critical bug. RTBC.
Comment #14
catchAgreed 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.
Comment #15
seutje CreditAttribution: seutje commentedso how bout that test? ;)
Comment #16
catchPretty sure this can't be tested - as soon as you refresh the front page it fixes itself.
Comment #17
dww@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...
Comment #18
bleen CreditAttribution: bleen commented+1 on RTBC
Comment #19
webchickI'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.
Comment #20
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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 :)
Comment #21
bleen CreditAttribution: bleen commentedStill works ... and comments abound :)
Comment #22
webchickHoly cow!! You are hired for official patch documentation duties! ;)
Committed to HEAD. Thanks! :D
Comment #23
seutje CreditAttribution: seutje commentedsweet, this was annoying the crap outta me :P
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedSorry 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?
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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...
Comment #26
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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:
>
Could we just put a
if (!isset())
check around it?Comment #28
catchAgreed 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.
Comment #29
bleen CreditAttribution: bleen commentedI 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...