Re: [fw-db] Comments on Zend_Db_Adapter_Sqlsrv

Hi,

as a contributor I'm responsible in answering these questions :)

I'm quite busy right now, but will do it in a few hours.

Juozas

On Wed, Jul 29, 2009 at 9:35 PM, Ralph Schindler <ralph.schindler@zend.com> wrote:

Andrew, thanks for the detailed input!

Zend_Db_Adapter_Sqlsrv
1) I see some references to Oracle and things like tnsnames.ora still in the
comments.

I will remove these pronto.  Sounds like I need to really read the comments that are in there instead of speed-reading them :)

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.

Well, the issue here is that we cant know what constants are valid as driver options (I guess we could make a list, but then the adapter would have to maintain that list).

The issue is that if we wanted to throw an exception, we'd have to supress any errors coming out of sqlsrv_connect() function.  Considering we dont fully know ALL of the different errors that could be thrown out of that function, its probably best to not supress them.

Ideally, if you were using Zend_Config, you would be able to have a section such that only the specific driver options for SQL Server extension are passed to sqlsrv_connect().

Does that make sense?

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.

Yeah, I see no real need to have a return value in this method since its a protected method.

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.

...

with CTEs, etc. On large tables, it can be the difference between queries
that finish in seconds and queries that finish in hours.)

I guess this is two part.  One is that if you think the behavior of limit() is wrong, lets try and get a test script going that will produce the bad behavior, that way i can unit test it, and also see if we have bad logic inside other adapters as well.

Secondly though, I dont know nearly enough about 2005 and 2008 differences to be able to say that one method is better than another with regards to who limit() works.  Perhaps you have a proposal here?

5) lastInsertId can be tricky because of scope, but it looks like what
you've done here is better than what I used.  :-)

Cool!

6) getServerVersion is returning the driver version, not the database server
version.

hmm, you might need to submit a bug for this. How would one get the 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.  :-\

Hmm, could you explain a little more here?

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?

I'd have to pass this onto the original contributor and ask why.

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?

I have yet to build an application with Sqlsrv, so I wouldnt know the pain points a developer might encounter with regards to errors and exceptions.  Perhaps this would be a good opportunity to speak with the contributor who contributed this adapter/driver?  Would you be interested in that?

Thoughts?

Andrew