Fixes the special characters in bindParam for PDO #224
Conversation
|
In the last commit, I changed the usage of |
|
Hi, is there any expected time frame for this to get merged in and released? Thanks! |
|
@alshayed I'll release in two weeks, sorry for the delay. |
| ', :' . | ||
| md5('text$') . | ||
| ')' | ||
| ); |
There was a problem hiding this comment.
I'd use sprintf here to make it more readable:
$this->statement->prepare(sprintf(
'INSERT INTO test (text_, text$) VALUES (:%s, :%s)',
md5('text_'),
md5('text$')
));| implode(",", $this->errorInfo()) | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Return early if $sql is empty:
if (empty($sql)) {
return;
}
if (false === $this->exec($sql)) {
throw new \Exception(/* ... */);
}
|
@weierophinney thanks for the review. I added all the feedbacks. There are some conflicts due to the date of this PR but I think we can manage it during the merge. |
Fixes the special characters in bindParam for PDO Conflicts: src/Adapter/Driver/Pdo/Pdo.php test/Adapter/Driver/Pdo/StatementTest.php
ghost
mentioned this pull request
|
@webimpress it's not safe to use characters like |
|
@ezimuel yes, I've seen them. And as I understand you replace names with md5 names, but you are not changing these names in the query string but only on binding. What if we change them in both places? |
|
@webimpress It's not considered a best practice to use these characters in bind param name. Why you want to allow a bad practice in our code base? Moreover, it will introduce high CPU usage, with preg_replace() + md5() execution for each param in SQL statements. This is not a reliable solution. |
|
@webimpress I know, this was my fault, sorry. |
|
Automatically changing anything in the query will make it very difficult to debug at database level. A common debugging procedure is looking through database logs, in SqlServer watching through real time activity monitor. Then, copy those queries and search for them in the codebase. Changing the names will no longer match logged queries with whats in code. I think instead should do validation for special chars at the same place you tried md5() and throw |
This PR solve #35 and replaces #178. The $name in bindParam are replaced with the following, due to the PDO restriction [0-9a-zA-Z_]:
For instance, a filed name
test$will be translated in:79f5d7a7e10367f279c2500dcd03b3deas bind param name. I used themd5()approach after some benchmark tests, see comments and commits below. I also provided unit test and integrational test.