| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
If you have multiple Drupal installations that share common data, it would be nice to redirect queries any database server without much work. db_prefix only works with different databases or tables on the same server. This issue is to allow multiple servers.
hook_query_alter is powerful, but it doesn't allow us to change the connection. Since the connection isn't actually used until after preExecute(), it could be changed. The problem is that hook_query_alter only allows one to call methods on the query, but not to return an entirely new query. So the solution is to either add a method to the query for changing the connection, or allow hook_query_alter to return a different query object (one presumably that was extended from the original object).
The attached patch is pretty simple change.
You would use this in a hook_query_alter as in the following code snippet:
function example_query_alter(QueryAlterableInterface $query) {
if ($query->hasTag('user_load_multiple')) {
$query->changeConnection(Database::getConnection('default', 'secondary'));
}
}| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| changeConnection.patch | 1.4 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 20,425 pass(es). | View details | Re-test |
Comments
#1
chx suggested in irc that rather than implement a method named changeConnection, that we instead implement &connection() that returns a pass-by-reference object. I'll give this a try, but I think this might be susceptible to the same problems; I'm not sure if we'll be able to change the connection object itself, or just the contents of the connection object.
#2
That's why you need a reference to $this->connection. with a reference, yes we can.
#3
Updated patch per chx's suggestion and new sample snippet:
function example_query_alter(QueryAlterableInterface $query) {if ($query->hasTag('user_load_multiple')) {
$connection = &$query->connection();
$connection = Database::getConnection('default', 'secondary');
}
}
#4
And here's an even cleaner example use case:
function example_query_user_load_multiple_alter(QueryAlterableInterface $query) {$connection = &$query->connection();
$connection = Database::getConnection('default', 'secondary');
}
#5
Moving connection to Query class so everything inherits it.
#6
Let's see what the bot thinks but I like it. Will try to get Crell's blessings.
#7
Seems reasonable and useful to me.
#8
I think the biggest concern I have with this is that any query class could be a driver specific implementation. See DatabaseConnection::Select for the logic behind this. So what happens when you have a SelectQuery_pgsql and switch the connection to an instance of DatabaseConnection_mysql? I can't imagine there being any change of that scenario ending well.
In core the only driver implementing SelectQuery_driver is PostgreSQL, and in contrib, Oracle and SQL Server have implementations as well.
#9
I am also not sure that we should be using a return-reference pattern here. Rather we should be using a setConnection() method, I suspect. I'll have to sleep on it.
#10
What's the use case where you need to change the connection object after the query is already created? You can run a query off of any connection object you want if it's your own query.
I share mikey_p's concern that this means you can easily setup situations where a query object is bound to an incompatible connection object; There's a reason that you create query objects off of the connection object. (Actually there's several, but this is one of them. :-) ) I need a better picture of why you'd want to do something so wacky here.
Unless there's a case for needing the connection object itself, I think I'd prefer a setConnection() method instead of a return-ref. It feels cleaner.
So I guess this is "needs more info".
#11
@Crell, sorry if I didn't communicate the use case above, I tried to. The use case is two Drupal installations on two different database servers, with some shared data. If the shared data is entities and you know that these entities aren't used in joins, then this is a very easy solution to get that data from the secondary database. This is a global solution for loading user's or taxonomies or files from another Drupal database on another server. AFAIK, we don't have any other solution for doing that.
As for if this is a return-reference or a set function, tell me what you want. I originally submitted the set function and @chx asked me to change it to the return-reference.
#12
db_set_active() still works much as it did before, and you can always run $db = Database::getConnection($key, $target); $db->query() if you don't want to change the "main" database. Manipulating the connection object in hook_query_alter will miss a whole lot of queries that are not built queries...
#13
There are issues with db_set_active() and there are issues with using hook_query_alter to change the connection.
I don't know the universal solution for using two databases. If you're going to use two databases, you just have to be careful. You have to know where your data is stored and you have to know what is called during the load. If you want to load everything during the entity load from the secondary database, you use db_set_active(). If you only want to load the entity, but not all other data, you use hook_query_alter.
What's the harm of having both options available?
#14
I think the answer to #8 is "don't do that", I don't see much of a use case for this outside site-specific situations, and in site-specific situations if we allow people to do something weird, they shouldn't blame us when weird turns to stupid. It seems like both options are here as patches, and I'm not sure what harm this will do.
#15
This solution also makes every entity load in other contrib modules, and in core, just work. I couldn't possibly wrap every call to my entity load in every core and contrib module I use.
#16
+1 - see #814312: Add getConnection() to SelectQueryExtender (I really just need the connection() getter, but I'm familiar with the context of Doug's original request and am totally down with that as well).
#17
This patch works for my use case (I just need to get the connection, don't need a reference to it). Not setting to RTBC, since I haven't tested the hook_query_alter usage...
#18
Couldn't this also be accomplished with a quick custom controller class that gets hook_entity_info_alter()'ed in for taxonomy terms (for instance)? That's one of the advantages of having loading mechanisms be swappable.
I know that doesn't cover mikeryan's issue, but it seems a cleaner way of handling entity loading like Doug is talking about.
#19
The fundamental problem is that entity loading is just one example and already Mike Ryan has another example...
#20
subscribe
#21
sub.
#22
#23
My understanding is that this is still on the table for 7, as it effectively blocks one from doing any slave targeting in hook_query_alter(). Feel free to switch back if that has changed. :)
#24
I would like to unsubscribe please. :)
FYI:
I started by using this patch to be able to change the database connection in the query alter but instead I just ended up implementing a SelectQueryExtender and changing the connection in the constructor of the extender class.
#25
@q0rban that was a cross post.
But I'll leave it at 8.x for now, since you can change the connection object if you implement an extender.
#26
Yes, but still somewhat of a bug (IMO) in 7.
#27
@q0rbon - the process is to fix bugs in 8.x and then consider backport. So bugs are always filed against 8.x first.
#28
Ok, thanks for the clarification. :)
#29
#30
If this is "won't fix" we should probably explain why. It's a feature request so I see no reason to close without explanation.
#31
Because Drupal is a democracy: we can do whatever with DBTNG whatever Crelll agrees to. He said don't do this so we are not. Basically he is fond of protected and heaven forbid we remove one.
#32
http://drupal4hu.com/node/293 is a better answer.
#33
that explanation does not apply. migrate module needs this and migrate is used by many sites. they should not all have to hack core.
#34
Why does migrate need to create a query object off of one database and then mutate it to use another later? There's absolutely nothing preventing it from doing:
<?php
$result = Database::getConnection('db1')->select($table)->...->execute();
$conn = Database::getConnection('db2');
foreach ($result as $record) {
$conn->insert($table)->...->execute();
}
?>
The support for directly fetching a given connection object and reusing it (rather than having to play with db_set_active()) was specifically intended for use cases like migrate that need to play with multiple databases simultaneously.
#35
We are using this to redirect all select queries to the slave database. How would one accomplish that without this patch and without hacking core?
<?phpfunction example_query_alter(QueryAlterableInterface $query) {
if (get_class($query) == 'SelectQuery') {
$connection = &$query->connection();
$connection->setTarget("slave");
}
}
?>
Updated patch attached.
#36
Ok, ericduran helped me find a way around without needing to hack core or use the above patch:
<?php
class SlaveTarget extends SelectQueryExtender {
public function __construct(SelectQueryInterface $query, DatabaseConnection $connection) {
if ($connection->getTarget() != 'slave') {
$connection = Database::getConnection('slave', $connection->getKey());
}
parent::__construct($query, $connection);
$this->addTag('SlaveTarget');
}
}
/**
* Implements hook_query_alter().
*/
function example_query_alter(QueryAlterableInterface $query) {
if (is_a($query, 'SelectQuery') && !$query->hasTag('SlaveTarget')) {
$query->extend('SlaveTarget');
}
}
?>