If in your settings.php you set your $db_url so something like this:
$db_url = 'mysql://username@localhost/databasename';
Every Drupal page will open with an php warning, making ajax and other features won't work property.
this patch solves for mysql databases.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

Status: Needs review » Needs work

We always put { } around our conditional and looping blocks.

I'd like to see the password explicitly set to '' if it is not set since other code might assume it is a string.

wundo’s picture

Status: Needs work » Needs review
FileSize
552 bytes

Thanks drumm,
I improved the patchs, please look it again.
I think I must do it to all other databases files.

profix898’s picture

I just tried reproduce your issue, but the following connection string
$db_url = 'mysql://dbuser@localhost/drupalcvs';
does not through any warnings on my install ... (without the patch)
Using MySQL 5.0 and PHP 5.1. Is this specific for certain versions?

wundo’s picture

Does your server show to the user the php warnings?

davemicc’s picture

This actually seems to just be an "undefined index" notice, not a warning (at least with PHP 5). There seems to be quite a few of these notices in the installer.

This patch is nearly identical to the one in #2, except it also fixes database.mysqli.inc. It does not appear database.pgsql.inc has this issue.

Jax’s picture

tested without patch > notice
- patched
Notice is gone.

When testing I use the mysql root account, each time I've already changed these lines. Glad it's finally patched :)

Yet it could simply be solved by doing

$url['pass'] = urldecode(@$url['pass']);

And urldecode(null) returns ''. So you've got everything you're looking for with just 1 character.
Which is the way I always did it :). Guess it's just coding style.

Looking at this made me thinking... There is urldecode($url[key]) in all the database files. That is what I call code replication! Shouldn't it be nicer to provide a wrapper function around parse_url that immediatly decodes different parts? You could even set a parameter to decode it or not.

drumm’s picture

Status: Needs review » Reviewed & tested by the community

@ is a horrible construct to use which simply covers up the underlying error and any other error messages.

The duplicated code is fine since it is the quickest way to fix the bug. Don't confuse or delay bug fixing with refactoring. I'd consider the larger refactoring to be an API change and probably not committable during the code freeze, but certainly welcome in a few months.

Since this got a positive review and the patch looks okay after a quick scan of the code, setting it ready to commit so it will be near the top of the queue when I, or someone else, gets around to full reviews for committing.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

Anonymous’s picture

Status: Fixed » Closed (fixed)
evoltech’s picture

Version: x.y.z » 5.20
Status: Closed (fixed) » Patch (to be ported)

I just confirmed this is still an issue in the DRUPAL-5-20 branch.

wundo’s picture

evoltech, which db engine are you using?

I couldn't reproduce it here with Drupal 5.20 and mysql/mysqli.

Also I noticed that 5.20 db_connect is mixing if() {} and ()? : ;, wouldn't be the case to uniformize the syntax?

wundo’s picture

Status: Patch (to be ported) » Fixed

Setting the status back to fixed, please change if it's the case.

Status: Fixed » Closed (fixed)

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