Re: FW: [PHP-DEV] OCIError patch

DOH! I guess I made a few presumptions: 1) The user would never pass a NULL sql: <? $conn = OCILogon ("bla", "bla", "bla"); $sql = NULL; $stmt = OCIParse ($conn, $sql); OCIExecute ($stmt, OCI_DEFAULT); $error = OCIError ($stmt); <-- seg fault here print_r ($error); ?> This condition would cause OCIAttrGet to set sqltext to NULL, leading me to the presumtion... 2) That add_assoc_string() would be able to handle a NULL pointer. It does not. So I made sure sqltext is not NULL when I send it to add_assoc_string. I've decided to send an empty string as opposed to simply omitting sqltext. This way the array returned always has the same number of elements. 3) A call to OCIError with a $conn arg or no arg would not affect anything. Well, it did. Seg fault. I've moved the calls to OCIAttrGet into the if (statement) { } block. I should have caught this from the very beginning. My bad. I'll conduct a few more tests. Anyone else want to help me test this? I've attached revised patches against the latest cvs and 4.1.2. Daniel Markus Fischer wrote: > Have you tested this patch also when there is no statement > but only a connection or any kind of global error? > > My concernc are what happens if sqltext remains a NULL > pointer? > >On Wed, Mar 13, 2002 at 11:32:03AM -0800, Daniel Ceregatti wrote : > >>This patch simply adds the original query text as the "sqltext" element >>and the error offset as the "offset" element. It was made against the >>latest CVS. Hopfully it'll suffice. If not, please let me know. >> >>Daniel >> >>Markus Fischer wrote: >> >>> Of course the offset is fine. But if I were you, I wouldn't >>> put the asterisk into the sql statement; just provide the >>> offset. _This_ is the greatest flexibility you can provide >>> because all the information is passed unmodified to the >>> developer. >>> >>> Serious environments need custom error handlers anyway. >>> Putting HTML inside or modifying error messages this way is >>> a bad thing [tm]. Just provide the raw components and >>> everyone else can decide how he presents the data to the end >>> user (that's the idea behind). >>> >>> And, for the patch, can you please make a unified diff >>> against altest CVS ? >>> >>> - Markus >>> >>>On Tue, Mar 12, 2002 at 03:19:27PM -0800, Walter A. Boring IV wrote : >>> >>>>I like the idea of having the sqltext in the return array, as well as >>>>the offset. This is a very usefull tool for debugging oracle sql >>>>queries. I believe its something that OCIError has been lacking. >>>>Anyone that uses Oracle along with sqlplus, you get the * under the >>>>broken portion of the query. I find it very usefull. >>>>If there is a general rule of not putting html inside return values >>>>for php internal functions, thats fine. But I still want an easy way of >>>>showing this type of info, without having to create my own wrapper >>>>function call to OCIError, just so I can highlight the broken portion of >>>>the query. >>>> >>>>my $0.02 >>>>Walt >>>> >>>>On Tue, 2002-03-12 at 15:08, Daniel Ceregatti wrote: >>>> >>>>>Yes. An example of what would be in the array returned by OCIError would >>>>>be: >>>>> >>>>>// Given the code below... >>>>> >>>>>$conn = OCILogon (bla bla bla); >>>>> >>>>>$sql = "select t.foo, t.bar from table t where t.id = 1"; >>>>> >>>>>$stmt = OCIParse ($conn, $sql); >>>>> >>>>>OCIExecute ($stmt, OCI_DEFAULT); >>>>> >>>>>$error = OCIError ($stmt); >>>>> >>>>>// What follows are the values of the elements of the array returned by >>>>>OCIError (presuming "bar" is an invalid column it the table): >>>>> >>>>>$error["code"] = 904 >>>>>$error["message"] = "ORA-00904: invalid column name" >>>>>$error["sqltext"] = "select t.foo, t.*bar from table t where t.id >>>>>= 1" >>>>>$error["offset"] = 16 >>>>> >>>>>The PHP user then has the option of using either the "sqltext" element >>>>> >>>>>directly, or using the "offset" element and the $sql variable to >>>>>create >>>>>any output they see fit. I think this allows for the greatest >>>>>felxibility, IMHO. >>>>> >>>>>Daniel >>>>> >>>>>Markus Fischer wrote: >>>>> >>>>>>On Tue, Mar 12, 2002 at 02:26:24PM -0800, Daniel Ceregatti wrote : >>>>>> >>>>>>>How about doing what sqlplus does and simply add an asterisk at that >>>>>>>point? I'm ok with rolling my own inside of the script by using the >>>>>>> >>>>>>>offset element. I simply think it'll do PHP users a service to have >>>>>>> >>>>>>>the "sqltext" in the array in _some_ form, even if only with >>>>>>>an >>>>>>>asterisk. >>>>>>> >>>>>> Asterisk at what point? Specified by offset? >>>>>> > >>diff -Naur php4-cvs/ext/oci8/oci8.c php4/ext/oci8/oci8.c >>--- php4-cvs/ext/oci8/oci8.c Wed Mar 13 11:14:18 2002 >>+++ php4/ext/oci8/oci8.c Wed Mar 13 11:24:40 2002 >>@@ -4218,10 +4218,12 @@ >> zval **arg; >> oci_statement *statement; >> oci_connection *connection; >>- text errbuf[512]; >>- sb4 errcode = 0; >>+ text errbuf[512]; >>+ sb4 errcode = 0; >> sword error = 0; >> dvoid *errh = NULL; >>+ ub2 errorofs = 0; >>+ text *sqltext = NULL; >> >> if (zend_get_parameters_ex(1, &arg) == SUCCESS) { >> statement = (oci_statement *) zend_fetch_resource(arg TSRMLS_CC, -1, NULL, NULL, 1, >> le_stmt); >>@@ -4258,10 +4260,28 @@ >> (ub4) sizeof(errbuf), >> (ub4) OCI_HTYPE_ERROR)); >> >>+ CALL_OCI_RETURN(statement->error, OCIAttrGet( >>+ (dvoid *)statement->pStmt, >>+ OCI_HTYPE_STMT, >>+ (text *) &sqltext, >>+ (ub4 *)0, >>+ OCI_ATTR_STATEMENT, >>+ statement->pError)); >>+ >>+ CALL_OCI_RETURN(statement->error, OCIAttrGet( >>+ (dvoid *)statement->pStmt, >>+ OCI_HTYPE_STMT, >>+ (ub2 *)&errorofs, >>+ (ub4 *)0, >>+ OCI_ATTR_PARSE_ERROR_OFFSET, >>+ statement->pError)); >>+ >> if (errcode) { >> array_init(return_value); >> add_assoc_long(return_value, "code", errcode); >>+ add_assoc_long(return_value, "offset", errorofs); >> add_assoc_string(return_value, "message", (char*) errbuf, 1); >>+ add_assoc_string(return_value, "sqltext", (char *) sqltext, 1); >> } else { >> RETURN_FALSE; >> } >> > >