-
Notifications
You must be signed in to change notification settings - Fork 822
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
Fixes for using dblib PDO driver. #6612
Conversation
These fixes allow *nix environments to connect to SQL Server using the dblib PDO driver and the silverstripe mssql module. - Only set MYSQL_ATTR_INIT_COMMAND when using the mysql driver, this constant isn't defined if the mysql pdo driver isn't installed - Supress warnings on getting the server version, attributes aren't supported by the dblib driver - Explicitly check for errors on sql exec, checking the return value isn't reliable for statements with no return value (e.g. USE database)
model/connect/PDOConnector.php
Outdated
@@ -227,7 +228,7 @@ public function exec($sql, $errorLevel = E_USER_ERROR) { | |||
$result = $this->pdoConnection->exec($sql); | |||
|
|||
// Check for errors | |||
if ($result !== false) { | |||
if (!$this->hasError($result)) { |
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.
The problem is that false
as a return value now reports hasError() = false, when before it was hasErrors() = true. Docs on this method:
PDO::exec() returns the number of rows that were modified or deleted by the SQL statement you issued. If no rows were affected, PDO::exec() returns 0.
I think the !== false
check was correct.
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.
You could also fix it by making hasError() return true for === false
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'm not sure what you mean by the false
return value here - this still returns the result set if the query was successful, or null
if there was an error.
The PDO::exec()
docs don't match with what I'm seeing - this is returning false
with a USE database
statement with the dblib driver, but the database select is successful.
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 reorganised the code a little here, so it's easier to read.
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.
Er, I mean, false is an error condition. the problem is that the existing hasError() does NOT treat false as an error condition, but rather an empty set.
Before
$result = false;
if ($result !== false) {
// will not be called
}
after
$result = false;
if (!$this->hasError($result)) {
// WILL be called, oops
}
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.
Doh, I pasted the wrong docs snippet.
Warning
This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.
Shouldn't we expect '0' to be non-error, and 'false' to be error?
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.
Hm, more digging confirms what you said. false
isn't always an error... I guess I got that understanding wrong I'm sorry!
However, could there yet be cases where false
is returned AND there is an error? Perhaps instead of assuming false
is automatically true or false, hasError
should call errorCode
on the connection object? That should meet both of our concerns shouldn't it? :D
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.
since exec() is designed for methods that don't return a result set, I suggest changing the hasError($result)
to hasError($this->pdoConnection)
. Do you think that would work?
Although this is something we can only do in 4, really anything we are finding we have to wrap in "is this a xxx kind of driver" should be logic that each connector provides. Each connector should provide the connection options, the DSN, and so on. |
@@ -178,7 +179,7 @@ public function connect($parameters, $selectDB = false) { | |||
} | |||
|
|||
public function getVersion() { | |||
return $this->pdoConnection->getAttribute(PDO::ATTR_SERVER_VERSION); | |||
return @$this->pdoConnection->getAttribute(PDO::ATTR_SERVER_VERSION); |
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.
Why do we need to suppress errors here?
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.
"Supress warnings on getting the server version, attributes aren't supported by the dblib driver"
model/connect/PDOConnector.php
Outdated
@@ -227,7 +228,7 @@ public function exec($sql, $errorLevel = E_USER_ERROR) { | |||
$result = $this->pdoConnection->exec($sql); | |||
|
|||
// Check for errors | |||
if ($result !== false) { | |||
if (!$this->hasError($result)) { |
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.
Er, I mean, false is an error condition. the problem is that the existing hasError() does NOT treat false as an error condition, but rather an empty set.
Before
$result = false;
if ($result !== false) {
// will not be called
}
after
$result = false;
if (!$this->hasError($result)) {
// WILL be called, oops
}
model/connect/PDOConnector.php
Outdated
@@ -227,7 +228,7 @@ public function exec($sql, $errorLevel = E_USER_ERROR) { | |||
$result = $this->pdoConnection->exec($sql); | |||
|
|||
// Check for errors | |||
if ($result !== false) { | |||
if (!$this->hasError($result)) { |
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.
Doh, I pasted the wrong docs snippet.
Warning
This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE. Please read the section on Booleans for more information. Use the === operator for testing the return value of this function.
Shouldn't we expect '0' to be non-error, and 'false' to be error?
model/connect/PDOConnector.php
Outdated
@@ -227,7 +228,7 @@ public function exec($sql, $errorLevel = E_USER_ERROR) { | |||
$result = $this->pdoConnection->exec($sql); | |||
|
|||
// Check for errors | |||
if ($result !== false) { | |||
if (!$this->hasError($result)) { |
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.
Hm, more digging confirms what you said. false
isn't always an error... I guess I got that understanding wrong I'm sorry!
However, could there yet be cases where false
is returned AND there is an error? Perhaps instead of assuming false
is automatically true or false, hasError
should call errorCode
on the connection object? That should meet both of our concerns shouldn't it? :D
model/connect/PDOConnector.php
Outdated
@@ -227,7 +228,7 @@ public function exec($sql, $errorLevel = E_USER_ERROR) { | |||
$result = $this->pdoConnection->exec($sql); | |||
|
|||
// Check for errors | |||
if ($result !== false) { | |||
if (!$this->hasError($result)) { |
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.
since exec() is designed for methods that don't return a result set, I suggest changing the hasError($result)
to hasError($this->pdoConnection)
. Do you think that would work?
I wrote some comments 6 days ago but forgot to submit review (so they were hidden). Sorry! |
I think we need to use |
Looks like you're right with checking the error on the connection, I'll fix that up. |
Yay a wild Andrew appears! |
Looks good; Let's see if travis wants to work today. |
@tractorcow are you happy with the latest incarnation of this? |
Yes. :) |
These fixes allow *nix environments to connect to SQL Server using
the dblib PDO driver and the silverstripe mssql module.
constant isn't defined if the mysql pdo driver isn't installed
supported by the dblib driver
value isn't reliable for statements with no return value (e.g.
USE database)