Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Adding unix_socket to dsn variables in PDO Adapter #48

Merged
merged 1 commit into from
Dec 9, 2015

Conversation

KIVagant
Copy link
Contributor

Issue #47 PR

@Maks3w
Copy link
Member

Maks3w commented Oct 28, 2015

Need test

@KIVagant
Copy link
Contributor Author

Ok, in progress...

@KIVagant
Copy link
Contributor Author

@Maks3w , I added test. Now look's better?

@Maks3w
Copy link
Member

Maks3w commented Oct 28, 2015

Seems ok. I'll forward this to maintainers with more experience of this for evaluation.

@KIVagant
Copy link
Contributor Author

KIVagant commented Nov 3, 2015

Hello again, @Maks3w . Who should to check the PR? Can I hope that it will be reviewed soon?

@Maks3w
Copy link
Member

Maks3w commented Nov 3, 2015

ping @zendframework/community-review-team

@@ -237,6 +240,9 @@ public function connect()
if (isset($charset) && $pdoDriver != 'pgsql') {
$dsn[] = "charset={$charset}";
}
if (isset($unix_socket)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot set the unix socket if a hostname or port is passed, this is really only relevant for mysql as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot set the unix socket if a hostname or port is passed

I think, PDO can resolve this issue without problems and zend-db code does not needed additional 'if-else' for this case. But, if you think another, I can add some checks for host and port variables.

This is really only relevant for mysql as well.

In fact, PDO_PGSQL DSN can connect to unix socket too, but libpq-connect used "host" variable for path to socket.
What do you think I need to do? Add here if-condition for mysql-pdo only? Or add special condition for converting $unix_socket to $host variable for PDO_PGSQL?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to add the if else statement since the manual does explicitly point it out for PDO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it might be better to throw an exception in that case; rather than leave it to undefined behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'll try to resend PR tomorrow

@KIVagant
Copy link
Contributor Author

KIVagant commented Dec 9, 2015

Sorry, I don't have enough time to finish and test this PR.

@KIVagant KIVagant closed this Dec 9, 2015
@mwillbanks mwillbanks reopened this Dec 9, 2015
@mwillbanks
Copy link
Contributor

Hello @KIVagant I'll take care of the update in this case and update it based on what is needed.

@KIVagant
Copy link
Contributor Author

KIVagant commented Dec 9, 2015

@mwillbanks, thank you.

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

Successfully merging this pull request may close these issues.

None yet

3 participants