use common sql interfaces

[prolic]

See also here: https://github.com/prolic/sql (We would need to move the repo to amphp organization)

If I missed something, let me know please.

[prolic]

see also https://github.com/amphp/postgres/pull/14

[Sevavietl]

Only a couple of days ago I was thinking about switching between MySQL and Postgres, and how problematic that would be without the common interface.

[prolic]

maybe we can also unify the Connector interface?

[prolic]

@kelunik changes done, I also invited you as collaborator, so you can transfer the repository.

[bwoebi]

Why did you rename the Connector.php file to Collector?

Also, unifying Connectors sounds like a good idea, the basic connection configuration is identical. However it probably should be of type string | ConnectionConfig then (with ConnectionConfig being a non-final class extended in each of the repositories with mysql/pg specific configuration).

[prolic]

@bwoebi ups, my bad, fixed

[kelunik]

@prolic Seems like I only have read / write access, but no admin access, which is required to transfer a repository.

[prolic]

@kelunik I transferred the repo to your private account, then you can move to amphp. Admin rights are not possible for private account's repos (I think).

[prolic]

@bwoebi about the connection options as argument for connector: sounds good, I'll try to find some time tomorrow to add this change.

[prolic]

@kelunik @trowski I also refactored the connection object, please review and let me know if I missed something and see also postgres-PR.

[trowski]

Looks good, just a couple of things:

Connection::connect() should not be part of the interface, as postgres connection objects do not have this method. As for switching away from the static constructor for Connection in this library, I don't feel strongly either way, but the current code will require some changes in Processor because the connection is in an unusable state until connect() is called. I probably went with the static constructor because it just seemed simpler. The static constructor also had a nice duality with the postgres library.

You repeat methods in interfaces that inherit, such as Connection and Pool. I feel this is confusing as I lose track of where the method is originally defined.

[prolic]

@trowski see f.e. here: https://github.com/prolic/postgres/blob/c503e0bd516b923d9e61bc23915e3bffec50859b/src/PgSqlConnection.php#L23

[prolic]

@trowski You repeat methods in interfaces that inherit, such as Connection and Pool. - Where exactly? I will fix it.

[prolic]

Another note, before we merge mysql / postgres changes, we should move sql repo into amphp organization and create an entry in packagist. Then we can re-run the travis tests for mysql / postgres and see if something is broken somewhere.

[prolic]

mmh.. thinking again about the connect method. Maybe I can make put final public static function connect(ConnectionConfig $config, CancellationToken $token = null): Promise in the interface as static constructor. I can build then the rest internally. For Mysql this means that $socket = yield Socket\connect($config->connectionString(), $connectContext, $token); is called within this static constructor, no need to pass a socket connection. Thoughts?

[trowski]

@prolic transaction() is repeated in Pool. isAlive(), close(), query(), transaction(), prepare(), and execute() are repeated in Connection.

before we merge mysql / postgres changes, we should move sql repo into amphp organization and create an entry in packagist.

Of course. Just need @kelunik to transfer it.

Maybe I can make put final public static function connect(ConnectionConfig $config, CancellationToken $token = null): Promise in the interface as static constructor.

Yeah, that should be fine. We can mock the socket when creating the Processor object passed to Connection for testing purposes, so the socket can be created in connect() as you suggest. Then just revert the changes you made with the connect() method in postgres as well (except using a ConnectionConfig object in postgres instead of a string of course).

[kelunik]

I've transferred the repository and added it to Packagist.

[kelunik]

Travis CI and Coveralls are enabled as well now.

[prolic]

Do I still have write access?

On Fri, Jun 29, 2018, 04:37 Niklas Keller notifications@github.com wrote:

Travis CI and Coveralls are enabled as well now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-401165492, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvBZk9SYbrIIb6LTF3vElOJ2xFR3Dks5uBT6CgaJpZM4U3u6y .

[kelunik]

You should already have an invitation for write access, yes. :-)

[prolic]

Nice. Will finish the rest tomorrow.

On Fri, Jun 29, 2018, 04:54 Niklas Keller notifications@github.com wrote:

You should already have an invitation for write access, yes. :-)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-401170200, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvNTex5wBLXO4vKQ_RnFTl7ctytFzks5uBUKKgaJpZM4U3u6y .

[prolic]

rebased to resolve conflicts

[prolic]

@kelunik travis didn't fetch latest amphp/sql changes, it's testing old commits, thus travis failed. any idea why?

[prolic]

next build again: - Installing amphp/sql (dev-master 57d7276): Loading from cache

but latest commit is 9d31f9be697421f46cc97489e9f2e7862c333b61

[prolic]

@kelunik maybe amphp/sql has not the packagist auto update enabled?

[kelunik]

