Developing a class that's passed an arbitrary SelectQuery in its constructor, I have a need to identify the DatabaseConnection for the query - but, there's no API to retrieve the connection. Easily solved...
| Comment | File | Size | Author |
|---|---|---|---|
| query-getConnection.patch | 709 bytes | mikeryan |
Comments
Comment #1
Crell commentedCan you be more specific about what you're doing and what the use case is? This sounds like there's some overlap with #802514: changeConnection from hook_query_alter.
Comment #2
mikeryanThis is in the context of working on #802264: Refactor map table support. I'd like to be able to automatically determine whether I can add a map table join to a source query, by comparing connections between the two (i.e., if they're on the same connection, or if both connections are MySQL and have matching host/credentials, they're good to go).
I don't have a need to change the connection, but the patch in #802514: changeConnection from hook_query_alter will suit me just fine. Setting this as a dupe of that.
Thanks.
Comment #3
mikeryanReopening, since #802514: changeConnection from hook_query_alter seems to be in limbo, and Crell has suggested an approach to that which does not address my issue here.
Comment #4
mikeryanquery-getConnection.patch queued for re-testing.
Comment #5
mikeryanquery-getConnection.patch queued for re-testing.
Comment #6
mikeryanBump... It still would be really helpful in migration scenarios (dealing with arbitrary queries, which might be on any connection) to be able to retrieve the connection.
Comment #7
moshe weitzman commentedLooks completely harmless. Was postponed in hopes of #802514: changeConnection from hook_query_alter but thats in limbo. This one is simpler.
Comment #8
webchickThis needs sign-off from Larry if we're going to do it. Is there really no workaround other than to change DBTNG? I get that this method would be handy, but I'm trying very hard to enforce a "no new features" rule in D7 since we're on the cusp of RC now. Need more information on how this is a bug and not merely a 'convenience' thing.
Comment #9
mikeryanIs there another means to retrieve the connection for a query? I don't see how else to do it.
This is a clear (to my mind, anyway) hole in the API, that can be patched cleanly and safely.
@Crell, opinion?
Comment #10
Crell commentedThere is currently no way to determine the connection that a given query object will use. That is, sort of, by design as we tried very hard to avoid modules needing to care what DB they're connecting to for portability reasons. I don't fully understand mikeryan's use case here since if you're dealing with multiple connections you should have the connection objects on hand anyway via Database::getConnection(), but I am not adverse to adding such a getter method.
The interweaving issue here is *modifying* the connection object post-query-creation, which would, by extension, make it possible to determine the connection object. That is not possible either and I am less inclined to make it possible, since that could let a module change a query object to point to an incompatible connection.
Either way, the patch above is incomplete because all it does is add a method to SelectQueryExtender. That doesn't actually work. :-) Such a patch would need to add a method to SelectQueryInterface, implement it in SelectQuery, and then implement it as a pass-through (like all of the other pass-through methods) on SelectQueryExtender.
Comment #11
mikeryanHuh, I wonder how I managed to post a null-op patch? Could've sworn I had tested it... Unfortunately, I don't think I'll be able to reroll by Thanksgiving.
There are a couple of use cases in the Migrate module. First, the big picture - Migrate defines base classes to do the housekeeping for data migration - for a particular application, you would implement a module that extends those classes. When migrating from a dbtng-compatible database, the implementing module builds a query (usually against a non-default connection) and passes that query to Migrate, which executes it, manages the result sets, etc. Therefore, all the Migrate module knows about the query is the query object itself - it didn't build the query, so it doesn't know the connection.
So, why would the Migrate module want to know the connection? Two use cases:
1. #802264: Refactor map table support: Migrate does housekeeping in related map and message tables. In the general case, it needs to go through each row returned by the source query and check it against those tables to see if the row needs to be (re-)migrated. If the source query is on the same connection as the housekeeping tables (or if the connections are "compatible" - i.e., MySQL, same server, same username/password), the map and message tables can be joined directly into the source query, which is a major performance enhancment. It would be, yes, a convenience for the migrate module to automatically determine if this optimization is possible and apply it - but to do so, it needs the connection. In the absence of this, we've got a mapJoinable boolean on the migration object that can be set to TRUE by the implementor when they know the tables can be joined, so this isn't a huge issue.
2. #941440: Added column autodetect for sql source: Another convenience for the migration implementor would be for the migrate module to automatically identify all the fields in the query. This does work today when the implementor has explicitly added the fields to the query, by calling the query's getFields() method, but if all fields for a table are included (->fields('n')) getFields() returns nothing for that table. The alternative I came up with involves finding the actual database from the connection and querying information_schema. Now that I look back at that code, what I should've asked for was to have getFields() return all fields when there's no explicit list...
So, I'll back off on this for now... My anal side wants to see getConnection() for the sake of a clean and complete API, but I understand it's not a priority at this stage of the game. I'll pursue that getFields() enhancement instead...
Comment #12
ericduran commentedsub
Comment #13
q0rban commentedsubscribe
Comment #20
volegerLooks like this issue outdated. If I'm wrong, that definitely requires update IS.