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

Fix returning the proper last inserted id in MSSQL #3523

Conversation

sarven
Copy link

@sarven sarven commented Apr 18, 2019

Q A
Type bug
BC Break yes
Fixed issues #3516

Summary

I discovered a problem when the table in MSSQL has a trigger with insert statements. Id returned by method lastInsertId is invalid. It isn't id from the table in where we were inserting data, but from the table that trigger was inserting data.

The problem concerns sqlsrv and pdo_sqlsrv extensions. The fix for sqlsrv was quite simple, I only added a few lines of code.

But the fix for pdo_sqlsrv required to rebuild the implementation of Connection and Statement. I made it similar to the sqlsrv implementation.

@sarven sarven force-pushed the 3516-invalid-the-last-inserted-id-when-the-table-has-a-trigger-with-an-insert-stmt-in-mssql branch from a10b812 to 2a08876 Compare April 19, 2019 16:13
@sarven sarven changed the base branch from master to develop May 18, 2019 10:55
@sarven sarven force-pushed the 3516-invalid-the-last-inserted-id-when-the-table-has-a-trigger-with-an-insert-stmt-in-mssql branch from 492f874 to 9bd8e85 Compare May 18, 2019 13:05
@sarven sarven force-pushed the 3516-invalid-the-last-inserted-id-when-the-table-has-a-trigger-with-an-insert-stmt-in-mssql branch from 456e5a8 to 79b93df Compare May 24, 2019 17:29
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

I'd need @deeky666 to check this patch, since he initially authored #2648 (which was closed due to its high complexity/risk for little gain).

I'd say that also here the same applies: if a prepared statement is causing side-effects through stored procedures and triggers, I'd rather call the schema "foobar'd" and "needs to be fixed there" rather than trying to fix issues on this end.

lib/Doctrine/DBAL/Driver/PDOSqlsrv/Connection.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Driver/PDOSqlsrv/Connection.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Driver/PDOSqlsrv/LastInsertId.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Driver/PDOSqlsrv/LastInsertId.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Driver/PDOSqlsrv/LastInsertId.php Outdated Show resolved Hide resolved
@morozov morozov force-pushed the develop branch 3 times, most recently from 10890d6 to 3836750 Compare June 27, 2019 06:29
@morozov morozov closed this Nov 3, 2019
@morozov morozov reopened this Nov 4, 2019
@morozov morozov changed the base branch from develop to master November 4, 2019 14:38
@warslett
Copy link

hey @sarven @Ocramius any update on this PR? looks like the branches might need clearing up (presuming this PR doesn't include 141 commits).

Base automatically changed from master to 4.0.x January 22, 2021 07:44
@morozov morozov closed this Jul 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants