fuel / orm

Fuel PHP Framework - Fuel v1.x ORM
http://fuelphp.com/docs/packages/orm/intro.html
151 stars 95 forks source link

The problem of DB insatace in transaction #365

Closed keech closed 9 years ago

keech commented 9 years ago

Problem

DB instance in transaction are different from the db instance made when the transaction starts.

Details

$db = \Database_Connection::instance("db_id"));
$db->start_transaction();
try{
    \Model\User::regist($params);
    \Model\Rank::regist($params);

    $db->commit_transaction();

}catch(\Exception $e){
    $db->rollback_transaction();
    throw $e;
}

When I create database connection like below,

$db = \Database_Connection::instance("db_id");

$db is instance of below.

\Config::get("db.db_id");

But,

\Model\User::regist($params);
\Model\Rank::regist($params);

will be used

\Config::get('db.active');

now.

My thinking

We should be able to use the same DB instance both DB instances in transaction and DB instance made when the transaction starts if argument of DB name is not provided.

keech commented 9 years ago

Sorry, I haven't considered scattering DB. So this issue should not be solved. I should set DB id as argument if I use transaction.

kenjis commented 9 years ago

Yes.

But there is no method to give connection to Orm\Model. It is still a problem.

emlynwest commented 9 years ago

It should be easy enough to add, I'll reopen this and take a look at it later/tomorrow.

kenjis commented 9 years ago

Thanks.

It may be better Orm\Query also has a method to set connection like:

$result = Model_User::find()
    ->where('user_id', $user_id)
    ->where('status', Model_User::STATUS_ACTIVE)
    ->connection('slave')
    ->get_one();

above code is from http://php6.tumblr.com/post/38309122629/fuelphp-advent-calendar-2012

emlynwest commented 9 years ago

@Keech As @kenjis says you can set the connection using the ->conection() method from the Model class. Due to this I don't see a need to add a similar method to Query as you do not create instances of Query manually. You should use Model::query() which will pass the needed instance to the Query object for you.

emlynwest commented 9 years ago

On closer inspection it seems there was no method to set the active DB instance to use so I have added this in the above commit.

kenjis commented 9 years ago

@stevewest I wanted to say, both Orm\Model and Orm\Query should have methods to set connections. Orm\Query has no method to set connections?

If Orm\Query had them, we could write like this:

$result = Model_User::query()
    ->where('user_id', $user_id)
    ->where('status', Model_User::STATUS_ACTIVE)
    ->connection('slave')
    ->get_one();

You added the methods to Orm\Model, so now we can set them via Orm\Model.

Model_User::set_connection('slave');
$result = Model_User::query()
    ->where('user_id', $user_id)
    ->where('status', Model_User::STATUS_ACTIVE)
    ->get_one();

Don't you think we need to add the methods in Orm\Query, do you?

emlynwest commented 9 years ago

I don't see any issue with adding that. I can do that now.

emlynwest commented 9 years ago

The first example you show there should now work with the above commit.