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

Fixes the special characters in bindParam for PDO #224

Merged
merged 4 commits into from Nov 29, 2017

Conversation

Projects
None yet
5 participants
@ezimuel
Member

ezimuel commented Mar 3, 2017

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_]:

return ':' . md5($name);

For instance, a filed name test$ will be translated in :79f5d7a7e10367f279c2500dcd03b3de as bind param name. I used the md5() approach after some benchmark tests, see comments and commits below. I also provided unit test and integrational test.

@ezimuel ezimuel referenced this pull request Mar 3, 2017

Closed

Fix PDO parameter names #178

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Mar 8, 2017

Member

In the last commit, I changed the usage of preg_replace_callback in favor of md5() for performance reason. I did some benchmark test showing a big difference. MD5 if from 3x to 4x faster than preg_replace_callback.

Member

ezimuel commented Mar 8, 2017

In the last commit, I changed the usage of preg_replace_callback in favor of md5() for performance reason. I did some benchmark test showing a big difference. MD5 if from 3x to 4x faster than preg_replace_callback.

@ezimuel ezimuel added the bug label Mar 8, 2017

@ezimuel ezimuel added this to the 2.9.0 milestone Mar 8, 2017

@alshayed

This comment has been minimized.

Show comment
Hide comment
@alshayed

alshayed Nov 9, 2017

Contributor

Hi, is there any expected time frame for this to get merged in and released? Thanks!

Contributor

alshayed commented Nov 9, 2017

Hi, is there any expected time frame for this to get merged in and released? Thanks!

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 10, 2017

Member

@alshayed I'll release in two weeks, sorry for the delay.

Member

ezimuel commented Nov 10, 2017

@alshayed I'll release in two weeks, sorry for the delay.

@weierophinney

Looks good; I've provided a little feedback on a few readability changes, but otherwise, ready to merge!

Show outdated Hide outdated test/Adapter/Driver/Pdo/StatementTest.php Outdated
Show outdated Hide outdated test/Adapter/Driver/Pdo/TestAsset/SqliteMemoryPdo.php Outdated
@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 29, 2017

Member

@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.

Member

ezimuel commented Nov 29, 2017

@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.

@weierophinney weierophinney merged commit 2e3bf2d into zendframework:master Nov 29, 2017

weierophinney added a commit that referenced this pull request Nov 29, 2017

Merge pull request #224 from ezimuel/fix/pdo-bindparam
Fixes the special characters in bindParam for PDO

Conflicts:
	src/Adapter/Driver/Pdo/Pdo.php
	test/Adapter/Driver/Pdo/StatementTest.php

weierophinney added a commit that referenced this pull request Nov 29, 2017

weierophinney added a commit that referenced this pull request Nov 29, 2017

weierophinney added a commit that referenced this pull request Nov 29, 2017

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Dec 7, 2017

Member

I'm removing this PR because introduces issue like #288. It's not possible to fix #35 because of PHP bug 43130. This bug won't fix because of this and this. Releasing zend-db 2.9.1 with this revert.

Member

ezimuel commented Dec 7, 2017

I'm removing this PR because introduces issue like #288. It's not possible to fix #35 because of PHP bug 43130. This bug won't fix because of this and this. Releasing zend-db 2.9.1 with this revert.

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Dec 7, 2017

Contributor

@ezimuel

It's not possible to fix #35

It is possible, IMHO.
What if we also replace fields in the query, not only on binding?

Contributor

webimpress commented Dec 7, 2017

@ezimuel

It's not possible to fix #35

It is possible, IMHO.
What if we also replace fields in the query, not only on binding?

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Dec 7, 2017

Member

@webimpress it's not safe to use characters like - in bind param name. Please, read the php.cvs 46848 and 47302.

Member

ezimuel commented Dec 7, 2017

@webimpress it's not safe to use characters like - in bind param name. Please, read the php.cvs 46848 and 47302.

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Dec 7, 2017

Contributor

@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?

Contributor

webimpress commented Dec 7, 2017

@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?

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Dec 7, 2017

Member

@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.

Member

ezimuel commented Dec 7, 2017

@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.

ezimuel added a commit to ezimuel/zend-db that referenced this pull request Dec 7, 2017

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Dec 7, 2017

Contributor

@ezimuel Ok, fine then, so the #35 should be marked as "invalid" or "won't fix" and never tried to be fixed 😄

Contributor

webimpress commented Dec 7, 2017

@ezimuel Ok, fine then, so the #35 should be marked as "invalid" or "won't fix" and never tried to be fixed 😄

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Dec 7, 2017

Member

@webimpress I know, this was my fault, sorry.

Member

ezimuel commented Dec 7, 2017

@webimpress I know, this was my fault, sorry.

@webimpress

This comment has been minimized.

Show comment
Hide comment
@webimpress

webimpress Dec 7, 2017

Contributor

@ezimuel It's fine, you've tried to fix #178. Everyone makes mistake...
Good that you handle it very quickly and we will have shortly 2.9.1. Thanks for it! :)

Contributor

webimpress commented Dec 7, 2017

@ezimuel It's fine, you've tried to fix #178. Everyone makes mistake...
Good that you handle it very quickly and we will have shortly 2.9.1. Thanks for it! :)

@alextech

This comment has been minimized.

Show comment
Hide comment
@alextech

alextech Dec 7, 2017

Contributor

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 InvalidArgumentException to give a nice message before letting it crash at driver level.

Contributor

alextech commented Dec 7, 2017

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 InvalidArgumentException to give a nice message before letting it crash at driver level.

weierophinney added a commit that referenced this pull request Dec 7, 2017

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Dec 7, 2017

Member

@alextech I added the throw Exception for invalid characters [^a-zA-Z0-9_] in my revert PR #289.

Member

ezimuel commented Dec 7, 2017

@alextech I added the throw Exception for invalid characters [^a-zA-Z0-9_] in my revert PR #289.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment