Support ASCII parameter binding

[gjdanis]

Q	A
Type	improvement
BC Break	no
Fixed issues	#4263

Summary

Implements the changes proposed in #4263.

[greg0ire]

You should use git rebase -i origin/3.0.x and drop any commit related to .idea, as asked in #4274 (comment)

Also, please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
[email protected], that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

[greg0ire]

docs/en/reference/types.rst should be updated, shouldn't it?

[morozov]

Great work so far!

Besides a couple of comments below, it'd be nice to have another functional test at the driver level that would demonstrate the newly introduced type will work on all platforms, not only on the ones that actually support the new binding type.

See BinaryTest for the reference.

[morozov]

Looks good. Please address the remaining comments and squash. It should be good to go.

[greg0ire]

I'm afraid I don't 100% understand the issue yet. In your example:

$statement = $connection->prepare(
   'SELECT * FROM ... WHERE myVarcharColumn = :value' -- will bind as N'...'
);

What exactly dictates that :value will be bound as N'…'? What does myVarcharColumn definition look like?

[gjdanis]

@greg0ire the driver code prepends the N character. If you use SQL Server Profiler you can see that the SQL that is sent to the server includes it. The actual code for this can be found here: https://github.com/microsoft/msphpsql/blob/5f76f838e5d44ca951a44bec6be1af05fae4bd93/source/pdo_sqlsrv/pdo_dbh.cpp#L1565

Here's my conversation with the developers of the driver about this isssue. There's also an RFC about this issue from the PDO maintainers which explains why they added other types for string binding.

In my example, we'd have a something like this:

CREATE TABLE #ExampleTable (
  ID INT NOT NULL,
  VarcharColumn VARCHAR(30) NOT NULL
)

CREATE NONCLUSTERED INDEX IX_ExampleTable_VarcharColumn
  ON #ExampleTable (VarcharColumn)

If we have a query like this:

SELECT *
FROM #ExampleTable
WHERE VarcharColumn = N'value'

We won't be able to use the index on VarcharColumn because the value we're binding for will require a type conversation for the values of the column we're querying.

If we bind with 'value', however, the types are the same and there's no conversation (and we're able to use the index as expected).

[greg0ire]

Ok I think I got this right thanks to your explanation: when using the new ascii type, PDO::SQLSRV_ENCODING_SYSTEM will be used which means, "bind as 8-bit chars", which in turns means the driver will not add the N that would result in the implicit cast of the column values to utf-8

[greg0ire]

Should this target 2.11.x or is there a BC-break somewhere in there?

[gjdanis]

@greg0ire I think it could target 2.11.x but a previous conversation with @morozov asked that I move it to 3.0.x: #4270 (comment)

Let me know if I should retarget.

[greg0ire]

He was asking you to target to 3.0.x, but that retarget was from master… @morozov should we re-evaluate this? I see no reason to postpone this to 3.0.x when we could have it in 2.11.x .

[greg0ire]

A few extra comments while you wait for a decision on the target branch

[morozov]

@morozov should we re-evaluate this? I see no reason to postpone this to 3.0.x when we could have it in 2.11.x .

I don't see a problem having it in 2.11.x I asked for 3.0.x because I didn't foresee it being ready before 2.11.0 is released and didn't want it to be a blocker.

@gjdanis if you want to retarget the PR, please do not open a new one. Edit and rebase this one instead.

The base branch was changed.

[greg0ire]

@gjdanis I addressed the changes for you, LGTM

[morozov]

@gjdanis it would be at least the same amount of work or even more. If you want this to be released in 2.11.0, we should retarget this PR against 2.11.x. I can do this myself.

[gjdanis]

@morozov I feel like there should be 2.11.x version of this code so that large codebases that are reliant on 2.0 versions of this library can benefit from the change without having to refactor their code to be 3.0.x compliant. If you're offering to do this work, I'll take you up on the offer 👍

[morozov]

@gjdanis please see if the rebased PR looks do you.

[gjdanis]

@morozov changes LGTM - thanks for the work to rebase!

[morozov]

Thanks, @gjdanis!

[greg0ire]

Strategy for the merge-up:

1. merge 2.11.x as it was before merging this PR (so git merge cce615a9e)
2. merge this PR as it was before the rebase ( git merge 3b8c8f9f6)
3. merge 2.11.x as it is/was after merging this PR, but discard changes (git merge -s ours 2f3d9bb61)
4. if 2.11.x moved since then, merge it.

Will try this soon.