Add TrustServerCertificate to SqlServerAdapter#2094
Add TrustServerCertificate to SqlServerAdapter#2094MasterOdin merged 3 commits intocakephp:masterfrom
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 |
|
|
||
| // option to turn on TrustServerCertificate | ||
| if (isset($options['trust_server_certificate'])) { | ||
| $dsn .= ';TrustServerCertificate=' . $options['trust_server_certificate']; |
There was a problem hiding this comment.
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.
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.
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.
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
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