Fix for SQLServer connection string and SQLserver database empty test

[LukeTowers]

As mentioned in your comment in the related issue, is this applicable to all instances of SQL Server? If not, is there a way we can detect more specifically when these tweaks are required?

[chrisvidal]

I just did a check on the SQL server version used in my test server.

it is a Microsoft SQL Azure 12, which shares a large code from SQL Server code.

One of the sure thing to fix in the code is the double comma sent in the connection string.

[bennothommo]

@chrisvidal Hmm, that DSN string doesn't seem right - looking at the PHP docs, none of their examples include spaces in the DSN, and the Server and Database terms are definitely capitalised: https://www.php.net/manual/en/ref.pdo-sqlsrv.connection.php.

[chrisvidal]

that is the PHP code provided by MS as example for connection string:

<?php
// PHP Data Objects(PDO) Sample Code:
try {
    $conn = new PDO("sqlsrv:server = tcp:abcdefhost.database.windows.net,1433; Database = oc-demo", "abcdefg", "{your_password_here}");
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch (PDOException $e) {
    print("Error connecting to SQL Server.");
    die(print_r($e));
}

?>

[chrisvidal]

there are 3 differences between the connection string:

• the use of spaces
• the use tcp: before the hostname
• the double comma

[chrisvidal]

and with this connection string, the install is working. with the original connection string it is not installing.

[bennothommo]

@chrisvidal The thing is though, Laravel doesn't use a DSN string like that (see https://github.com/laravel/framework/blob/5.5/src/Illuminate/Database/Connectors/SqlServerConnector.php for how they create their DSN string), and ostensibly, October CMS is working without having a specialised DSN like the one you have suggested. So I can't help but feel that something else is in play here.

[bennothommo]

Do you have to use tcp: in your October CMS config?

[chrisvidal]

I was thinking of that indeed.

the install connection string does not work without being updated. when OC is install, the Laravel DSN string seems to be working fine, I didn't see any issue while browsing OC backend and install a theme and few plugins.

hence my conclusion is that there is an issue only with the wizard installer connection string.

in MS Azure SQL doc, there is an example of connection string

https://docs.microsoft.com/en-us/sql/connect/php/example-application-pdo-sqlsrv-driver?view=azuresqldb-current

$conn = new PDO( "sqlsrv:server=$serverName ; Database=AdventureWorks", "", "");  

I believe the spaces are important. the tcp: might be specific to my Azure install, I am not sure.

I am waiting for my client to provide me with a test environment, I will then have another opportunity to perform some test with this connection string.

Anything you would like me to test?

[chrisvidal]

Do you have to use tcp: in your October CMS config?

I copy/paste what the Azure env. provide me. I am not sure what makes a real difference between the 2 connection strings, as it worked out immediately when I copy/paste theirs.

In a new client env., I will test with and without this tcp: and see.

[bennothommo]

@chrisvidal Thanks for the further info. If you're up to it, could you perhaps try an echo and die (or Xdebug, if you've got it installed) before this line here on your October CMS install that works on SQL Server, and see what DSN is returned?

Maybe add this line above the highlighted line:

echo $this->getDsn($config); die();

This might give us a hint as to the differences between October/Laravel uses, and what the installer uses. The file should be vendor/laravel/framework/src/Illuminate/Database/Connectors/SqlServerConnector.php in your October CMS install.

[chrisvidal]

the echo is returning this sqlsrv:Server=sqldemohost.database.windows.net,1433;Database=oc-demo

so maybe the only issue with the installer connection string was the double comma

[bennothommo]

@chrisvidal I'd say that's it - would you mind adjusting your PR based on this and re-test, and let us know how that goes?

[chrisvidal]

do I update this PR or create a new one? if update, how do I do that? (never did before)

[chrisvidal]

ok, I created 2 PR

[bennothommo]

@chrisvidal sorry, I meant make the changes to this PR. I'll close the other two you have created.

[chrisvidal]

@bennothommo that's my point, hehe, how do I update that PR?

[bennothommo]

@chrisvidal Looks like you already did (https://github.com/octobercms/install/pull/106/commits/aa28f5e5086ac9adf1ead6bd5add9ca04ba0bdcf) - when you created the PR, you would've created a branch on your fork with your changes in it. Just make any additional changes to that branch and it will show up in this PR :)

[chrisvidal]

sorry I don't get it :( I can't update anything on this file (aa28f5e)

[chrisvidal]

do I update the original file again?

[LukeTowers]

@chrisvidal just update the original file on your fork of this repository: https://github.com/chrisvidal/install

[chrisvidal]

@LukeTowers mmm I kind of did that this morning already, and Ben deleted my new PR.

[bennothommo]

@chrisvidal Are all the changes here all that is required to fix the issue? - https://github.com/octobercms/install/pull/106/files

[chrisvidal]

@bennothommo yes exactly.

[bennothommo]

@chrisvidal No worries - that's the change list for this PR, so if you have tested this and it all works, I'm happy to merge this.

[chrisvidal]

Yes I have tested on 2 new install with Azure SQL.