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

Fixed invalid quoting for postgresql adapter #98

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

Fixed invalid quoting for postgresql adapter #98

michalbundyra opened this issue Jan 16, 2020 · 12 comments

Comments

@michalbundyra
Copy link
Member

As described in zendframework/zend-db#161


Originally posted by @xorock at zendframework/zend-db#162

@michalbundyra
Copy link
Member Author

Just in case maintainers deem necessary to fix this before I am done with my overly prolonged enterprise of making sequences feature complete, I would suggest that at least this PR should be fixed in
https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pgsql/Connection.php#L267
and sequence feature changed to

return $sequence['nextval'] = $this->tableGateway->adapter->driver->getLastGeneratedValue();

Two reasons:

  1. By redirecting to Connection instead of writing out SQL for sequence value querying, if the developer is using PDO then this is not a problem because https://github.com/zendframework/zend-db/blob/master/src/Adapter/Driver/Pdo/Connection.php#L419 takes care of this more accurately at driver level. Also helps with other engines preventing an ugly switch/case block.
  2. Code duplication resulting in chance that code from other places is relying on this causing errors.

If any queries are done manually they should be done using prepared statements to avoid overload on database engines to recreate execution plan for multitude of similar queries.


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

@michalbundyra
Copy link
Member Author

@alextech this PR can be closed in favor of your zendframework/zend-db#187 ?


Originally posted by @ezimuel at zendframework/zend-db#162 (comment)

@michalbundyra
Copy link
Member Author

@ezimuel yes


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

@michalbundyra
Copy link
Member Author

@ezimuel also very closely related to #233 you asked about earlier.


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

@michalbundyra
Copy link
Member Author

Maybe instead of closing it You can simply accept it (for now)? It does not interfere with anything else and it's strange, that so simple one-liner was not reviewed and fixed for over a year. All signs in the sky and earth indicate that the next few months / years will pass before someone approves the @alextech fixes.


Originally posted by @xorock at zendframework/zend-db#162 (comment)

@michalbundyra
Copy link
Member Author

@xorock fair enough, if ever get back to my stuff I can always rebase (as part of relearning process I would have to undergo at that point anyway)

However!, this business of quoteIdentifierChain vs quoteIdentifier quoteValue that is pointed out in the review by andrey-mokhov must stop which I fix in #233. Maybe you can put a word in for that?

Edit--- n/m quoteValue is unrelated to identifier chain. I meant to say identifier vs identifierChain which I think fixes inconsistencies you reported when using schema names.

Edit2. I saw a nice blog post just coming out promising these new features, so lets just use that as motivation for focusing on solving the architecture difficulties :) zend team really is busy.


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

@michalbundyra
Copy link
Member Author

@xorock

All signs in the sky and earth indicate that the next few months / years will pass before someone approves the @alextech fixes.

This sort of comment is not helpful, and disrespectful to the people maintaining the library. Please review our code of conduct, specifically points 5-7.

While I understand your frustration with a patch not being merged on a time schedule convenient to yourself, please be aware that we have around 200 components and modules to maintain, with only a couple dozen (mostly volunteer) people with commit rights. Please respect their efforts and time.


Originally posted by @weierophinney at zendframework/zend-db#162 (comment)

@michalbundyra
Copy link
Member Author

Sorry @weierophinney but I think disrespectful was when @alextech put a lot of effort into creating tests and fixing entire system and ezimuel just closed it: zendframework/zend-db#187 (comment). I fully understand how much components You are maintaining, I'm with ZF from ~v0.7 but also I think zend-db is one of the most important elements of the entire framework and should be patched much faster. As Your team noticed https://framework.zend.com/blog/2017-12-06-zend-db-2.9.0.html?utm_source=dlvr.it&utm_medium=twitter "This is our first new feature release in over 18 months".
I don't do it because of my personal benefits (I fixed it in my code long time ago). We all do it so that the community can benefit.


Originally posted by @xorock at zendframework/zend-db#162 (comment)

@michalbundyra
Copy link
Member Author

@xorock

I think disrespectful was when @alextech put a lot of effort into creating tests and fixing entire system and ezimuel just closed it: #187

Yes this was a mistake in a hurry, but the PR is open and nothing is lost.

I think zend-db is one of the most important elements of the entire framework

Sorry, but that's only your feeling. Compare with the number of downloads on packagist.org: https://packagist.org/?q=zend


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

@michalbundyra
Copy link
Member Author

Funny thing is we're not discussing how to improve zend-db but other unimportant things. Truth is, both schemas and aliases (if I remember well Matthew noticed this bug ~3years ago, for example try to alias table and then DELETE something) needs to be rewritten. We should think how to do this.

As You know, packagist show statistics for all downloads. If You check requirements for zend-mvc, zend-form and zend-expressive You will know why these and not other packages are at the first place in statistics. zend-db is required manually.


Originally posted by @xorock at zendframework/zend-db#162 (comment)

@michalbundyra
Copy link
Member Author

@xorock

Funny thing is we're not discussing how to improve zend-db but other unimportant things.

It's really simple: with the new release zend-db is now in the focus. 😄

…needs to be rewritten. We should think how to do this.

Go for it! Slack and the forum is open for suggestions and discussions.


Originally posted by @froschdesign at zendframework/zend-db#162 (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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants