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

PostgreSql adapter #11

Closed
wants to merge 23 commits into from
Closed

PostgreSql adapter #11

wants to merge 23 commits into from

Conversation

agborkowski
Copy link
Contributor

PostgreSql adapter

@masom
Copy link
Contributor

masom commented Jun 20, 2011

Awesome, beat me to it.

@mariuswilms
Copy link
Member

How much do MySql and PostgreSql adapters actually have in common? Could they subclass a common base class?
We probably need a rdbms base class. Other rdbms adapters are already in sight (oracle, drizzle?).

Also what will the policy be with adapters we cannot (regularly) test ourselves?

@mariuswilms
Copy link
Member

First of all thanks @agborkowski and @zdrap for your hard work!

I've just reviewed the attached patches a bit more and have a few tips for improvement.

  • Commit messages could be more clearer (and more verbose where it makes sense).
  • Commit messages should be using English.
  • Try to merge commits where possible i.e. the ones related to tests.
  • When adding initial functionality you most often need just one solid commit. You could add the adapter in one commit and the tests for it in another.
  • Try to leave out noise in your patches i.e. [bf0e4dc]. I guess this wasn't intentional and that github doesn't make it clear with the pull request system.

@agborkowski
Copy link
Contributor Author

@DavidPersson we're start using lithium, git and github in our inc. so pls. forgive some mistake.

@mariuswilms
Copy link
Member

@agborkowski Let me rephrase that as it maybe came around wrong: It is really great and appreciated that you're putting time into improving Lithium. I took the pull request as a chance to make the submitted patches a cool and even better thing. So no worries.

My first comment is much more of a side note to how adapter additions should be handled in general, directed at the team.

@agborkowski
Copy link
Contributor Author

@DavidPersson of course i'm sure understand You, its good point (this commit) to start make a good pieces of lithium by community !

anyway sombody know (mysql_unbuffered_query) similar function in pgsql ?

@nateabele
Copy link
Member

@agborkowski The PostgreSQL driver for PHP does not support unbuffered queries. That's okay though, your adapter does not need to support every feature of other adapters. I'll try to test it and merge in your patches this weekend. Thanks again for the awesome work!

@nateabele
Copy link
Member

This pull request has been merged to the postgres development branch, which will be merged into the next release. Any further work should be branched from there. Thanks again for your efforts.

@nateabele nateabele closed this Jul 7, 2011
@xorock
Copy link

xorock commented Jan 16, 2012

I just can't understand one thing. Lithium is strictly connected to PHP 5.3. Why then, adapters are not written extending PDO?

@agborkowski
Copy link
Contributor Author

i think low level adapter are faster than create next abstract layer over PDO

@daschl
Copy link
Contributor

daschl commented Jan 16, 2012

@xorock @agborkowski We are currently in the process of moving our datasources over to PDO. MySQL has already been moved, sqlite3 is under way and postgres is also planned.

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

Successfully merging this pull request may close these issues.

None yet

7 participants