-
Notifications
You must be signed in to change notification settings - Fork 397
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
persistent connections working again #1295
Conversation
16e8110
to
2e5284a
Compare
We should rather completely drop Propel\Runtime\Adapter\Pdo\PdoStatement instead of using it only for HHVM. |
alternatively we should drop HHVM support. not sure this runtime is relevant for somebody. |
or both :P |
With this PR I wanted only to make persistent connections available again and didn't want to alter HHVM in any way cause I don't use it and don't have it and didn't want to make any backward-incompatible changes. Droping HHVM support could be a separate PR though |
so ... any other thoughts about merging this one and restoring persistent connections in propel? |
What do you mean by dropping hhvm support? This drops basically the overwritten class support, for whatever that was used. Let us just fix it the right way and remove |
2e5284a
to
a293938
Compare
@marcj I removed |
|
||
namespace Propel\Runtime\Adapter\Pdo; | ||
|
||
use Propel\Runtime\Connection\StatementInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this interface Propel\Runtime\Connection\StatementInterface
is not used somewhere as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it's used in quite few places, shouldn't it's usages be converted to usages of \PdoStatement
as well, am I getting You right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
Nice, good work, thanks! |
great news, thank you too :) |
Backported to data-mapper branch. |
Hi guys. This pull request broke something that used to work: #1413 In composer.json, I've currently had to make sure propel use the commit prior to this:
Does anyone have a workaround so I don't have to be stuck on this commit? |
Persistent connections are available again by not overriding PDO statement class
fixes this error:
ref: #1225