SQLAnywhere corrections for DSN generation and persistent connections #2405
Conversation
…tion ', if $persistent=false, resource_type == 'SQLAnywhere connection'
…s case "host=ip1:port,ip2:port" servername and dbname are optional but can also be used without host parameter
|
@andipohl the issue is valid but you are mixing in |
|
Also missing a functional test. |
| @@ -50,7 +50,7 @@ public function __construct($dsn, $persistent = false) | |||
| { | |||
| $this->connection = $persistent ? @sasql_pconnect($dsn) : @sasql_connect($dsn); | |||
|
|
|||
| if ( ! is_resource($this->connection) || get_resource_type($this->connection) !== 'SQLAnywhere connection') { | |||
| if ( ! is_resource($this->connection) || !preg_match('/^SQLAnywhere.*connection.*/',get_resource_type($this->connection)) ) { | |||
There was a problem hiding this comment.
You should change this as follows:
$allowedResourceTypes = array(
'SQLAnywhere connection',
'SQLAnywhere persistent connection',
);
$resourceType = get_resource_type($this->connection);
if ( ! is_resource($this->connection) || in_array($resourceType, $allowedResourceTypes, true)) {
// ...
}…. In this case "host=ip1:port,ip2:port" servername and dbname are optional but can also be used without host parameter" This reverts commit 0698cba.
* removed "charset" patch * implemeted resource_type compare with in_array instead of regex * implemented functional tests for connection and statement.
| if ( ! is_resource($this->connection) || get_resource_type($this->connection) !== 'SQLAnywhere connection') { | ||
| $resourceType = get_resource_type($this->connection); | ||
|
|
||
| if ( ! is_resource($this->connection) || !in_array($resourceType, $allowedResourceTypes, true)) { |
There was a problem hiding this comment.
TBH I think we might safely remove the resource type check at all. We should just leave the is_resource() check, everything else is driver specific implementation detail.
removed unnecessary setup/teardown-methods
I am also of the opinion that the resource type checks are unhappy. We depend on driver internals.
|
I'm going to keep far away from persistent connections myself, therefore not merging into 2.5.5 myself. @deeky666 feel free to re-assign a milestone when you checked this in more depth. |
|
What is the reason for not taking the patch in last/next release? |
|
@deeky666 is our sqlanywhere guy and dbal maintainer but he is a bit busy these days. On that node persistent connections sounds very scary in my ears. Do there exist something like mysqlproxy and pgbouncer for sqlanywhere? If yes i would recommend to take a look at those instead. |
|
@andipohl sorry for the delay but thank you! |
|
Seems like this one introduced build failure on Oracle. |
|
Dammit, we need to check for the driver in the setup. I will send a PR. Thanks. |
I would use persistent connections. But the check with get_resource_type failed because of different strings I get for persistent and non-persistent connections.
buildDSN could not be used for mirrored database installations. Here the host-parameter has to be a list of ip-adresses, comma-separated, e.g. host=ip1:port,ip2:port
other possibility is to leave host blank and use
Please merge this changes.