Skip to content

Conversation

@morozov
Copy link
Contributor

@morozov morozov commented Mar 31, 2021

Closes #1244.

There are existing functional tests that might cover this case, e.g.:

// return the last sequence number is sequence name is provided
$lastSeq = $conn->lastInsertId($sequenceName);

But the issue is only reproducible with a certain server configuration which cannot be specified at the connection level. Please let me know if you have a suggestion for a test.

@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #1245 (b10b6ac) into dev (f7e24bd) will not change coverage.
The diff coverage is n/a.

❗ Current head b10b6ac differs from pull request most recent head 5ec99ba. Consider uploading reports for the commit 5ec99ba to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #1245   +/-   ##
=======================================
  Coverage   82.86%   82.86%           
=======================================
  Files          24       24           
  Lines        7236     7236           
=======================================
  Hits         5996     5996           
  Misses       1240     1240           
Impacted Files Coverage Δ
...vs16/x86/php-8.0.2-src/ext/sqlsrv/php_sqlsrv_int.h
.../x86/php-8.0.2-src/ext/sqlsrv/shared/core_sqlsrv.h
.../phpdev/vs16/x86/php-8.0.2-src/ext/sqlsrv/conn.cpp
.../x86/php-8.0.2-src/ext/sqlsrv/shared/core_conn.cpp
.../phpdev/vs16/x86/php-8.0.2-src/ext/sqlsrv/stmt.cpp
.../php-8.0.2-src/ext/pdo_sqlsrv/shared/core_stmt.cpp
...vs16/x86/php-8.0.2-src/ext/pdo_sqlsrv/pdo_util.cpp
.../php-8.0.2-src/ext/pdo_sqlsrv/shared/core_conn.cpp
.../x86/php-8.0.2-src/ext/sqlsrv/shared/core_stmt.cpp
.../vs16/x86/php-8.0.2-src/ext/pdo_sqlsrv/pdo_dbh.cpp
... and 38 more

@yitam
Copy link
Contributor

yitam commented Mar 31, 2021

Thank you @morozov for your contribution to the issue you've reported. I'll review this and get back to you.
Just so you know, we only accept pull requests to dev branch, not master, mainly because of some existing policy explained in the readme. Would you please change the branch?

@yitam yitam self-assigned this Mar 31, 2021
@morozov morozov changed the base branch from master to dev March 31, 2021 15:44
@morozov
Copy link
Contributor Author

morozov commented Mar 31, 2021

@yitam I retargeted the PR. Would it make sense for you to change the default repository branch from master to dev? This way, PRs will target dev by default.

@yitam
Copy link
Contributor

yitam commented Mar 31, 2021

@yitam I retargeted the PR. Would it make sense for you to change the default repository branch from master to dev? This way, PRs will target dev by default.

Thank you for your prompt action, @morozov. Whether to change the default branch is not my call, but I will bring this up for sure.

@yitam
Copy link
Contributor

yitam commented Mar 31, 2021

Looks good, @morozov . If you like to enhance the test, add -e MSSQL_COLLATION=Latin1_General_100_CS_AS to https://github.com/microsoft/msphpsql/blob/dev/azure-pipelines.yml#L117

@yitam
Copy link
Contributor

yitam commented Mar 31, 2021

@morozov looks like the other existing tests don't work well with the new collation. Please feel free to revert the recent commit. I'll put this in our backlog to address later.

@yitam yitam merged commit fcd7b64 into microsoft:dev Mar 31, 2021
@morozov morozov deleted the issues/1244 branch March 31, 2021 20:30
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.

PDO::lastInsertId($name) doesn't work with a case-sensitive collation

2 participants