Reverted change handling bigint output parameters #744

david-puglielli · 2018-04-10T00:19:21Z

Reverted #567. Fixes the issue in #699.

This change is

coveralls · 2018-04-10T00:33:28Z

Coverage decreased (-0.08%) to 73.221% when pulling bbf951c on david-puglielli:Revert-bigint-change into dd64980 on Microsoft:dev.

yitam · 2018-04-10T22:43:03Z

test/functional/pdo_sqlsrv/pdo_bigint_outparam.phpt

@@ -26,7 +26,8 @@ insertRow($conn, $tbname, array("c1_bigint" => 922337203685479936));
 $outSql = "{CALL $spname (?)}";
 $bigintOut = 0;
 $stmt = $conn->prepare($outSql);
-$stmt->bindParam(1, $bigintOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE);
+// $stmt->bindParam(1, $bigintOut, PDO::PARAM_INT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE);
+$stmt->bindParam(1, $bigintOut, PDO::PARAM_STR, 2048);
 $stmt->execute();
 printf("Large bigint output:\n" );


please remove the commented lines in the test

yitam · 2018-04-10T22:44:57Z

test/functional/sqlsrv/sqlsrv_bigint_outparam.phpt

-$stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_OUT)));
+// $stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_OUT)));
+// $stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_OUT, null, SQLSRV_SQLTYPE_BIGINT))); <-- this works
+$stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_OUT, null, SQLSRV_SQLTYPE_VARCHAR(256)))); //<-- this also works
 sqlsrv_execute($stmt);


same for this one: remove the commented lines and unnecessary comments
also, you might want to do one as BIGINT and the other as VARCHAR (256 is unnecessarily large... you can try a smaller size for the VARCHAR)

yitam

Please review the pdo ae output tests too.

yitam · 2018-04-11T19:14:38Z

test/functional/sqlsrv/sqlsrv_bigint_outparam.phpt

 sqlsrv_execute($stmt);
 printf("Large bigint output:\n");
 var_dump($bigintOut);
 printf("\n");

 // Call stored procedure with SQLSRV_PARAM_INOUT
 $bigintOut = 0;
-$stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_INOUT)));
+$stmt = sqlsrv_prepare($conn, $outSql, array(array(&$bigintOut, SQLSRV_PARAM_INOUT, null, SQLSRV_SQLTYPE_VARCHAR(20))));


Having size = 20 might be a bit too small if it's a negative number (of course in this example it's fine)

yitam · 2018-04-11T19:15:19Z

test/functional/pdo_sqlsrv/pdo_bigint_outparam.phpt

@@ -35,7 +35,7 @@ printf("\n");
 // Call stored procedure with inout
 $bigintOut = 0;
 $stmt = $conn->prepare($outSql);
-$stmt->bindParam(1, $bigintOut, PDO::PARAM_INT | PDO::PARAM_INPUT_OUTPUT, PDO::SQLSRV_PARAM_OUT_DEFAULT_SIZE);
+$stmt->bindParam(1, $bigintOut, PDO::PARAM_STR | PDO::PARAM_INPUT_OUTPUT, 2048);
 $stmt->execute();


Perhaps not 2048 but a size that's a bit larger than 20?

codecov-io · 2018-04-11T19:56:54Z

Codecov Report

Merging #744 into dev will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev     #744      +/-   ##
==========================================
- Coverage   78.99%   78.89%   -0.11%     
==========================================
  Files          25       25              
  Lines        7190     7149      -41     
==========================================
- Hits         5680     5640      -40     
+ Misses       1510     1509       -1

Impacted Files	Coverage Δ
...x86/php-7.1.16-src/ext/sqlsrv/shared/core_stmt.cpp	`83.71% <0%> (-0.6%)`	⬇️
...php-7.1.16-src/ext/pdo_sqlsrv/shared/core_stmt.cpp	`64.99% <0%> (-0.36%)`	⬇️
...php-7.1.16-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h	`80.14% <0%> (-0.22%)`	⬇️
...x86/php-7.1.16-src/ext/sqlsrv/shared/core_sqlsrv.h	`83.46% <0%> (-0.2%)`	⬇️
.../php/x86/php-7.1.16-src/ext/pdo_sqlsrv/pdo_dbh.cpp	`92.25% <0%> (+0.2%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd64980...bbf951c. Read the comment docs.

david-puglielli · 2018-04-11T20:22:08Z

test/functional/pdo_sqlsrv/pdo_ae_output_param_binary_size.phpt

-                                printValues($errMsg, $det, $rand, $ord0, $ord1);
-                            }
+                            echo "Something went wrong. This should not have happened\n";
+                            printValues($errMsg, $det, $rand, $ord0, $ord1);


What happens if $pdoParamType is not PARAM_STR or PARAM_BOOL? Is this else branch not hit?

There are only five different PDO PARAM types, PARAM_NULL and PARAM_LOB will result in an exception, and now PARAM_INT will be skipped with non-AE. With AE, PARAM_INT will also result in an exception.

yitam

Unless you have more questions about the pdo tests?

david-puglielli · 2018-04-11T21:17:35Z

Nope, I'll merge when AppVeyor completes successfully.

Reverted change handling bigint output parameters

2960738

david-puglielli requested a review from yitam April 10, 2018 00:19

yitam reviewed Apr 10, 2018

View reviewed changes

david-puglielli added 2 commits April 10, 2018 19:45

Updated failing tests

8c681f2

Fixed binary test

99a11c1

yitam reviewed Apr 11, 2018

View reviewed changes

david-puglielli commented Apr 11, 2018

View reviewed changes

Minor fixes

bbf951c

yitam approved these changes Apr 11, 2018

View reviewed changes

david-puglielli merged commit 454bc27 into microsoft:dev Apr 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverted change handling bigint output parameters #744

Reverted change handling bigint output parameters #744

david-puglielli commented Apr 10, 2018 •

edited

Loading

coveralls commented Apr 10, 2018 •

edited

Loading

yitam Apr 10, 2018

yitam Apr 10, 2018

yitam left a comment

yitam Apr 11, 2018

yitam Apr 11, 2018

codecov-io commented Apr 11, 2018 •

edited

Loading

david-puglielli Apr 11, 2018

yitam Apr 11, 2018

david-puglielli Apr 11, 2018

yitam left a comment

david-puglielli commented Apr 11, 2018

Reverted change handling bigint output parameters #744

Reverted change handling bigint output parameters #744

Conversation

david-puglielli commented Apr 10, 2018 • edited Loading

coveralls commented Apr 10, 2018 • edited Loading

yitam Apr 10, 2018

Choose a reason for hiding this comment

yitam Apr 10, 2018

Choose a reason for hiding this comment

yitam left a comment

Choose a reason for hiding this comment

yitam Apr 11, 2018

Choose a reason for hiding this comment

yitam Apr 11, 2018

Choose a reason for hiding this comment

codecov-io commented Apr 11, 2018 • edited Loading

Codecov Report

david-puglielli Apr 11, 2018

Choose a reason for hiding this comment

yitam Apr 11, 2018

Choose a reason for hiding this comment

david-puglielli Apr 11, 2018

Choose a reason for hiding this comment

yitam left a comment

Choose a reason for hiding this comment

david-puglielli commented Apr 11, 2018

david-puglielli commented Apr 10, 2018 •

edited

Loading

coveralls commented Apr 10, 2018 •

edited

Loading

codecov-io commented Apr 11, 2018 •

edited

Loading