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

SqlServerConnection::transaction() should use PDO transactions with sqlsrv driver (see issue #1882) #1883

Closed
wants to merge 1 commit into from

Conversation

Corvillus
Copy link

The connection seems to do transactions properly if the generic PDO transaction implementation is called instead of the raw "begin tran / commit tran / rollback tran" commands get called on the sqlsrv driver.

@taylorotwell
Copy link
Member

OK. I know we changed it for a reason. So I'm assuming this change is going to break other people?

@Corvillus
Copy link
Author

Yeah, issue #534. Essentially sqlsrv (the native Windows driver) supports PDO transactions. dblib (the Linux FreeTDS driver) does not. So explicitly checking for sqlsrv as the driver name and calling the generic Connection::transaction() (PDO implementation) shouldn't break it for the dblib users that still need the raw transaction commands, but it would probably be good for a *nix user to test first before merging.

@WMeldon
Copy link
Contributor

WMeldon commented Jul 29, 2013

I can run a test on this if no one else is running the dblib drivers.

@Corvillus
Copy link
Author

Sure, that would be great. I keep meaning to test on my Mac, but keep forgetting about this (until the error reappears after a composer update that is). #2143 seems to be a different fix to the same issue but it fails integration tests.

@WMeldon
Copy link
Contributor

WMeldon commented Aug 26, 2013

Whoops, didn't read this carefully enough. I'm using dblib so I won't be able to test this outside of the initial if statement.

Haven't got any Windows servers to test this on.

@taylorotwell
Copy link
Member

Done on 4.0 branch.

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.

3 participants