-
Notifications
You must be signed in to change notification settings - Fork 893
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
Add TrustServerCertificate to SqlServerAdapter #2094
Conversation
It should be enabled through configuration. For this, it needs some new configuration key like |
6d018c8
to
d0b534e
Compare
Hi, added it as a configuration option. Could you look at it again? @garas |
@@ -77,6 +77,11 @@ public function connect() | |||
} | |||
$dsn .= ';database=' . $options['name'] . ';MultipleActiveResultSets=false'; | |||
|
|||
// option to turn on TrustServerCertificate | |||
if (isset($options['trust_server_certificate'])) { | |||
$dsn .= ';TrustServerCertificate=' . $options['trust_server_certificate']; |
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.
Should we handle converting the boolean into a string? Having users define a string true is a bit counter-intuitive.
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 wonder if there's a way to generalize this so that we don't need to add handlers for all possible additional allowed options for SQLServer. We use the prefix sqlsrv_attr_
for detecting PDO settings, perhaps we can do a similar prefix for DSN specific settings, or have a key that expects to point at an associative array of settings? This way, we don't need to have this same if statement setup for a number of different options, and in this case, I think having them be whatever the user inputs makes sense (e.g. a string) as that's what the settings page calls for.
I would probably lean towards the array option (dsn_options
?) as that seems more easily extendable to other adapters if we ever need it.
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.
Added dsn_options
public function testConnectionWithDsnOptions() | ||
{ | ||
$options = $this->adapter->getOptions(); | ||
$options['dsn_options'] = ['TrustServerCertificate' => 'true']; | ||
$this->adapter->setOptions($options); | ||
$this->assertInstanceOf('PDO', $this->adapter->getConnection()); | ||
} |
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.
This test isn't overly valuable as it doesn't really test that the dsn_options
were used in any way, but there's currently not a good way to do that. This method is also similar with how we test the effect of port
missing as well:
phinx/tests/Phinx/Db/Adapter/SqlServerAdapterTest.php
Lines 62 to 68 in 48d8e1f
public function testConnectionWithoutPort() | |
{ | |
$options = $this->adapter->getOptions(); | |
unset($options['port']); | |
$this->adapter->setOptions($options); | |
$this->assertInstanceOf('PDO', $this->adapter->getConnection()); | |
} |
so it's not like this is the only place we have this. I'd like to address this in a separate PR, so I'm fine with merging this for now as is.
Fixes #2093
This PR adds TrustServerCertificate=true to the connection string. This is needed for sqlserver 2017 and 2019. Tested that phinx works with sqlsrv 2019 after this fix.
@markstory