Incorrect Handling of LastInsertId in SQLSrvConnection

[bertoverflow]

Bug Report

Q	A
BC Break	yes
Version	2.9.0

Summary

The behaviour of lastInsertId() might be incorrect since bugifx cadd79c. At least it has significantly changed.

Current behaviour

When an Doctrine\DBAL\Driver\SQLSrv\SQLSrvStatement is executed and contains an INSERT-Statement the following code is automatically added to the statement: ";SELECT SCOPE_IDENTITY() AS LastInsertId;"

Before commit cadd79c the above lastInsertId (and thus SCOPE_IDENTIY()) was returned by the function lastInsertId() in class Doctrine\DBAL\Driver\SQLSrv\SQLSrvConnection when no name-parameter is given.

Since the change introduced by the bugfix cadd79c "SELECT @@IDENTITY" is returned. The lastInsertId-related code in the SQLSrvStatement and the class Doctrine\DBAL\Driver\SQLSrv\LastInsertId is no longer actually used (basically dead code).

However, using @@IDENTITY is probably not what the user expects. According to the official Microsoft-Documentation:
https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/retrieving-identity-or-autonumber-values

Function	Description
SCOPE_IDENTITY	Returns the last identity value within the current execution scope. SCOPE_IDENTITY is recommended for most scenarios.
@@IDENTITY	Contains the last identity value generated in any table in the current session. @@IDENTITY can be affected by triggers and may not return the identity value that you expect.
IDENT_CURRENT	Returns the last identity value generated for a specific table in any session and any scope.

I could unfortunately not find any documentation or hint about why this bugfix was introduced. It basically changed the behaviour of lastInsertId() for the SQLSrvConnection to return @@Identiy instead of SCOPE_IDENTITY. This could affect applications using that driver and relying on the old behaviour.

EDIT:
I just checked the commit again. It was already introduced in Version 2.7.0 during this pull request: #2617.
Still not sure why and I still think the behaviour is incorrect. If the @@IDENTITY-behaviour is actually wanted, at least the unused code should be removed.

[morozov]

@seeraeubert, cadd79c was made to satisfy existing tests before adding SQL Server to the build matrix. It indeed might be not the best implementation. Please create a pull request with a failing test which would describe the issue in code.

[morozov]

Closing due to the lack of details.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.