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

Use lower-case object names in PDO::lastInsertId($name) #1245

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Use lower-case object names in PDO::lastInsertId($name) #1245

merged 1 commit into from
Mar 31, 2021

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