Re: [fw-db] Comments on Zend_Db_Adapter_Sqlsrv

Hi,

I added getServerVersion for all adapters
(http://framework.zend.com/issues/browse/ZF-5099).
Initially it was for Oracle specific SQL statement (difference between
8i and more recent versions:
http://framework.zend.com/issues/browse/ZF-5082).
Most of drivers offer a function like get_server_version or
get_client_version but they don't return this version in PHP style to be
able to use version_compare().
With pdo_mssql, I used the return of: "SELECT
SERVERPROPERTY('productversion')"

Mickael.

Juozas a écrit :
> Hi,
>
> initially I used Oracle adapter/statement code as a framework to
> create sql functionality, so 1.) might be true. I/Ralph will fix it.
>
> Checking options in 2.) might be possible, but not necessary needed :)
> I mean current approach will work with future sqlsrv_* improvements
> also, because we relay on that driver to check if options exist.
>
> Transactions functions throws exceptions because it seemed for me that
> errors in sql transactions are more dangerous than in other drivers
> (maybe). Returning false/true is different from abstract adapter so
> probably not good idea to use and user code shouldn't handle return in
> sqlsrv-specific way. That was my idea, but I might be wrong :)
>
> Limit stuff is wid in sql server it self :) I have seen ways to do it
> in 2008-only (which are more elegant), but since this driver focuses
> on general case I chose to use good-old approach :) For a possible bug
> maybe you can create full test-case? That would be really helpful.
>
> Lastinsertid was made with intention (again) to not to break abstract
> adapter. It works in all general cases and will only break in some
> "crazy" cases :) However, adapter can't decide tha and developer
> should know, that in complex queries it might break. As I said - it
> works in all normal cases and is tested with unit-tests :)
>
> Server version is a little bit tricky. We use
> http://msdn.microsoft.com/en-us/library/cc296165%28SQL.90%29.aspx to
> get version and we don't know actual server version - only client info
> is available. Are there sql queries to get actual server version and
> do we want to use it? It seemed for me that getServerVersion is used
> to check for driver/server version specific cases so driver version in
> this case is not a full bug :)
>
> Statement first point - can't understand :)
>
> fetchArray is again work-around for abstract cases. Adapter wants to
> have ability to make columns lowercase,uppercase,default hence we need
> to loop through all keys and do it. Although keys are generated once
> per statement and this operation is not expensive so I don't think
> that is a problem. Correct me if I'm wrong :)
>
> Very good point about error messages ;) I'm definitely going to look
> at it and will probably change exception to store all errors and give
> access to first one/all/etc. It won't break anything just give
> addition options.
>
> This driver was created with an intention to have (as much as
> possible) transparent adapter - only a few unit-tests from abstract
> suite are marked as skipped (bind by name etc.) and all others work
> just fine.
>
> Thanks a lot for these comments!
>
> Juozas
>
> On Wed, Jul 29, 2009 at 8:09 PM, Andrew Ballard <aballard@gmail.com
> <mailto:aballard@gmail.com>> wrote:
>
>
> I'm glad to see an adapter for the SQL Server Driver for PHP
> (Sqlsrv) being
> added to Zend_Framework with the upcoming 1.9 release. Overall
> they look
> similar to what I have been using, so it will be good to see them
> become
> part of the core. After looking at RC1, I have a few observations:
>
>
> Zend_Db_Adapter_Sqlsrv
> 1) I see some references to Oracle and things like tnsnames.ora
> still in the
> comments.
>
> 2) In _connect(), I think it would be more efficient for the
> adapter to
> throw an exception on any driver options that are not supported by
> the DLL
> rather than passing them off to sqlsrv_connect() and letting it
> return an
> error.
>
> 3) I see that the transaction handlers (_beginTransaction, _commit
> and
> _rollback) now return void and raise exceptions on failure. I
> simply had
> them pass back the return value of the corresponding sqlsrv_
> function, but
> it appears that Zend_Db_Adapter_Abstract now returns a self
> reference to
> provide a fluent interface. I guess this implementation is
> consistent with
> the change in the abstract base class.
>
> 4) The implementation of limit() looks like it uses a pretty common
> technique I have seen in other algorithms, but I think it contains
> a slight
> flaw regarding the last "page" in the result set.
>
> For example if the total rows matching the WHERE clause is 55, the
> value of
> rows-per-page is 10, and selected page number is 6, the inner
> query would
> request the top 60 (offset + rows = 50 + 10) rows. Since there are
> only 55
> total rows, the inner query will return all 55 rows. The outer
> query then
> takes the bottom 10 of the set returned by the inner query. In
> this case,
> that would be rows 46-55 rather than just 51-55.
>
> In my class, I started with this approach, but I modified it to add an
> additional WHERE condition so that it would only grab the rows in
> the bottom
> 10 that were not also part of the top 50. Due to its complexity I
> left room
> for the adapter to select alternate algorithms [yet to be added]
> for queries
> against newer versions since there are now better ways to
> implement this in
> 2005 and 2008. (The trouble is, depending on the tables you may
> get better
> or worse performance by using either row_number() or by various
> approaches
> with CTEs, etc. On large tables, it can be the difference between
> queries
> that finish in seconds and queries that finish in hours.)
>
> 5) lastInsertId can be tricky because of scope, but it looks like what
> you've done here is better than what I used. :-)
>
>
> 6) getServerVersion is returning the driver version, not the
> database server
> version.
>
>
>
> Zend_Db_Statement_Sqlsrv
> 1) I like the addition of the class constants for the parameter
> directions.
> Perhaps the other constants should be added as well? A couple of them
> probably have to be implemented as static functions, rather than
> members of
> that enumeration array though. :-\
>
> 2) I'm curious why internally fetch() always fetches a row as two
> separate
> arrays (keys and values) and then uses the fetch-mode to determine
> whether
> to discard one and return the other, or merge them together (and
> optionally
> cast the combined array to an object). Is that faster than calling
> either
> sqlsrv_fetch_array with the appropriate 2nd parameter or
> sqlsrv_fetch_object?
>
>
> Error handling:
> I am glad to see the full array returned by sqlsrv_errors() being
> passed to
> the exception constructors. A single query could return multiple error
> messages, especially from stored procedures, and often the last error
> message returned is simply "Statement failed." I wish there was a
> good way
> to be able to access either the full array of messages and codes
> or specific
> items in the errors array rather than just getting the first/last code
> returned.
>
> I'm not sure either of these are good ideas, but what about either
> having
> getMessage() return each() on the message array or else providing
> getMessages() and getCodes() as additional methods on those classes?
>
> Thoughts?
>
> Andrew
> --
> View this message in context:
> http://www.nabble.com/Comments-on-Zend_Db_Adapter_Sqlsrv-tp24725644p24725644.html
> Sent from the Zend DB mailing list archive at Nabble.com.
>
>