fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
805 stars 338 forks source link

DB::select() doesn't use readonly instance basically... #2062

Open ysshir opened 7 years ago

ysshir commented 7 years ago

DB::select() used to return Database_Query_Builder_Select class without connection before, but now it returns with connection. And the connection is created by \Database_Connection::instance() method.

\Database_Connection::instance() is defined as

public static function instance($name = null, array $config = null, $writable = true)

so the connection is always master one.

Actually I can use readonly instances in some ways like below.

  1. pass arguments to the execute method

    DB::select()->...->execute('default')

  2. set the static $_connection field of Model_Crud

    protected static $_connection = 'default';

but I think it should use readonly instances, when it is select query. so I suggest to change the DB::select() method like below

public static function select($args = null) {
    return \Database_Connection::instance(null, null, false)->select(func_get_args());
}

any idea ?? thanks.

WanWizard commented 7 years ago

That is the logical result from switching to hardcoded static calls to connection objects. And it needs the connection because that determines the driver, and the driver determines the SQL syntax to be used.

The reason it uses the 'default' connection is in the name, it is the default connection. If you don't want that, define another default. If you have a database setup where you have read-only slaves, set them as your default, and use the option in either Model_Crud or the ORM to define both a read-only and a read-write connection, so you don't have this issue anymore.

WanWizard commented 7 years ago

I've looked into the code, and I don't see any functional change. Database_Connection::instance() does not create a database connection, it only saves the driver name and creates a driver instance. It is only when you execute() that the connection is made, either to the database given when the instance was created, or to the database passed on as argument to execute(). Same as always.

We try to take very good care in not breaking backward compatibility, even with a major change like a rewrite of the database layer. None of our applications needed a code change when we introduced it.

So I am struggling to understand what exactly your problem is. DB::select() does NOT return an object with a connection, it returns a query object linked to a driver instance, which is required to know the SQL dialect to use. That driver object doesn't do anything until you compile the query or execute it.

ysshir commented 7 years ago

Thank you for reply, and sorry for late reply.

And if you cannot understand my English, please let me know, I'll try to explain with google translate.

I think I can understand what you are talking about.

It can be fixed, if I set readonly instance as default. But Model_Crud::find() used to use one of reaonly instances randomly.

DB::select() was changed on dbe536aa369f5a0287030518af36702873a71f5a. before the change, DB:select returned select query object without $_connection property. therefore the connection is determined in execute() method with this code \Database_Connection::instance($db, null, ! $this instanceof \Database_Query_Builder_Select); \Database_Connection::instance($db, null, false) returns one of readonly instances randomly.

So I thought default function should use readonly intances. thanks.

WanWizard commented 7 years ago

As I said, the change was needed because the classes are no longer used statically, and they now need to know for which SQL dialect the class is instantiated.

So if Model_Crud now uses the incorrect database connection, it needs to be changed to adapt to the new database layer, not they other way around. I haven't used Model_Crud myself (ever), so I need to look into the code to see what is needed.