-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Delegate quoteIdentifier operation to quoteIdentifierChain #77
Comments
Thanks for the hint! Originally posted by @froschdesign at zendframework/zend-db#233 (comment) |
How does that one small sprintf() change lowers unit test coverage across all files? Originally posted by @alextech at zendframework/zend-db#233 (comment) |
@alextech you mentioned that this PR fixes some potential issue in SQL syntax generation. Can we found and example? We need to prove with a unit test that this PR fixes something. Now, we have the same unit test of before and this PR seems not necessary. Originally posted by @ezimuel at zendframework/zend-db#233 (comment) |
@ezimuel pretty much every quoteIdentifier call across the codebase when using PostgreSQL or SQL Server including userland usage in other libraries and application code https://github.com/zendframework/zend-db/pull/247/files/bfb73c8bf4be5d98207a9911eca14aabdadd79c8#diff-0113773e7838ad267d6360c243edc298R235 Or related random example from other library zendframework/zend-log#63, The reoccurring theme is the need to check database type and if an object path was given for every attempt to quote an identifier to decide if For explanation of what I mean by "object path", look last comment in #206 and a typical usage example in the issue description. ( I cannot remember if this PR can fix that issue directly, but its a step towards it, as with it, identifiers can be expressed as an array or if string given, explode on object path delimiter to create needed array, and be able to quote it without having to worry about it being chain or what) Originally posted by @alextech at zendframework/zend-db#233 (comment) |
|
Clean up identifier quoting. quoteIdentifierChain was doing the same thing quoteIdentifier was doing but without safety of using predefined escape tokens. In turn, quoteIdentifier was producing same result as quoteIdentifierChain by adding quotes around the token, which can be simplified by immediately converting identifier to array, pass it to identiferChain, and achieve same result. Overriding is now necessary only if there are really different rules by the engine. Eg. DB2 seems to be doing something very different.
This will help a lot with consistency in DB engines that have multiple identifiers in the path of a database objects. Eg. PostgreSQL always has trouble between choosing identifier vs chain. This can potentially solve a good portion of bad syntax generation for them.
No unit tests added because this is well covered by existing cases; nothing new or different is generated.
Originally posted by @alextech at zendframework/zend-db#233
The text was updated successfully, but these errors were encountered: