Sqlsrv fixes - cursor type fix, utf8 support, transaction support, varbinary update support #5851
Conversation
|
travis build failure, see https://travis-ci.org/zendframework/zf2/jobs/19294649 |
| if (!$this->resource) { | ||
| $this->connect(); | ||
| } | ||
| if (sqlsrv_begin_transaction($this->resource)===false) |
|
Thanks guys. I'll make those changes. As for the reference, it has been awhile but I believe it has to do with how the varbinary update need to happen. |
| @@ -117,7 +117,11 @@ public function delete($table = null) | |||
| */ | |||
| public function prepareStatementForSqlObject(PreparableSqlInterface $sqlObject, StatementInterface $statement = null) | |||
| { | |||
| $statement = ($statement) ?: $this->adapter->getDriver()->createStatement(); | |||
| $statementReceived = true; | |||
There was a problem hiding this comment.
These kind of SqlSrv specific things need to happen in the SqlServer Platform Decorators, they are located in Zend\Db\Sql\Platform.
|
By and large, this PR won't work as is. A couple of large scale issues:
That said, the charset and transaction support looks good. If we can find a resolution to the above issues, perhaps we can get it into the next mini point release. |
|
A method to set the prepare options and perhaps another to check/get them would be great. No argument on the select decorator being the place to set the cursor type but I'm not sure what the preferred path forward would be. It seems like either a one off method call (which doesn't seem clean) or a change to the StatementInterface or StatementContainerInterface would be needed. Also, it is worth noting that the utf8 character support cannot happen without fixing the cursor type issue. A foreach loop against a result set will actually cause a driver error that will crash the web server if the character set is utf8 and the default cursor type is used. |
|
What about $statement->setPrepareOptions() and $statement->setPrepareParams()? If the params aren't set default to the existing ? finding logic. Although I kinda like a couple of optional arguments in $statement->prepare, so maybe something like prepare function($sql = null, $params = null, $options = null)? It would also be nice to be able to test if options were set so a statement could be prepared explicitly with a usable cursor type like "buffered" but have the decorator override the default cursor "forward" with a select to avoid errors. |
|
@marcelto this PR needs a rebase |
|
A few things. This PR is going in the right direction, but not yet there ;) First, I'm not fond of introducing the $params to the StatementInterface::prepare(). Reason being is that in general, parameters don't need to be known at prepare time for all adapters. SQL Server seems to be the outlier. The PDO version of the SQL Server driver doesn't have this requirement (yeah, I know it could potentially be emulating prepare under the hood, but that's neither here nor there). That leaves us with $options. I think it's totally acceptable that the Also, what about complimenting this with what I suggested above |
| if (!$this->resource) { | ||
| $this->connect(); | ||
| } | ||
| if (sqlsrv_begin_transaction($this->resource)===false) { |
There was a problem hiding this comment.
Add spaces around operators (=== here).
|
Final comment: rebase off of current master to ensure we can merge this cleanly, and I think it should be ready. |
|
Ok, hope this is what you are looking for. On an unrelated note, apigility is awesome. |
Sqlsrv fixes - cursor type fix, utf8 support, transaction support, varbinary update support
|
|
||
| //set statement cursor type | ||
| if ($statementContainer instanceof Statement) { | ||
| $statementContainer->setPrepareOptions(array('Scrollable'=>\SQLSRV_CURSOR_STATIC)); |
There was a problem hiding this comment.
\SQLSRV_CURSOR_STATIC - is a constant? May be it should be without \ ?
can you create test for this?
Please merge this stuff, mssql support is buggy and very limited without it.