getkirby-v2 / toolkit

This is the deprecated toolkit for Kirby v2.
http://getkirby.com
81 stars 50 forks source link

Query class: Preserving bindings between database calls in 2.3.0 #169

Closed ivi-admin closed 8 years ago

ivi-admin commented 8 years ago

We were getting the following error when making multiple calls on the same table:

exception 'PDOException' with message 'SQLSTATE[HY093]: Invalid parameter number: parameter was not defined'

Using the Database\Query->debug() method, we could tell that bindings were preserved between database calls. For example:

$users = $database->table('users');
$user = $users->where('email','=',$email)->first();

And later on in the file:

$updated = $users->where('id','=',$user->id)->update(array('last_login' => 'NOW()'));

When using debug() on that, the amount of bindings would be more than were defined in the update query. We were able to get around that by defining $users = $database->table('users'); once more above the update query.

That could be intended behavior, to require a new instance of the Database\Query class, but since it wasn't a clean update from 2.2.3 to 2.3.0 and it wasn't listed in the changelog, we thought it might be a bug. Please let us know if that will be changed (clearing out the bindings before setting new ones), or if each database call should use the Database->table() method.

ivi-admin commented 8 years ago

Actually nevermind, this was a mistake on our part -- a result of running the recursive git pull origin master on submodules but not the subsequent recursive git checkout master.

ivi-admin commented 8 years ago

Sorry to keep updating this, but it was actually closed prematurely. After updating an additional site in a dev environment and checking out the master branch of the updated submodules, we've been experiencing numerous database errors. We've been experiencing errors with the different aggregate() methods and setting table aliases in queries. Please let us know if you need a specific code sample, because that's something we'd be more comfortable sharing privately.

lukasbestle commented 8 years ago

Yes, the new database classes may break code. That's also good because a few security issues are prevented by that. However there may be some legit use-cases that are now not possible anymore.

Code samples and the error messages you get from them would be great. Please send me an email to lukas@. Thank you.

You wrote above that this specific issue was fixed by checking out the master branch. Is that still the case or did the error appear again?

ivi-admin commented 8 years ago

Would that be lukas at getkirby?

There is one client's site that was fixed by checking out the master branch, but another client's site was not fixed by doing so (a part of the site that's much more database-intensive), and we had to revert to 2.2.3 for them.

And yes, definitely good to err on the side of security! We're just trying to get a sense of what the update process will be like as we iterate out across all of our Kirby clients. Thanks!

lukasbestle commented 8 years ago

Yes, that's correct. I didn't post the full address because of spam bots. :)

What I just wanted to know here: Is the issue with binding preservation fixed for you? It's only natural that there may be other bugs, but they will end up in different GitHub issues.

ivi-admin commented 8 years ago

Ah, sorry about that. Yes, the binding preservation seems to be fixed. I'll email with the code sample for the remaining issues we're having. Thanks!