Table prefixes should be per database connection
David Stosik - November 26, 2007 - 14:57
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
Hello,
I think this one can only go in 7.x, as 6.x code is frozen...
Currently, we can specify a prefix per table name, and a default one, in the settings.php file. For example, we can have
<?php
$db_url = 'mysql://user:password@dbserver/db';
$db_prefix = 'drupal_';
?>For a single prefix applied to all tables.
Or we can have
<?php
$db_url = 'mysql://user:password@dbserver/db';
$db_prefix['users'] = '';
$db_prefix['default'] = 'drupal_';
?>To prefix with 'drupal_' all tables, except users table.
The lack comes when one is using multiple databases, with one connection string for each. The prefix will then apply to all tables, whatever the current database is. For example, we can have the following:
<?php
$db_url['default'] = 'mysql://user:password@dbserver/db';
$db_url['other'] = 'mysql://user2:password2@dbserver/other_db';
$db_prefix = 'drupal_';
?>What I suggest is to put a new level in the db_prefix array, to become like this:
<?php
$db_url['default'] = 'mysql://user:password@dbserver/db';
$db_url['other'] = 'mysql://user2:password2@dbserver/other_db';
$db_prefix['default']['default'] = 'drupal_'; // prefix all tables of my drupal database
$db_prefix['default']['users'] = ''; // don't prefix the users tables of my drupal database
$db_prefix['other']['default'] = 'other_';
?>This would allow a better abstraction.
Please let me know what do you think of such a feature.
Thanks,
David
#1
Could anyone please give me his opinion on this subject?
Thanks,
David
#2
Hi David - I'm also having this issue. At this point I've only just started delving in to the problem, but sounds like a sensible feature request to me.
Sam
#3
The multiple database connexion feature of Drupal is designed to allow module developers to query data outside of Drupal. In that case, the prefixing doesn't make much sense.
The best way to do this is for you to manually set $db_prefix after you db_set_active() call, and restore it afterwards.
#4
I'm posting this just as a simple example of how I hacked quickly round the problem. Might help someone else get on with the job.
<?php
function dbprefix_fix( $db_name )
{
global $db_prefix;
if( 'other_db' == $db_name )
{
$db_prefix = '';
db_set_active( 'other_db' );
}else{
$db_prefix = array(
'default' => 'surf_',
'users' => 'shared_',
'sessions' => 'shared_',
'role' => 'shared_',
'authmap' => 'shared_',
'sequences' => 'shared_',
'profile_fields' => 'shared_',
'profile_values' => 'shared_',
'users_roles' => 'shared_',
);
db_set_active( 'default' );
}
}
?>
#5
Agreed, table prefixes should be per database connection.
The DB:TNG patch #225450: Database Layer: The Next Generation is about to land, and will make this easy. Postponed until that.
#6
#7
DBTNG is in!
I am very much in favor of this. The way I'd suggest implementing it is to make 'prefix' another key in the $databases array:
<?php$databases['default']['default'] = array(
'type' => 'mysql',
'host' => 'locahost',
// ...
'prefix' => array('default' => 'foo_')
);
?>
The tricky part is that some modules do some really funky stuff with prefixing, including simpletest. So you'll have to be careful that simpletest doesn't break. Of course, simpletest may be able to work even better now if it can just establish its own database connection that defines its own prefixes... :-)
#8
Here is a full patch for this.
What this does:
- remove $db_prefix an re-implement it as a per connection 'prefix' key in the $databases array.
- add DatabaseConnection::(get|set)Prefix()
- add Database::renameConnection() and Database::removeConnection()
- change SimpleTest logic to take advantage of the new API:
- in setUp() : SimpleTest renames the default connection, then clones its parameters to create a new default connection with a prefix
- in tearDown() : SimpleTest removes the prefixed connection and rename the old default connection back
- moved simpletest prefix detection in conf_init()
- clean-up *a lot* install.php, by removing several unneeded references to $db_url (!!) and $db_prefix
This allowed me to find a bug in the database layer (#329080: Statics in objects are not per-instance). The fix in that other issue is required to make this work.
#9
Visual inspection only, as I just reviewed the linked patch and CNW'ed it. :-)
- I'm not sure we need to give prefix its own array/string. Why not tell each connection its entire DB info structure and let it use that?
- Hooray for removing the evil regex hack from openConnection()! :-)
-
if (preg_match("/^simpletest\d+/", $prefix = Database::getActiveConnection()->getPrefix())) {If the prefix is an array, that's going to break in exciting ways, isn't it?- For performance, since we're pre-processing the info array anyway, how about we pre-process a string prefix into an array with one key, default? That should save us some is_array() calls later, and an array_key_exists(), probably. (Just assume that default always exists, and pre-set it to '' if not specified.)
- DatabaseTestCase::setUp() has some error logging code in it. Probably OK for now, but let's remember to remove it before committing.
And looking at this patch, I realized that with addConnection() we can now remove references to the global $databases array from the database API entirely and just assign the connections from $databases in conf_init(). Sweet! Probably a separate patch, though. :-)
#10
Rerolled and revamped version. Should take care of every remark from #9. Needs a complete review.
#11
The last submitted patch failed testing.
#12
Hmm...it appears that automatic testing of patches that change the db installer is a big fail. I've run across the same situation in #346494: DB drivers need to be able to change the configure database form during install.
#13
Visual inspection of #10:
- There's a lot of git flotsam at the top of the file. Not sure if that's going to cause problems when applying or not.
- DatabaseConnection: "The default prefix used by that database connection". "This", not "That", since it's describing the self-same object. Ibid for non-default prefixes.
- Er, I don't get why we have default and non-default prefixes as separate properties? OK, I get it when I read down as far as setPrefix, but it should probably be a bit better documented.
- Ooo, prefixTables() gets so much shorter! :-)
<?php$this->setPrefix($connection_options['prefix'] ? $connection_options['prefix'] : '');
?>
That will throw a notice if $connection_options['prefix'] is not set at all. It needs an isset() or empty().
- There's a lot of changes in DrupalWebTestCase::tearDown() that don't look related to the DB prefixing at all. Are those really necessary here?
- The code removed from tearDown() includes the code that removes all of the simpletest-specific tables. I don't see that code replaced anywhere else, though. Simply removing the connection doesn't delete the tables (I would hope :-) ). So won't that leave a lot of testing tables lying about?
- The examples in default.settings.php don't quite make it clear that they're talking about the prefix key of a $databases entry. Perhaps expand the default samples of that, too?
Exciting!
#14
Thanks for the review, Crell.
It's perfectly innocuous, and quite cool ;) This is really not the first GIT patch I submit here ;)
Changed that to this, and added a bit of documentation to defaultPrefix.
Oups.
This is an artifact of trying to reroll a three-months old patch ;) I fixed those. When ignoring whitespace changes, the only differences are:
<?php
@@ -899,8 +907,8 @@ class DrupalWebTestCase {
* and reset the database prefix.
*/
protected function tearDown() {
- global $db_prefix, $user;
- if (preg_match('/simpletest\d+/', $db_prefix)) {
+ global $user;
+
// Delete temporary files directory and reset files directory path.
file_unmanaged_delete_recursive(file_directory_path());
variable_set('file_directory_path', $this->originalFileDirectory);
@@ -912,8 +920,9 @@ class DrupalWebTestCase {
db_drop_table($ret, $name);
}
- // Return the database prefix to the original.
- $db_prefix = $this->originalPrefix;
+ // Get back to the original connection.
+ Database::removeConnection('default');
+ Database::renameConnection('simpletest_original_default', 'default');
// Return the user to the original one.
$user = $this->originalUser;
@@ -937,7 +946,6 @@ class DrupalWebTestCase {
// Close the CURL handler.
$this->curlClose();
- }
}
?>
I extended them a bit and added a 'prefix' to most of the example $databases.
#15
The last submitted patch failed testing.
#16
Rerolled (some conflicts since chx' update.php patch went in).
#17
The last submitted patch failed testing.
#18
subscribing. #140860: Consistent table names and database handling of table names will be directly related to this