Steps to reproduce:
1. Install Drupal via the user interface.
2. Enter "My Site" for the site name on the installer's configuration page.
3. When you go to the site, you'll see that the site is actually named "Drupal" instead.

The problem isn't fixed until you clear some caches. (For what it's worth, simply visiting admin/config seems to do it, so whatever cache is cleared on that page visit is the culprit.)

A very similar bug existed in the early days of Drupal 7, and the code which fixed it is known to have regressed in Drupal 8 (see #1394648: The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors). So I would have thought the patch there would have fixed this, but it doesn't seem to. This may actually be a different bug.

I'm marking this major since it's very noticeable and user facing and one of the main form fields people tend to fill out in the installer.

CommentFileSizeAuthor
#9 before.jpg7.32 KBxjm
#9 after.jpg18.02 KBxjm
#6 drupal8.database-class.6.patch2.82 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

It seems that the variable cache isn't being cleared at all.

Another instance of this is in drupal_install_profile_distribution_name() which is evident on /admin/modules after installing with minimal.
The name of the install profile isn't retrieved properly from the variables, so it tries to use standard, causing Notice: Undefined index: distribution_name in drupal_install_profile_distribution_name() (line 111 of /Users/tim/www/d8/core/includes/install.inc).

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Closed (duplicate)

This is fixed by the partially finished patch in #973436: Overzealous locking in variable_initialize(), so marking as a duplicate.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Interesting find! But that patch is a new feature, so I'm curious what actually caused this regression in the first place. I wonder if there's something more fundamental going on here.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Closed (duplicate) » Active

I agree, #973436: Overzealous locking in variable_initialize() is a new feature and doesn't look like it's going to be ready anytime soon.

@beejeebus noted some variable cache clearing bugs after Drupal installation in IRC some hours ago, which sounded closely related to the symptoms being outlined here.

In terms of potential causes, #636454: Cache tag support landed recently.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

I think I figured it out. The bug is here (and in several other places in InstallBackend.php also):

  function delete($cid) {
    try {
      if (class_exists('Database')) {
        parent::delete($cid);
      }
    }
    catch (Exception $e) {}
  }

That class_exists() is checking a class that doesn't exist anymore; the code needs to check for "Drupal\Core\Database\Database" instead. And even adding a "use" statement at the top of the file won't help here, because it's a string so the fully-qualified name needs to be passed.

I'm guessing for the installer bug, if we roll that change into the patch at #1394648: The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors it would probably fix things (and there are already tests over there too).

However, this isn't the only place in the codebase that's using class_exists() in this way so we may have other similar bugs also.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.database-class.6.patch2.82 KB

Awesome finding!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

this patch fixes the issues i was seeing - nice find!

sad panda though - i guess this means we broke it months ago.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Alrighty, let's get this in. Not testable, because we're still not able to test the installer yet.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs review
FileSize
after.jpg18.02 KB
before.jpg7.32 KB

I tested the patch locally and confirmed that it resolves the issue.

Before applying the patch, immediately after installing a fresh copy of D8 and setting a site name other than "Drupal":

before.jpg

After the patch, new fresh install, immediately after installing:

after.jpg

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

Oopsie. Agreed it's RTBC.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Looks good, but it actually is testable (indirectly, at least).

I just rolled these changes into the patch at #1394648: The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors and the tests there should reveal this bug. That issue touches more problems than just this one so it can be separate, but it would be great to get those tests in because it's unfortunate that this stuff keeps breaking.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@David_Rothstein: You're not actually testing the installer over there, so I fail to see how this is actually testable. :) I'm currently working on a couple of patches that happen to try to invoke the Drupal installer via API or UI, and apparently, each one of those is blowing up (killing) the entire Apache web server process. Given the experience of those patches, we're still far away from functional tests for the installer that assert the actual behavior.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Well, it's not a functional test, but it directly tests the cache behavior that causes this bug, and therefore exposes the bug.

I'm going to go bump the priority of that issue to "major". (Another option would be to split up the tests from that issue and move some of them over here before committing this patch, but I'm not sure it's worth the effort.)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

This looks good to me, and the other issue adds tests, shame it was broken for so long, although I'm slightly glad that cache tags wasn't responsible :p

Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.