Added support for available DSN parameters for the PDOSqlsrv driver by aashmelev · Pull Request #3033 · doctrine/dbal

aashmelev · 2018-02-28T14:54:29Z

This is done to transfer parameters to the DSN.
The available parameters are described by link http://php.net/manual/en/ref.pdo-sqlsrv.connection.php

aashmelev · 2018-05-14T08:15:34Z

At the moment, there is no way to pass parameters to the driver.

Please review this pull-request.

morozov · 2018-05-14T19:40:44Z

@aashmelev thank you for the patch. From what I see, the non-PDO SQL Server driver supports the same DSN parameters, so it might make sense to move this logic to AbstractSQLServerDriver.

Besides that, it would be great if we could avoid mixing DBAL-specific parameters like dbname and platform-specific ones in the same namespace ($params). Doing so requires listing all known platform-specific parameters to distinct between them. E.g. sqlsrv_connect() takes $connectionOptions as a seaprate parameter. So could try doing we.

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

aashmelev · 2018-05-15T15:10:50Z

@morozov thanks for the answer! I changed the code.

The problem is that the pdo_sqlsrv driver receives the connection parameters only via the dsn and it does not receive the parameters via the driverOptions parameter like the sqlsrv driver.

It's not working for the pdo_sqlsrv driver (https://docs.microsoft.com/en-us/sql/connect/php/pdo-construct?view=sql-server-2017):

DriverManager::getConnection([
    'url' => 'mssql://user:password@server/dbname,
    'driverOptions' => [
        'APP' => 'APP_NAME',
    ]
]);

But it's working for the sqlsrv driver (https://docs.microsoft.com/en-us/sql/connect/php/sqlsrv-connect?view=sql-server-2017):

DriverManager::getConnection([
    'url' => 'sqlsrv://user:password@server/dbname,
    'driverOptions' => [
        'APP' => 'APP_NAME',
    ]
]);

I suggest adding the new parameter connectionOptions to pass the connection options (https://docs.microsoft.com/en-us/sql/connect/php/connection-options?view=sql-server-2017), and then passing them through the dsn into the PDO instance:

DriverManager::getConnection([
    'url' => 'mssql://user:password@server/dbname,
    'connectionOptions' => [
        'APP' => 'APP_NAME',
    ]
]);

morozov · 2018-05-15T18:38:53Z

Why not use the driverOptions key which is supported by the wrapper connection generically and is part of the Driver::connect() inteface? I mean, instead of introducing a new parameter, we start properly supporting the existing one.

aashmelev · 2018-05-15T19:45:34Z

I thought about it, but then we'll have to keep a list of connection options. And this is not very good, as you wrote about it above. Or, to distinguish them, is it enough that PDO constants have a numeric key?

morozov · 2018-05-15T21:32:34Z

To summarize the state we're in: there's already an inconsistency in how we structure connection and driver options in existing drivers:

In MysqliConnection, the $driverOptions argument accepts the options which mysqli::set_options() accepts and the flags key which may contain the flags which will be passed to mysqli::real_connect.
In all PDO-based drivers, $driverOptions are the options which will be passed to the PDO constructor.
In the SQL Server driver, $driverOptions are connection options like "APP", "MultipleActiveResultSets", etc.

In the PDO SQL Server driver, we need to combine both 2 and 3.

Or, to distinguish them, is it enough that PDO constants have a numeric key?

I think it's better than introducing a new option. This way, the PDO SQL Server configuration will look the same as the non-PDO SQL Server (will contain dsn-specific parameters) and will look like other PDO configurations (will contain PDO:: constants).

I think, in the future, every driver will need to have more than one $driverOptions parameter specific to their nature. E.g. PDO-based drivers will have explicit $pdoOptions, etc.

aashmelev · 2018-05-16T14:29:18Z

I made changes to the code.
It is important to note that the MultipleActiveResultSets parameter is no longer accepted via the DSN, but it is only accepted through the driver options. This breaks backward compatibility.

morozov · 2018-05-16T17:59:58Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

+            $dsn .= ';Database=' . $params['dbname'];
        }

