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.

Sign up for GitHub

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

Jump to bottom

implement throwing exception when ATTR_PERSISTENT is in connection #324

Merged
merged 4 commits into from
Mar 20, 2017
Merged

implement throwing exception when ATTR_PERSISTENT is in connection #324

merged 4 commits into from
Mar 20, 2017

Conversation

yukiwongky
Copy link
Contributor

@yukiwongky yukiwongky commented Mar 14, 2017 •
edited
Loading

fix for #65
fix by throwing exception when ATTR_PERSISTENT is in connection option
added test

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 55.359% when pulling b701fab on v-kaywon:attrPersistent into 9bd42e4 on Microsoft:dev.

@yukiwongky yukiwongky requested a review from Hadis-Knj March 15, 2017 00:22
Hadis-Knj
Hadis-Knj reviewed Mar 17, 2017
View reviewed changes
test/pdo_sqlsrv/pdo_065_construct_prefetch.phpt Outdated
$dsn = "sqlsrv:Server = $serverName;Database = $database;";
try{
echo "Testing a connection with ATTR_PREFETCH before ERRMODE_EXCEPTION...\n";
// setting PDO::ATTR_PERSISTENT in PDO constructor returns an exception
Copy link

Choose a reason for hiding this comment

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

comment is ATTR_PERSISTENT

Hadis-Knj
Hadis-Knj reviewed Mar 17, 2017
View reviewed changes
Copy link

@Hadis-Knj Hadis-Knj left a comment

Choose a reason for hiding this comment

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

since this is a fix for heap corruption in PDO, please run valgrind/Dr.Memory to verify that memory hasn't leaked after throwing the exception, in the presence of try catch block or in the absence of it

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 55.372% when pulling c9492ae on v-kaywon:attrPersistent into 9bd42e4 on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 55.372% when pulling 2019880 on v-kaywon:attrPersistent into 9bd42e4 on Microsoft:dev.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 55.372% when pulling 2019880 on v-kaywon:attrPersistent into 9bd42e4 on Microsoft:dev.

@Hadis-Knj Hadis-Knj merged commit 1a11767 into microsoft:dev Mar 20, 2017
@yukiwongky yukiwongky deleted the attrPersistent branch March 20, 2017 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hadis-Knj Hadis-Knj Hadis-Knj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yukiwongky @coveralls @Hadis-Knj