laminas / laminas-db

Database abstraction layer, SQL abstraction, result set abstraction, and RowDataGateway and TableDataGateway implementations
https://docs.laminas.dev/laminas-db/
BSD 3-Clause "New" or "Revised" License
119 stars 66 forks source link

Fixed invalid quoting for postgresql adapter #98

Closed michalbundyra closed 3 years ago

michalbundyra commented 4 years ago

As described in https://github.com/zendframework/zend-db/issues/161


Originally posted by @xorock at https://github.com/zendframework/zend-db/pull/162

michalbundyra commented 4 years ago

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 https://github.com/zendframework/zend-db/pull/162#issuecomment-285839306

michalbundyra commented 4 years ago

@alextech this PR can be closed in favor of your https://github.com/zendframework/zend-db/pull/187 ?


Originally posted by @ezimuel at https://github.com/zendframework/zend-db/pull/162#issuecomment-349026702

michalbundyra commented 4 years ago

@ezimuel yes


Originally posted by @alextech at https://github.com/zendframework/zend-db/pull/162#issuecomment-349050046

michalbundyra commented 4 years ago

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


Originally posted by @alextech at https://github.com/zendframework/zend-db/pull/162#issuecomment-349051818

michalbundyra commented 4 years ago

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 https://github.com/zendframework/zend-db/pull/162#issuecomment-349873285

michalbundyra commented 4 years ago

@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 https://github.com/zendframework/zend-db/pull/162#issuecomment-349875474

michalbundyra commented 4 years ago

@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 https://github.com/zendframework/zend-db/pull/162#issuecomment-349986994

michalbundyra commented 4 years ago

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: https://github.com/zendframework/zend-db/pull/187#issuecomment-346635709. 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 https://github.com/zendframework/zend-db/pull/162#issuecomment-350204131

michalbundyra commented 4 years ago

@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 https://github.com/zendframework/zend-db/pull/162#issuecomment-350209180

michalbundyra commented 4 years ago

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 https://github.com/zendframework/zend-db/pull/162#issuecomment-350244764

michalbundyra commented 4 years ago

@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 https://github.com/zendframework/zend-db/pull/162#issuecomment-350247478

weierophinney commented 3 years ago

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: