kohana / database

A Kohana module for database interactions, building queries, and prepared statements
http://kohanaframework.org/documentation
158 stars 140 forks source link

Update Where.php #75

Closed hujiyang closed 10 years ago

hujiyang commented 10 years ago

When

DB::select()->from('users')->where('id', 'IN', array(1,2,3));

Kohana Database will act as

SELECT * FROM `users` WHERE `id` IN (1,2,3)

But when

DB::select()->from('users')->where('id', 'IN', array());

The SQL:

SELECT * FROM `users` WHERE `id` IN ()

will run a fault.

So the change is When

DB::select()->from('users')->where('id', 'IN', array());

The SQL will be:

SELECT * FROM `users` WHERE 1 = 0
kemo commented 10 years ago

I'm not sure if I like this. It will produce unexpected query results and some debugging pain + the code you pushed is broken (using $op where it's not available)

acoulton commented 10 years ago

@kemo I like the idea that passing an empty IN array will return an empty result rather than throwing - I sort of think that is expected. I use that pattern elsewhere and it does make it easier to handle some cases in particular where the list of items for the IN is dynamic.

But obviously the code would need to work, and ideally be covered by a unit test.

kemo commented 10 years ago

@acoulton I like the idea, just don't like how the query gets changed. IMO it should dismiss empty IN() statements at query compilation time

acoulton commented 10 years ago

@kemo yeah, that makes more sense probably.

lyt8384 commented 10 years ago

That looks goodI think somay be very useful! up~32 ups~

stenote commented 10 years ago

32 zans!!!!

zGaron commented 10 years ago

geilivebal ..

enov commented 10 years ago

Sorry to close this as it targets the master branch. @hujiyang if you can make another PR targeting 3.3/develop and taking into consideration @kemo's comment? Also, kindly add some unit tests as @acoulton suggested :)

Thanks!

zombor commented 10 years ago

Also, these commits need better messages. This is unacceptable. At the very least commit messages need to reference issue numbers.

enov commented 10 years ago

@zombor, better messages is needed indeed, the title generated automatically by Github is not enough. But we can find PRs of commits by entering its SHA into the search bar.

zombor commented 10 years ago

PRs should be rejected by default if they don't supply a sufficient commit message describing in detail what was changed and what issue it references. Describing this stuff in the PR description is nice, but it needs to be in the commit instead. Github is not the end-all-be-all data source for history. The repository itself is.

enov commented 10 years ago

Hmm, I understand now what you mean @zombor.

Ikke commented 10 years ago

Yes, this was bothering me as well. Note that when you supply a good commit message, github will auto fill it in the description.

Use the first line as a summary, then leave a blank line, and then explain why the change is made. On Oct 1, 2014 5:16 PM, "Samuel Demirdjian" notifications@github.com wrote:

Hmm, I understand now what you mean @zombor https://github.com/zombor.

— Reply to this email directly or view it on GitHub https://github.com/kohana/database/pull/75#issuecomment-57480842.