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

Loosen type hint from DBAL PDOConnection to simply PDO object #306

Merged
merged 1 commit into from Oct 20, 2015
Merged

Loosen type hint from DBAL PDOConnection to simply PDO object #306

merged 1 commit into from Oct 20, 2015

Conversation

andylibrian
Copy link
Contributor

Refer to #304

@lsmith77
Copy link
Member

looks safe to me ..

@dbu wdyt?

@lsmith77 lsmith77 added review and removed wip/poc labels Oct 20, 2015
@dbu
Copy link
Member

dbu commented Oct 20, 2015

if this is needed, https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Connection.php#L1357 is lying as \PDO is not implementing Connection. @andylibrian if you change the typehint to the \Doctrine\DBAL\Driver\Connection class instead of the PDOConnection, does it work too? that would seem the correct fix.

@dbu
Copy link
Member

dbu commented Oct 20, 2015

though the sqliteCreateFunction method is not in Connection or PDOConnection itself, but directly in \PDO. so lets go for \PDO. can you adjust the @param annotation please, and remove the now unused use statement?

@andylibrian
Copy link
Contributor Author

Pushed. I squashed it as well. Please review and let me know your feedback. Thanks!

dbu added a commit that referenced this pull request Oct 20, 2015
…teFunctions

Loosen type hint from DBAL PDOConnection to simply PDO object
@dbu dbu merged commit 5bea743 into jackalope:master Oct 20, 2015
@lsmith77 lsmith77 removed the review label Oct 20, 2015
@dbu
Copy link
Member

dbu commented Oct 20, 2015

thanks. please ping us if you need a stable release tag for this. we just did one this morning, i would think to let things sit a bit until we see if you find other bugfixes that we should include in the release.

@andylibrian
Copy link
Contributor Author

No problem, not in a rush. It can wait for the next stable release. Thanks again.

@dbu dbu mentioned this pull request Jan 11, 2016
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

3 participants