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 MSSQL CLOB column type to support Unicode #4987

Closed
wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Nov 14, 2021

Q A
Type bug
BC Break yes

Summary

VARCHAR MSSQL type does not support Unicode, fix CLOB mapping to NVARCHAR. This is also consistent with standard string mapping which uses NVARCHAR already.

@mvorisek mvorisek changed the base branch from 3.1.x to 2.13.x November 14, 2021 13:02
@mvorisek mvorisek force-pushed the fix_mssql_clob branch 4 times, most recently from 6e4fc34 to e820ca0 Compare November 16, 2021 10:43
@morozov
Copy link
Member

morozov commented Nov 21, 2021

It doesn't look like a bug so far. If it is, please provide the steps to reproduce and cover the change with an integration test. The mapping cannot be changed in a patch release, especially in 2.x. Additionally, upgrade testing should be done to understand the impact on backward compatibility.

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 21, 2021

It doesn't look like a bug so far.

why not? text (CLOB) type represent a long string type, eg. non-case sensitive type with full Unicode / UTF-8 support. It is true for all DB platforms except MSSQL and this PR fixes this inconsistency.

steps to replicate:

  1. create table with a text column
  2. insert ❤️
  3. read it back

@morozov
Copy link
Member

morozov commented Nov 21, 2021

How are these steps related to Unicode?

@mvorisek
Copy link
Contributor Author

How are these steps related to Unicode?

updated steps, previously, I posted steps for a different issue

@morozov
Copy link
Member

morozov commented Nov 21, 2021

text (CLOB) type represent a long string type, eg. non-case sensitive type with full Unicode / UTF-8 support

Where does it come from?

@morozov morozov removed the Bug label Nov 21, 2021
@morozov morozov removed this from the 2.13.6 milestone Nov 21, 2021
@mvorisek
Copy link
Contributor Author

where is the definition of string, text vs. blob type mapping to DB

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Please identify the upgrade impact and retarget towards 3.2.x.

if ($doctrineType->getName() === Types::TEXT) {
// We require a commented text type in order to distinguish between text and string
// as both (have to) map to the same native type.
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Only TEXT maps to VARCHAR(MAX). Is it possible to identify this during the schema introspection? See

case 'varchar':
// TEXT type is returned as VARCHAR(MAX) with a length of -1
if ($length === -1) {
$dbType = 'text';
}
break;

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Mar 30, 2022
@mvorisek
Copy link
Contributor Author

non-stale

@github-actions github-actions bot removed the Stale label Mar 31, 2022
@morozov morozov closed this Apr 3, 2022
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 3, 2022

text DBAL type does support UTF-8 with every common DB vendor excl. MSSQL and I belive MSSQL should support it too. That is what whis PR does.

About the upgrade path request, yes, the request is legit, but I have no time and experience to impl. it easily, thus I would like to keep this PR open for others to help with that. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023
@mvorisek mvorisek deleted the fix_mssql_clob branch August 27, 2023 15:26
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.

3 participants