Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ysshir opened this issue May 18, 2017 · 4 comments
Open

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

ysshir opened this issue May 18, 2017 · 4 comments

Comments

@ysshir
Copy link

ysshir commented May 18, 2017

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
Copy link
Member

WanWizard commented May 18, 2017

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
Copy link
Member

WanWizard commented May 18, 2017

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
Copy link
Author

ysshir commented May 25, 2017

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 dbe536a.
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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants