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

Fix setting persistent option for PDO connection #2092

Merged

Conversation

paulermo
Copy link
Contributor

@paulermo paulermo commented May 10, 2022

This fixes the ability to use persistent connections by PDO.
One can set generic PDO attributes (#1904) but the attribute - \PDO::ATTR_PERSISTENT - is useless while setting it by setAttribute after PDO object instantiation:

Note:
If you wish to use persistent connections, you must set PDO::ATTR_PERSISTENT in the array of driver options passed to the PDO constructor. If setting this attribute with PDO::setAttribute() after instantiation of the object, the driver will not use persistent connections.
https://www.php.net/manual/en/pdo.connections.php

From the point of view of usage, nothing changes: like other PDO arguments we specify attr_persistent => true config option. Before the PDO instatiation this attribute (if specified) is passed to driver options instead of setting in setAttribute after it.

It's done in Mysql, Postgres and SQLite adapters. SQL Server PDO driver does not support this specific attribute.

tests/Phinx/Db/Adapter/MysqlAdapterTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/PostgresAdapterTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/SQLiteAdapterTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/MysqlAdapterTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/PostgresAdapterTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/SQLiteAdapterTest.php Outdated Show resolved Hide resolved
@paulermo paulermo requested a review from MasterOdin May 11, 2022 10:50
Copy link
Member

@MasterOdin MasterOdin left a comment

Choose a reason for hiding this comment

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

Does attr_persistent work for SQL Server? If not, would there be any harm in passing it as an option anyway?

If either of the above is "yes", I think it would make sense to lift the code setting this option out of the specific adapters into the PdoAdapter::createPdoConnection method, having it be:

        $adapterOptions = $this->getOptions() + [
            'attr_errmode' => PDO::ERRMODE_EXCEPTION,
        ];
        if (isset($adapterOptions['attr_persistent'])) {
          $driverOptions[PDO::ATTR_PERSISTENT] = $adapterOptions['attr_persistent'];
        }

This way we are following DRY principles.

@paulermo
Copy link
Contributor Author

Does attr_persistent work for SQL Server? If not, would there be any harm in passing it as an option anyway?

No, SQL Server does not support persistent connections. And that's why I have moved the setting of driver attribute out from PDOAdapter to specific MySQL, Postgres and SQLite. For SQL Server nothing changes in comparison to current behavior.

This way we are following DRY principles.

Yes, that was my first solution (7a49fa2), but I have found that it could break SQL server functionality and moved it out in 2b86062.

@paulermo paulermo requested a review from MasterOdin June 6, 2022 11:51
@MasterOdin
Copy link
Member

Alright, I found microsoft/msphpsql#65 which confirms that setting that will in fact cause the driver to throw an exception, so we cannot set it for all drivers. I'm fine with just ignoring it for mssql as opposed to throwing an exception like cakephp/database does.

@MasterOdin MasterOdin changed the title support PDO persistent connection Fix setting persistent option for PDO connection Jul 4, 2022
@MasterOdin MasterOdin merged commit 11ec6ee into cakephp:master Jul 4, 2022
@dereuromark
Copy link
Member

I think we need to merge this master branch into 0.x and then stop using master (as default).
@markstory Do you have access to change default branch?

@othercorey
Copy link
Member

We have a small discussions on this in the phinx-dev channel. Leaving it up to @markstory still, but @MasterOdin had some thoughts after I explained the naming alignment.

@MasterOdin
Copy link
Member

I've opened #2097 to discuss the branch switch and can carry this conversation there.

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.

5 participants