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

persistent connections working again #1295

Merged
merged 3 commits into from
Nov 16, 2016
Merged

Conversation

prgTW
Copy link
Contributor

@prgTW prgTW commented Nov 11, 2016

Persistent connections are available again by not overriding PDO statement class

fixes this error:

Warning: PDO::setAttribute(): SQLSTATE[HY000]: General error: PDO::ATTR_STATEMENT_CLASS cannot be used with persistent PDO instances

ref: #1225

@marcj
Copy link
Member

marcj commented Nov 11, 2016

We should rather completely drop Propel\Runtime\Adapter\Pdo\PdoStatement instead of using it only for HHVM.

@staabm
Copy link
Member

staabm commented Nov 11, 2016

alternatively we should drop HHVM support. not sure this runtime is relevant for somebody.

@marcj
Copy link
Member

marcj commented Nov 11, 2016

or both :P

@prgTW
Copy link
Contributor Author

prgTW commented Nov 11, 2016

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

@prgTW
Copy link
Contributor Author

prgTW commented Nov 14, 2016

so ... any other thoughts about merging this one and restoring persistent connections in propel?

@marcj
Copy link
Member

marcj commented Nov 14, 2016

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 Propel\Runtime\Adapter\Pdo\PdoStatement completely, instead of using it only for hhvm.

@prgTW
Copy link
Contributor Author

prgTW commented Nov 14, 2016

@marcj I removed Propel\Runtime\Adapter\Pdo\PdoStatement class as You suggested


namespace Propel\Runtime\Adapter\Pdo;

use Propel\Runtime\Connection\StatementInterface;
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@prgTW prgTW changed the title persistent connections working again (if not using HHVM) persistent connections working again Nov 14, 2016
@marcj marcj merged commit b1f8262 into propelorm:master Nov 16, 2016
@marcj
Copy link
Member

marcj commented Nov 16, 2016

Nice, good work, thanks!

@prgTW prgTW deleted the fix/issue-1225 branch November 16, 2016 22:49
@prgTW
Copy link
Contributor Author

prgTW commented Nov 16, 2016

great news, thank you too :)

@marcj
Copy link
Member

marcj commented Nov 20, 2016

Backported to data-mapper branch.

@kinekt4
Copy link

kinekt4 commented Sep 25, 2017

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:

"propel/propel": "dev-master#38f1044e9b57cb7f68b59224f1830bde7009673f"

Does anyone have a workaround so I don't have to be stuck on this commit?

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.

4 participants