Skip to content
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

Closed
michalbundyra opened this issue Jan 16, 2020 · 5 comments
Closed

Delegate quoteIdentifier operation to quoteIdentifierChain #77

michalbundyra opened this issue Jan 16, 2020 · 5 comments

Comments

@michalbundyra
Copy link
Member

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

@michalbundyra
Copy link
Member Author

@alextech

No unit tests added because this is well covered by existing cases; nothing new or different is generated.

Thanks for the hint!


Originally posted by @froschdesign at zendframework/zend-db#233 (comment)

@michalbundyra
Copy link
Member Author

How does that one small sprintf() change lowers unit test coverage across all files?


Originally posted by @alextech at zendframework/zend-db#233 (comment)

@michalbundyra
Copy link
Member Author

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

@michalbundyra
Copy link
Member Author

@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 zendframework/zend-db#161 (Ignore the sequence part)

Or related random example from other library zendframework/zend-log#63,
some of my DDL PRs...

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 quoteIdentifier or quoteIdentifierChain should be used, both within zend db codebase, and in application code using zend db. Any place where this is forgotten, such as pull request I linked, results in crashes that @xorock brought up in several places. As I was investigating why that is, I noticed how both functions essentially do the same thing with minor difference so I consolidated the usage. The lack of unit tests in itself proves that the PR does not break any existing usage; the whole point is to fix this inconsistency that makes all existing code work as it did before, while not needing to check database type and existence of path in the future.

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)

@weierophinney
Copy link
Member

This package is considered feature-complete, and is now in security-only maintenance mode, following a decision by the Technical Steering Committee.
If you have a security issue, please follow our security reporting guidelines.
If you wish to take on the role of maintainer, please nominate yourself

If you are looking for an actively maintained package alternative, we recommend:

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

No branches or pull requests

2 participants