@prolic The Packagist service is enabled, but sure why it's not auto-updated. Manually updated it on Packagist now.

[prolic]

Ok, can you restart mysql travis build? So I can see if I missed something.

On Fri, Jun 29, 2018, 19:16 Niklas Keller notifications@github.com wrote:

@prolic https://github.com/prolic The Packagist service is enabled, but sure why it's not auto-updated. Manually updated it on Packagist now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-401324927, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvBRYhbOSMY5ticp05BSgdCwcGaFeks5uBgyCgaJpZM4U3u6y .

[prolic]

https://packagist.org/packages/amphp/sql is not updated, thus 7.0 tests fail on travis. 7.1 + 7.2 tests have segmentation fault on travis.

[trowski]

@prolic Updated Pool in amphp/sql. Feel free to update this PR based on that. Something came up that needs my attention today, but I should have time to look at this again tonight.

[prolic]

@trowski @kelunik updated

[prolic]

@trowski In AbstractPool class we have this: https://github.com/amphp/sql/blob/master/src/AbstractPool.php#L75

But there is no doPrepare method anymore. Hence all tests fail.

[trowski]

Oh yeah… thats why prepare() was separated like that. Fixed.

[prolic]

Can you restart Travis build please?

On Sat, Jun 30, 2018, 21:59 Aaron Piotrowski notifications@github.com wrote:

Oh yeah… thats why prepare() was separated like that. Fixed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-401542998, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvN8pQ427psUwAc3m1R_msidOKuUvks5uB4RPgaJpZM4U3u6y .

[bwoebi]

Nothing against purism, but $db = yield Mysql\connect(Mysql\ConnectionConfig::parseConnectionString("host=".DB_HOST.";user=".DB_USER.";pass=".DB_PASS.";db=".DB_NAME)); is just ugly.

[prolic]

We can import that directly if you prefer

On Thu, Jul 5, 2018, 02:01 Bob Weinand notifications@github.com wrote:

Nothing against purism, but $db = yield Mysql\connect(Mysql\ConnectionConfig::parseConnectionString("host=".DB_HOST.";user=".DB_USER.";pass=".DB_PASS.";db=".DB_NAME)); is just ugly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-402535875, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvJvk6WsmwU-qkNol4eqLnKkMsb5Uks5uDQMBgaJpZM4U3u6y .

[bwoebi]

Yeah, mysql\connect should try to parse parameter as connection string if's not a config

[prolic]

I don't like this type juggling stuff. If it's an object do this, if it's a string do that, etc. Better to add another function like Mysql\connectFromString

On Thu, Jul 5, 2018, 02:04 Bob Weinand notifications@github.com wrote:

Yeah, mysql\connect should try to parse parameter as connection string if's not a config

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-402536278, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvIOZporSyNuV4tpLZeWakZePHCtdks5uDQPGgaJpZM4U3u6y .

[kelunik]

I don't like the connection string at all. Let's just accept an object?

$config = (new Amp\Mysql\ConnectionConfig)
    ->withHost(...)
    ->withPort(...)
    ->withUsername(...)
    ->withPassword(...)
    ->withDatabase(...);

$connection = yield Amp\Mysql\connect($config);

[prolic]

Yes

On Thu, Jul 5, 2018, 02:25 Niklas Keller notifications@github.com wrote:

I don't like the connection string at all. Let's just accept an object?

$config = (new Amp\Mysql\ConnectionConfig) ->withHost(...) ->withPort(...) ->withUsername(...) ->withPassword(...) ->withDatabase(...);

$connection = yield Amp\Mysql\connect($config);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-402538890, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvCvQq7oLDaa70CtosqMw6eShaHETks5uDQiJgaJpZM4U3u6y .

[prolic]

@trowski what needs to be done to get this merged?

[trowski]

@prolic I don't have time to look in detail right now, but it needs to be updated to the latest changes in amphp/sql if it isn't already. Use amphp/postgres for guidance if there's implementation questions or ask in IRC. 😄

[prolic]

Ok, I'll check this weekend Sascha-Oliver Prolic

Am Do., 13. Sep. 2018 um 22:48 Uhr schrieb Aaron Piotrowski < notifications@github.com>:

@prolic https://github.com/prolic I don't have time to look in detail right now, but it needs to be updated to the latest changes in amphp/sql if it isn't already. Use amphp/postgres for guidance if there's implementation questions or ask in IRC. 😄

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/amphp/mysql/pull/74#issuecomment-421033980, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYEvG_uz3Sz1EQA2Mf2q6LoBS0o4SvPks5uanA6gaJpZM4U3u6y .

[prolic]

Some tests are still failing. Unfortunately I'm out of time for today. Feel free to pick up my work and continue on it.

[prolic]

and segmentation fault on travis!