According to http://drupal.org/node/125105#comment-229460, it sounds a good idea if we are able to replace our database connection string as array format. Moreover, I would like to point out the handling of eGroupWare (http://www.egroupware.org/) as example (within /header.inc.php):

        /* eGroupWare domain-specific db settings */
        $GLOBALS['egw_domain']['example.com'] = array(
                'db_host' => 'localhost',
                'db_port' => '3306',
                'db_name' => 'egroupware',
                'db_user' => 'egroupware',
                'db_pass' => 'passwd',
                // Look at the README file
                'db_type' => 'mysql',
                // This will limit who is allowed to make configuration modifications
                'config_user'   => 'root',
                'config_passwd' => '76a2173be6393254e72ffa4d6df1030a'
        );

Not too complicated, isn't it? Maybe we should give a some love to this issue, so let it ship with D6?

CommentFileSizeAuthor
#10 drupal-6.x-dev-db_connect-0.1.patch3.34 KBhswong3i

Comments

Crell’s picture

Crell commented
Version: 6.x-dev » 7.x-dev

Too late for D6. Maybe in D7.

hswong3i’s picture

hswong3i commented

Yup, seems it should belong to D7... BTW, the open of drupal-7.x-dev should not too far from today, I will come back and give some love to this issue. The idea sounds pretty :)

kbahey’s picture

kbahey commented

I support this because I suggested this in the past, before we had an installer. The $db_url = 'mysql://blah:blah@blah/blah' was too cryptic and painful to edit by hand.

So breaking these things in their discrete component would help lessen errors.

Now that we have an installer, this point is not as important.

However, not having to parse the file, and having the variables already accessible to any part of the code that needs it can be a good thing.

I think it should simply be:

$GLOBALS['db']['default'] = array(
 'type' => 'mysql',
 'db' => 'databasename',
 'user' => 'databaseuser',
 'pass' => 'databasepassword',
 'host' => 'databasehost',
);

$GLOBALS['db']['other'] = array(
 'type' => 'mysql',
 'db' => 'databasename',
 'user' => 'databaseuser',
 'pass' => 'databasepassword',
 'host' => 'databasehost',
);

We can differ on the names of the array keys, but there is no need to overcomplicate it with extra stuff

Anonymous’s picture

Anonymous (not verified) commented

Subscribing.

I'm not sure how I feel about this. I happen to like the one string format and setting the GLOBALS array in the settings file seems a bit wrong to me. Parsing the string only happens once so there isn't really a savings performance wise.

Crell’s picture

Crell commented

There's no need to explicitly call $GLOBAL. Currently conf_init() declares $db_url global, then includes the settings.php file, so it gets stuck into the global namespace that way. Now granted, globals are always the wrong answer that's beside the point for now. :-)

I also favor splitting up the connection string format, actually. It does simplify the code involved, and eliminates the need to use URL escape characters for wacky characters in passwords (which some of my clients always insist on using, just to be annoying).

hswong3i’s picture

hswong3i commented
Priority: Normal » Critical

When I am working with pdo_mysql, I try to create install.pdo_mysql.inc (just as like as install.mysqli.inc), update install.inc with new option, and start the installation. But a critical error is occurred: pdo_mysql://... syntax is not allowed by parse_url and so connection handling gone mad!

I think this should be a critical issue :(

Crell’s picture

Crell commented
Priority: Critical » Normal

hswong: I ran into the same issue in my earlier PDO attempt. My solution at the time was just to use pdomysql instead of pdo_mysql. Of course, for D7 the solution is to assume that mysql means PDO-MySQL, pgsql means PDO-PostgreSQL, etc.

That said, no, it's not critical.

Anonymous’s picture

Anonymous (not verified) commented

for D7 the solution is to assume that mysql means PDO-MySQL, pgsql means PDO-PostgreSQL, etc.

I sure hope that if that happens mysql, et alia will still mean mysql when pdo isn't available.

Crell’s picture

Crell commented

Since Drupal 7 assumes PHP 5.2, assuming PDO is a given because it's part of the stock compile of PHP 5.1+.

Basically, we can't improve our database layer as long as we are supporting ext/mysql, which is the weakest database driver for PHP.

hswong3i’s picture

hswong3i commented
Status: Active » Needs review
StatusFileSize
new drupal-6.x-dev-db_connect-0.1.patch3.34 KB

A simple patch via D6 CVS HEAD, which should able to help pdo_mysql/pdo_pgsql development will minimum effort. The idea is very simple: since "schema" part of parse_url() don't allow any special characters (e.g. "_"), we can store the database type within "fragment" part which don't have any limitation. Just place a dummy string "database" within "schema" part so parse_url() can function correctly.

I test this patch by copy existing database.mysqli.inc/install.mysqli.inc as database.pdo_mysql.inc/install.pdo_mysql.inc, hack drupal_detect_database_types() for this new database type, and start installation. All procedures are functioning and successful.

Crell’s picture

Crell commented
Status: Needs review » Needs work

That is a horribly ugly syntax, and is not, in fact, an array. See pdo.php in my sandbox. We don't need to make the connection string backwards for PDO. However, getting rid of the url format entirely and using a real array is probably a good idea for D7, but not yet.

hswong3i’s picture

hswong3i commented

The truth is I am not trying to work out a "perfect version" right now, but a functional version. According to the changed in #10, my personal research project Siren is now able to support pdo_mysql and pdo_pgsql without critical problem caused by the limitation of existing connection string syntax. On the other hand, surly we can foresee this syntax is also suitable for ibm_db2 :)

Anyway, I am now focusing if LDAP user backend would be a solution for our slow account/permission SQL query. This is just something similar as case of eGroupware: store user account and group in LDAP, and other configuration in SQL. In this case, we may need to have 2 connection settings, and the connection setting for LDAP may not suitable to handle with URL syntax, so array format maybe a MUST. Fine, all of this are still my brainstorming ;-p

(This is part of my personal research project Siren's issue.)

Crell’s picture

Crell commented
Assigned: hswong3i » Crell
Status: Needs work » Fixed

The new DB layer has an array-based format.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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