drush_valid_db_credentials() is used to validate the bootstrap to DRUSH_BOOTSTRAP_DRUPAL_DATABASE. If it does return TRUE the bootstrap can happen. There's a problem in some circumstances where it return TRUE and it will not prevent further errors.

drush_valid_db_credentials() assumes yes and return TRUE:
* if PDO is not available
* if PDO is available but mysql driver is not available

if it does return TRUE the bootstrap validation success and so does the bootstrap itself, that is just printing a not-always valid message: "Successfully connected to the Drupal database.".

I've made some tests with drush cache clear reproducing #567062: command "drush status" returning HTML-output and error "Drush command could not be completed." and those are the facts I've seen:

  • If no PDO-mysql and no php-mysql this is the drush/drupal output:
    PDO support not available. Could not pre-validate database credentials. Assuming success
    Drush bootstrap phase : _drush_bootstrap_drupal_database()
    Successfully connected to the Drupal database.
    {drupal dumps a maintenance page because it is not able to connect to the database}.
    
  • If PDO-mysql but no php-mysql driver: drush is able to connect and verify db credentials but drupal is not able to connect to the database. Same messages as above but the first one, as PDO-mysql is available.
  • If no PDO available and php-mysql driver available: drush will not be able to verify credentials (it will "assume yes") and drupal is able to connect and do the work.
    PDO support not available. Could not pre-validate database credentials. Assuming success
    Drush bootstrap phase : _drush_bootstrap_drupal_database() 
    Successfully connected to the Drupal database.            
    

Drupal 7 will come with PDO support so I figure out we will get rid of this asymmetry but in drupal 5,6 using PDO in drush and php-mysql will need to handle special cases and extra checks. For sure we must check php-mysql is available in php-cli. We can assume yes if PDO is not available but we also need to check that php-mysql is available and can't assume yes on that.

I wonder what's the need for PDO currently.. if it is not used anywhere else in drush (nor aegir) but to validate the database bootstrap phase.

Do we need to simply add a extra check for php-mysql if drupal version in (5,6) or refactor that function to check credentials by using php-mysql or PDO based on the drupal version?

Comments

moshe weitzman’s picture

Assigned: Unassigned » adrian
Priority: Normal » Minor
moshe weitzman’s picture

Component: Code » PM (dl, en, up ...)
Assigned: adrian » jonhattan
Priority: Minor » Major

I'm not married to PDO for this. We can check for php-mysql. Patches wanted.

jonhattan’s picture

Component: PM (dl, en, up ...) » Base system (internal API)
jonhattan’s picture

Status: Active » Needs review
StatusFileSize
new6.18 KB

Here's a rewrite. It uses mysql / pgsql or PDO based on the drupal version you'r bootstrapping. It doesn't implement a separate case for mysqli but perhaps it is oportune to just avoid other edge case.

It still needs some polish in error messages and also testing against postgres. To see the error messages #962754: debug messages not available until DRUSH_BOOTSTRAP_DRUSH overcome is needed.

moshe weitzman’s picture

At minumum, we need to test this on D6/D6/D7 mysql. Ideally we would do same for pgsql. Can anyone confirm that these cases work? I'd like to commit this soon.

greg.1.anderson’s picture

I'll do the postgres testing. I'll need to install ubuntu 10.10 to do d7 + postgres, as d7 requires a certain minimum version of php to work. Will even do d5 + postgres.

greg.1.anderson’s picture

This patch works as expected under D6 + pgsql.

jonhattan’s picture

StatusFileSize
new6.14 KB

Previous patch didn't work for D7.

I doubt if using drush_bootstrap_error('ERROR_ID', $message) or drush_log($message, 'bootstrap');. Adrian did use drush_log() and this patch does so. Previous one did use drush_bootstrap_error().

Advantage of using drush_log() is that errors are printed as they happen but it needs --debug. Advantage of drush_bootstrap_error() is that errors are always printed if the command doesn't hit the needed bootstrap level but they're printed unordered, at the end. Anyway, teaching users to report bugs with --debug is the way to go.

greg.1.anderson’s picture

#8 works as it should with D5 and D6 + pgsql.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work.

jonhattan’s picture

Status: Reviewed & tested by the community » Fixed

Commited.

Status: Fixed » Closed (fixed)

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

sun’s picture

Status: Closed (fixed) » Needs work
+++ includes/environment.inc	8 Nov 2010 20:31:39 -0000
@@ -1029,42 +1029,92 @@
+        case 'mysql':
+        case 'mysqli':
+          $connection = @mysql_connect($creds['host'], $creds['user'], $creds['pass']);

Critical problem: mysql_connect() may perfectly be not available when $type is mysqli.

One of my environments uses mysqli, so PHP's mysql extension has been purposively left out, since only mysqli is needed, and I'm getting:

Drush command terminated abnormally due to an unrecoverable error.   ←[31;40m←[1m[error]←[0m
Error: Call to undefined function mysql_connect() in
includes\environment.inc, line 1119

Powered by Dreditor.

jonhattan’s picture

Status: Needs work » Needs review
StatusFileSize
new1.47 KB

Implemented. It works here. Marking as needs review just in case I missed something.

jonhattan’s picture

StatusFileSize
new1.47 KB

Indeed I missed the db name.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that resolves the fatal error.

jonhattan’s picture

Status: Reviewed & tested by the community » Fixed

committed.

Status: Fixed » Closed (fixed)

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

kotnik’s picture