-        if (isset($params['MultipleActiveResultSets'])) {


We can't have this breaking change in master. Thank you for pointing out. Please restore this and add a deprecation notice in UPGRADE.md for 2.8.0. After this gets in, a separate PR can be filed against develop which drops the support of this parameter.

morozov · 2018-05-16T18:00:22Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

     */
    public function connect(array $params, $username = null, $password = null, array $driverOptions = [])
    {
+        $splitedOptions = $this->splitOptions($driverOptions);


It should be $splitOptions (irregular verb).

morozov · 2018-05-16T18:02:36Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php


-        if (isset($params['MultipleActiveResultSets'])) {
-            $dsn .= '; MultipleActiveResultSets=' . ($params['MultipleActiveResultSets'] ? 'true' : 'false');
+        if (! empty($connectionOptions)) {


This condition seems redundant. If the method is invoked with an empty set, it will return an empty string which is acceptable.

morozov · 2018-05-16T18:06:17Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

+        }
+
+        return [
+            'driverOptions' => $driverOptions,


Given it's a private API, instead of introducing named keys, would it be cleaner to just use a tuple? Like:

function splitOptions() { return [$connectionOptions, $driverOptions]; } [$connectionOptions, $driverOptions] = splitOptions();

morozov · 2018-05-16T18:06:37Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

+     * @param string[] $connectionOptions
+     * @return string
+     */
+    private function getConnectionOptionsDsn(array $connectionOptions)


Please add a return type instead of the annotation.

morozov · 2018-05-16T18:47:58Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

+     * @param array $options
+     * @return array
+     */
+    private function splitOptions(array $options)


Please use a return type.

morozov

Please see a few more comments. Besides those, could you think of a way of testing the changes? Preferably, it should be a functional test showing that both PDO driver options and SQL Server connection options are effective and therefore the patch works as expected.

E.g., setting WSID to some value and then executing HOST_NAME() to see if it reached the server (see the article).

morozov · 2018-05-17T20:18:07Z

UPGRADE.md

@@ -1,302 +1,308 @@
-# Upgrade to 2.7


This shouldn't have changed.

morozov · 2018-05-17T20:21:38Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php


        if (isset($params['dbname'])) {
-            $dsn .= ';Database=' .  $params['dbname'];
+            $dsn .= ';Database=' . $params['dbname'];


It would make sense to replace it with $connectionOptions['Database'] = $params['dbname']. This way, the updated logic of building DSN will be covered by existing functional tests.

morozov · 2018-05-17T20:23:51Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

        }

        if (isset($params['MultipleActiveResultSets'])) {
            $dsn .= '; MultipleActiveResultSets=' . ($params['MultipleActiveResultSets'] ? 'true' : 'false');


Could you move this under $connectionOptions instead of having the DSN building logic in multiple places?

aashmelev · 2018-05-18T08:01:53Z

Okay, I'll do the functional tests as you wrote about it above.

aashmelev · 2018-05-18T14:35:07Z

l added tests

morozov

Please see some more comments and revert the irrelevant changes in UPGRADE.md.

morozov · 2018-05-22T22:34:34Z

tests/Doctrine/Tests/DBAL/Driver/PDOSqlsrv/DriverTest.php

+
+    public function testConnectionOptions()
+    {
+        $this->skipWhenNotUsingPdoSqlsrv();


I believe it's supposed to work with sqlsrv as well.

morozov · 2018-05-23T04:15:26Z

tests/Doctrine/Tests/DBAL/Driver/PDOSqlsrv/DriverTest.php

+     */
+    private function skipWhenNotUsingPdoSqlsrv()
+    {
+        if (isset($GLOBALS['db_type']) && $GLOBALS['db_type'] === 'pdo_sqlsrv') {


Please check connection platform or driver depending on what's being tested, not the global variables.

morozov · 2018-05-23T17:14:20Z

UPGRADE.md


 ``Doctrine\DBAL\Connection::TRANSACTION_*`` were moved into ``Doctrine\DBAL\TransactionIsolationLevel`` class without the ``TRANSACTION_`` prefix. 

-## DEPRECATION: direct usage of the PDO APIs in the DBAL API


This shouldn't have been removed. Also, the deprecation note on using MultipleActiveResultSets seems mistakenly gone.

morozov · 2018-05-23T17:16:37Z

tests/Doctrine/Tests/DBAL/Driver/AbstractSQLServerDriverTest.php

+        return static::createDriver();
+    }
+
+    protected function checkForSkippingTest(AbstractSQLServerDriver $driver) : void


Please remove this method. The abstract class shouldn't use any knowledge of its descendants.

morozov · 2018-05-23T17:17:41Z

tests/Doctrine/Tests/DBAL/Driver/AbstractSQLServerDriverTest.php

        );
    }
+
+    protected function getDriver() : AbstractSQLServerDriver


Please remove this method. You can use $this->_conn->getDriver() instead.

morozov · 2018-05-23T17:19:33Z

tests/Doctrine/Tests/DBAL/Driver/PDOSqlsrv/DriverTest.php

+
+    protected function checkForSkippingTest(AbstractSQLServerDriver $driver) : void
+    {
+        if (extension_loaded('pdo_sqlsrv') && $driver instanceof Driver) {


As long as this test uses a real connection, it should be part of the functional suite, where the availability of the extension will be checked in setUp().

morozov · 2018-05-23T17:20:55Z

tests/Doctrine/Tests/DBAL/Driver/AbstractSQLServerDriverTest.php

+        $driver = $this->getDriver();
+        $this->checkForSkippingTest($driver);
+        $connection = $this->getConnection($driver, ['APP' => 'APP_NAME']);
+        $result     = $connection->query('select APP_NAME() as app')->fetch();


You can use fetchColumn() instead of fetch() and remove the "app" alias for simplicity.

Please use upper case for SQL keywords for better readability ('SELECT').

morozov · 2018-05-23T17:25:34Z

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php

+     * Separates a connection options from a driver options
+     *
+     * @param mixed[] $options
+     * @return mixed[][]


I know @Majkl578 won't like these mixed annotation. Taking into account that most existing tools will not recognize annotations like (int|string)[], would it be fair to replace them with int[]|string[] and int[][]|string[][] respectively?

Probably, better than mixed. 👍

aashmelev · 2018-05-23T20:07:32Z

Thanks for the comments!
I renamed tests/Doctrine/Tests/DBAL/Functional/Driver/PDOSqlsrv/Driver.php because it should be called DriverTest.php

aashmelev · 2018-06-13T13:27:33Z

Perhaps it would be better if I make a separate PR to rename the file?

morozov · 2018-06-14T00:12:08Z

@aashmelev, no need to make another PR for the rename. Let's keep everything in one place. If you want me to help you rename the file, rebase the PR or squash the commits, let me know.

aashmelev · 2018-06-14T11:18:19Z

@morozov, I squashed the commits.

morozov

Thanks, @aashmelev!

@morozov

Release v2.8.0 [![Build Status](https://travis-ci.org/doctrine/dbal.svg?branch=v2.8.0)](https://travis-ci.org/doctrine/dbal) This is a minor release of Doctrine DBAL that aggregates over 30 fixes and improvements developed over the last 3 months. This release includes all changes of the 2.7.x series, as well as feature additions and improvements that couldn’t land in patch releases. **Backwards Compatibility Breaks** This doesn't contain any intentional Backwards Compatibility (BC) breaks. **Dependency Changes** * The dependency on [doctrine/common](https://github.com/doctrine/common) is removed. DBAL now depends on [doctrine/cache](https://github.com/doctrine/cache) and [doctrine/event-manager](https://github.com/doctrine/event-manager) instead. Please see details in the [UPGRADE.md](UPGRADE.md) documentation. **Deprecations** * The usage of binary fields whose length exceeds the maximum field size on a given platform is deprecated. Please use binary fields of a size which fits all target platforms, or use blob explicitly instead. * The usage of DB-generated UUIDs is deprecated. Their format is inconsistent across supported platforms and therefore the feature is not portable. Use a PHP library (e.g. [ramsey/uuid](https://packagist.org/packages/ramsey/uuid)) to generate UUIDs on the application side. **New features** * Initial support of MySQL 8. * Initial support of PostgreSQL 11. * Ability to evaluate arbitrary SQL expressions via `AbstractPlatform::getDummySelectSQL()`. **Improvements and Fixes** * Improved support of binary fields on Oracle and IBM DB2. * Improved SQL Server configuration capabilities via both `sqlsrv` and `pdo_sqlsrv`. * Improved handling of `AUTOINCREMENT`ed primary keys in SQLite. * Integration tests are run against IBM DB2 on Travis CI. * Code coverage is collected for the Oracle platform on continuousphp. Total issues resolved: **33** **Deprecations:** - [3187: Deprecate usage of binary fields whose length exceeds maximum](doctrine#3187) thanks to @morozov - [3188: Deprecated usage of binary fields whose length exceeds the platform maximum](doctrine#3188) thanks to @morozov - [3192: Added more information to the deprecation notice](doctrine#3192) thanks to @morozov - [3212: Deprecated usage of DB-generated UUIDs](doctrine#3212) thanks to @morozov **New Features:** **Bug Fixes:** - [3149: Introduced binary binding type to support binary parameters on Oracle](doctrine#3149) thanks to @morozov - [3178: Fix incorrect exception thrown from SQLAnywhere16Platform](doctrine#3178) thanks to @Majkl578 - [3044: Functional test for allowing dynamic intervals in date sub/add](doctrine#3044) thanks to @AshleyDawson - [3049: Test failures caused by invalid database connection result in fatal error](doctrine#3049) thanks to @Majkl578 **Improvements:** - [3033: Added support for available DSN parameters for the PDOSqlsrv driver](doctrine#3033) thanks to @aashmelev - [3128: Add MySQL 8 reserved keywords](doctrine#3128) thanks to @mlocati - [3143: initialize sql array into platform files](doctrine#3143) thanks to @AlessandroMinoccheri - [3173: Fix composer branch aliases](doctrine#3173) thanks to @Majkl578 - [3157: When building a limit query, zero offset without a limit should be ignored](doctrine#3157) thanks to @morozov - [3109: Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL()](doctrine#3109) thanks to @morozov - [3141: allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests)](doctrine#3141) thanks to @TimoBakx - [3180: Import simplified version of Common\Util\Debug for var dumping purposes](doctrine#3180) thanks to @Majkl578 **Documentation Improvements:** - [3117: Added badges for the develop branch in README](doctrine#3117) thanks to @morozov - [3125: Upgrading docs](doctrine#3125) thanks to @jwage - [3144: added improvement type into pull request template](doctrine#3144) thanks to @AlessandroMinoccheri **Code Quality Improvements:** - [3025: Added PHPStan, apply changes for level 3](doctrine#3025) thanks to @Majkl578 - [3200: Php Inspections (EA Ultimate): minor code tweaks](doctrine#3200) thanks to @kalessil - [3204: Fix typo in AbstractPlatform](doctrine#3204) thanks to @Majkl578 - [3205: Ignore OCI-* classes in static analysis (no stubs)](doctrine#3205) thanks to @Majkl578 **Continuous Integration Improvements:** - [3102: Use newer PHPUnit to prevent crashes on failures](doctrine#3102) thanks to @Majkl578 - [3112: Removed hard-coded configuration filenames from the test runner](doctrine#3112) thanks to @morozov - [3133: Travis DB2](doctrine#3133) thanks to @Majkl578, @morozov - [3135: AppVeyor tweaks, retry coverage upload on failure](doctrine#3135) thanks to @Majkl578 - [3137: Workaround for the inability to use a post-PHPUnit script on ContinuousPHP](doctrine#3137) thanks to @morozov - [3151: MSSQL DLL 5.2.0 has been released.](doctrine#3151) thanks to @photodude - [3160: Test against Postgres 11](doctrine#3160) thanks to @Majkl578 **Dependencies** - [3193: DBAL 2.8 needs Common 2.9](doctrine#3193) thanks to @Majkl578 - [3176: Eliminate dependency on doctrine/common](doctrine#3176) thanks to @Majkl578 - [3181: Remove dependency on doctrine/common](doctrine#3181) thanks to @Majkl578 # gpg: Signature made Fri Jul 13 06:02:10 2018 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found # Conflicts: # .gitignore # README.md # lib/Doctrine/DBAL/Version.php # tests/appveyor/mssql.sql2008r2sp2.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2012sp1.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.pdo_sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.sqlsrv.appveyor.xml # tests/travis/mariadb.mysqli.travis.xml

aashmelev force-pushed the aashmelev-feature-dsn branch 4 times, most recently from 37dd3b0 to 5e47017 Compare March 1, 2018 12:22

morozov reviewed May 15, 2018

View reviewed changes

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Driver.php Outdated Show resolved Hide resolved

aashmelev force-pushed the aashmelev-feature-dsn branch from 5d569e4 to f6aa45b Compare May 15, 2018 14:10

aashmelev force-pushed the aashmelev-feature-dsn branch from 0bc8d03 to fe88026 Compare May 16, 2018 13:34

morozov reviewed May 16, 2018

View reviewed changes

aashmelev force-pushed the aashmelev-feature-dsn branch 2 times, most recently from c215da3 to 149f8dd Compare May 17, 2018 14:46

morozov reviewed May 17, 2018

View reviewed changes

aashmelev force-pushed the aashmelev-feature-dsn branch 8 times, most recently from 6de1410 to 8a58f4a Compare May 22, 2018 20:14

morozov reviewed May 23, 2018

View reviewed changes

aashmelev force-pushed the aashmelev-feature-dsn branch from 94b31c9 to 5466d01 Compare May 23, 2018 14:17

aashmelev force-pushed the aashmelev-feature-dsn branch 2 times, most recently from 435f825 to ef9c088 Compare May 23, 2018 15:13

morozov reviewed May 23, 2018

View reviewed changes

aashmelev force-pushed the aashmelev-feature-dsn branch 3 times, most recently from 5d70ffa to 81611eb Compare June 14, 2018 11:14

morozov force-pushed the aashmelev-feature-dsn branch from 81611eb to 49a4a27 Compare June 14, 2018 15:36

Added support for connection options for the PDOSqlsrv driver

931d9a5

morozov force-pushed the aashmelev-feature-dsn branch from 49a4a27 to 931d9a5 Compare June 14, 2018 16:02

morozov approved these changes Jun 14, 2018

View reviewed changes

morozov merged commit 62fe3fe into doctrine:master Jun 14, 2018

Majkl578 added this to the 2.8.0 milestone Jun 14, 2018

Majkl578 assigned morozov Jun 14, 2018

Majkl578 added pdo_sqlsrv PDO Drivers Connections Configuration labels Jun 14, 2018

morozov mentioned this pull request Jan 10, 2019

pdo_mysql with persistent connection #2315

Closed

morozov mentioned this pull request Jul 25, 2021

Support APP name for pdo_sqlsrv connections #3895

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022


		``Doctrine\DBAL\Connection::TRANSACTION_*`` were moved into ``Doctrine\DBAL\TransactionIsolationLevel`` class without the ``TRANSACTION_`` prefix.

		## DEPRECATION: direct usage of the PDO APIs in the DBAL API

Uh oh!

Conversation

aashmelev commented Feb 28, 2018

Uh oh!

aashmelev commented May 14, 2018

Uh oh!

morozov commented May 14, 2018

Uh oh!

Uh oh!

aashmelev commented May 15, 2018 • edited by morozov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morozov commented May 15, 2018

Uh oh!

aashmelev commented May 15, 2018

Uh oh!

morozov commented May 15, 2018

Uh oh!

aashmelev commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aashmelev commented May 18, 2018

Uh oh!

aashmelev commented May 18, 2018

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aashmelev commented May 23, 2018

Uh oh!

aashmelev commented Jun 13, 2018

Uh oh!

morozov commented Jun 14, 2018

Uh oh!

aashmelev commented Jun 14, 2018

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

aashmelev commented May 15, 2018 •

edited by morozov

Loading