While investigating #719730: drop the sequences and queue tables from D5 during the D6 -> D7 upgrade I found stumbled upon a very annoying blank screen for which the only error was in Apache's error.log:


[Fri Feb 19 14:49:31 2010] [error] [client 72.0.207.41] PHP Fatal error: Call to undefined function _system_rebuild_theme_data() in /srv/aegir/drupal-7.x-2010219/includes/theme.inc on line 588

The problem is an edge case when there's an error in the requirements checks during the upgrade. This issue was partly discussed in #563106: Cannot upgrade from Drupal 6 to Drupal 7 - meta issue (i *think?) but I don't feel the core issue was addressed at all.

The core issue is that the exception handler assumes a certain level of bootstrap (system.module loaded, namely) to display the theme. It needs system_rebuild_theme_data() to bootstrap the maintenance theme, but _drupal_maintenance_theme() doesn't actually load the system.module at all in maintenance mode.

So this barfs out with a blank page if (say) there's an error in the requirements checks.

The attached patch loads the system.module if there's no DB *or* we're in maintenance mode (it used to only load it if there was no DB).

CommentFileSizeAuthor
#16 719850-db-errors.patch2.01 KBandypost
#15 719850-db-errors.patch2.01 KBandypost
#14 719850-db-errors.patch1.95 KBandypost
#11 719850-db-errors.patch1.94 KBandypost
#2 719850_errors.patch1.55 KBanarcat
errors.patch1.55 KBanarcat

Comments

Status: Needs review » Needs work

The last submitted patch, errors.patch, failed testing.

anarcat’s picture

anarcat CreditAttribution: anarcat commented
Status: Needs work » Needs review
FileSize
719850_errors.patch1.55 KB

Silly git..

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

Good hit we can't rely to database in maintenance mode too!

dries’s picture

dries CreditAttribution: dries commented

I'm a bit confused. If we can't rely on the database in maintenance mode, why does db_is_active() return TRUE? It seems like the extra check would not be required.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review
anarcat’s picture

anarcat CreditAttribution: anarcat commented

Basically, db_is_active() and MAINTENANCE_MODE are orthogonal: we could be in maintenance and have an active DB connexion, from what I understand. At least, db_is_active() doesn't check that at all:

function db_is_active() {
  return Database::isActiveConnection();
}

and later:

  final public static function isActiveConnection() {
    return !empty(self::$activeKey) && !empty(self::$connections) && !empty(self::$connections[self::$activeKey]);
  }

... and I think that's fine. It's perfectly okay to be in maintenance mode and kick in database queries. But if we're in maintenance mode, some modules may not already be loaded, and that's what my patches tries to accomplish: it loads the system module properly, not only if the db is inactive (which can happen outside of MAINTENANCE_MODE) but also during maintenance operations, where drupal is not fully bootstrap.

I'm making this up as I go along here, I haven't studied the bootstrap code of D7 well enough to be an authoritative source on this. The patch, however, fixes the issue at hand and doesn't cause any regression that we could find.

wongsableng’s picture

wongsableng CreditAttribution: wongsableng commented

I also have problems why does db_is_active() return TRUE?

anarcat’s picture

anarcat CreditAttribution: anarcat commented

Probably because the db is active (?!) which, as I said, can happen if, during maintenance mode, a database connection *is* active.

This is exactly what happened in my case: there was a database error in the update_fix_d7_requirements(), which happens in update.php before the actual page is rendered, so that Drupal can bootstrap properly. But since the error happened before the bootstrap, system.module wasn't loaded and the theme engine was therefore broken.

So we have two cases where we need to do something special in _drupal_maintenance_theme(): when the database is not bootstrapped (that's currently done, although I'm unsure why) and when we're maintenance mode (that's not done right now, and will give the blank page if there are similar errors during bootstrap). I just pulled the bootstrap code out of the non-maintenance mode to make it execute if we're in maintenance mode or if the db is unavailable.

amc’s picture

amc CreditAttribution: amc commented

subscribing

barinder’s picture

barinder CreditAttribution: barinder commented

facing same issue.... after applying #2 patch
getting this error
PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6to7.blocked_ips' doesn't exist: SELECT 1 FROM {blocked_ips} WHERE ip = :ip; Array ( [:ip] => 127.0.0.1 ) in drupal_is_denied() (line 1688 of /var/www/d6to7/includes/bootstrap.inc).

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
719850-db-errors.patch1.94 KB

@anarcat Do you propose to load system.module always when in maintenance mode or when db is not active?

This is already maintenance page but we dont know why we here so I think it's no matter on mode we should check only database.

EDIT: Error happens because for modes (install, update) no check for db_active() is happen!

Status: Needs review » Needs work

The last submitted patch, 719850-db-errors.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Yes! It's wrong to call db_is_active() before including files...

So the check was right - to load system.module only if mode not in (install,update) but check for db_is_active() is wrong because it lives in database.inc

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
719850-db-errors.patch1.95 KB

I think that enough to check for existence db_is_active also loading of system.module is required to possible call _system_rebuild_theme_data() in list_themes()

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
719850-db-errors.patch2.01 KB

Finally, seems more clear if separate check for database and system

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
719850-db-errors.patch2.01 KB

Patch in #15 is wrong because I forget about "!" before function_exists() :(

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Closed (duplicate)

Duplicate of #688294: Switch from db_is_active() to proper exception catching.

karschsp’s picture

karschsp CreditAttribution: karschsp commented
Status: Closed (duplicate) » Needs review

#16: 719850-db-errors.patch queued for re-testing.

Curi’s picture

Curi CreditAttribution: Curi commented

Okay, so in addition to the above patch (anyone else slightly amused that the old pre-patch code includes the database.inc after calling db_is_active?), the actual cause of the WSOD needs to be addressed.

The backtrace shows that update_fix_d7_requirements in update.inc is trying to create the semaphore table, but it already exists. Now, someone who's been designing the core code (namely, not me) should decide if that creation is even needed since it exists in d6 (and, from a cursory glance, I do not see any calls to drop the semaphore table).

EDIT: actually, I do not know off-hand when the semaphore table was added to Drupal's system schema. And, I don't have the time right now (at work, currently) to look it up. So, if it was added somewhere along the line in d6, I suppose, somewhere in all the updates, that will have to be checked prior to the need for the semaphore in menu_rebuild (according to the comment?)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Closed (duplicate)

This is still a duplicate of #688294: Switch from db_is_active() to proper exception catching, if you think it's not, please document why before re-opening.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@catch this issue also fixes that theme could be unavailable! I'm trying to pay special attention on on!

Curi’s picture

Curi CreditAttribution: Curi commented

Not to mention there is an issue completely unrelated to the db_is_active call with the semaphore table being created when it already exists and with an exception thrown from the call to db_create_table not being handled thus causing the first issue with the WSOD to appear...