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

Added error handling for LOB types as output parameters #584

Merged
merged 3 commits into from
Nov 3, 2017
Merged

Added error handling for LOB types as output parameters #584

merged 3 commits into from
Nov 3, 2017

Conversation

yitam
Copy link
Contributor

@yitam yitam commented Nov 3, 2017

#231


This change is Reviewable

@yitam yitam requested a review from david-puglielli November 3, 2017 17:07
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 73.014% when pulling e5a3767 on yitam:textTypes into 9bbc673 on Microsoft:dev.

@@ -422,6 +422,12 @@ void core_sqlsrv_bind_param( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT param_

// if it's an output parameter and the user asks for a certain type, we have to convert the zval to that type so
// when the buffer is filled, the type is correct
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 you should amend this comment to explain the error check that you added below. "But first, check that we are not using a LOB type with an output parameter" or something like that.

$initData = "ShortString";
$callResult = $initData;

// Make sure not to specify the PHP type
Copy link
Contributor

Choose a reason for hiding this comment

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

Why must you not specify the php type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a copy of another test for issue #231. I can remove the comment if you like. It doesn't matter for this particular test

Copy link
Contributor

Choose a reason for hiding this comment

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

Then may as well remove it...

@codecov-io
Copy link

codecov-io commented Nov 3, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #584      +/-   ##
==========================================
- Coverage   74.86%   74.84%   -0.02%     
==========================================
  Files          50       50              
  Lines       14927    14933       +6     
==========================================
+ Hits        11175    11177       +2     
- Misses       3752     3756       +4
Impacted Files Coverage Δ
...php-7.1.11-src/ext/pdo_sqlsrv/shared/core_stmt.cpp 67.68% <0%> (-0.08%) ⬇️
...php-7.0.25-src/ext/pdo_sqlsrv/shared/core_stmt.cpp 66.11% <0%> (-0.08%) ⬇️
...x86/php-7.1.11-src/ext/sqlsrv/shared/core_stmt.cpp 80.67% <0%> (-0.07%) ⬇️
...x86/php-7.0.25-src/ext/sqlsrv/shared/core_stmt.cpp 78.8% <0%> (-0.07%) ⬇️
...x86/php-7.0.25-src/ext/sqlsrv/shared/core_sqlsrv.h 43.72% <0%> (ø) ⬆️
...php/x86/php-7.0.25-src/ext/pdo_sqlsrv/pdo_util.cpp 83.83% <0%> (ø) ⬆️
...php/x86/php-7.1.11-src/ext/pdo_sqlsrv/pdo_util.cpp 90.21% <0%> (ø) ⬆️
...x86/php-7.1.11-src/ext/sqlsrv/shared/core_sqlsrv.h 79.52% <0%> (ø) ⬆️
...rojects/php/x86/php-7.0.25-src/ext/sqlsrv/util.cpp 82.03% <0%> (ø) ⬆️
...php-7.1.11-src/ext/pdo_sqlsrv/shared/core_sqlsrv.h 76.56% <0%> (ø) ⬆️
... and 2 more

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 9bbc673...ef14400. Read the comment docs.

|| sql_type == SQL_WLONGVARCHAR || sql_type == SQL_LONGVARBINARY),
// If the user specifies a certain type for an output parameter, we have to convert the zval
// to that type so that when the buffer is filled, the type is correct. But first,
// should check that if a LOB type is specified.
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 you mean '...should check if a LOB type is specified.'

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 73.014% when pulling b44a5dd on yitam:textTypes into 9bbc673 on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0009%) to 73.014% when pulling ef14400 on yitam:textTypes into 9bbc673 on Microsoft:dev.

@yitam yitam merged commit 628e0c9 into microsoft:dev Nov 3, 2017
@yitam yitam deleted the textTypes branch November 3, 2017 23:42
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