Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workaround for unixODBC bug returning error when getting numcol #344

Merged
merged 21 commits into from
Apr 4, 2017

Conversation

yukiwongky
Copy link
Contributor

@yukiwongky yukiwongky commented Mar 28, 2017

FIx for #336
unixODBC returns error when getting the numcol after pdo::exec on a query that returns SQL_NO_DATA
workaround by returning 0 so pdo::exec doesn't throw an exception

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 55.244% when pulling 7c4efbc on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 55.244% when pulling 2788965 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 55.244% when pulling 3f0d3b6 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 55.244% when pulling 7230e30 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 55.503% when pulling 20806e6 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 55.503% when pulling e90d67a on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 55.503% when pulling e90d67a on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@yukiwongky yukiwongky requested review from yitam and ulvii March 29, 2017 00:34
Copy link
Contributor

@yitam yitam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when stmt->current_results is NULL?

@yukiwongky
Copy link
Contributor Author

@yitam , it should not do anything when it is NULL. I should put

if ( r == -1 && stmt->executed && strcmp( reinterpret_cast<const char*>( error->sqlstate ), "HY010" ) == 0 )
  	 return 0;

inside the if block if ( stmt->current_results != NULL )
to prevent sigmentation fault at error->sqlstate.

Thanks you for your sharp eyes :)

// (HY010 error should not return if stmt->execute is true)
#ifndef _WIN32
sqlsrv_error_auto_ptr error;
if ( stmt->current_results != NULL ) {
Copy link
Contributor

@yitam yitam Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to make changes, but you can actually declare the variable error inside the IF block ;)
Or, consider checking for two conditions: if current_results != NULL && stmt->executed is true

--FILE--
<?php
require_once("pdo_tools.inc");

Copy link
Contributor

@yitam yitam Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you don't have to include "pdo_tools.inc", but since you have it, you might want to create a temporary table instead (no need to check for a table's existence then drop it later)

$stmt = $conn->prepare($sql);
$stmt->execute();
if ($conn->errorCode() == 00000)
echo "prepare OK\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you need quotes around 00000?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 55.503% when pulling b9a2c09 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 55.468% when pulling a5f999e on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 55.272% when pulling a5f999e on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 55.468% when pulling 94d2f25 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

// but it should have succeeded with r=0 (SQL_SUCCESS) and no error
// instead of throwing an exception, return 0 if the r=-1, stament has been executed, and has a HY010 error
// (HY010 error should not return if stmt->execute is true)
#ifndef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you execute a large batch of SQL statements, for example 10000 inserts? According to this page, SQLNumResultCols() might return HY010 when executing function is still executing.

echo "prepare with args OK\n";

//test direct exec
$stmt = $conn->exec($sql);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test would be more accurate, if we check the actual return value of $conn->exec($sql); too.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 55.441% when pulling d87c03f on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 55.441% when pulling 9d033e7 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 51.983% when pulling 20c4fe6 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.3%) to 52.141% when pulling 8f67221 on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 55.595% when pulling ff3a23f on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 55.613% when pulling df3e23a on v-kaywon:error_numcol into 6809f0f on Microsoft:dev.

@yitam yitam merged commit 52e0599 into microsoft:dev Apr 4, 2017
@yukiwongky yukiwongky deleted the error_numcol branch June 30, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants