The includes/update.inc makes a guess on the fact that the drupal driver and the PDO driver has the same name for the same database. This is not true, for example for oracle... The call their drivers "oci" not "oracle".
I think this check should be removed:

 elseif (isset($databases['default']['default']['driver']) && !in_array($databases['default']['default']['driver'], PDO::getAvailableDrivers())) {
    $message = '<h2>A PDO database driver is required!</h2><p>You need to enable the PDO_' . strtoupper($databases['default']['default']['driver']) . ' database driver for PHP ' . DRUPAL_MINIMUM_PHP . ' or higher so that Drupal 7 can access the database.</p>';
  }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: update.php does PDO driver naming guess » update.inc requires Drupal driver and PDO driver named the same for no reason
Version: 7.0 » 8.x-dev
Category: bug » task
Damien Tournoud’s picture

Status: Active » Needs review
FileSize
1.82 KB

Status: Needs review » Needs work

The last submitted patch, 1029080-update-database-pdo.patch, failed testing.

xjm’s picture

Version: 8.x-dev » 7.x-dev
Priority: Major » Normal

update_prepare_d8_bootstrap() does not include this check, so this is a D7 issue.

Also, since no one has reported issues because of this since D7 release, I don't think this is major.

brianV’s picture

Status: Needs work » Needs review

#2: 1029080-update-database-pdo.patch queued for re-testing.

Patch applied cleanly locally here against latest HEAD, so I'm thinking it got caught up in the testbot being funky at some point.

Status: Needs review » Needs work

The last submitted patch, 1029080-update-database-pdo.patch, failed testing.

brianV’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

patch updated.

Two things needed to be addressed:

1. database.inc needed to be included before we call db_installer_object() as it calls a static function in the Database class.

2. DatabaseTasks::hasPDODriver() class needed to be made public since we are calling it from outside the class.

chx’s picture

This really falls into the try category of the "do it or do not, there is no try". Just try { connect to the database } catch { run screaming } .

radalin’s picture

This patch looks promosing, however I did not really like the idea of making a "require" call there. Instead we might move the drupal_bootsrap_database there. This way we make sure that everything related to the database is bootstrapped.

Why hasPdoDriver is protected anyway? I have checked the class and found a "installable" method. It basically calls the hasPdoDriver but it's public and seems better. I have uploaded the patch, what do you think about it?

brianV’s picture

I like radalin's approach in #9 - I never noticed DatabaseTasks::installable() was available, so that saves a lot of work as well.

Also, I tested this patch against an Oracle installation (which fails with core code due to the pdo_oci php driver name vs. 'oracle' drupal name), and this handles it gracefully.

radalin’s picture

I have also checked what would happen if no database conf is defined on the settings file. Update page seems to redirect to the installation page without any errors etc.

Pancho’s picture

Category: task » bug
Status: Needs review » Needs work
Issue tags: +pdo, +oracle

Also, since no one has reported issues because of this since D7 release, I don't think this is major.

Oh this has been reported, see #1504352: Cannot run Drupal update.php with Oracle Driver.:
radalin: "I'm trying to run drupal/update.php with the Oracle driver installed. However there is a problem with the update scripts which shows an error like:"
Renee S: "Confirmed"
brianV: "In practice, we've been commenting out that check prior to running update.php."

It's just that Oracle dbas are experts enough to handle it and the check is easy enough to comment out... but still it's just another hurdle to implement Drupal on Oracle, so we should get rid of it.

Patch in #9 looks good and still cleanly applies.
Some nitpicks:

+    // Bootstrap database anyway to check driver info
+    // Allow the database system to work even if the registry has not been
+    // created yet.
+    drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);

I'd remove the first comment line. We don't bootstrap the database only to check driver info, and the rest is quite self-explanatory.

-    $message = '<h2>A PDO database driver is required!</h2><p>You need to enable the PDO_' . strtoupper($databases['default']['default']['driver']) . ' database driver for PHP ' . +      if (!$installer->installable()) {
+        $message = '<h2>A PDO database driver is required!</h2><p>You need to enable the proper PDO driver for ' . $databases['default']['default']['driver'] . ' for PHP ' . DRUPAL_MINIMUM_PHP . ' or higher so that Drupal 7 can access the database.</p>';

Would be nice to get a bit more specific than "the proper PDO driver for foo_db". Therefore we'd need a public getter for $this->pdoDriver.
Also, requiring DRUPAL_MINIMUM_PHP isn't enough. The driver might require an even higher PHP version as in the case of PostgreSQL which requires PHP 5.2.11 because of incompatibilities. We'd therefore need a public getter for a variable $this->minimumPhpVersion or such.
Both additions IMHO are worth it.

And then, is it correct that there is no check at all in update_prepare_d8_bootstrap()? Otherwise we should probably implement a reasonable check in D8 and backport it to D7.

chx’s picture

Assigned: Unassigned » chx
Issue summary: View changes
katannshaw’s picture

Issue tags: +sqlsrv

This same error appears for me when running updatedb in Drush. I'm using the SQL Server Driver.

chx’s picture

Assigned: chx » Unassigned
dasginganinja’s picture

Issue tags: +mysqlnd

I just ran into this bug. The driver in use is the MySQL native driver (mysqlnd) since I am using it with the APDQC module.

dasginganinja’s picture

Per my report in #16 the error turned out to be that PDO-mysql wasn't re-enabled after a switch from php-mysql to php-mysqlnd.