opened
joshkel
username_0
The sqlsrv driver for PHP supports a [long list of options](https://docs.microsoft.com/en-us/sql/connect/php/connection-options?view=sql-server-2017), but relatively few are supported by CodeIgniter's sqlsrv driver.

Can the driver be extended to support the full range of options?

One approach would be to define a bunch of optional database parameters. However, since the SQL Server driver may add new options at any time, and since most of these new options would only have meaning for the SQL Server driver, adding an optional `dbdriver_params` setting makes sense to me. For example:

```php
$db['default'] = [
    'hostname' => 'sql-server',
    'username' => 'dbuser',
    'password' => 'secret',
    'database' => 'dbname',
    'dbdriver' => 'sqlsrv',
    'dbdriver_params' => [
        'LoginTimeout' => 10,
        'MultiSubnetFailover' => 'yes',
        'ApplicationIntent' => 'ReadOnly',
    ]
];
```

I can submit a PR if there's interest (and if this approach is acceptable).
Support more sqlsrv options
issue
created
narfbg
username_1
Some of these are integrated to match our own, universal settings and others (like `Scrollable`) we enforce, so adding something like `dbdriver_params` would create possible conflicts between two sets of configurations.
I suppose we could blacklist the ones that could conflict, but then there's an even bigger issue - doing it for sqlsrv means doing it for all drivers and all kinds of platform-specific settings, often altering behavior and making our Query Builder inconsistent, beating its entire purpose.

So that's a No by default, but if you have some particular use case or otherwise strong reasoning for including a particular feature, we could work on that.
comment
created
joshkel
username_0
Thanks for the quick reply.

So, thinking out loud, I think I'd argue that there are already a few ways to break encapsulation and/or configure conflicting settings: directly access the `conn_id` and use database APIs to put it in a state that CodeIgniter doesn't expect, or use PDO with a DSN that CodeIgniter doesn't parse. Since there's already some possibility of a developer making problems for themselves by doing something CodeIgniter doesn't expect, and since it would be potentially very hard for a web framework to fully support every possible database option, it seems valuable to me to offer a standard "escape hatch" of sorts to pass through arbitrary options, with the documented understanding that some options could cause problems.

But I've obviously a lot less familiar with the CodeIgniter database layer's design and tradeoffs than you are, so I could be wrong.

The particular SQL Server setting that we're missing is `MultiSubnetFailover`.

After opening this issue, I noticed that CI_DB_pdo_sqlsrv_driver does support several SQL Server options (as individual config settings, rather than via a generic catch-all `dbdriver_params`).  However, it doesn't support `MultiSubnetFailover`, and CI_DB_sqlsrv_driver doesn't support most SQL Server options.  Therefore, if you don't want to support generic `dbdriver_params`, would something like this work?

* Add `MultiSubnetFailover` (and other options as appropriate; I could review the SQL Server docs to see if anything else looks like it should be added) to the list of supported options within CI_DB_pdo_sqlsrv_driver.
* Combine and consolidate the list of supported options between CI_DB_pdo_sqlsrv_driver and CI_DB_sqlsrv_driver: instead of duplicating code, have a string array giving supported config settings, and update both classes to refer to that.  (Would it work to make this a static propery of CI_DB_sqlsrv_driver and have CI_DB_pdo_sqlsrv_driver access that?)
comment
created
narfbg
username_1
I guess the first thing can work, yes, but not the second one ... it's not a matter of just looking up whether an option exists or not. `sqlsrv_connect()` expects an array of options, while PDO_SQLSRV needs them written into the connection string; it's also possible that values need to be sent in different formats and/or even that the same option exists under different names.
comment
created
joshkel
username_0
Fair enough. I do think I'd make a different trade-off, but I really appreciate the explanation.

Regarding CI_DB_pdo_sqlsrv_driver versus CI_DB_sqlsrv_driver: It seems like it ought to work. sqlsrv_connect and PDO_SQLSRV are [documented](https://docs.microsoft.com/en-us/sql/connect/php/connection-options?view=sql-server-2017) as taking the same options. I could imagine data type handling for a DSN string versus an array being a problem - except that CI_DB_pdo_sqlsrv_driver currently simply [concatenates strings together](https://github.com/bcit-ci/CodeIgniter/blob/7480b4c89fadd542bd9f410c0d7657a67f169c1a/system/database/drivers/pdo/subdrivers/pdo_sqlsrv_driver.php#L102) to make its DSN, so any questions of data type handling do not appear to be a problem in practice.

And, if you don't want to worry about this right now, I do have a workaround - we're using the PDO driver with a DSN, and that lets us pass in whatever parameters we want. It just seemed odd to me that CodeIgniter has three different ways to connect to SQL Server (PDO with DSN, PDO with individual database config options, and sqlsrv), and each way of connecting has a different set of supported SQL Server options (totally flexible with DSN, somewhat complete with PDO with individual options, and minimal with sqlsrv).

Thanks.
comment
created
narfbg
username_1
Well, yes - that's what I meant. Serializing key/value pairs from array into a string isn't the same mechanism as simply passing an array of options.

Anyway, if you can provide me with an assorted list of options that you think should be added (and where), we'll review and eventually implement them. A PR would be even better.
comment
closed
narfbg
username_1
issue