ccmbenchmark / ting

Lightweight PHP datamapper
http://tech.ccmbg.com/ting/doc/3.x/en/index.html
Apache License 2.0
1 stars 5 forks source link

Fix boolean values support in prepared query #58

Closed Deuchnord closed 2 years ago

Deuchnord commented 2 years ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Licence Apache-2.0
Fixed tickets Ø

When executing prepared query on MySQL, the boolean values are currently converted to their string equivalent:

php > var_dump((string) true);
string(1) "1"
php > var_dump((string) false);
string(0) ""

Especially for false, this leads to an SQL error, because MySQL doesn't expect an empty string for such value. In this PR, we convert the boolean values to an integer in order to give MySQL 0 (== false) or 1 (== true).

Important note: this issue could have been avoided by using the mysqli_prepare(). Is there a reason for not using it in the first place?

xavierleune commented 2 years ago

@Deuchnord thanks for the pull request. Actually Ting supports raw queries and prepared statements, that's up to the user to decide the option to retain. Driver::quoteValues is used in the first case, while mysqli_prepare is used in the latter.

xavierleune commented 2 years ago

@Deuchnord thanks for the pull request. Actually Ting supports raw queries and prepared statements, that's up to the user to decide the option to retain. Driver::quoteValues is used in the first case, while mysqli_prepare is used in the latter.

But I think we might have another issue in Statement::execute, which internally calls mysqli_stmt::bind_param, boolean values seems not to be bound to an integer. By the way, the syntax call_user_func_array(array($this->driverStatement, 'bind_param'), $values); could probably be rewritten to $this->driverStatement->bind_param(...$values);, now that we support only PHP >= 7.1. What do you think ?

Deuchnord commented 2 years ago

Actually Ting supports raw queries and prepared statements, that's up to the user to decide the option to retain. Driver::quoteValues is used in the first case, while mysqli_prepare is used in the latter.

I'm not sure to understand the difference between raw queries and prepared statements if they actually both do some preparation-ish processing. Isn't it the point of raw queries to execute them exactly like they have been written?

But I think we might have another issue in Statement::execute, which internally calls mysqli_stmt::bind_param, boolean values seems not to be bound to an integer.

Good catch! It's exactly the same bug, so I'll fix it here too :)

By the way, the syntax call_user_func_array(array($this->driverStatement, 'bind_param'), $values); could probably be rewritten to $this->driverStatement->bind_param(...$values);, now that we support only PHP >= 7.1. What do you think ?

I agree with you, it would make it much more readable :+1:

xavierleune commented 2 years ago

I'm not sure to understand the difference between raw queries and prepared statements if they actually both do some preparation-ish processing. Isn't it the point of raw queries to execute them exactly like they have been written?

They are not doing exactly the same job. A prepared statement is prepared by the database, it means you have minimum 2 round trips PHP <-> Database, 1 to prepare the statement, the 2nd to execute it. Using raw queries with the same syntax offers a good compromise security vs performance. You also can execute raw queries without setting any param, but I would discourage it, as you have to escape values on your own, which means there is a higher chance of doing it wrong. Personally I prefer to always use prepared statements, but it's only my opinion.

Thanks for the work on the Statement !